849014 - IonMonkey: Crash [@ js::RegExpGuard::operator*] or "Assertion failure: isRegExp(),"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Map.prototype.has = /x/.test
x = [false]
y = Map()
Array.prototype.forEach.call(x, (function() {
    y.has('')
}))

asserts js debug shell on m-c changeset 71395a927025 with --ion-eager at Assertion failure: isRegExp(), and crashes js opt shell at a weird memory address with js::RegExpGuard::operator* on the stack. Another weird memory address 0x00000001006942bb is also on the stack.

Locking s-s pending further security analysis, assuming sec-critical since a weird memory address is on the stack.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   109162:d41ca12d2527
user:        Nicolas B. Pierron
date:        Wed Oct 03 15:13:40 2012 -0700
summary:     Bug 792220 - Remove lookupProperty to prevent interpreter reentrance. r=jandem

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

nbp, is bug 792220 a possible regressor?

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Check type inference before optimizing regexp calls.

js::regexp_test_raw assume that the object given as argument is a RegExp object, as the assertion hit in debug mode illustrate.  This function is only used by IonMonkey which inline calls to regexp_test & regexp_exec.

The only detail is that this optimization did not check for the class attach with the known object.  This patch check if there is a common class to all objects listed in the Type Object and ensure that all are RegExp objects before optimizing the call site.

This means that call-site with a different kind of object will not inline the regexp_test function call, and it will fallback on a fallible JS native call which will throw an exception when it got executed.

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 724225 [details] [diff] [review]
Check type inference before optimizing regexp calls.

Review of attachment 724225 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Thank you for fixing this.

Comment 5

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

This needs a security rating and sec-approval on the patch before any checkin here.

Gary, when you pin down a regression to a specific timeframe, please mark the status flags for affected versions if you can.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The regressing bug did not land on 17. Guessing at wontfix for 19.

Comment 7

[Nicolas B. Pierron [:nbp]]

Set as sec-critical as any private data of an object can be interpreted as a RegExp object, and decision such as compiling can be taken based on injected data.  So I think there is a *complex* vector of attack hidden behind the lack of this check.

Comment 8

[Nicolas B. Pierron [:nbp]]

Comment on attachment 724225 [details] [diff] [review]
Check type inference before optimizing regexp calls.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- Find that this is only related to a few functions calls from Ion.
- Look what happen when something is not a regexp flow into these functions. [make it crash]
- Find which fields are set by other structure which might be read as a RegExp and change the behaviour in an unexpected way.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- Just the code by it-self.

Which older supported branches are affected by this flaw?
- All branches with IonMonkey enabled.

If not all supported branches, which bug introduced the flaw?
- Landing IonMonkey, on Sep 12.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- Not yet, should be easy to make if there is any issue.

How likely is this patch to cause regressions; how much testing does it need?
- The patch is unlikely to cause any regressions as it add another condition to enable the optimization, and the default will fallback to a slower call mechanism which is already well tested.
- The patch use the type info which is already known and used, and it add a check for the known class of a StackTypeSet and handle all its possible outputs correctly.

Comment 9

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

Please create branch patches and nominate them since this is a sec-critical issue.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Critsmash requests a landing for this since sec-approval+ has been given, thanks.

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: [aurora / beta] Same patch.

Same patch as attachment 724225 [details] [diff] [review], backported to aurora and beta branches.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Landing IonMonkey (Sep 12, fx 18)
User impact if declined: crash, potential injection of bad values into the regexp execution / compiler ?!
Testing completed (on m-c, etc.): Not yet.
Risk to taking this patch (and alternatives if risky): extremely low (see comment 8)
String or UUID changes made by this patch: N/A

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> Critsmash requests a landing for this since sec-approval+ has been given,
> thanks.

Tell me when this is best to land knowing the security issue involved in this Bug.
As 19 is not considered, I think it is best to wait until the next train shift.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> > Critsmash requests a landing for this since sec-approval+ has been given,
> > thanks.
> 
> Tell me when this is best to land knowing the security issue involved in
> this Bug.
> As 19 is not considered, I think it is best to wait until the next train
> shift.

We should have green on inbound, then on central, then at least a day later will the approvals for branches usually be granted.

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 725082 [details] [diff] [review]
[aurora / beta] Same patch.

This patch does not compile because getKnownClass does not exists.  If think we can inline it if needed, unless I can find a substitute for it.

Comment 15

[Nicolas B. Pierron [:nbp]]

getKnownClass has been added as part of Bug 832360, I am looking for a substitute for it.

Comment 16

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

(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> > Critsmash requests a landing for this since sec-approval+ has been given,
> > thanks.
> 
> Tell me when this is best to land knowing the security issue involved in
> this Bug.
> As 19 is not considered, I think it is best to wait until the next train
> shift.

Once sec-approval+ is given, that is the sign that it is time to land on trunk unless some caveats are given (such as "Don't check this in until date X"). That's why we have sec-approval in the first place.

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Al Billings [:abillings] from comment #16)
> (In reply to Nicolas B. Pierron [:nbp] from comment #12)
> > (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> > > Critsmash requests a landing for this since sec-approval+ has been given,
> > > thanks.
> > 
> > Tell me when this is best to land knowing the security issue involved in
> > this Bug.
> > As 19 is not considered, I think it is best to wait until the next train
> > shift.
> 
> Once sec-approval+ is given, that is the sign that it is time to land on
> trunk unless some caveats are given (such as "Don't check this in until date
> X"). That's why we have sec-approval in the first place.

Ok, I am still working on the backport version first, as the backported version will be different.

Comment 18

[Nicolas B. Pierron [:nbp]]

Attachment: [aurora/beta] Check regexp object before evaluating regexp.

This patch differ from the previous one as the getKnownClass function is not yet introduced in aurora and beta branches.

Instead of checking ahead of time if the class of the function is correct or not, we just check at run-time if the object we got is really a RegExp or not.  If this is not a regexp then we fall back to the usual path to generate an exception.

Sadly the generated exception is not identical to what is expected or seen usually, but this is still better than crashing the browser.  I not sure if there is any safe way to recover the correct "test" JSFunction, instead of the NullValue.  So in the mean time I kept the NullValue as a low-risk approach.

Comment 19

[Alex Keybl [:akeybl]]

This needs to land on central no later than Monday, and Aurora/Beta on Tuesday in order to be considered for FF20.

Comment 20

[Sean Stangl [:sstangl]]

Comment on attachment 725144 [details] [diff] [review]
[aurora/beta] Check regexp object before evaluating regexp.

Review of attachment 725144 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/RegExp.cpp
@@ +679,5 @@
> +
> +    Value vp[3];
> +    AutoArrayRooter rooter(cx, 3, vp);
> +    JS::CallArgs args = JS::CallArgsFromVp(1, vp);
> +    args.setCallee(NullValue()); // will cause a different error message.

Use "incorrect" instead of "different", but it should still be fine.

@@ +683,5 @@
> +    args.setCallee(NullValue()); // will cause a different error message.
> +    args.setThis(ObjectValue(*regexp));
> +    args[0] = StringValue(input);
> +
> +    JSBool res = js::regexp_test(cx, 3, vp);

Add comment noting that this is called so that it throws.

Comment 21

[Nicolas B. Pierron [:nbp]]

Comment on attachment 725144 [details] [diff] [review]
[aurora/beta] Check regexp object before evaluating regexp.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Landing IonMonkey (Sep 12, fx 18)

User impact if declined: crash, potential injection of bad values into the regexp execution / compiler ?!

Testing completed (on m-c, etc.): No testing on mc as the patch is totally different.

Risk to taking this patch (and alternatives if risky):
- This patch is a bit risky, but instead of preventing any bad inputs flowing into the bad function, it check the input inside the called function and emulate a call to the default function.
- [decoder] This patch introduce a *differential testing* bug by returning
“TypeError: null is not a function” instead of “TypeError: test method called on incompatible *”

String or UUID changes made by this patch: N/A

Comment 22

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/730fbb35edd5

Comment 23

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 09f72f45a0b7).

Comment 24

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Fix should have landed:

http://hg.mozilla.org/mozilla-central/rev/730fbb35edd5

Comment 25

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 26

[Nicolas B. Pierron [:nbp]]

Comment on attachment 725144 [details] [diff] [review]
[aurora/beta] Check regexp object before evaluating regexp.

gkw, decoder: I am confident about this patch, but as this patch does not apply on central can you fuzz this patch for beta, to ensure there is no additional errors in this patch.

Comment 27

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 725144 [details] [diff] [review]
[aurora/beta] Check regexp object before evaluating regexp.

This doesn't apply cleanly on mozilla-beta - I've verified this with Nicolas beside me.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Do we need an updated patch for aurora/beta uplift here?  We're going to build with our fifth week beta tomorrow and this should really get into that build.  Please update/provide options for getting this code in.

Comment 29

[Nicolas B. Pierron [:nbp]]

(In reply to Lukas Blakk [:lsblakk] from comment #28)
> Do we need an updated patch for aurora/beta uplift here?  We're going to
> build with our fifth week beta tomorrow and this should really get into that
> build.  Please update/provide options for getting this code in.

Apparently my trees were not up to date :(

Checking with recent trees, verified against tbpl. Aurora can use attachment 724225 [details] [diff] [review], and Beta will need slightly modified version of attachment 725144 [details] [diff] [review].  I am working on it.

Comment 30

[Nicolas B. Pierron [:nbp]]

Attachment: [beta] Check regexp object before evaluating regexp.

Keep the review from attachment 725144 [details] [diff] [review], as this is identical except that it reuse regexp_test_raw which has been added instead of adding it, but getKnownClass is still not backported to beta, so attachment 724225 [details] [diff] [review] will not work.

gkw, decoder: This patch should apply fine on beta.  Can you check with this one instead.  I verified that the test case is fixed by this patch.

Comment 31

[Nicolas B. Pierron [:nbp]]

Comment on attachment 724225 [details] [diff] [review]
Check type inference before optimizing regexp calls.

[Approval Request Comment]
see comment 11

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Resetting the flags that were wiped in comment 29

Comment 33

[Christian Holler (:decoder)]

I'm testing this on beta now, with special focus on RegExp. So far nothing found.

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 726355 [details] [diff] [review]
[beta] Check regexp object before evaluating regexp.

Given nothing found yet - let's get this on beta now so we know it will be in our fifth week's beta going to build later today.

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 725144 [details] [diff] [review]
[aurora/beta] Check regexp object before evaluating regexp.

Correct me if I'm reading this bug wrong, but I believe this patch needs only aurora approval (applies cleanly) and the other patch I approved covers the beta branch.

Comment 36

[Nicolas B. Pierron [:nbp]]

Comment on attachment 725144 [details] [diff] [review]
[aurora/beta] Check regexp object before evaluating regexp.

(In reply to Lukas Blakk [:lsblakk] from comment #35)
> Comment on attachment 725144 [details] [diff] [review]
> [aurora/beta] Check regexp object before evaluating regexp.
> 
> Correct me if I'm reading this bug wrong, but I believe this patch needs
> only aurora approval (applies cleanly) and the other patch I approved covers
> the beta branch.

Sorry, I forgot to mark this patch as obsolete, as described in comment 29.
Attachment 726355 [details] [diff] contains the modified version of this patch.

I reset the aurora approval as this patch is obsolete.

Comment 37

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 726355 [details] [diff] [review]
[beta] Check regexp object before evaluating regexp.

I have not found anything after a round of overnight fuzzing on beta. feedback+

Comment 38

[Nicolas B. Pierron [:nbp]]

Comment on attachment 724225 [details] [diff] [review]
Check type inference before optimizing regexp calls.

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd540c741136

Comment 39

[Nicolas B. Pierron [:nbp]]

Comment on attachment 726355 [details] [diff] [review]
[beta] Check regexp object before evaluating regexp.

https://hg.mozilla.org/releases/mozilla-beta/rev/e1633ced70b0

Comment 40

[Christian Holler (:decoder)]

Comment on attachment 726355 [details] [diff] [review]
[beta] Check regexp object before evaluating regexp.

No issues found over night.