929261 - (CVE-2013-5615) GetElementIC typed array stub doesn't property respect monitoredResult()

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Eric Faust [:efaust]]

We should not generate typed array stubs (or any IC stub, for that matter) that know a priori that both they could generate a type that is not in the observed typesets, and that they will not be monitored.

We even have a flag for just this purpose in GetElementIC...we just seem to blissfully ignore it.

This works great, until the whole house burns down. I am currently standing in the ashes of my former home.

This manifests with the patch in 923717 applied, on asm.js/testZOOB.js.

Marking s-s, since this causes fairly random behavior, as it runs afoul of our optimization scheme.

Don't know how old it is, guessing at least through 25...

Comment 1

[Eric Faust [:efaust]]

CC shu for discussion of analagous issues in GetElementParIC

Comment 2

[Eric Faust [:efaust]]

So, I should make a more general statement about how this was found.

Running testZOOB.js with --ion-eager leads TI to believe that the access to the typed array will /always/ result in |undefined|, so instead of a type barrier, we just push and use |undefined| as an "optimization" (we still have to run the cache code, for bailouts). Unfortunately, one of the typed array accesses yields 0, which is not monitored, and so we don't invalidate and the |undefined| is used instead, leading to incorrectness.

Comment 3

[Eric Faust [:efaust]]

Attachment: fixTATI.patch

Shu came up with a better plan that involving TI directly.

Why not just not generate stubs for the out of bounds case. This imposes the invariant that we will always monitor the success type at stub-generation time. Since out of bounds accessses fall back anyways, they are covered by default.

Cute.

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 820648 [details] [diff] [review]
fixTATI.patch

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

Looks good.

::: js/src/jit/IonCaches.cpp
@@ +3119,5 @@
> +
> +    if (!idval.isInt32() && !idval.isString())
> +        return false;
> +
> +

Nit: extra newline

@@ +3120,5 @@
> +    if (!idval.isInt32() && !idval.isString())
> +        return false;
> +
> +
> +    // Don't emit a stub if the access is out of bounds. We make to make

s/make to//

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 820648 [details] [diff] [review]
fixTATI.patch

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

The patch does a pretty good job hilighting the problem, but setting up the exact situation took a combination of non-release flags being set in testing. It's remarkably unlikely to hit this in the wild. Even our fuzzers missed it.

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

A little. It's clear from the comments the problem we are fixing, but weaponization, as stated above, is highly non-trivial.

Which older supported branches are affected by this flaw?

I just looked, and it looks like it should affect release as well, though I haven't directly checked by testing. Probably we can bank that the exploit is hard enough to find that we can WONTFIX it there, though.

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

It's difficult to say for sure between the ICs and TI, but I think bug 851792 introduced the dubious, now corrected, logic.


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

I don't have them at present. They should be reasonably straightforward to create. Since they involve generating slightly fewer IC stubs, the main concern is performance. There is very little risk of correctness issue.

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

It should be performance tested, especially on uplifted branches.

Comment 6

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

Comment on attachment 820648 [details] [diff] [review]
fixTATI.patch

sec-approval+ for checkin but not until November 12. We're just making final release builds for Firefox 25 next week so this needs to wait a couple of weeks to go in.

Also, if you can't prepare Aurora and Beta patches, someone should. If this affects current release, this also affects ESR24 so it should get a patch there as well.

Comment 7

[Andrew McCreight [:mccr8]]

This can land now.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e0352fd12d8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

This applies to Aurora/Beta without issue. Needs some fixing up for esr24.

Comment 10

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

Comment on attachment 820648 [details] [diff] [review]
fixTATI.patch

Approving for Aurora and Beta. Someone please get it in and make (and nominate) an ESR24 patch.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e0352fd12d8

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f115afa37597
https://hg.mozilla.org/releases/mozilla-beta/rev/80d76b93040f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/80d76b93040f

Comment 14

[Tracy Walker [:tracy]]

Is there anything QA can verify here?

Comment 15

[Eric Faust [:efaust]]

Attachment: esr24-tati.patch

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

It's been known affected, and the only reason we didn't push it already is that a patch didn't apply.
User impact if declined:
Security concern. 
Fix Landed on Version:
trunk, aurora, beta
Risk to taking this patch (and alternatives if risky): 
very small. Performance, if anything.
String or UUID changes made by this patch: 
None
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Eric Faust [:efaust]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/a35e169462e8

Comment 17

[Matt Wobensmith [:mwobensmith][:matt:]]

Tracy - comment 14 - w/r/t verification, there appear to be a few things here. 

One is confirming that the actual security problem has been mitigated. 

Also - based on comment 5 - this could be tested for correctness and performance. 

Based on comments above, it appears that the threat is obscure enough that creating - and verifying - via a test would probably not yield much benefit. 

I assume correctness would be covered in a test suite somewhere. Comment 2 indicates that a test exists that can expose this behavior and (theoretically) catch any regression in the future. Eric, is this true? Do we have this covered?

Performance is another thing, and I'm not aware of what regression tests we have for gauging that impact. Based on this, I don't think we can do much to verify pre-release, but we should have a way to understand or measure the performance impact of this and other similar changes in general.

Comment 18

[Tracy Walker [:tracy]]

ok, thanks Matt.

Comment 19

[Eric Faust [:efaust]]

(In reply to Matt Wobensmith from comment #17)
> Tracy - comment 14 - w/r/t verification, there appear to be a few things
> here. 
> 
> One is confirming that the actual security problem has been mitigated. 
> 
> Also - based on comment 5 - this could be tested for correctness and
> performance. 
> 
> Based on comments above, it appears that the threat is obscure enough that
> creating - and verifying - via a test would probably not yield much benefit. 
> 

I believe this is true.

> I assume correctness would be covered in a test suite somewhere. Comment 2
> indicates that a test exists that can expose this behavior and
> (theoretically) catch any regression in the future. Eric, is this true? Do
> we have this covered?
>

Yeah, it's in the suite, and a few more things in the pipeline will hammer it soon. It's just hard to get the compiler to want to generate this particular way of handling the array access.
 
> Performance is another thing, and I'm not aware of what regression tests we
> have for gauging that impact. Based on this, I don't think we can do much to
> verify pre-release, but we should have a way to understand or measure the
> performance impact of this and other similar changes in general.

It shouldn't have noticeable performance impacts. Most people do not do well-timed out of bounds accesses and then suddenly switch to doing in-bounds ones.

I think we are OK, from a QA point of view.

Comment 20

[Matt Wobensmith [:mwobensmith][:matt:]]

Super, thanks for the info, Eric. Much appreciated.