setuid root programs linked with NSPR allow elevation of privilege (CVE-2006-4842)

RESOLVED FIXED in 4.6.3

Status

NSPR
NSPR
P1
critical
RESOLVED FIXED
11 years ago
2 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Alexei Volkov)

Tracking

({fixed1.8.1})

4.6.3
fixed1.8.1
Bug Flags:
blocking1.8.0.8 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] local)

Attachments

(2 attachments, 2 obsolete attachments)

3.10 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Wan-Teh Chang
: superreview+
Mike Schroepfer
: approval1.8.1+
Details | Diff | Splinter Review
2.25 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Wan-Teh Chang
: superreview+
Mike Schroepfer
: approval1.8.1+
Details | Diff | Splinter Review
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.
(Reporter)

Comment 1

11 years ago
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?
Assignee: wtchang → nelson

Comment 2

11 years ago
That approach sounds good.
(Assignee)

Updated

11 years ago
Assignee: nelson → alexei.volkov.bugs
Priority: -- → P1
(Assignee)

Comment 3

11 years ago
Created attachment 236906 [details] [diff] [review]
proposed patch
Attachment #236906 - Flags: superreview?(wtchang)
Attachment #236906 - Flags: review?(nelson)
(Reporter)

Comment 4

11 years ago
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

11 years ago
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

11 years ago
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
Attachment #236906 - Flags: superreview?(wtchang) → superreview+

Comment 7

11 years ago
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).
(Reporter)

Comment 8

11 years ago
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.
(Reporter)

Comment 9

11 years ago
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.
Whiteboard: [sg:moderate] local
(Assignee)

Comment 10

11 years ago
Created attachment 237011 [details] [diff] [review]
disable logs for suid prs. variable range check
Attachment #237011 - Flags: superreview?(wtchang)
Attachment #237011 - Flags: review?(nelson)
(Assignee)

Comment 11

11 years ago
Comment on attachment 236906 [details] [diff] [review]
proposed patch

obsolete by attachment 237011 [details] [diff] [review]
Attachment #236906 - Attachment is obsolete: true
Attachment #236906 - Flags: review?(nelson)
(Reporter)

Comment 12

11 years ago
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.
Attachment #237011 - Flags: review?(nelson) → review-

Comment 13

11 years ago
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).
Attachment #237011 - Flags: superreview?(wtchang)
Attachment #237011 - Flags: superreview-
Attachment #237011 - Flags: review?(nelson)
Attachment #237011 - Flags: review-

Comment 14

11 years ago
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

11 years ago
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)
(Reporter)

Comment 16

11 years ago
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.
Attachment #237011 - Flags: review?(nelson) → review-
(Reporter)

Updated

11 years ago
Target Milestone: --- → 4.6.3
(Assignee)

Comment 17

11 years ago
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).
Attachment #237011 - Attachment is obsolete: true
Attachment #237162 - Flags: superreview?(wtchang)
Attachment #237162 - Flags: review?(nelson)
(Assignee)

Updated

11 years ago
Attachment #237162 - Attachment description: check process ids closer to PR_GetEnv call → logs initialization fix
(Assignee)

Comment 18

11 years ago
Created attachment 237166 [details] [diff] [review]
performance variables fix
Attachment #237166 - Flags: superreview?(wtchang)
Attachment #237166 - Flags: review?(nelson)
(Reporter)

Updated

11 years ago
Attachment #237166 - Flags: review?(nelson) → review+

Comment 19

11 years ago
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.
Attachment #237162 - Flags: superreview?(wtchang) → superreview+

Comment 20

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

r=wtc.  Let me test this patch first (the change to
pratom.c).
Attachment #237166 - Flags: superreview?(wtchang) → superreview+
(Reporter)

Comment 21

11 years ago
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. :)
Attachment #237162 - Flags: review?(nelson) → review+
(Assignee)

Comment 22

11 years ago
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

11 years ago
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).
(Assignee)

Comment 24

11 years ago
/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
(Assignee)

Comment 25

11 years ago
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

11 years ago
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.
Attachment #237162 - Flags: approval1.8.1?

Updated

11 years ago
Attachment #237166 - Flags: approval1.8.1?

Comment 27

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

a=schrep for drivers.
Attachment #237162 - Flags: approval1.8.1? → approval1.8.1+

Comment 28

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

a=schrep for drivers.
Attachment #237166 - Flags: approval1.8.1? → approval1.8.1+

Comment 29

11 years ago
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).
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Flags: blocking1.8.0.8?

Comment 30

11 years ago
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.
Flags: blocking1.8.0.8? → blocking1.8.0.8-
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/
Group: security

Comment 32

11 years ago
This is also CVE-2006-4842.  Figured I'd add it to the report for anyone searching  by CVE.
Summary: setuid root programs linked with NSPR allow elevation of privilege → setuid root programs linked with NSPR allow elevation of privilege (CVE-2006-4842)
You need to log in before you can comment on or make changes to this bug.