Closed Bug 849014 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox20 + fixed
firefox21 + fixed
firefox22 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update][adv-main20+])

Crash Data

Attachments

(3 files, 2 obsolete files)

Attached file 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.
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
Blocks: 792220
nbp, is bug 792220 a possible regressor?
Flags: needinfo?(nicolas.b.pierron)
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.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Attachment #724225 - Flags: review?(sstangl)
Flags: needinfo?(nicolas.b.pierron)
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.
Attachment #724225 - Flags: review?(sstangl) → review+
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.
The regressing bug did not land on 17. Guessing at wontfix for 19.
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.
Keywords: sec-critical
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.
Attachment #724225 - Flags: sec-approval?
Attachment #724225 - Flags: sec-approval? → sec-approval+
Please create branch patches and nominate them since this is a sec-critical issue.
Critsmash requests a landing for this since sec-approval+ has been given, thanks.
Flags: needinfo?(nicolas.b.pierron)
Attached patch [aurora / beta] Same patch. (obsolete) — Splinter Review
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
Attachment #725082 - Flags: approval-mozilla-beta?
Attachment #725082 - Flags: approval-mozilla-aurora?
Flags: needinfo?(nicolas.b.pierron)
(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.
(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 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.
Attachment #725082 - Attachment is obsolete: true
Attachment #725082 - Flags: approval-mozilla-beta?
Attachment #725082 - Flags: approval-mozilla-aurora?
getKnownClass has been added as part of Bug 832360, I am looking for a substitute for it.
Depends on: 832360
(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.
(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.
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.
Attachment #725144 - Flags: review?(sstangl)
This needs to land on central no later than Monday, and Aurora/Beta on Tuesday in order to be considered for FF20.
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.
Attachment #725144 - Flags: review?(sstangl) → review+
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
Attachment #725144 - Flags: approval-mozilla-beta?
Attachment #725144 - Flags: approval-mozilla-aurora?
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 09f72f45a0b7).
Fix should have landed:

http://hg.mozilla.org/mozilla-central/rev/730fbb35edd5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update]
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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.
Attachment #725144 - Flags: feedback?(gary)
Attachment #725144 - Flags: feedback?(choller)
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.
Attachment #725144 - Flags: feedback?(gary)
Attachment #725144 - Flags: feedback?(choller)
Attachment #725144 - Flags: feedback-
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.
Flags: needinfo?(nicolas.b.pierron)
(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.
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.
Attachment #726355 - Flags: review+
Attachment #726355 - Flags: feedback?(gary)
Attachment #726355 - Flags: feedback?(choller)
Attachment #726355 - Flags: approval-mozilla-beta?
Comment on attachment 724225 [details] [diff] [review]
Check type inference before optimizing regexp calls.

[Approval Request Comment]
see comment 11
Attachment #724225 - Flags: approval-mozilla-aurora?
I'm testing this on beta now, with special focus on RegExp. So far nothing found.
Attachment #724225 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #726355 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #725144 - Flags: approval-mozilla-beta?
Attachment #725144 - Flags: approval-mozilla-beta-
Attachment #725144 - Flags: approval-mozilla-aurora?
Attachment #725144 - Flags: approval-mozilla-aurora+
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.
Attachment #725144 - Attachment is obsolete: true
Attachment #725144 - Flags: approval-mozilla-aurora+
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+
Attachment #726355 - Flags: feedback?(gary) → feedback+
Comment on attachment 724225 [details] [diff] [review]
Check type inference before optimizing regexp calls.

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd540c741136
Comment on attachment 726355 [details] [diff] [review]
[beta] Check regexp object before evaluating regexp.

https://hg.mozilla.org/releases/mozilla-beta/rev/e1633ced70b0
Comment on attachment 726355 [details] [diff] [review]
[beta] Check regexp object before evaluating regexp.

No issues found over night.
Attachment #726355 - Flags: feedback?(choller) → feedback+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main20+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.