This code in psql/command.c allows *any* system user to place a
predictably named symbolic link in /tmp and use it to alter/destroy
files owned by the user running psql. (tested - postgresql 7.0.2).

All the information a potential attacker would need are available via a
simple 'ps'.

It might (untested) also allow an another user to exploit the race
between the closing of the file by the editor and the re-reading of its
contents to execute arbitrary SQL commands.

IMHO these files, if they must be created in /tmp should at least be
created O_EXCL, but there are still editor vulnerabilities with opening
any files in a world writeable directory (see recent joe Vulnerability:
http://lwn.net/2000/1123/a/sec-joe.php3)

My system is RedHat 6.2 on an i686, with Postgresql 7.0.2 but the same
code currently exists in CVS (or at least CVS-web).

I am not subscribed to this list, so please CC me for replies.  (Also
tell me if there is a more appropriate forum for this, but
www.postgresql.org doesn't have a listed security issue address).
-- 
Andrew Bartlett
abartlet@pcug.org.au

Sat, 25 Nov 2000 11:28:42 +1100
Andrew Bartlett <abartlet@pcug.org.au>
SECURITY: psql allows symlink games in /tmp 
Andrew Bartlett wrote:
> 
> This code in psql/command.c allows *any* system user to place a
> predictably named symbolic link in /tmp and use it to alter/destroy
> files owned by the user running psql. (tested - postgresql 7.0.2).
> 
> All the information a potential attacker would need are available via a
> simple 'ps'.
> 
> It might (untested) also allow an another user to exploit the race
> between the closing of the file by the editor and the re-reading of its
> contents to execute arbitrary SQL commands.
> 
> IMHO these files, if they must be created in /tmp should at least be
> created O_EXCL, but there are still editor vulnerabilities with opening
> any files in a world writeable directory (see recent joe Vulnerability:
> http://lwn.net/2000/1123/a/sec-joe.php3)
> 
> My system is RedHat 6.2 on an i686, with Postgresql 7.0.2 but the same
> code currently exists in CVS (or at least CVS-web).
> 
> I am not subscribed to this list, so please CC me for replies.  (Also
> tell me if there is a more appropriate forum for this, but
> www.postgresql.org doesn't have a listed security issue address).
> --
> Andrew Bartlett
> abartlet@pcug.org.au

Sorry, forgot to inlude the offending code....

(This is part of do_edit, called from edit_file and the \e query buffer
editing fuction)

        if (filename_arg)
                fname = filename_arg;

        else
        {
                /* make a temp file to edit */
#ifndef WIN32
                mode_t                oldumask;
                const char *tmpdirenv = getenv("TMPDIR");

                sprintf(fnametmp, "%s/psql.edit.%ld.%ld",
                                tmpdirenv ? tmpdirenv : "/tmp",
                                (long) geteuid(), (long) getpid());
#else
                GetTempFileName(".", "psql", 0, fnametmp);
#endif
                fname = (const char *) fnametmp;

#ifndef WIN32
                oldumask = umask(0177);
#endif
                stream = fopen(fname, "w");
#ifndef WIN32
                umask(oldumask);
#endif

 
-- 
Andrew Bartlett
abartlet@pcug.org.au

Sat, 25 Nov 2000 11:42:02 +1100
Andrew Bartlett <abartlet@pcug.org.au>
Re: SECURITY: psql allows symlink games in /tmp
Thanks for the pointer.  Here is a diff to fix the problem.  How does it
look to you?

> This code in psql/command.c allows *any* system user to place a
> predictably named symbolic link in /tmp and use it to alter/destroy
> files owned by the user running psql. (tested - postgresql 7.0.2).
> 
> All the information a potential attacker would need are available via a
> simple 'ps'.
> 
> It might (untested) also allow an another user to exploit the race
> between the closing of the file by the editor and the re-reading of its
> contents to execute arbitrary SQL commands.
> 
> IMHO these files, if they must be created in /tmp should at least be
> created O_EXCL, but there are still editor vulnerabilities with opening
> any files in a world writeable directory (see recent joe Vulnerability:
> http://lwn.net/2000/1123/a/sec-joe.php3)
> 
> My system is RedHat 6.2 on an i686, with Postgresql 7.0.2 but the same
> code currently exists in CVS (or at least CVS-web).
> 
> I am not subscribed to this list, so please CC me for replies.  (Also
> tell me if there is a more appropriate forum for this, but
> www.postgresql.org doesn't have a listed security issue address).
> -- 
> Andrew Bartlett
> abartlet@pcug.org.au
> 


-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

? config.log
? config.cache
? config.status
? GNUmakefile
? src/Makefile.custom
? src/GNUmakefile
? src/Makefile.global
? src/log
? src/crtags
? src/backend/postgres
? src/backend/catalog/global.description
? src/backend/catalog/global.bki
? src/backend/catalog/template1.bki
? src/backend/catalog/template1.description
? src/backend/port/Makefile
? src/bin/initdb/initdb
? src/bin/initlocation/initlocation
? src/bin/ipcclean/ipcclean
? src/bin/pg_config/pg_config
? src/bin/pg_ctl/pg_ctl
? src/bin/pg_dump/pg_dump
? src/bin/pg_dump/pg_restore
? src/bin/pg_dump/pg_dumpall
? src/bin/pg_id/pg_id
? src/bin/pg_passwd/pg_passwd
? src/bin/pgaccess/pgaccess
? src/bin/pgtclsh/Makefile.tkdefs
? src/bin/pgtclsh/Makefile.tcldefs
? src/bin/pgtclsh/pgtclsh
? src/bin/pgtclsh/pgtksh
? src/bin/psql/psql
? src/bin/scripts/createlang
? src/include/config.h
? src/include/stamp-h
? src/interfaces/ecpg/lib/libecpg.so.3.2.0
? src/interfaces/ecpg/preproc/ecpg
? src/interfaces/libpgeasy/libpgeasy.so.2.1
? src/interfaces/libpgtcl/libpgtcl.so.2.1
? src/interfaces/libpq/libpq.so.2.1
? src/interfaces/perl5/blib
? src/interfaces/perl5/Makefile
? src/interfaces/perl5/pm_to_blib
? src/interfaces/perl5/Pg.c
? src/interfaces/perl5/Pg.bs
? src/pl/plperl/blib
? src/pl/plperl/Makefile
? src/pl/plperl/pm_to_blib
? src/pl/plperl/SPI.c
? src/pl/plperl/plperl.bs
? src/pl/plpgsql/src/libplpgsql.so.1.0
? src/pl/tcl/Makefile.tcldefs
Index: src/bin/psql/command.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/bin/psql/command.c,v
retrieving revision 1.38
diff -c -r1.38 command.c
*** src/bin/psql/command.c	2000/11/13 23:37:53	1.38
--- src/bin/psql/command.c	2000/11/25 06:18:33
***************
*** 13,19 ****
  #include <ctype.h>
  #ifndef WIN32
  #include <sys/types.h>			/* for umask() */
! #include <sys/stat.h>			/* for umask(), stat() */
  #include <unistd.h>				/* for geteuid(), getpid(), stat() */
  #else
  #include <win32.h>
--- 13,20 ----
  #include <ctype.h>
  #ifndef WIN32
  #include <sys/types.h>			/* for umask() */
! #include <sys/stat.h>			/* for stat() */
! #include <fcntl.h>				/* open() flags */
  #include <unistd.h>				/* for geteuid(), getpid(), stat() */
  #else
  #include <win32.h>
***************
*** 1397,1403 ****
  	FILE	   *stream;
  	const char *fname;
  	bool		error = false;
! 
  #ifndef WIN32
  	struct stat before,
  				after;
--- 1398,1405 ----
  	FILE	   *stream;
  	const char *fname;
  	bool		error = false;
! 	int			fd;
! 	
  #ifndef WIN32
  	struct stat before,
  				after;
***************
*** 1411,1417 ****
  	{
  		/* make a temp file to edit */
  #ifndef WIN32
- 		mode_t		oldumask;
  		const char *tmpdirenv = getenv("TMPDIR");
  
  		sprintf(fnametmp, "%s/psql.edit.%ld.%ld",
--- 1413,1418 ----
***************
*** 1422,1436 ****
  #endif
  		fname = (const char *) fnametmp;
  
! #ifndef WIN32
! 		oldumask = umask(0177);
! #endif
! 		stream = fopen(fname, "w");
! #ifndef WIN32
! 		umask(oldumask);
! #endif
  
! 		if (!stream)
  		{
  			psql_error("couldn't open temp file %s: %s\n", fname, strerror(errno));
  			error = true;
--- 1423,1433 ----
  #endif
  		fname = (const char *) fnametmp;
  
! 		fd = open(fname, O_WRONLY|O_CREAT|O_EXCL, 0600);
! 		if (fd != -1)
! 			stream = fdopen(fd, "w");
  
! 		if (fd == -1 || !stream)
  		{
  			psql_error("couldn't open temp file %s: %s\n", fname, strerror(errno));
  			error = true;
Sat, 25 Nov 2000 01:19:20 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: SECURITY: psql allows symlink games in /tmp
Looks like what I would have done if I knew C.

The only issue remaining is a policy issue as to if psql should call an
editor in /tmp at all, considering the issues raised bye the recent joe
vulnerability, ie can we trust the editor not to do a crazy thing, like
not creating a similarly predictable backup-file name etc.  It should at
least be documented so that a more parinoid sys-admin can make sure that
users use a private TMPDIR.

Thanks for the quick response,

Andrew Bartlett
abartlet@pcug.org.au

Bruce Momjian wrote:
> 
> Thanks for the pointer.  Here is a diff to fix the problem.  How does it
> look to you?
> 
> > This code in psql/command.c allows *any* system user to place a
> > predictably named symbolic link in /tmp and use it to alter/destroy
> > files owned by the user running psql. (tested - postgresql 7.0.2).
> >
> > All the information a potential attacker would need are available via a
> > simple 'ps'.
> >
> > It might (untested) also allow an another user to exploit the race
> > between the closing of the file by the editor and the re-reading of its
> > contents to execute arbitrary SQL commands.
> >
> > IMHO these files, if they must be created in /tmp should at least be
> > created O_EXCL, but there are still editor vulnerabilities with opening
> > any files in a world writeable directory (see recent joe Vulnerability:
> > http://lwn.net/2000/1123/a/sec-joe.php3)
> >
> > My system is RedHat 6.2 on an i686, with Postgresql 7.0.2 but the same
> > code currently exists in CVS (or at least CVS-web).
> >
> > I am not subscribed to this list, so please CC me for replies.  (Also
> > tell me if there is a more appropriate forum for this, but
> > www.postgresql.org doesn't have a listed security issue address).
> > --
> > Andrew Bartlett
> > abartlet@pcug.org.au
> >
> 
> --
>   Bruce Momjian                        |  http://candle.pha.pa.us
>   pgman@candle.pha.pa.us               |  (610) 853-3000
>   +  If your life is a hard drive,     |  830 Blythe Avenue
>   +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026
> 
>   ------------------------------------------------------------------------
> ? config.log
> ? config.cache
> ? config.status
> ? GNUmakefile
> ? src/Makefile.custom
> ? src/GNUmakefile
> ? src/Makefile.global
> ? src/log
> ? src/crtags
> ? src/backend/postgres
> ? src/backend/catalog/global.description
> ? src/backend/catalog/global.bki
> ? src/backend/catalog/template1.bki
> ? src/backend/catalog/template1.description
> ? src/backend/port/Makefile
> ? src/bin/initdb/initdb
> ? src/bin/initlocation/initlocation
> ? src/bin/ipcclean/ipcclean
> ? src/bin/pg_config/pg_config
> ? src/bin/pg_ctl/pg_ctl
> ? src/bin/pg_dump/pg_dump
> ? src/bin/pg_dump/pg_restore
> ? src/bin/pg_dump/pg_dumpall
> ? src/bin/pg_id/pg_id
> ? src/bin/pg_passwd/pg_passwd
> ? src/bin/pgaccess/pgaccess
> ? src/bin/pgtclsh/Makefile.tkdefs
> ? src/bin/pgtclsh/Makefile.tcldefs
> ? src/bin/pgtclsh/pgtclsh
> ? src/bin/pgtclsh/pgtksh
> ? src/bin/psql/psql
> ? src/bin/scripts/createlang
> ? src/include/config.h
> ? src/include/stamp-h
> ? src/interfaces/ecpg/lib/libecpg.so.3.2.0
> ? src/interfaces/ecpg/preproc/ecpg
> ? src/interfaces/libpgeasy/libpgeasy.so.2.1
> ? src/interfaces/libpgtcl/libpgtcl.so.2.1
> ? src/interfaces/libpq/libpq.so.2.1
> ? src/interfaces/perl5/blib
> ? src/interfaces/perl5/Makefile
> ? src/interfaces/perl5/pm_to_blib
> ? src/interfaces/perl5/Pg.c
> ? src/interfaces/perl5/Pg.bs
> ? src/pl/plperl/blib
> ? src/pl/plperl/Makefile
> ? src/pl/plperl/pm_to_blib
> ? src/pl/plperl/SPI.c
> ? src/pl/plperl/plperl.bs
> ? src/pl/plpgsql/src/libplpgsql.so.1.0
> ? src/pl/tcl/Makefile.tcldefs
> Index: src/bin/psql/command.c
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/bin/psql/command.c,v
> retrieving revision 1.38
> diff -c -r1.38 command.c
> *** src/bin/psql/command.c      2000/11/13 23:37:53     1.38
> --- src/bin/psql/command.c      2000/11/25 06:18:33
> ***************
> *** 13,19 ****
>   #include <ctype.h>
>   #ifndef WIN32
>   #include <sys/types.h>                        /* for umask() */
> ! #include <sys/stat.h>                 /* for umask(), stat() */
>   #include <unistd.h>                           /* for geteuid(), getpid(), stat() */
>   #else
>   #include <win32.h>
> --- 13,20 ----
>   #include <ctype.h>
>   #ifndef WIN32
>   #include <sys/types.h>                        /* for umask() */
> ! #include <sys/stat.h>                 /* for stat() */
> ! #include <fcntl.h>                            /* open() flags */
>   #include <unistd.h>                           /* for geteuid(), getpid(), stat() */
>   #else
>   #include <win32.h>
> ***************
> *** 1397,1403 ****
>         FILE       *stream;
>         const char *fname;
>         bool            error = false;
> !
>   #ifndef WIN32
>         struct stat before,
>                                 after;
> --- 1398,1405 ----
>         FILE       *stream;
>         const char *fname;
>         bool            error = false;
> !       int                     fd;
> !
>   #ifndef WIN32
>         struct stat before,
>                                 after;
> ***************
> *** 1411,1417 ****
>         {
>                 /* make a temp file to edit */
>   #ifndef WIN32
> -               mode_t          oldumask;
>                 const char *tmpdirenv = getenv("TMPDIR");
> 
>                 sprintf(fnametmp, "%s/psql.edit.%ld.%ld",
> --- 1413,1418 ----
> ***************
> *** 1422,1436 ****
>   #endif
>                 fname = (const char *) fnametmp;
> 
> ! #ifndef WIN32
> !               oldumask = umask(0177);
> ! #endif
> !               stream = fopen(fname, "w");
> ! #ifndef WIN32
> !               umask(oldumask);
> ! #endif
> 
> !               if (!stream)
>                 {
>                         psql_error("couldn't open temp file %s: %s\n", fname, strerror(errno));
>                         error = true;
> --- 1423,1433 ----
>   #endif
>                 fname = (const char *) fnametmp;
> 
> !               fd = open(fname, O_WRONLY|O_CREAT|O_EXCL, 0600);
> !               if (fd != -1)
> !                       stream = fdopen(fd, "w");
> 
> !               if (fd == -1 || !stream)
>                 {
>                         psql_error("couldn't open temp file %s: %s\n", fname, strerror(errno));
>                         error = true;

-- 
Andrew Bartlett
abartlet@pcug.org.au

Sat, 25 Nov 2000 17:46:05 +1100
Andrew Bartlett <abartlet@pcug.org.au>
Re: SECURITY: psql allows symlink games in /tmp
> Looks like what I would have done if I knew C.
> 
> The only issue remaining is a policy issue as to if psql should call an
> editor in /tmp at all, considering the issues raised bye the recent joe
> vulnerability, ie can we trust the editor not to do a crazy thing, like
> not creating a similarly predictable backup-file name etc.  It should at
> least be documented so that a more parinoid sys-admin can make sure that
> users use a private TMPDIR.

Not sure it is worth the addition.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Sat, 25 Nov 2000 09:28:13 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: SECURITY: psql allows symlink games in /tmp