Closed Bug 367736 Opened 14 years ago Closed 12 years ago

potential sign problems in nsEscapeCount


(Core :: XPCOM, defect)

Not set





(Reporter: guninski, Assigned: michal)


(Keywords: verified1.8.1.17, verified1.9.0.2, Whiteboard: [sg:high?])


(2 files)

potential sign problems in nsEscapeCount 
76 static char* nsEscapeCount(
 77     const char * str,
 [78]     PRInt32 len,
 79     nsEscapeMask flags,
 80     PRInt32* out_len)
 81 //----------------------------------------------------------------------------------------
 82 {
 83     if (!str)
 84         return 0;
 [86]     int i, extra = 0;
 87     static const char hexChars[] = "0123456789ABCDEF";
 89     register const unsigned char* src = (const unsigned char *) str;
 [90]     for (i = 0; i < len; i++)
 91     {
 92         if (!IS_OK(*src++))
 [93]             extra += 2; /* the escape, plus an extra byte for each nibble */
 94     }
 [96]     char* result = (char *)nsMemory::Alloc(len + extra + 1);

a potentia issue here, probably exploitable on 64
bit systems.

1. [76] and [86] are signed. if len == 0x80000000 == 1 << 31 the loop
is not entered and [96] allocates just (1<<31) +1 luckily positive to
avoid sign promotion.

2. for len == 0x55555555 (about 1365MB) and always doing += 2
the allocation evaluates to zero meaning trouble. 
for len > 0x55555555 sign also causes trouble.

potential exploit via feeding a string via the gopher protocol seems
unfeasible: doesn't succeed after 12 hours on a maching with 1.5G RAM.

proposed solution:
fixing the sign and checking for overflow of |extra += 2|
believe that can fix this. in addition, a |strlen| probably may be saved, leading to small speedup but not affecting asymptotic running time.
Assignee: dveditz → nobody
Component: Security → XPCOM
Product: Mozilla Application Suite → Core
QA Contact: seamonkey → xpcom
this may be a problem even on 32 bit systems:

in [1] "luckily positive" may be wrong, though this doesn't seem right:

char* result = (char *)nsMemory::Alloc(len + extra + 1);

on 32 bit system in theory it will overflow at about 1.4GB (testing buries me in a panglo loop)

on 64 bit system basically this causes trouble:

(unsigned long)( (int) 1 + (int)-1) == 0

Whiteboard: [sg:investigate] [sg:high?]
Assignee: nobody → cbiesinger
Flags: wanted1.9.0.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.14?
this is a real problem on 64 bit systems with about 8GB virtual memory.
esc.html takes about 7GB VM and about 30 seconds to execute on machine with 8G RAM.

#5  0x00002b6dea59f89d in nsEscapeCount (
    str=0x2aac52005018 "�������������������������������������������������������������������..., 
    len=1610612736, flags=url_XPAlphas, out_len=0x0)
    at /opt/joro/firefox-cvs/mozilla/xpcom/io/nsEscape.cpp:115
#6  0x00002b6dea59f9f2 in nsEscape (
    str=0x2aac52005018 "�������������������������������������������������������������������..., 
    at /opt/joro/firefox-cvs/mozilla/xpcom/io/nsEscape.cpp:147
#7  0x0000000000f6384f in nsFSURLEncoded::URLEncode (this=0x25e9da0, 
    aStr=@0x7fffc0f215e0, aEncoded=@0x7fffc0f21720)
    at /opt/joro/firefox-cvs/mozilla/content/html/content/src/nsFormSubmission.cpp:586
#8  0x0000000000f63a36 in nsFSURLEncoded::AddNameValuePair
(gdb) frame 5
#5  0x00002b6dea59f89d in nsEscapeCount (
    str=0x2aac52005018 "�������������������������������������������������������������������..., 
    len=1610612736, flags=url_XPAlphas, out_len=0x0)
    at /opt/joro/firefox-cvs/mozilla/xpcom/io/nsEscape.cpp:115
115                                     *dst++ = hexChars[c & 0x0f];    /* low nibble */
(gdb) x/4x dst
0x2aab54003000: Cannot access memory at address 0x2aab54003000
(gdb) p/x len
$1 = 0x60000000
(gdb) p/x extra
$2 = 0xc0000000
(gdb) p/x extra + len
$3 = 0x20000000
(gdb) x/4x dst-16
0x2aab54002ff0: 0x42254645      0x46422546      0x25464525      0x42254642
the memory requirement almost sure can be lowered.

probably if one sacrifices network traffic in ftp/gopher (hitting nsEscapeCount).

or some better math in Z/2^32Z avoiding sign promotion...
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Flags: blocking1.9.0.1?
Flags: blocking1.8.1.16+
Flags: blocking1.8.1.15+
Flags: blocking1.9.0.1? → blocking1.9.0.1-
Michal can you take this?
Assignee: cbiesinger → michal
Attached patch fixSplinter Review
I removed strlen() in nsEscape(), because whole string must be searched in nsEscapeCount() anyway, so the length is calculated there.

All calculations are done with size_t type. If more that 4GB is needed, nsEscapeCount() fails because of problem with nsMemoryAllocate().

I also found out that if nsEscape() fails when submitting form, error is propagated only to frame 4 where it is ignored. I'm not sure if it isn't potential security problem to post incomplete form.

#0  nsEscape (str=0x7fffbc872840 "���", flags=url_XPAlphas)
    at /opt/moz/TRUNK/mozilla/xpcom/io/nsEscape.cpp:163
#1  0x00002aaaab7806a3 in nsFSURLEncoded::URLEncode (this=0x2aaabb727ca0, aStr=@0x7fffbc872910, 
    at /opt/moz/TRUNK/mozilla/content/html/content/src/nsFormSubmission.cpp:586
#2  0x00002aaaab780861 in nsFSURLEncoded::AddNameValuePair (this=0x2aaabb727ca0, 
    aSource=0x2aaabbdfd8e0, aName=@0x7fffbc872d90, aValue=@0x7fffbc872c50)
    at /opt/moz/TRUNK/mozilla/content/html/content/src/nsFormSubmission.cpp:350
#3  0x00002aaaab7ad286 in nsHTMLInputElement::SubmitNamesValues (this=0x2aaabbdfd860, 
    aFormSubmission=0x2aaabb727ca0, aSubmitElement=0x0)
    at /opt/moz/TRUNK/mozilla/content/html/content/src/nsHTMLInputElement.cpp:2578
#4  0x00002aaaab797520 in nsHTMLFormElement::WalkFormElements (this=0x2aaaba21a9e0, 
    aFormSubmission=0x2aaabb727ca0, aSubmitElement=0x0)
    at /opt/moz/TRUNK/mozilla/content/html/content/src/nsHTMLFormElement.cpp:1213
(gdb) f 4
#4  0x00002aaaab797520 in nsHTMLFormElement::WalkFormElements (this=0x2aaaba21a9e0, 
    aFormSubmission=0x2aaabb727ca0, aSubmitElement=0x0)
    at /opt/moz/TRUNK/mozilla/content/html/content/src/nsHTMLFormElement.cpp:1213
1213	    sortedControls[i]->SubmitNamesValues(aFormSubmission, aSubmitElement);
(gdb) l
1208	  // Walk the list of nodes and call SubmitNamesValues() on the controls
1209	  //
1210	  PRUint32 len = sortedControls.Length();
1211	  for (PRUint32 i = 0; i < len; ++i) {
1212	    // Tell the control to submit its name/value pairs to the submission
1213	    sortedControls[i]->SubmitNamesValues(aFormSubmission, aSubmitElement);
1214	  }
1216	  return NS_OK;
1217	}
Attachment #326882 - Flags: review?(shaver)
// calls NS_Alloc_P(size_t) which calls PR_Malloc(PRUint32), so there is
// no chance to allocate more than 4GB using nsMemory::Alloc()

this casting size_t to int32 may be a problem on 64 bit systems...
(In reply to comment #8)

IMHO the only problem is that if you ask e.g. for 5GB of memory you'll get 1GB. And of course you don't know it. That's why there is a check against PR_UINT32_MAX.

I'll file another bug about this.
i didn't write about the case in this bug, i wrote in general.

string C functions PL_str... use int32. so basically this is potentially buggy:


just a general example
does anyone know if there is a bug on the PL_str* stuff ?

i have 8G ram and 8G swap, yet have trouble creating strings >4G because of memory usage...

this may be a problem if 64bit version are officially released - linux vendors do this.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.2?
Dan said he might have time to review this.
Comment on attachment 326882 [details] [diff] [review]


Still need a module peer review, I bet shaver's busy lately so let's try bsmedberg
Attachment #326882 - Flags: superreview+
Attachment #326882 - Flags: review?(shaver)
Attachment #326882 - Flags: review?(benjamin)
Attachment #326882 - Flags: review?(benjamin) → review+
If we're going to block on this (which I think we should), we need both a 1.8 and 1.9 branch patch for it. Marking blocking for now since it's blocking We should also get this in our test suite.
Flags: in-testsuite?
Flags: blocking1.9.0.2?
Flags: blocking1.9.0.2+
>We should also get this in our test suite.

the testcase will use *a lot* of VM, so unless the testing server has about 8G ram i doubt it should be included in the automated tests.
Attachment #326882 - Flags: approval1.8.1.17?
Keywords: checkin-needed
Comment on attachment 326882 [details] [diff] [review]

I'm guessing this applies to as well, so requesting approval there.
Attachment #326882 - Flags: approval1.9.0.2?
Can we get this landed on trunk asap so we can look at approving for 1.9/1.8?
Pushed to mozilla-central:
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
branch still crashes with the testcase.

64bit linux trunk crashes on startup at the moment, so can't test it on 64 bit trunk now.
this seems fixed on trunk.

after completely leaving the testcase, memory usage goes to about 2.5G and doesn't seem to drop - this may be a leak.
Comment on attachment 326882 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers.
Attachment #326882 - Flags: approval1.9.0.2?
Attachment #326882 - Flags: approval1.9.0.2+
Attachment #326882 - Flags: approval1.8.1.17?
Attachment #326882 - Flags: approval1.8.1.17+
Keywords: checkin-needed
Whiteboard: [sg:investigate] [sg:high?] → [sg:high?][needs check on 1.8/1.9 branches]
Dan or Benjamin, can you please check this in on both the 1.8 and 1.9.0 branches?
mozilla/xpcom/io/nsEscape.cpp 	1.42
Whiteboard: [sg:high?][needs check on 1.8/1.9 branches] → [sg:high?]
Comment on attachment 326882 [details] [diff] [review]

a=asac for
Attachment #326882 - Flags: approval1.8.0.15+
Flags: blocking1.8.0.15+
Bob, can you verify this fix on your 64-bit Linux VM?
i don't crash anymore on linux 64 both trunk and 1.9 branch.

peak VM usage is about 9G
(In reply to comment #26)
> Bob, can you verify this fix on your 64-bit Linux VM?

Unfortunately not. I converted my workstation to ESXi and don't have a 64bit VM set up on it yet and even when I do get one, it will be difficult to test this case since the VM won't be able to use 8G. Unfortunately, I was not able to convince others I needed more ram when configuring the system. The 64bit VM on lb-dell02 only as 2G and can't be used in this circumstance either.

georgi's verification is sufficient.

georgi: can you verify on the 1.8.1 branch?
I've just tested latest MOZILLA_1_8_BRANCH on 64-bit fedora9 and it didn't crash.
verified on 1.8 branch - no crash.

though i get warnings in a clean profile:

WARNING: NS_ENSURE_TRUE(escapedBuf) failed, file /opt/pub/firefox-branch181/mozilla/content/html/content/src/nsFormSubmission.cpp, line 589
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file /opt/pub/firefox-branch181/mozilla/content/html/content/src/nsFormSubmission.cpp, line 363
WARNING: ERROR: Form history file is corrupt, now deleting it., file /opt/pub/firefox-branch181/mozilla/toolkit/components/satchel/src/nsFormHistory.cpp, line 458
thanks Michal and georgi!
btw, quite similar unfixed overflow is 
Bug 445117 integer overlfow in MsgEscapeHTML
Group: core-security
You need to log in before you can comment on or make changes to this bug.