Buffer overrun in NSPR logging

RESOLVED FIXED in 4.3

Status

NSPR
NSPR
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.54 KB, patch
Details | Diff | Splinter Review
Problem:
export NSPR_LOG_MODULES=`perl -e 'print "A"x10000'`
mozilla
run-mozilla.sh: line 72:  2074 Floating point exception$prog ${1+"$@"}
mozilla -g
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1024 (LWP 2093)]
0x41414141 in ?? ()
(gdb) 
0x41 == 'A'

Georgi Guninski
(Reporter)

Updated

16 years ago
Assignee: mstoltz → wtc
Component: Security: General → NSPR
Product: Browser → NSPR
QA Contact: bsharma → wtc
(Reporter)

Comment 1

16 years ago
->NSPR.
(Assignee)

Comment 2

16 years ago
Created attachment 94268 [details] [diff] [review]
Proposed patch

This is an off-by-one error.  We tried to prevent
buffer overrun by specifying a width for the %[]
conversion, but the width we specify is the size
of the buffer, which does not leave any space for
the terminating null byte.

The fix is to specify a width of the size of the
buffer minus one.

I also took the opportunity to use the EOF macro
instead of the constant -1.  This is a code cleanup
and has nothing to do with this bug.

Please review this patch.
(Reporter)

Comment 3

16 years ago
Comment on attachment 94268 [details] [diff] [review]
Proposed patch

Is it possible to use the DEFAULT_BUF_SIZE macro as part of the sscanf format
string? That way, if the buffer size is ever changed, the sscanf operation will
pick up the change automatically. If this is not practical, please add a
comment where the buffer size is defined that says "Security-Critical: If you
change this size, you must also change the sscanf format strings to be size-1."

With that change, r=mstoltz.
Attachment #94268 - Flags: review+
(Assignee)

Comment 4

16 years ago
Created attachment 94574 [details] [diff] [review]
Proposed patch v2

It's rather tricky to construct the sscanf format strings
from a macro.  (I would need to play with the preprocessor
stringize operator.)  So I just added the comments you
suggested.
Attachment #94268 - Attachment is obsolete: true
(Assignee)

Comment 5

16 years ago
Fix checked into the tip and NSPRPUB_PRE_4_2_CLIENT_BRANCH
of NSPR.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.3
(Reporter)

Comment 6

15 years ago
Does NSPR_LOG_MODULES only work in debug builds?

Did this change make it into NS 7.01 (Blackbird)?
(Assignee)

Comment 7

15 years ago
NSPR_LOG_MODULES also works in optimized builds if the
code is compiled with the macro FORCE_PR_LOG defined.
I don't know if any Mozilla code is compiled with
FORCE_PR_LOG defined.

This change is not in NETSCAPE_7_01_RTM_RELEASE.
Removing confidential flag for bugs fixed over a year ago
Group: security
You need to log in before you can comment on or make changes to this bug.