Обсуждение: Security of ODBC debug log file leaves something to be desired

Поиск
Список
Период
Сортировка

Security of ODBC debug log file leaves something to be desired

От
Tom Lane
Дата:
I got a complaint here
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=154126
pointing out that when you set debug=1, the generated log file
is world-readable by default, which doesn't seem like a good
idea when it may contain your password.  Also, since the name
of the file is pretty predictable, there is an opportunity
for a symlink redirection attack (though I doubt anything
really interesting could be accomplished that way).

Any thoughts about fixing this?  It's hard to believe no one
has pointed it out before, so I was wondering if there was some
good reason for doing it like this.

            regards, tom lane

Re: Security of ODBC debug log file leaves something to be desired

От
Mischa Sandberg
Дата:
Quoting Tom Lane <tgl@sss.pgh.pa.us>:

> I got a complaint here
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=154126
> pointing out that when you set debug=1, the generated log file
> is world-readable by default, which doesn't seem like a good
> idea when it may contain your password.
> Any thoughts about fixing this?  It's hard to believe no one
> has pointed it out before, so I was wondering if there was some
> good reason for doing it like this.

Read your comments in the bug report. No, it is not intentional.
It is just YA case of ODBC paying only lip service to security.
There are still many commercial drivers that send the password over TCP,
in the clear. Gah.

Both the driver manager and the driver write to the debug log.
Each of them is responsible for not doing things like that!
File permissions on the log itself are rather weak protection.

When I worked for Simba (ODBC kit/SQL engine company, now owned by orbital.com)
we patch iODBC so that the password string was overwritten with "*"s
before it was logged; and our driver kit did the same.
It is simple to fix. Can't guarantee I can make the time right now;
having too much antifun with Postgres performance on Solaris.

--
"Dreams come true, not free." -- S.Sondheim, ITW


Re: Security of ODBC debug log file leaves something to be

От
Marko Ristola
Дата:
What psqlodbc versions RedHat uses?

At least there are three Enterprise versions:
- Red Hat ES 2
- Red Hat ES 3
- Red Hat ES 4

I know, that ES 2 and ES 3 use different psqlodbc versions.
I don't know about ES 4.
Which ODBC revisions they are in psqlodbc PostgreSQL CVS?
Each of them might need a different fix.

I don't know, wether I can fix those versions myself.
Here is an attached bugfix for the CVS HEAD.

---------------------------------------

I wrote and attached a fix for the following things
for the CVS HEAD branch (see the attached patch):
1. File permissions; chmod go-rwx for the log file: fixed in this patch.
SVr4, SVID, POSIX, X/OPEN, BSD 4.3 for umask()

2. Pipe redirection problem: fixed in this patch.
SVr4, SVID, POSIX, X/OPEN, BSD 4.3 for fd=open()
IEEE Std1003.1-1988 (POSIX.1) for fdopen().

These are not touched in the patch:
3. Log files are easy to guess: not fixed, because the whole idea of
logging is easy guessability.
Maybe the log directory could be changed?
4. Plaintext passwords are not accepted in log files: CVS HEAD is
already OK.

These fixes work with Debian Sarge, compiled by myself.
I don't know, wether these fixes even compile with Windows XP or some
other system.
I hope, that they compile with many architectures because of the POSIX
compatibility.

I fixed the pipe redirection problem by forcing the creation of the log
file.
If the log file exists already, no logging is done.
It prints an error message into stderr (only once).
So it can't be a pipe or a soft link! This problem remains with NFS:
open() with O_EXCL does not work properly with NFS (see man 2 open).

Marko Ristola


Mischa Sandberg wrote:

>Quoting Tom Lane <tgl@sss.pgh.pa.us>:
>
>
>
>>I got a complaint here
>>https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=154126
>>pointing out that when you set debug=1, the generated log file
>>is world-readable by default, which doesn't seem like a good
>>idea when it may contain your password.
>>Any thoughts about fixing this?  It's hard to believe no one
>>has pointed it out before, so I was wondering if there was some
>>good reason for doing it like this.
>>
>>
>
>Read your comments in the bug report. No, it is not intentional.
>It is just YA case of ODBC paying only lip service to security.
>There are still many commercial drivers that send the password over TCP,
>in the clear. Gah.
>
>Both the driver manager and the driver write to the debug log.
>Each of them is responsible for not doing things like that!
>File permissions on the log itself are rather weak protection.
>
>When I worked for Simba (ODBC kit/SQL engine company, now owned by orbital.com)
>we patch iODBC so that the password string was overwritten with "*"s
>before it was logged; and our driver kit did the same.
>It is simple to fix. Can't guarantee I can make the time right now;
>having too much antifun with Postgres performance on Solaris.
>
>
>

Index: misc.h
===================================================================
RCS file: /usr/local/cvsroot/psqlodbc/psqlodbc/misc.h,v
retrieving revision 1.40
diff -u -r1.40 misc.h
--- misc.h    9 Jul 2004 18:50:51 -0000    1.40
+++ misc.h    10 Apr 2005 18:33:45 -0000
@@ -20,6 +20,10 @@
 */
 #define MY_LOG

+/*  File creation needs protected permission masks.
+    File creation is better protected with a secure umask() setting.
+    umask value 0077 will result with file creation of 0666 & ~077 = 0600 */
+#define LOGFILE_PERM_MASK 077

 /*    Uncomment Q_LOG to compile in the qlog() statements (Communications log, i.e. CommLog).
     This logfile contains serious log statements that are intended for an
Index: misc.c
===================================================================
RCS file: /usr/local/cvsroot/psqlodbc/psqlodbc/misc.c,v
retrieving revision 1.40
diff -u -r1.40 misc.c
--- misc.c    9 Jul 2004 18:50:51 -0000    1.40
+++ misc.c    10 Apr 2005 18:33:46 -0000
@@ -17,6 +17,8 @@
 #include <stdio.h>
 #include <stdarg.h>
 #include <string.h>
+#include <sys/stat.h>
+#include <fcntl.h>

 #ifndef WIN32
 #include <pwd.h>
@@ -58,6 +60,39 @@
     return;
 }

+/*    Create the given file. Only the owner can read or write the file.
+    Return a pointer into the new stream, or NULL value on error.
+*/
+FILE *
+create_logfile(const char *dirname, const char *prefix, char *filename)
+{
+    int fd;
+    FILE *fp;
+    mode_t orig_mask;
+    static int warned=0;
+
+    orig_mask = umask(LOGFILE_PERM_MASK);
+
+    generate_filename(dirname, prefix, filename);
+
+    fd = open(filename, O_WRONLY | O_CREAT | O_EXCL | O_APPEND, S_IRUSR|S_IWUSR);
+
+    if (fd == -1) {
+        if (!warned) fprintf(stderr,"Creating log file %s failed: %s.\n",filename,strerror(errno));
+        warned=1;
+    } else {
+        /* must conform to open(f,O_WRONLY|O_APPEND) above */
+        fp = fdopen(fd, PG_BINARY_A);
+    }
+
+    umask(orig_mask);
+
+    if (fp != NULL)
+        setbuf(fp, NULL);
+
+    return fp;
+}
+
 #if defined(WIN_MULTITHREAD_SUPPORT)
 CRITICAL_SECTION    qlog_cs, mylog_cs;
 #elif defined(POSIX_MULTITHREAD_SUPPORT)
@@ -123,11 +158,7 @@
         va_start(args, fmt);

         if (!LOGFP)
-        {
-            generate_filename(MYLOGDIR, MYLOGFILE, filebuf);
-            LOGFP = fopen(filebuf, PG_BINARY_A);
-            setbuf(LOGFP, NULL);
-        }
+            LOGFP = create_logfile(MYLOGDIR, MYLOGFILE, filebuf);

 #ifdef    WIN_MULTITHREAD_SUPPORT
 #ifdef    WIN32
@@ -168,11 +199,7 @@
         va_start(args, fmt);

         if (!LOGFP)
-        {
-            generate_filename(QLOGDIR, QLOGFILE, filebuf);
-            LOGFP = fopen(filebuf, PG_BINARY_A);
-            setbuf(LOGFP, NULL);
-        }
+            LOGFP = create_logfile(QLOGDIR, QLOGFILE, filebuf);

         if (LOGFP)
             vfprintf(LOGFP, fmt, args);