Closed
Bug 351470
Opened 18 years ago
Closed 18 years ago
setuid root programs linked with NSPR allow elevation of privilege (CVE-2006-4842)
Categories
(NSPR :: NSPR, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
4.6.3
People
(Reporter: nelson, Assigned: alvolkov.bgs)
Details
(Keywords: fixed1.8.1, Whiteboard: [sg:moderate] local)
Attachments
(2 files, 2 obsolete files)
3.10 KB,
patch
|
nelson
:
review+
wtc
:
superreview+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
2.25 KB,
patch
|
nelson
:
review+
wtc
:
superreview+
mtschrep
:
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•18 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•18 years ago
|
||
That approach sounds good.
Assignee | ||
Updated•18 years ago
|
Assignee: nelson → alexei.volkov.bugs
Priority: -- → P1
Assignee | ||
Comment 3•18 years ago
|
||
Attachment #236906 -
Flags: superreview?(wtchang)
Attachment #236906 -
Flags: review?(nelson)
Reporter | ||
Comment 4•18 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•18 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•18 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•18 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•18 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•18 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.
Updated•18 years ago
|
Whiteboard: [sg:moderate] local
Assignee | ||
Comment 10•18 years ago
|
||
Attachment #237011 -
Flags: superreview?(wtchang)
Attachment #237011 -
Flags: review?(nelson)
Assignee | ||
Comment 11•18 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•18 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•18 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•18 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•18 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•18 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•18 years ago
|
Target Milestone: --- → 4.6.3
Assignee | ||
Comment 17•18 years ago
|
||
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•18 years ago
|
Attachment #237162 -
Attachment description: check process ids closer to PR_GetEnv call → logs initialization fix
Assignee | ||
Comment 18•18 years ago
|
||
Attachment #237166 -
Flags: superreview?(wtchang)
Attachment #237166 -
Flags: review?(nelson)
Reporter | ||
Updated•18 years ago
|
Attachment #237166 -
Flags: review?(nelson) → review+
Comment 19•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 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•18 years ago
|
Attachment #237166 -
Flags: approval1.8.1?
Comment 27•18 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•18 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•18 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).
Updated•18 years ago
|
Flags: blocking1.8.0.8?
Comment 30•18 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.
Updated•18 years ago
|
Flags: blocking1.8.0.8? → blocking1.8.0.8-
Comment 31•18 years ago
|
||
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•18 years ago
|
||
This is also CVE-2006-4842. Figured I'd add it to the report for anyone searching by CVE.
Updated•18 years ago
|
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.
Description
•