Last Comment Bug 458679 - (CVE-2008-5502) Crash [@ js_DeflateString]
(CVE-2008-5502)
: Crash [@ js_DeflateString]
Status: VERIFIED FIXED
[sg:critical?]
: assertion, crash, testcase, verified1.9.0.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: jsfunfuzz
  Show dependency treegraph
 
Reported: 2008-10-05 23:51 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2011-06-13 10:01 PDT (History)
12 users (show)
dveditz: blocking1.9.0.5+
dveditz: wanted1.8.1.x-
asac: wanted1.8.0.x-
bob: in‑testsuite+
bob: in‑litmus-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
crash log (3.02 KB, text/plain)
2008-10-05 23:51 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
fix v1 (1.86 KB, patch)
2008-10-06 04:51 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
fix v2 (1.92 KB, patch)
2008-10-06 04:54 PDT, Igor Bukanov
crowderbt: review+
Details | Diff | Splinter Review
patch for 1.9.0.x (2.03 KB, patch)
2008-10-17 01:29 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details | Diff | Splinter Review
patch for 1.8.1.x (2.03 KB, patch)
2008-10-17 01:29 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details | Diff | Splinter Review
diff between trunk and both patches (2.13 KB, text/plain)
2008-10-17 01:31 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
1.9.0.x testcase (310 bytes, text/javascript)
2008-10-18 22:40 PDT, Jesse Ruderman
no flags Details
patch for 1.9.0.x - v2 (1.98 KB, patch)
2008-10-18 23:09 PDT, Gary Kwong [:gkw] [:nth10sd]
igor: review+
dveditz: approval1.9.0.5+
Details | Diff | Splinter Review
e4x/Regress/regress-458679-01.js (2.11 KB, text/plain)
2008-11-12 13:33 PST, Bob Clary [:bc:]
no flags Details
e4x/Regress/regress-458679-02.js (2.37 KB, text/plain)
2008-11-12 13:34 PST, Bob Clary [:bc:]
no flags Details
js1_7/extensions/regress-458679.js (2.56 KB, text/plain)
2008-11-12 13:35 PST, Bob Clary [:bc:]
no flags Details
Sample on Mac OS X (69.03 KB, text/plain)
2008-11-14 10:49 PST, Bob Clary [:bc:]
no flags Details
pstack output on centos5 32bit (5.35 KB, text/plain)
2008-11-14 20:23 PST, Bob Clary [:bc:]
no flags Details

Description Gary Kwong [:gkw] [:nth10sd] 2008-10-05 23:51:05 PDT
Created attachment 341891 [details]
crash log

function f()
{
  for (var i = 1; i < dps.length; ++i) {
    var a = "";
    var b = "";
    var c = "";
  }
}

function stringOfLength(n)
{
  if (n == 0) {
    return "";
  } else if (n == 1) {
    return "\"";
  } else {
    var r = n % 2;
    var d = (n - r) / 2;
    var y = stringOfLength(d);
    return y + y + stringOfLength(r);
  }    
}


this.__defineGetter__('x', this.toSource);
while (x.length < 12000000) { 
  let q = x;
  s = q + q; 
}
print(x.length);
<e4x>{x}</e4x>;


Huge thanks to Jesse Ruderman for helping to reduce this massive testcase. These lines crash js trunk shell at 0x0000000006b30000.

This also asserts debug shell at "Assertion failure: nbytes != 0, at jsapi.cpp:1841". Related to bug 421623 or bug 457521?

Nominating security sensitive because they crash at this scary address. Nominating wanted-1.9.0.x as well because we have seen variants of this testcase crashing 1.9.0.x.
Comment 1 Igor Bukanov 2008-10-06 04:43:43 PDT
Here is a simplified test case:

~/m/31-ff/js/src $ cat ~/s/x.js
var x = "<";

while (x.length < 12000000)
    x += x;

<e4x>{x}</e4x>;
~/m/31-ff/js/src $ ./Linux_All_DBG.OBJ/js ~/s/x.js
segmentation fault

The reason for the bug is that GetXMLEntity assumes that FastAppendChar is infallible and continues to read characters from the buffer.
Comment 2 Igor Bukanov 2008-10-06 04:51:41 PDT
Created attachment 341907 [details] [diff] [review]
fix v1

The patch adds checks to GetXMLEntity to verify that the string buffer state after each FastAppendChar.
Comment 3 Igor Bukanov 2008-10-06 04:54:32 PDT
Created attachment 341908 [details] [diff] [review]
fix v2

The new version has stricter assert for error reporting in GetXMLEntity.
Comment 4 Igor Bukanov 2008-10-10 06:17:43 PDT
landed - http://hg.mozilla.org/mozilla-central/rev/f7838da9f1a8
Comment 5 Gary Kwong [:gkw] [:nth10sd] 2008-10-17 01:29:08 PDT
Created attachment 343521 [details] [diff] [review]
patch for 1.9.0.x

Patch portage to Gecko 1.9.0.x.
Comment 6 Gary Kwong [:gkw] [:nth10sd] 2008-10-17 01:29:52 PDT
Created attachment 343522 [details] [diff] [review]
patch for 1.8.1.x

Patch portage for 1.8.1.x
Comment 7 Gary Kwong [:gkw] [:nth10sd] 2008-10-17 01:31:50 PDT
Created attachment 343523 [details]
diff between trunk and both patches

Here's a diff of the backported patches against the trunk patch to hopefully prevent more bad backports..
Comment 8 Gary Kwong [:gkw] [:nth10sd] 2008-10-17 01:34:00 PDT
note: I did the |cvs diff| from the js/src/ directory..
Comment 9 Gary Kwong [:gkw] [:nth10sd] 2008-10-17 02:47:15 PDT
The testcase in comment #0 / comment #1 applies only to the trunk, for the testsuite. They are not branch testcases.
Comment 10 Daniel Veditz [:dveditz] 2008-10-17 10:14:49 PDT
Comment on attachment 343521 [details] [diff] [review]
patch for 1.9.0.x

Since there's no verification testcase for the branches we're nervous taking this at the end of the cycle. Maybe next time.
Comment 11 Gary Kwong [:gkw] [:nth10sd] 2008-10-18 22:05:45 PDT
I've discovered a 1.9.0.x testcase that crashes at js_DeflateString (not an easy one to find, required lots of CPU time) which I've just sent to Jesse for further reduction. Will comment when it's done.
Comment 12 Jesse Ruderman 2008-10-18 22:40:43 PDT
Created attachment 343772 [details]
1.9.0.x testcase

This crashes my 1.9.0.x debug js shell.
Comment 13 Gary Kwong [:gkw] [:nth10sd] 2008-10-18 23:09:39 PDT
Created attachment 343776 [details] [diff] [review]
patch for 1.9.0.x - v2

This one applies cleanly and resolves the crash. The shell now shows "typein:17: InternalError: script stack space quota is exhausted" instead of crashing. Bringing over review. Re-requesting approval1.9.0.4 now that the testcase is found.
Comment 14 Gary Kwong [:gkw] [:nth10sd] 2008-10-18 23:11:28 PDT
diff for 1.9.0.x patch v2:

$ diff getxml_entity_458679.patch patch-v2.diff 
1c1
< Index: 31-ff/js/src/jsscan.cpp
---
> Index: js/src/jsscan.c
3,5c3,8
< --- 31-ff.orig/js/src/jsscan.cpp	2008-10-06 13:45:09.000000000 +0200
< +++ 31-ff/js/src/jsscan.cpp	2008-10-06 13:53:30.000000000 +0200
< @@ -809,23 +809,27 @@ GetXMLEntity(JSContext *cx, JSTokenStrea
---
> RCS file: /cvsroot/mozilla/js/src/jsscan.c,v
> retrieving revision 3.147
> diff -u -8 -p -r3.147 jsscan.c
> --- js/src/jsscan.c	4 Aug 2008 20:51:34 -0000	3.147
> +++ js/src/jsscan.c	19 Oct 2008 05:39:40 -0000
> @@ -808,23 +808,27 @@ GetXMLEntity(JSContext *cx, JSTokenStrea
33c36
< @@ -901,16 +905,18 @@ GetXMLEntity(JSContext *cx, JSTokenStrea
---
> @@ -900,16 +904,18 @@ GetXMLEntity(JSContext *cx, JSTokenStrea
Comment 15 Gary Kwong [:gkw] [:nth10sd] 2008-10-18 23:12:23 PDT
Comment on attachment 343521 [details] [diff] [review]
patch for 1.9.0.x

review+ brought over to updated patch.
Comment 16 Gary Kwong [:gkw] [:nth10sd] 2008-10-18 23:20:22 PDT
Comment on attachment 343522 [details] [diff] [review]
patch for 1.8.1.x

More comprehensive testing showed 1.8.1.x branch isn't affected by this at all - obsoleting patch. The shell works as expected.
Comment 17 Gary Kwong [:gkw] [:nth10sd] 2008-10-18 23:21:49 PDT
Updating flags and nominating blocking1.9.0.4? due to discovery of 1.9.0.x branch testcase.
Comment 18 Daniel Veditz [:dveditz] 2008-10-20 12:11:09 PDT
Comment on attachment 343776 [details] [diff] [review]
patch for 1.9.0.x - v2

Would you mind getting an independent review on the port -- we don't want any bad branch merges and there are some differences in the branch version of the engine that might have hidden implications. Either Igor or Brian is fine.

We're closing down 1.9.0.4, so request approval on the next review.
Comment 19 Gary Kwong [:gkw] [:nth10sd] 2008-10-20 19:39:07 PDT
Comment on attachment 343776 [details] [diff] [review]
patch for 1.9.0.x - v2

Re-requesting review as requested.
Comment 20 Daniel Veditz [:dveditz] 2008-11-03 11:42:00 PST
Comment on attachment 343776 [details] [diff] [review]
patch for 1.9.0.x - v2

approved for 1.9.0.5, a=dveditz for release-drivers

(please watch tinderbox for the tree to open before landing)
Comment 21 Igor Bukanov 2008-11-12 01:02:27 PST
To Gary: will you land the 1.9.0.* patch? The code freeze for 1.9.0.5 is Monday, November 17 at 11:59pm PST.
Comment 22 Gary Kwong [:gkw] [:nth10sd] 2008-11-12 01:03:43 PST
(In reply to comment #21)
> To Gary: will you land the 1.9.0.* patch? The code freeze for 1.9.0.5 is
> Monday, November 17 at 11:59pm PST.

Igor, I'd love to, but I don't have commit access to js/
Comment 23 Igor Bukanov 2008-11-12 01:46:06 PST
I landed the patch from the comment 13 to 1.9.0 branch:

Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.148; previous revision: 3.147
done
Comment 24 Gary Kwong [:gkw] [:nth10sd] 2008-11-12 01:55:49 PST
$ ./js-opt-moz190-intelmac 
js> var x = "<";
js> 
js> while (x.length < 12000000)
    x += x;

(lots of output spew containing "<")

js> 
js> <e4x>{x}</e4x>;
typein:6: InternalError: script stack space quota is exhausted
js> 


Now it shows the "InternalError: script stack space quota is exhausted" error message instead of crashing.

Thanks Igor! Verified.
Comment 25 Bob Clary [:bc:] 2008-11-12 12:59:00 PST
Igor, the InternalError: script stack space quota is exhausted is thrown for the test cases in 1.9.1 but not in 1.8.1. Is the exception expected on 1.9.1?
Comment 26 Igor Bukanov 2008-11-12 13:06:21 PST
(In reply to comment #25)
> Igor, the InternalError: script stack space quota is exhausted is thrown for
> the test cases in 1.9.1 but not in 1.8.1. Is the exception expected on 1.9.1?

The exception is expected on 1.9.* as the XML parser uses script stack quota space in 1.9.* and the space is limited to prevent DOS.
Comment 27 Bob Clary [:bc:] 2008-11-12 13:33:35 PST
Created attachment 347843 [details]
e4x/Regress/regress-458679-01.js
Comment 28 Bob Clary [:bc:] 2008-11-12 13:34:18 PST
Created attachment 347844 [details]
e4x/Regress/regress-458679-02.js
Comment 29 Bob Clary [:bc:] 2008-11-12 13:35:13 PST
Created attachment 347845 [details]
js1_7/extensions/regress-458679.js
Comment 30 Gary Kwong [:gkw] [:nth10sd] 2008-11-12 18:30:14 PST
Bob, verified?
Comment 31 Bob Clary [:bc:] 2008-11-14 10:23:26 PST
(In reply to comment #29)
> Created an attachment (id=347845) [details]
> js1_7/extensions/regress-458679.js

This test appears to hang the browser but not the shell. Igor?
Comment 32 Igor Bukanov 2008-11-14 10:35:48 PST
(In reply to comment #31)
> This test appears to hang the browser but not the shell. Igor?

Hm, do you mean that the shell terminates with some out-of-something exception while browser simply hangs? If so, could try to capture a stack trace at some moment when the browser busy executing something?
Comment 33 Bob Clary [:bc:] 2008-11-14 10:49:13 PST
Created attachment 348218 [details]
Sample on Mac OS X

shell terminates normally with no error, browser hangs.
Comment 34 Igor Bukanov 2008-11-14 12:25:01 PST
(In reply to comment #33)
> Created an attachment (id=348218) [details]

Hm, how do I read this to get a stack trace?
Comment 35 Jesse Ruderman 2008-11-14 13:54:42 PST
It's a sample of a hang, so instead of a single stack trace, you see a tree representing hundreds of stack traces.
Comment 36 Bob Clary [:bc:] 2008-11-14 20:23:13 PST
Created attachment 348309 [details]
pstack output on centos5 32bit

Igor, is this any better?
Comment 37 Bob Clary [:bc:] 2008-11-21 17:36:55 PST
David, the not found test in bug 462939 is from this bug.
Comment 38 Al Billings [:abillings] 2008-12-01 15:27:07 PST
BC, can you verify this fix for 1.9.0.5?
Comment 39 Bob Clary [:bc:] 2008-12-02 08:19:45 PST
verified no crash 1.9.0.5 shell/browser e4x/Regress/regress-458679-01.js, e4x/Regress/regress-458679-02.js, no crash in shell/browser js1_7/extensions/regress-458679.js but the test does hang/timeout in the browser. testsed on mac os x 10.5, centos5, winxp
Comment 40 Gary Kwong [:gkw] [:nth10sd] 2008-12-02 20:21:56 PST
(In reply to comment #39)
> verified no crash 1.9.0.5 shell/browser e4x/Regress/regress-458679-01.js,
> e4x/Regress/regress-458679-02.js, no crash in shell/browser
> js1_7/extensions/regress-458679.js but the test does hang/timeout in the
> browser. testsed on mac os x 10.5, centos5, winxp

Bob, perhaps you should file the hang as a new bug for Igor?
Comment 41 Bob Clary [:bc:] 2008-12-05 04:24:11 PST
filed Bug 468058
Comment 42 Bob Clary [:bc:] 2008-12-06 03:19:05 PST
verified mozilla-central, tracemonkey
Comment 43 Alexander Sack 2008-12-16 11:27:31 PST
not for 1.8.0
Comment 44 Bob Clary [:bc:] 2009-01-14 08:20:07 PST
tests checked in http://hg.mozilla.org/mozilla-central/rev/98e0818f1bb4 as well as cvs.

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