Last Comment Bug 367736 - potential sign problems in nsEscapeCount
: potential sign problems in nsEscapeCount
Status: VERIFIED FIXED
: [sg:high?]
: verified1.8.1.17, verified1.9.0.2
Product: Core
Classification: Components
Component: XPCOM
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Michal Novotny (:michal)
: xpcom
:
:
:
  Show dependency treegraph
 
Reported: 2007-01-22 06:31 PST by georgi - hopefully not receiving bugspam
Modified: 2008-09-23 21:26 PDT (History)
12 users (show)
shaver: blocking1.9.0.1-
samuel.sidler+old: blocking1.9.0.2+
samuel.sidler+old: wanted1.9.0.x+
dveditz: blocking1.8.1.17+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
samuel.sidler+old: in‑testsuite?
See Also:
Crash Signature:


Attachments
esc.html - uses 7GB VM (396 bytes, text/html)
2008-03-25 04:23 PDT, georgi - hopefully not receiving bugspam
no flags Details
fix (3.04 KB, patch)
2008-06-26 04:09 PDT, Michal Novotny (:michal)
benjamin: review+
dveditz: superreview+
dveditz: approval1.8.1.17+
dveditz: approval1.9.0.2+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Summon comment box

Description georgi - hopefully not receiving bugspam 2007-01-22 06:31:47 PST
potential sign problems in nsEscapeCount

http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsEscape.cpp#76 
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;
 85 
 [86]     int i, extra = 0;
 87     static const char hexChars[] = "0123456789ABCDEF";
 88 
 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     }
 95 
 [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|
Comment 1 georgi - hopefully not receiving bugspam 2007-01-29 00:51:58 PST
believe that can fix this. in addition, a |strlen| probably may be saved, leading to small speedup but not affecting asymptotic running time.
Comment 2 georgi - hopefully not receiving bugspam 2008-02-11 07:17:27 PST
this may be a problem even on 32 bit systems:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/io/nsEscape.cpp&rev=1.41&mark=90-96#90

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


Comment 3 georgi - hopefully not receiving bugspam 2008-03-25 04:23:52 PDT
Created attachment 311557 [details]
esc.html - uses 7GB VM
Comment 4 georgi - hopefully not receiving bugspam 2008-03-25 04:25:34 PDT
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 "�������������������������������������������������������������������..., 
    flags=url_XPAlphas)
    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
(gdb)
Comment 5 georgi - hopefully not receiving bugspam 2008-03-25 04:31:20 PDT
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...
Comment 6 Mike Schroepfer 2008-06-24 20:04:48 PDT
Michal can you take this?
Comment 7 Michal Novotny (:michal) 2008-06-26 04:09:02 PDT
Created attachment 326882 [details] [review]
fix

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, 
    aEncoded=@0x7fffbc872a30)
    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	  }
1215	
1216	  return NS_OK;
1217	}
Comment 8 georgi - hopefully not receiving bugspam 2008-06-26 04:20:05 PDT
// 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...
Comment 9 Michal Novotny (:michal) 2008-06-26 04:27:46 PDT
(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.
Comment 10 georgi - hopefully not receiving bugspam 2008-06-26 04:50:53 PDT
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:

MALLOC32(PL_strlen(str))
PL_strcpy(str,something)

just a general example
Comment 11 georgi - hopefully not receiving bugspam 2008-06-26 04:53:32 PDT
http://mxr.mozilla.org/mozilla-central/source/nsprpub/lib/libc/include/plstr.h#72
71 PR_EXTERN(PRUint32)
72 PL_strlen(const char *str);
Comment 12 georgi - hopefully not receiving bugspam 2008-07-09 04:32:24 PDT
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.
Comment 13 Samuel Sidler (old account; do not CC) 2008-08-11 15:04:56 PDT
Dan said he might have time to review this.
Comment 14 Daniel Veditz 2008-08-13 02:09:07 PDT
Comment on attachment 326882 [details] [review]
fix

sr=dveditz

Still need a module peer review, I bet shaver's busy lately so let's try bsmedberg
Comment 15 Samuel Sidler (old account; do not CC) 2008-08-14 16:01:51 PDT
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 1.8.1.17. We should also get this in our test suite.
Comment 16 georgi - hopefully not receiving bugspam 2008-08-15 00:53:33 PDT
>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.
Comment 17 Samuel Sidler (old account; do not CC) 2008-08-16 21:17:59 PDT
Comment on attachment 326882 [details] [review]
fix

I'm guessing this applies to 1.9.0.2 as well, so requesting approval there.
Comment 18 Samuel Sidler (old account; do not CC) 2008-08-17 15:51:48 PDT
Can we get this landed on trunk asap so we can look at approving for 1.9/1.8?
Comment 19 Benjamin Smedberg [:bsmedberg] 2008-08-19 07:10:26 PDT
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/076a6ab7bfe2
Comment 20 georgi - hopefully not receiving bugspam 2008-08-20 00:36:12 PDT
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.
Comment 21 georgi - hopefully not receiving bugspam 2008-08-20 05:04:20 PDT
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 22 Daniel Veditz 2008-08-20 14:57:46 PDT
Comment on attachment 326882 [details] [review]
fix

Approved for 1.8.1.17 and 1.9.0.2, a=dveditz for release-drivers.
Comment 23 Samuel Sidler (old account; do not CC) 2008-08-23 20:07:02 PDT
Dan or Benjamin, can you please check this in on both the 1.8 and 1.9.0 branches?
Comment 24 Gavin Sharp 2008-08-25 13:30:31 PDT
mozilla/xpcom/io/nsEscape.cpp 	1.42
mozilla/xpcom/io/nsEscape.cpp 	1.35.2.1 
Comment 25 Alexander Sack 2008-08-31 06:21:14 PDT
Comment on attachment 326882 [details] [review]
fix

a=asac for 1.8.0.15
Comment 26 Al Billings [:abillings] 2008-09-02 16:13:24 PDT
Bob, can you verify this fix on your 64-bit Linux VM?
Comment 27 georgi - hopefully not receiving bugspam 2008-09-03 01:28:35 PDT
i don't crash anymore on linux 64 both trunk and 1.9 branch.

peak VM usage is about 9G
Comment 28 Bob Clary [:bc:] 2008-09-03 05:31:18 PDT
(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?
Comment 29 Michal Novotny (:michal) 2008-09-03 07:25:05 PDT
I've just tested latest MOZILLA_1_8_BRANCH on 64-bit fedora9 and it didn't crash.
Comment 30 georgi - hopefully not receiving bugspam 2008-09-03 07:39:07 PDT
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
Comment 31 Bob Clary [:bc:] 2008-09-03 07:43:53 PDT
thanks Michal and georgi!
Comment 32 georgi - hopefully not receiving bugspam 2008-09-03 07:55:38 PDT
btw, quite similar unfixed overflow is 
Bug 445117 integer overlfow in MsgEscapeHTML

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