Last Comment Bug 455987 - integer overflow in nsEscape
: integer overflow in nsEscape
Status: RESOLVED FIXED
[sg:moderate]
: fixed1.8.1.18, fixed1.9.0.4, fixed1.9.1
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: P1 critical (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
Depends on: 445117 464998
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-18 19:01 PDT by Daniel Veditz [:dveditz]
Modified: 2009-01-28 07:32 PST (History)
11 users (show)
benjamin: blocking1.9.1+
dveditz: blocking1.9.0.4+
dveditz: blocking1.8.1.18+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
prevent nsEscapeHTML overflow (3.91 KB, patch)
2008-10-22 00:59 PDT, Daniel Veditz [:dveditz]
benjamin: review+
benjamin: superreview+
Details | Diff | Splinter Review
cvs diff -w version of above (1.81 KB, patch)
2008-10-22 01:00 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review
same for places (1.05 KB, patch)
2008-10-22 01:01 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review
nits fixed, what was checked in (7.21 KB, patch)
2008-10-24 16:38 PDT, Daniel Veditz [:dveditz]
samuel.sidler+old: approval1.8.1.18+
samuel.sidler+old: approval1.9.0.4+
asac: approval1.8.0.next+
Details | Diff | Splinter Review
diff -w version of the above (2.75 KB, patch)
2008-10-24 16:38 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2008-09-18 19:01:05 PDT
+++ This bug was initially created as a clone of Bug #445117 +++

>  /* XXX Hardcoded max entity len. The +1 is for the trailing null. */
>  char *rv = (char *) nsMemory::Alloc(strlen(string) * 6 + 1);
>
> |* 6| overflows at about 683M on 32 bit platform.

The issue Georgi noted in bug 445117 also occurs in xpcom/io/nsEscape.cpp -- the same pattern happens in nsEscapeHTML() and nsEscapeHTML2()

I'm not sure if you can practically stuff 683M into the things that use nsEscapeHTML, maybe if the transport is gzipped.
http://mxr.mozilla.org/mozilla/search?string=nsEscapeHTML
Comment 1 georgi - hopefully not receiving bugspam 2008-09-19 01:32:28 PDT
dveditz you mean something different from Bug 367736 ?
Comment 2 georgi - hopefully not receiving bugspam 2008-09-19 01:36:03 PDT
aaa, i see:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsEscape.cpp#292

292   PRUnichar *resultBuffer = (PRUnichar *)nsMemory::Alloc(aSourceBufferLen *
293                             6 * sizeof(PRUnichar) + sizeof(PRUnichar('\0')));
Comment 3 georgi - hopefully not receiving bugspam 2008-09-19 01:57:37 PDT
>I'm not sure if you can practically stuff 683M

i suppose receiving it is possible.

more of problems for exploiting this probably are:

1. 683 MB on the wire consumes much more VM so OOM may be hit
2. one must avoid crash. though if one does overlapping copy (the returned ptr is in low address) it may not crash.
Comment 4 georgi - hopefully not receiving bugspam 2008-09-19 02:07:22 PDT
in theory 683MB need not be on the wire - they may be javascript generated probably
Comment 5 Daniel Veditz [:dveditz] 2008-09-19 22:40:34 PDT
Yeah, a document title in places, say, or something fed through the feed sanitizer.
Comment 6 Daniel Veditz [:dveditz] 2008-10-21 13:59:14 PDT
This code got copied to nsPlacesImportExportService.cpp, apparently because the use of string apis in the nsEscape copy caused problems?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/places/src/nsPlacesImportExportService.cpp&rev=1.65#239

Dietrich: what's the story there? Is the situation different now so this code can call the standard escape routine? Simple enough to fix in both places, but if it's no longer necessary getting rid of duplicate code would be nice.
Comment 7 Daniel Veditz [:dveditz] 2008-10-22 00:59:59 PDT
Created attachment 344247 [details] [diff] [review]
prevent nsEscapeHTML overflow

In addition to the patch I cleaned up a bunch of tabs. Will attach a -w patch next to make review easier.
Comment 8 Daniel Veditz [:dveditz] 2008-10-22 01:00:43 PDT
Created attachment 344248 [details] [diff] [review]
cvs diff -w version of above
Comment 9 Daniel Veditz [:dveditz] 2008-10-22 01:01:45 PDT
Created attachment 344249 [details] [diff] [review]
same for places
Comment 10 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2008-10-24 10:28:23 PDT
Comment on attachment 344247 [details] [diff] [review]
prevent nsEscapeHTML overflow

>Index: xpcom/io/nsEscape.cpp

>+    char *rv = nsnull;
>+    /* XXX Hardcoded max entity len. The +1 is for the trailing null. */
>+    PRUint32 len = PL_strlen(string);
>+    if (len < (PR_UINT32_MAX / 6)) {
>+        rv = (char *)nsMemory::Alloc( (len * 6) + 1 );
>+    }
>+    char *ptr = rv;
> 
>-	return(rv);
>+    if(rv)

This would be a lot clearer if you took a shortcut path:

  if (len >= PR_UINT32_MAX / 6)
    return nsnull;

  char *rv = (char*) NS_Alloc((6 * len) + 1);
  if (!rv)
    return nsnull;

  ... conversion code

This reduces the overall indent and makes the control flow more apparent.

Also please use NS_Alloc instead of nsMemory::Alloc in all new code.

r=me with those nits addressed
Comment 11 Daniel Veditz [:dveditz] 2008-10-24 16:38:23 PDT
Created attachment 344700 [details] [diff] [review]
nits fixed, what was checked in
Comment 12 Daniel Veditz [:dveditz] 2008-10-24 16:38:53 PDT
Created attachment 344701 [details] [diff] [review]
diff -w version of the above
Comment 13 Daniel Veditz [:dveditz] 2008-10-24 16:44:56 PDT
checked in: http://hg.mozilla.org/mozilla-central/rev/c6d4904201b5
Comment 14 Samuel Sidler (old account; do not CC) 2008-10-24 20:13:34 PDT
Comment on attachment 344700 [details] [diff] [review]
nits fixed, what was checked in

Approved for 1.8.1.18 and 1.9.0.4. a=ss
Comment 15 Daniel Veditz [:dveditz] 2008-10-24 22:25:04 PDT
Checked into the 1.8 and 1.9.0 branches
Comment 16 Al Billings [:abillings] 2008-11-04 12:35:44 PST
Any word on a mechanism to exercise this code as we discussed the other day, Dan?
Comment 17 Alexander Sack 2008-11-10 09:00:34 PST
Comment on attachment 344700 [details] [diff] [review]
nits fixed, what was checked in

a=asac for 1.8.0 branch
Comment 18 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-01-28 07:32:57 PST
This landed before 1.9.1 branched

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