Re: [HACKERS] 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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (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; boolerror = false; ! #ifndef WIN32 struct stat before, after; --- 1398,1405 FILE *stream; const char *fname; boolerror = false; ! int fd; ! #ifndef WIN32 struct stat before,
Re: [HACKERS] 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 [EMAIL PROTECTED] | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup.| Drexel Hill, Pennsylvania 19026
Re: [HACKERS] 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
[EMAIL PROTECTED]
--
Bruce Momjian| http://candle.pha.pa.us
[EMAIL PROTECTED] | (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;
boolerror = false;
!
#ifndef WIN32
struct stat before,
after;
--- 1398,1405
FILE *stream;
const char *fname;
boolerror = 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,
