Comment 1 Nelson Bolyard (seldom reads bugmail) 2006-09-05 16:02:28 PDT

Taking.  Thinking out loud here.  I am inclined to change both _PR_InitLog 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/io/prlog.c&rev=3.34#205
and PR_SetLogFile
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/io/prlog.c&rev=3.34#372
so that both implement a test for being in setuid programs, and both disallow
any logging or opening log files while being setuid root.  The logic would be
to insert something like

if (realUID != effectiveUID)
    silently return without doing anything

Comments on that approach?

Comment 2 Wan-Teh Chang 2006-09-05 17:44:30 PDT

That approach sounds good.

Comment 3 Alexei Volkov 2006-09-05 18:37:36 PDT

Created attachment 236906 [details] [diff] [review]
proposed patch

Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-09-05 19:19:19 PDT

Comment on attachment 236906 [details] [diff] [review]
proposed patch

I think we want to disable all environment variable controls over NSPR 
logging in setuid programs, and not only the setting of the output file 
name.  e.g. here:

     _pr_logLock = PR_NewLock();
+#ifdef XP_UNIX
+    if (getuid() != geteuid())
+        return;
+#endif /* XP_UNIX */
     ev = PR_GetEnv("NSPR_LOG_MODULES");

But I'm willing to be pursuaded otherwise.

Wan-Teh, what do you think?

Comment 5 Robert Relyea 2006-09-06 09:08:03 PDT

Longer term, I'd like to see a way of 'safely' enabling logging for setuid programs, but for now I think Nelson is right, It's best to nip the problem at the bud. A quick review shows no obvious issues with turning on logging to standard out, but in the interest of closing the hole, It's probably best to always disable  logging when setuid is turned on.

bob

Comment 6 Wan-Teh Chang 2006-09-06 10:19:56 PDT

Comment on attachment 236906 [details] [diff] [review]
proposed patch

r=wtc.

Add a short comment describing the problem.

I verified that the header file <unistd.h> required
for getuid() and geteuid() is already included
(in mozilla/nsprpub/pr/include/md/_unixos.h).

>-/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Please change tab-width to 8.  This file uses tabs in
only two places, but with inconsistent tab widths (4 and 8).
A tab width of 8 looks better.

It is okay to use a log file specified with a PR_SetLogFile
call.

It's not clear whether it could be a security problem to log
to stderr.

I also reviewed the NSPR source tree for similar problems.
- I reviewed all the PR_GetEnv() and getenv() calls.
- I reviewed all the PR_Open, PR_OpenFile, fopen, open, CreateFile,
  and dlopen calls.

I didn't find anything that requires immediate action.  But
there are two things we should do.

1. We should apply the same fix to the log file specified in
   the environment variable NSPR_TRACE_LOG.
   (mozilla/nsprpub/pr/src/misc/prtrace.c)

   The function that opens the file, PR_RecordTraceEntries,
   will only be called:
   - if you call the function directly, or
   - if you call the macro PR_RECORD_TRACE_ENTRIES and
     DEBUG or FORCE_NSPR_TRACE is defined.

   NSPR itself doesn't call PR_RecordTraceEntries or
   PR_RECORD_TRACE_ENTRIES, and NSPR optimized build is
   compiled with FORCE_NSPR_TRACE undefined.

2. We should make sure we check the following environment
   variables's values for proper range:
   - NSPR_FD_CACHE_SIZE_LOW
   - NSPR_FD_CACHE_SIZE_HIGH
   - NSPR_ATOMIC_HASH_LOCKS

Comment 7 Wan-Teh Chang 2006-09-06 10:50:34 PDT

Comment on attachment 236906 [details] [diff] [review]
proposed patch

Alexei, I suggest that you set logFile to NULL
after the fclose(logFile) and PR_Close(logFile)
calls in _PR_LogCleanup.

I verified that prlog.c won't crash if logFile
is NULL (which causes PR_LogPrint to return
immediately, effectively turning off logging).

Comment 8 Nelson Bolyard (seldom reads bugmail) 2006-09-06 11:30:37 PDT

Wan-Teh, I was/am concerned that PR logging to stderr might reveal info from
files that are not readable by the user running the setuid program, and might
be used as a way to read otherwise unreadable files.

Comment 9 Nelson Bolyard (seldom reads bugmail) 2006-09-06 11:42:07 PDT

I agree that we must also ensure that the environment variables named in
comment 6 can do no harm in setuid root programs, and I agree with comment 7.

Comment 10 Alexei Volkov 2006-09-06 15:52:30 PDT

Created attachment 237011 [details] [diff] [review]
disable logs for suid prs. variable range check

Comment 11 Alexei Volkov 2006-09-06 15:53:52 PDT

Comment on attachment 236906 [details] [diff] [review]
proposed patch

obsolete by attachment 237011 [details] [diff] [review]

Comment 12 Nelson Bolyard (seldom reads bugmail) 2006-09-06 17:21:25 PDT

Comment on attachment 237011 [details] [diff] [review]
disable logs for suid prs. variable range check

In setuid programs, environment variables should have NO effect in 
functions _PR_InitFdCache and _PR_MD_INIT_ATOMIC.
The functions should behave as if no environment variables were set.

Regarding setting logfile to NULL, I believe Wan-Teh's point was that 
_PR_InitLog should set them to NULL in setuid programs.  
It's also good to set them to NULL in _PR_LogCleanup as your patch does.

I agree with your patch for PR_RecordTraceEntries, since it is the only
caller of function InitializeRecording.

Comment 13 Wan-Teh Chang 2006-09-06 17:31:43 PDT

Comment on attachment 237011 [details] [diff] [review]
disable logs for suid prs. variable range check

Alexei,

Since the original problem is more important than the
other problems I found by code review, please separate
the patch into two patches, so that the fix for the
original problem can be reviewed and checked in as soon
as possible.

1. io/prfdcach.c

>+    if (_pr_fd_cache.limit_low < 0)
>+        _pr_fd_cache.limit_low = 0;
>+    if (_pr_fd_cache.limit_high > FD_SETSIZE)
>+        _pr_fd_cache.limit_high = FD_SETSIZE;
>     if (_pr_fd_cache.limit_high < _pr_fd_cache.limit_low)
>         _pr_fd_cache.limit_high = _pr_fd_cache.limit_low;

_pr_fd_cache.limit_low also needs to be capped at FD_SETSIZE.
You can trace the above code with limit_low=2*FD_SETSIZE and
limit_high=0.  It will set limit_high to 2*FD_SETSIZE.

2. io/prlog.c

> void _PR_InitLog(void)
> {
>     char *ev;
> 
>     _pr_logLock = PR_NewLock();
> 
>+#ifdef XP_UNIX
>+   /* disallow setuid processes use arbitrary value from NSPR_LOG_FILE
>+    * as a log file name to prevent clobberring any file on the system. */ 
>+    if (getuid() != geteuid()) {
>+        return;
>+    }
>+#endif /* XP_UNIX */

This check should be moved right before PR_GetEnv("NSPR_LOG_FILE").
This allows logging with an explicit PR_SetLogFile call to work.

***Note: I am fine with checking in without the comment before
we announce the security vulnerability.

3. misc/pratom.c

>+		else if (num_atomic_locks < DEFAULT_ATOMIC_LOCKS) 
>+			num_atomic_locks = DEFAULT_ATOMIC_LOCKS;

The minimum number of atomic locks should be 1.

4. misc/prtrace.c

>+#include "primpl.h"
> #include "prtrace.h"
...
> #include "prerror.h"

Since "primpl.h" includes all NSPR headers, once you include primpl.h,
you don't need to include any other NSPR header.
 
>+#ifdef XP_UNIX
>+   /* disallow setuid processes use arbitrary value from NSPR_TRACE_LOG
>+    * as a log file name to prevent clobberring any file on the system. */ 
>+    if (getuid() != geteuid()) {
>+        return;
>+    }
>+#endif /* XP_UNIX */

I would move this check to InitializeRecording to be close to
the PR_GetEnv("NSPR_TRACE_LOG") call (i.e., the effect should
be as if the PR_GetEnv("NSPR_TRACE_LOG") call returned NULL).

Comment 14 Wan-Teh Chang 2006-09-06 17:40:54 PDT

The environment variables NSPR_FD_CACHE_SIZE_LOW,
NSPR_FD_CACHE_SIZE_HIGH, and NSPR_ATOMIC_HASH_LOCKS
only affect performance, so it is fine to use them
in setuid programs.

Since logFile is initialized to NULL, it's not
necessary to set logFile to NULL explicitly in
setuid programs.

Comment 15 Wan-Teh Chang 2006-09-06 17:47:07 PDT

For completeness, here are the environment variables that
NSPR checks.  I omitted the environment variables that
are only checked in debug builds or on OpenVMS (which
doesn't seem to be supported any more).

NSPR_LOG_MODULES
NSPR_LOG_FILE

NSPR_TRACE_LOG

NSPR_FD_CACHE_SIZE_LOW
NSPR_FD_CACHE_SIZE_HIGH

NSPR_ATOMIC_HASH_LOCKS

NSPR_NATIVE_THREADS_ONLY (a boolean)

NSPR_INHERIT_FDS

NSPR_USE_ZONE_ALLOCATOR (a boolean)

NSPR_OS2_NO_HIRES_TIMER (a boolean)

NSPR_AIX_SEND_FILE_USE_DISABLED (a boolean)

Comment 16 Nelson Bolyard (seldom reads bugmail) 2006-09-06 17:51:01 PDT

Comment on attachment 237011 [details] [diff] [review]
disable logs for suid prs. variable range check

Wan-Teh, I disagree that the newer issues are less important than the original one.  In particular, the fix in prtrace.c is just as important.

Comment 17 Alexei Volkov 2006-09-07 12:37:45 PDT

Created attachment 237162 [details] [diff] [review]
logs initialization fix

check process ids before PR_GetEnv calls for reasons explaned in Wan-teh comment.

remove comments(some how we need to remember to integrate them later).

Comment 18 Alexei Volkov 2006-09-07 12:41:33 PDT

Created attachment 237166 [details] [diff] [review]
performance variables fix

Comment 19 Wan-Teh Chang 2006-09-07 15:01:42 PDT

Comment on attachment 237162 [details] [diff] [review]
logs initialization fix

r=wtc.  In io/prlog.c, 

>+#ifdef XP_UNIX
>+        if (getuid() != geteuid()) {
>+            return;
>+    }
>+#endif /* XP_UNIX */

The indentation of the closing brace } is wrong.

Comment 20 Wan-Teh Chang 2006-09-07 15:17:58 PDT

Comment on attachment 237166 [details] [diff] [review]
performance variables fix

r=wtc.  Let me test this patch first (the change to
pratom.c).

Comment 21 Nelson Bolyard (seldom reads bugmail) 2006-09-07 15:45:55 PDT

Comment on attachment 237162 [details] [diff] [review]
logs initialization fix

r=nelsonb.  I was going to point out the indentation problem, but Wan-Teh beat me to it. :)

Comment 22 Alexei Volkov 2006-09-07 16:00:25 PDT

Checking in attachment 237162 [details] [diff] [review]:
/cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v  <--  prlog.c
new revision: 3.35; previous revision: 3.34
/cvsroot/mozilla/nsprpub/pr/src/misc/prtrace.c,v  <--  prtrace.c
new revision: 3.9; previous revision: 3.8

Comment 23 Wan-Teh Chang 2006-09-07 16:08:17 PDT

Comment on attachment 237166 [details] [diff] [review]
performance variables fix

Alexei, I have tested this patch on Linux.  You
can check it in now.

It turns out that the change to pratom.c is not
necessary because
- num_atomic_locks is unsigned, so a negative value
  becomes a huge unsigned value, and is capped at
  MAX_ATOMIC_LOCKS (4096), and
- PR_FloorLog2(0) is 0, so a value of 0 also gets
  set to 1.

Your patch will make the 0 case clearer (the limit of
log(0) is negative infinity, so we can avoid this tricky
case).

Comment 24 Alexei Volkov 2006-09-07 16:34:56 PDT

/cvsroot/mozilla/nsprpub/pr/src/io/prfdcach.c,v  <--  prfdcach.c
new revision: 3.13; previous revision: 3.12
/cvsroot/mozilla/nsprpub/pr/src/misc/pratom.c,v  <--  pratom.c
new revision: 3.19; previous revision: 3.18

Comment 25 Alexei Volkov 2006-09-07 17:01:10 PDT

Checked in on the 4.6 branch:

/cvsroot/mozilla/nsprpub/pr/src/io/prfdcach.c,v  <--  prfdcach.c
new revision: 3.12.2.1; previous revision: 3.12
/cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v  <--  prlog.c
new revision: 3.34.2.1; previous revision: 3.34
/cvsroot/mozilla/nsprpub/pr/src/misc/pratom.c,v  <--  pratom.c
new revision: 3.18.2.1; previous revision: 3.18
/cvsroot/mozilla/nsprpub/pr/src/misc/prtrace.c,v  <--  prtrace.c
new revision: 3.8.2.1; previous revision: 3.8

Comment 26 Wan-Teh Chang 2006-09-07 18:24:32 PDT

Comment on attachment 237162 [details] [diff] [review]
logs initialization fix

Requesting MOZILLA_1_8_BRANCH checkin approval.
I'd like to keep MOZILLA_1_8_BRANCH in synch with
the official NSPR 4.6.3 release.

Comment 27 Mike Schroepfer 2006-09-08 16:35:17 PDT

Comment on attachment 237162 [details] [diff] [review]
logs initialization fix

a=schrep for drivers.

Comment 28 Mike Schroepfer 2006-09-08 16:35:35 PDT

Comment on attachment 237166 [details] [diff] [review]
performance variables fix

a=schrep for drivers.

Comment 29 Wan-Teh Chang 2006-09-12 11:22:39 PDT

The two patches have been checked in on the NSPR trunk (NSPR 4.7),
NSPR_4_6_BRANCH (NSPR 4.6.3), NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.9 alpha),
and MOZILLA_1_8_BRANCH (Mozilla 1.8.1).

Comment 30 Wan-Teh Chang 2006-09-20 14:45:43 PDT

Since Mozilla clients are not setuid programs, this bug
doesn't affect them.  As I noted in comment 26, the reason
I wanted the fix on the MOZILLA_1_8_BRANCH is that I wanted
Firefox 2.0 to use an official NSPR release (NSPR 4.6.3).
It's not because Firefox 2.0 needs the fix.

Comment 31 Daniel Veditz [:dveditz] 2006-10-12 11:20:21 PDT

This is public, clearing the security sensitive flag:

http://sunsolve.sun.com/search/document.do?assetkey=1-26-102658-1&searchclause
http://secunia.com/advisories/22348/

Comment 32 Kees Cook 2006-11-22 14:32:51 PST

This is also CVE-2006-4842.  Figured I'd add it to the report for anyone searching  by CVE.