Closed
Bug 367736
Opened 17 years ago
Closed 15 years ago
potential sign problems in nsEscapeCount
Categories
(Core :: XPCOM, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: guninski, Assigned: michal)
Details
(Keywords: verified1.8.1.17, verified1.9.0.2, Whiteboard: [sg:high?])
Attachments
(2 files)
396 bytes,
text/html
|
Details | |
3.04 KB,
patch
|
benjamin
:
review+
dveditz
:
superreview+
dveditz
:
approval1.8.1.17+
dveditz
:
approval1.9.0.2+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
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|
Reporter | ||
Comment 1•17 years ago
|
||
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
Reporter | ||
Comment 2•16 years ago
|
||
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
Whiteboard: [sg:investigate] [sg:high?]
Updated•15 years ago
|
Assignee: nobody → cbiesinger
Flags: wanted1.9.0.x?
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.14?
Reporter | ||
Comment 3•15 years ago
|
||
Reporter | ||
Comment 4•15 years ago
|
||
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)
Reporter | ||
Comment 5•15 years ago
|
||
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...
Updated•15 years ago
|
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Updated•15 years ago
|
Flags: blocking1.9.0.1?
Flags: blocking1.8.1.16+
Flags: blocking1.8.1.15+
Flags: blocking1.9.0.1? → blocking1.9.0.1-
Comment 6•15 years ago
|
||
Michal can you take this?
Assignee | ||
Updated•15 years ago
|
Assignee: cbiesinger → michal
Assignee | ||
Comment 7•15 years ago
|
||
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 }
Attachment #326882 -
Flags: review?(shaver)
Reporter | ||
Comment 8•15 years ago
|
||
// 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...
Assignee | ||
Comment 9•15 years ago
|
||
(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.
Reporter | ||
Comment 10•15 years ago
|
||
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
Reporter | ||
Comment 11•15 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/nsprpub/lib/libc/include/plstr.h#72 71 PR_EXTERN(PRUint32) 72 PL_strlen(const char *str);
Reporter | ||
Comment 12•15 years ago
|
||
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.
Updated•15 years ago
|
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.2?
Comment 13•15 years ago
|
||
Dan said he might have time to review this.
Comment 14•15 years ago
|
||
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
Attachment #326882 -
Flags: superreview+
Attachment #326882 -
Flags: review?(shaver)
Attachment #326882 -
Flags: review?(benjamin)
Updated•15 years ago
|
Attachment #326882 -
Flags: review?(benjamin) → review+
Comment 15•15 years ago
|
||
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.
Flags: in-testsuite?
Flags: blocking1.9.0.2?
Flags: blocking1.9.0.2+
Reporter | ||
Comment 16•15 years ago
|
||
>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.
Assignee | ||
Updated•15 years ago
|
Attachment #326882 -
Flags: approval1.8.1.17?
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 17•15 years ago
|
||
Comment on attachment 326882 [details] [diff] [review] fix I'm guessing this applies to 1.9.0.2 as well, so requesting approval there.
Attachment #326882 -
Flags: approval1.9.0.2?
Comment 18•15 years ago
|
||
Can we get this landed on trunk asap so we can look at approving for 1.9/1.8?
Comment 19•15 years ago
|
||
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/076a6ab7bfe2
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 20•15 years ago
|
||
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.
Reporter | ||
Comment 21•15 years ago
|
||
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•15 years ago
|
||
Comment on attachment 326882 [details] [diff] [review] fix Approved for 1.8.1.17 and 1.9.0.2, 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+
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [sg:investigate] [sg:high?] → [sg:high?][needs check on 1.8/1.9 branches]
Comment 23•15 years ago
|
||
Dan or Benjamin, can you please check this in on both the 1.8 and 1.9.0 branches?
Comment 24•15 years ago
|
||
mozilla/xpcom/io/nsEscape.cpp 1.42 mozilla/xpcom/io/nsEscape.cpp 1.35.2.1
Whiteboard: [sg:high?][needs check on 1.8/1.9 branches] → [sg:high?]
Comment 25•15 years ago
|
||
Comment on attachment 326882 [details] [diff] [review] fix a=asac for 1.8.0.15
Attachment #326882 -
Flags: approval1.8.0.15+
Updated•15 years ago
|
Flags: blocking1.8.0.15+
Comment 26•15 years ago
|
||
Bob, can you verify this fix on your 64-bit Linux VM?
Reporter | ||
Comment 27•15 years ago
|
||
i don't crash anymore on linux 64 both trunk and 1.9 branch. peak VM usage is about 9G
Comment 28•15 years ago
|
||
(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?
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.2 → verified1.9.0.2
Assignee | ||
Comment 29•15 years ago
|
||
I've just tested latest MOZILLA_1_8_BRANCH on 64-bit fedora9 and it didn't crash.
Reporter | ||
Comment 30•15 years ago
|
||
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
Reporter | ||
Comment 32•15 years ago
|
||
btw, quite similar unfixed overflow is Bug 445117 integer overlfow in MsgEscapeHTML
Updated•15 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•