Bug 929261 (CVE-2013-5615)

GetElementIC typed array stub doesn't property respect monitoredResult()

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

({regression, sec-high})

unspecified
mozilla28
x86_64
Linux
regression, sec-high
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 wontfix, firefox25 wontfix, firefox26 fixed, firefox27 fixed, firefox28 fixed, firefox-esr2426+ fixed, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed)

Details

(Whiteboard: [adv-main26+][adv-esr24.2+][qa-])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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...
Group: mozilla-corporation-confidential → core-security
(Assignee)

Comment 1

5 years ago
CC shu for discussion of analagous issues in GetElementParIC
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 820648 [details] [diff] [review]
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.
Attachment #820648 - Flags: review?(shu)

Comment 4

5 years ago
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//
Attachment #820648 - Flags: review?(shu) → review+
(Assignee)

Comment 5

5 years ago
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.
Attachment #820648 - Flags: sec-approval?
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.
Attachment #820648 - Flags: sec-approval? → sec-approval+
status-b2g18: --- → ?
status-firefox24: --- → wontfix
status-firefox-esr24: --- → affected
Whiteboard: [checkin after 11/12]
This can land now.
This applies to Aurora/Beta without issue. Needs some fixing up for esr24.
status-firefox25: affected → wontfix
status-firefox28: --- → fixed
Flags: needinfo?(efaustbmo)
Keywords: branch-patch-needed
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.
Attachment #820648 - Flags: approval-mozilla-beta+
Attachment #820648 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/1e0352fd12d8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Is there anything QA can verify here?
Keywords: verifyme

Updated

5 years ago
status-b2g-v1.1hd: --- → ?

Updated

5 years ago
tracking-firefox-esr24: --- → 26+
Keywords: checkin-needed
Whiteboard: [adv-main26+]
(Assignee)

Comment 15

5 years ago
Created attachment 8341475 [details] [diff] [review]
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.
Attachment #8341475 - Flags: approval-mozilla-esr24?
Flags: needinfo?(efaustbmo)
Keywords: branch-patch-needed, checkin-needed
Attachment #8341475 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
(Assignee)

Comment 16

5 years ago
https://hg.mozilla.org/releases/mozilla-esr24/rev/a35e169462e8
status-firefox-esr24: affected → fixed
Alias: CVE-2013-5615
Whiteboard: [adv-main26+] → [adv-main26+][adv-esr24.2+]
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.
ok, thanks Matt.
Keywords: verifyme
Whiteboard: [adv-main26+][adv-esr24.2+] → [adv-main26+][adv-esr24.2+][qa-]
(Assignee)

Comment 19

5 years ago
(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.
Super, thanks for the info, Eric. Much appreciated.
Blocks: 851792
status-b2g18: ? → unaffected
status-b2g-v1.1hd: ? → unaffected
Keywords: regression
Group: core-security
You need to log in before you can comment on or make changes to this bug.