Last Comment Bug 351470 - setuid root programs linked with NSPR allow elevation of privilege (CVE-2006-4842)
: setuid root programs linked with NSPR allow elevation of privilege (CVE-2006-...
Status: RESOLVED FIXED
[sg:moderate] local
: fixed1.8.1
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: 4.0
: All All
: P1 critical (vote)
: 4.6.3
Assigned To: Alexei Volkov
: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-05 14:11 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2015-08-19 02:04 PDT (History)
10 users (show)
dveditz: blocking1.8.0.8-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch (1.70 KB, patch)
2006-09-05 18:37 PDT, Alexei Volkov
wtc: superreview+
Details | Diff | Splinter Review
disable logs for suid prs. variable range check (5.23 KB, patch)
2006-09-06 15:52 PDT, Alexei Volkov
nelson: review-
wtc: superreview-
Details | Diff | Splinter Review
logs initialization fix (3.10 KB, patch)
2006-09-07 12:37 PDT, Alexei Volkov
nelson: review+
wtc: superreview+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review
performance variables fix (2.25 KB, patch)
2006-09-07 12:41 PDT, Alexei Volkov
nelson: review+
wtc: superreview+
mtschrep: approval1.8.1+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2006-09-05 14:11:53 PDT
I believe this affects all Unix/Linux systems, but there is no bugzilla 
platform/OS target that covers all those, so I have chosen All/All.

NSPR logging is controlled with a couple of environment variables, 
one to enable it, and a second to control the name of the log file.
This appears to all work in "optimized" (non-debug) builds.  

So, if any setuid root program is linked with NSPR, any user can clobber
any file on the system (any root writable file) by setting NSPR's 
environment variables to log to that file, and then running a setuid root
program linked with NSPR.
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.

Note You need to log in before you can comment on or make changes to this bug.