367736 - potential sign problems in nsEscapeCount

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[georgi - hopefully not receiving bugspam]

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]

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]

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]

Attachment: esc.html - uses 7GB VM

Comment 4

[georgi - hopefully not receiving bugspam]

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]

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]

Michal can you take this?

Comment 7

[Michal Novotny [:michal]]

Attachment: 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]

// 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]]

(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]

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]

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]

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)]

Dan said he might have time to review this.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 326882 [details] [diff] [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)]

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]

>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)]

Comment on attachment 326882 [details] [diff] [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)]

Can we get this landed on trunk asap so we can look at approving for 1.9/1.8?

Comment 19

[Benjamin Smedberg]

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/076a6ab7bfe2

Comment 20

[georgi - hopefully not receiving bugspam]

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]

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 [:dveditz]]

Comment on attachment 326882 [details] [diff] [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)]

Dan or Benjamin, can you please check this in on both the 1.8 and 1.9.0 branches?

Comment 24

[:Gavin Sharp [email: [e-mail]]]

mozilla/xpcom/io/nsEscape.cpp 	1.42
mozilla/xpcom/io/nsEscape.cpp 	1.35.2.1 

Comment 25

[Alexander Sack]

Comment on attachment 326882 [details] [diff] [review]
fix

a=asac for 1.8.0.15

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Bob, can you verify this fix on your 64-bit Linux VM?

Comment 27

[georgi - hopefully not receiving bugspam]

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] (inactive)]

(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]]

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]

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] (inactive)]

thanks Michal and georgi!

Comment 32

[georgi - hopefully not receiving bugspam]

btw, quite similar unfixed overflow is 
Bug 445117 integer overlfow in MsgEscapeHTML