Thread: SECURITY: psql allows symlink games in /tmp
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
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;
> 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, Pennsylvania19026
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