Bug 458679 (CVE-2008-5502)

Crash [@ js_DeflateString]

VERIFIED FIXED

Status

()

defect
--
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: gkw, Assigned: igor)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.5 +
wanted1.8.1.x -
wanted1.8.0.x -
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(9 attachments, 4 obsolete attachments)

Reporter

Description

11 years ago
Posted file 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.
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
Assignee

Updated

11 years ago
Assignee: general → igor
Assignee

Comment 1

11 years ago
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.
Assignee

Comment 2

11 years ago
Posted patch fix v1 (obsolete) — Splinter Review
The patch adds checks to GetXMLEntity to verify that the string buffer state after each FastAppendChar.
Attachment #341907 - Flags: review?(crowder)
Assignee

Comment 3

11 years ago
Posted patch fix v2Splinter Review
The new version has stricter assert for error reporting in GetXMLEntity.
Attachment #341907 - Attachment is obsolete: true
Attachment #341908 - Flags: review?(crowder)
Attachment #341907 - Flags: review?(crowder)

Updated

11 years ago
Attachment #341908 - Flags: review?(crowder) → review+

Updated

11 years ago
Whiteboard: [sg:critical?]
Assignee

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Reporter

Comment 5

11 years ago
Posted patch patch for 1.9.0.x (obsolete) — Splinter Review
Patch portage to Gecko 1.9.0.x.
Attachment #343521 - Flags: review?(igor)
Reporter

Comment 6

11 years ago
Posted patch patch for 1.8.1.x (obsolete) — Splinter Review
Patch portage for 1.8.1.x
Attachment #343522 - Flags: review?(igor)
Reporter

Comment 7

11 years ago
Posted file diff between trunk and both patches (obsolete) —
Here's a diff of the backported patches against the trunk patch to hopefully prevent more bad backports..
Reporter

Comment 8

11 years ago
note: I did the |cvs diff| from the js/src/ directory..
Reporter

Comment 9

11 years ago
The testcase in comment #0 / comment #1 applies only to the trunk, for the testsuite. They are not branch testcases.
Flags: in-testsuite?
Assignee

Updated

11 years ago
Attachment #343521 - Flags: review?(igor) → review+
Assignee

Updated

11 years ago
Attachment #343522 - Flags: review?(igor) → review+
Reporter

Updated

11 years ago
Attachment #343521 - Flags: approval1.9.0.4?
Reporter

Updated

11 years ago
Attachment #343522 - Flags: approval1.8.1.18?
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.
Attachment #343521 - Flags: approval1.9.0.4? → approval1.9.0.5?
Reporter

Comment 11

11 years ago
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

11 years ago
Posted file 1.9.0.x testcase
This crashes my 1.9.0.x debug js shell.
Reporter

Comment 13

11 years ago
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.
Attachment #343521 - Attachment is obsolete: true
Attachment #343776 - Flags: review+
Attachment #343776 - Flags: approval1.9.0.4?
Attachment #343521 - Flags: approval1.9.0.5?
Reporter

Comment 14

11 years ago
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
Reporter

Comment 15

11 years ago
Comment on attachment 343521 [details] [diff] [review]
patch for 1.9.0.x

review+ brought over to updated patch.
Attachment #343521 - Flags: review+
Reporter

Updated

11 years ago
Attachment #343522 - Attachment is obsolete: true
Attachment #343522 - Flags: review+
Attachment #343522 - Flags: approval1.8.1.18?
Reporter

Comment 16

11 years ago
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.
Reporter

Comment 17

11 years ago
Updating flags and nominating blocking1.9.0.4? due to discovery of 1.9.0.x branch testcase.
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
Flags: blocking1.9.0.4?
Flags: blocking1.9.0.4? → blocking1.9.0.5?
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.
Attachment #343776 - Flags: review+
Attachment #343776 - Flags: approval1.9.0.4?
Reporter

Updated

11 years ago
Attachment #343523 - Attachment is obsolete: true
Reporter

Comment 19

11 years ago
Comment on attachment 343776 [details] [diff] [review]
patch for 1.9.0.x - v2

Re-requesting review as requested.
Attachment #343776 - Flags: review?(igor)
Assignee

Updated

11 years ago
Attachment #343776 - Flags: review?(igor) → review+
Flags: blocking1.9.0.5? → blocking1.9.0.5+
Reporter

Updated

11 years ago
Attachment #343776 - Flags: approval1.9.0.5?
Flags: wanted1.8.1.x-
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)
Attachment #343776 - Flags: approval1.9.0.5? → approval1.9.0.5+
Assignee

Comment 21

11 years ago
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.
Reporter

Comment 22

11 years ago
(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/
Keywords: checkin-needed
Assignee

Comment 23

11 years ago
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
Keywords: fixed1.9.0.5
Reporter

Comment 24

11 years ago
$ ./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.
Status: RESOLVED → VERIFIED
Reporter

Updated

11 years ago
Keywords: checkin-needed

Comment 25

11 years ago
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?
Status: VERIFIED → RESOLVED
Last Resolved: 11 years ago11 years ago
Assignee

Comment 26

11 years ago
(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.

Updated

11 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
Reporter

Comment 30

11 years ago
Bob, verified?

Comment 31

11 years ago
(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?
Assignee

Comment 32

11 years ago
(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

11 years ago
Posted file Sample on Mac OS X
shell terminates normally with no error, browser hangs.
Assignee

Comment 34

11 years ago
(In reply to comment #33)
> Created an attachment (id=348218) [details]

Hm, how do I read this to get a stack trace?

Comment 35

11 years ago
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

11 years ago
Igor, is this any better?

Comment 37

11 years ago
David, the not found test in bug 462939 is from this bug.
BC, can you verify this fix for 1.9.0.5?

Comment 39

11 years ago
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
Reporter

Comment 40

11 years ago
(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

11 years ago
filed Bug 468058

Comment 42

11 years ago
verified mozilla-central, tracemonkey
Status: RESOLVED → VERIFIED

Comment 43

11 years ago
not for 1.8.0
Flags: wanted1.8.0.x-
Alias: CVE-2008-5502
Group: core-security
Crash Signature: [@ js_DeflateString]
You need to log in before you can comment on or make changes to this bug.