1296087 - IonCache getter stub may not correctly restore clobbered register

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Attachment: incomplete_test_case.js

There's a single typo in IonCaches.cpp that means that the object (receiver) register may be left overwritten with another object or group's memory address, with the previous value left in the stack. The circumstances required to trigger this are somewhat specific:

1. A GenerateCallGetter stub must be generated in Ion code.

2. The GenerateCallGetter stub has to be using the same register for both receiver object and scratch. Since all callers pass in the output register as scratch, this means that the receiver and output register must be the same.

3. GeneratePrototypeGuards must generate at least one prototype guard. This means that at least one object from the receiver up to (but not including) the holder must have the UNCACHEABLE_PROTO flag set. This only happens for non-singleton objects who either have had their prototype mutated in the past or were part of a prototype chain in which a downstream object's prototype was mutated.

4. The prototype of that object must be mutated after the cache stub has been generated. The next time the cache stub is used, the previously-generated guard from GeneratePrototypeGuards will fail. The problem: execution will then jump to the next stub without restoring the clobbered object register or fixing the stack.

The code is definitely incorrect, but I haven't been able to manually trigger condition (1). If I edit IonCaches.cpp to directly use the object register as scratch, the issue described above occurs. The tricky part is setting up the code so that the register allocator will reuse the receiver register as the output register. Two previous attempts at patching this area of the code demonstrate that this *can* happen (https://bugzilla.mozilla.org/show_bug.cgi?id=1033873 and https://bugzilla.mozilla.org/show_bug.cgi?id=1046597), but both of their test cases trigger it through the global object's __proto__ property. Both the global object and its prototype are singletons, and both have immutable prototypes, so they can't be used to set up the UNCACHEABLE_PROTO flag as described in condition (3).

If it *is* possible to trigger this in the real world, or if the fragile set of circumstances potentially preventing it from being triggered changes, the result (user code corrupting the stack and replacing a memory address held in a register) sounds pretty bad, so I'm marking this as security-sensitive. Fortunately the patch is trivial.

Comment 1

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Attachment: Bug 1296087: Jump to correct cache failure label

Comment 2

[Nicolas B. Pierron [:nbp]]

Today, is sounds to me that this code is reachable via the NameIC [1,2], which explicitly alias the object register as being the scratch register.  GetPropertyIC should not have this issue since the register allocator is not allowed to alias the output value-register with the object register.

The only I know to change the scope chain with custom object is by using the `with` keyword.

[1] http://searchfox.org/mozilla-central/rev/f433f0dd7158d7bfc4c4607161fc6baa88b5a9f4/js/src/jit/IonCaches.cpp#4892,4902-4904
[2] http://searchfox.org/mozilla-central/rev/f433f0dd7158d7bfc4c4607161fc6baa88b5a9f4/js/src/jit/IonCaches.cpp#5016,5022,5024

Comment 3

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

I've poked at NameIC, but it only takes this code path if |obj| is a global object [1]. This means the holder is either the global object itself or something on its prototype chain. In practice, the global object and its prototype (a) both have immutable prototypes, so a direct mutation isn't possible, and (b) are both singletons, so a mutation on something with the global object on its prototype chain won't end up propagating UNCACHEABLE_PROTO to the global object or its prototype.

If I understand correctly, |with| doesn't help a potential attacker trigger this. Is that right?

Either way, I'll try to get this checked in following the security procedures.

[1] http://searchfox.org/mozilla-central/rev/f433f0dd7158d7bfc4c4607161fc6baa88b5a9f4/js/src/jit/IonCaches.cpp#4920-4921

Comment 4

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Comment on attachment 8782136 [details] [diff] [review]
Bug 1296087: Jump to correct cache failure label

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

As per the discussion above and my own testing, I believe it may be impossible to trigger this issue in practice. This is incidental, however, and if one of several moving parts in the system were to change it could be opened up.

Turning this from a crash into an exploit would require either careful massaging of the register allocator to take advantage of the clobbered value (hard in a running browser) or taking advantage of the misaligned stack (potentially easier).

> Do comments in the patch, the check-in comment, or tests included
> in the patch paint a bulls-eye on the security problem?

Yes, the change in the patch is a few characters long and the problem is extremely obvious.

> Which older supported branches are affected by this flaw?

This issue is present in all branches dating back to at least Firefox 33.

> If not all supported branches, which bug introduced the flaw?

Bug 851109 introduced the original issue found in bug 1033873 and bug 1046597. This corrects an error in the patches for those two bugs.

> Do you have backports for the affected branches?
> If not, how different, hard to create, and risky will they be?

Trivial, it's a single typo.

> How likely is this patch to cause regressions;
> how much testing does it need?

Not likely at all, the typo fix is obviously correct.

Comment 5

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

Comment on attachment 8782136 [details] [diff] [review]
Bug 1296087: Jump to correct cache failure label

As a sec-moderate, this doesn't need sec-approval to land on trunk. You can just land it.

Comment 6

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abe23b9940e2

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Michael Smith [:mismith] from comment #3)
> I've poked at NameIC, but it only takes this code path if |obj| is a global
> object [1]. […]
> 
> If I understand correctly, |with| doesn't help a potential attacker trigger
> this. Is that right?

That sounds correct.
The |with| keyword is just the easiest way I know to put a random object on the environment chain (scope chain).

(In reply to Michael Smith [:mismith] from comment #3)
> […] This means the holder is either the global object itself or
> something on its prototype chain. In practice, the global object and its
> prototype (a) both have immutable prototypes, so a direct mutation isn't
> possible, […]

Still, I recall we had a TI issue related to the fact that the window global object of web pages was changing prototype.  Jan gave me a link to JS_SplicePrototype [2] (servo is using JS_SetPrototype).

But I agree, the global should still be a singleton.  I don't know about the prototype chain.

[1] http://searchfox.org/mozilla-central/rev/f80822840bc5f3d2d3cae3ece621ddbce72e7f54/js/src/jsfriendapi.cpp#98

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/abe23b9940e2

We should probably consider backporting this to at least some branches. Michael, can you please do the requests on that?

Comment 9

[Marcia Knous [:marcia]]

Tracking 51+ for this sec-moderate issue.

Comment 10

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

> We should probably consider backporting this to at least some branches. Michael, can you please do the requests on that?

Sure. How do I go about doing that? This is a new process for me.

Comment 11

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

> Still, I recall we had a TI issue related to the fact that the window global object of web pages was changing prototype.  Jan gave me a link to JS_SplicePrototype

JS_SplicePrototype sets class and prototype directly without going through SetClassAndProto, so it won't trigger UNCACHEABLE_PROTO. But I'm now somewhat concerned that this doesn't seem to be doing anything to invalidate ICs. Is this only used immediately after constructing objects?

> (servo is using JS_SetPrototype).

That, on the other hand, could set UNCACHEABLE_PROTO on non-singletons in the chain.

Do you know when/why we change the prototypes of global objects?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to Michael Smith [:mismith] from comment #10)
> Sure. How do I go about doing that? This is a new process for me.

Click the Details link on your attached patch. Set "approval‑mozilla‑aurora" (and whatever other branches you want to nominate it for uplift to) to ? and fill out the questions that appear in the comment field. You can fill out one set of questions for all branches at the same time (rather than doing the same thing for each branch individually).

Comment 13

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Comment on attachment 8782136 [details] [diff] [review]
Bug 1296087: Jump to correct cache failure label

> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Fixes a single typo that may not be exploitable in the wild, but if it is, could have a bad security impact.

> User impact if declined:

Risk that a way to exploit this is discovered down the road, leading to stack and register corruption with a memory address controlled by script.

> Fix Landed on Version:

Slated for Firefox 51.

> Risk to taking this patch (and alternatives if risky): 

Minimal, the patch fixes a single typo.

> String or UUID changes made by this patch: 

None.

> See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

> Approval Request Comment
> [Feature/regressing bug #]:

Bug 1296087

> [User impact if declined]:

(See above.)

> [Describe test coverage new/current, TreeHerder]:

Existing tests plus the tiny size of the patch cover the possibility of this breaking something. The issue itself that this patch protects against doesn't have a test because it may not actually be possible to trigger the bad condition in practice.

> [Risks and why]:

Little risk, the patch is a single (but important) typo fix. Worst I can think of is that, since the patch is so straightforward, it's pretty obvious what it's fixing.

> [String/UUID change made/needed]:

None.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8782136 [details] [diff] [review]
Bug 1296087: Jump to correct cache failure label

Sec fix, looks low risk, let's try this on aurora and beta 8. 
I see the reasoning but not sure I would say yes to this for esr, up to sylvestre.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8782136 [details] [diff] [review]
Bug 1296087: Jump to correct cache failure label

Doesn't match the esr criteria as it is only a sec-moderate, not taking it to esr.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4aacede9f1b3
https://hg.mozilla.org/releases/mozilla-beta/rev/90a602a48534

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Michael Smith [:mismith] from comment #11)
> > Still, I recall we had a TI issue related to the fact that the window global object of web pages was changing prototype.  Jan gave me a link to JS_SplicePrototype
> 
> JS_SplicePrototype sets class and prototype directly without going through
> SetClassAndProto, so it won't trigger UNCACHEABLE_PROTO. But I'm now
> somewhat concerned that this doesn't seem to be doing anything to invalidate
> ICs.

AFAIK, we have no mechanism for "invalidating" inline caches.  The only mechanism we have for cleaning-up is either to remove all of them, when we reach the limited the number of stubs, or when we garbage collect them.

> Is this [=JS_SplicePrototype] only used immediately after constructing objects?

That's what I recall, but bz or bhackett might know better.

> > (servo is using JS_SetPrototype).
> 
> That, on the other hand, could set UNCACHEABLE_PROTO on non-singletons in
> the chain.
> 
> Do you know when/why we change the prototypes of global objects?

I do not know.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> (servo is using JS_SetPrototype)

Not anymore, because it totally makes performance suck, as expected.  See https://github.com/servo/servo/pull/13009 and the discussion in https://github.com/servo/servo/issues/12979#issuecomment-241959407

> Is this [=JS_SplicePrototype] only used immediately after constructing objects?

Yes, JS_SplicePrototype is meant to be used immediately after constructing objects, or at least before those objects can have escaped to script.  That is the entire point of it.  If only we actually wrote docs in our API headers... :(

In practice, the only users of JS_SplicePrototype are places that create the global, and JS_NewObjectWithUniqueType; the latter is used for singletons like DOM prototypes.  In both cases the splice call happens before script should be able to get its paws on the object.

> Do you know when/why we change the prototypes of global objects?

Any time it needs to have a prototype other than Object.prototype, right?  I mean, there's a chicken-and-egg problem.  Say I want to create a window global on the web.  It's supposed to have Window.prototype as its prototype.  But to even create the Window.prototype object I need to be in the right compartment, which means I have to create the compartment, which means creating its global.  And I can't specify the right prototype at this point, because it doesn't exist yet.  So we end up just creating the global with Object.prototype as the proto, then creating the correct prototype, then using JS_SplicePrototype to set things up.

Given the JS_SetImmutablePrototype bits we do on globals now, we should be able to assume that the proto chain of a global does not mutate after that JS_SplicePrototype call, and certainly does not mutate after we might have any JIT guards on that global.

Comment 19

[Brian Hackett [Laid off!]]

JS_SplicePrototype should only be used on objects that haven't escaped to script yet.  (There's a comment to that effect in the function's implementation, though it isn't very well written.)