Assertion failure: MIR instruction returned value with unexpected type, at jit/IonMacroAssembler.cpp:1223

VERIFIED FIXED in Firefox 27, Firefox OS v1.2

Status

()

Core
JavaScript Engine: JIT
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks: 1 bug, {assertion, sec-high, testcase})

Trunk
mozilla29
x86
Linux
assertion, sec-high, testcase
Points:
---

Firefox Tracking Flags

(firefox26 wontfix, firefox27+ fixed, firefox28+ fixed, firefox29+ fixed, firefox-esr2427+ fixed, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [jsbugmon:update,ignore][adv-main27+][adv-esr24.3+])

Attachments

(4 attachments)

(Reporter)

Description

4 years ago
The following testcase asserts on mozilla-central revision cd3e9359fd64 (run with --fuzzing-safe --ion-eager):


function test() {
    var arr = new Uint32Array(100);
    arr[50] = 0xffffee00;
    for (var i=0; i<200; i++)
      res = arr[i || this];
}
test();
(Assignee)

Updated

4 years ago
Whiteboard: [jsbugmon:update,bisect]
(Assignee)

Comment 1

4 years ago
Good catch. This is another TI problem with the Ion GetElement typed array stub (see also bug 929261). The result typeset is int32-or-undefined, but then we do this:

    if (output.hasValue())
        masm.loadFromTypedArray(arrayType, source, output.valueReg(), true,
                                elementReg, &popAndFail);

Passing true for allowDouble, so we'll happily read the double value (0xffffee00) out of the Uint32Array. The IC should probably have an allowDouble flag or something.
(Assignee)

Updated

4 years ago
Flags: needinfo?(jdemooij)
(Reporter)

Updated

4 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 2

4 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/1b91cf5c8407
user:        Jan de Mooij
date:        Sat Dec 14 14:32:35 2013 +0100
summary:     Bug 949475 - Add some debug-only sanity checks. r=bhackett

This iteration took 1.054 seconds to run.
This doesn't sound great, so I'm setting it to sec-high.  Feel free to adjust as needed.
Keywords: sec-high
(Assignee)

Comment 4

4 years ago
Created attachment 8355508 [details] [diff] [review]
Patch

Set an allowDoubleResult flag on the IC if it's okay to return a double and use that for Uint32Array.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8355508 - Flags: review?(efaustbmo)
Flags: needinfo?(jdemooij)
(Assignee)

Updated

4 years ago
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
status-firefox-esr24: --- → affected
tracking-firefox27: --- → ?
tracking-firefox28: --- → ?
tracking-firefox29: --- → ?
tracking-firefox-esr24: --- → ?

Comment 5

4 years ago
Comment on attachment 8355508 [details] [diff] [review]
Patch

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

OK, r=me with question below answered.

::: js/src/jit/MIR.cpp
@@ +2663,5 @@
>      return typedArray_->viewData();
>  }
>  
> +bool
> +MGetElementCache::allowDoubleResult() const

OK, this will work.

I was briefly considering a scheme in which we checked the state of TI at stub generation time, rather than cache creation time, and then generated sutbs based on that information, but this is simpler, if less responsive to change over time.

The only difference that I can see is that in this scheme, if we choose to go to the interpreter from the fallback path, and monitor a double result, we can then use that information to generate better stubs in the future for other typed arrays.

Is there some specific reason, other than simplicity, that we chose to check here, rather than in canAttachTypedArrayElement()?
Attachment #8355508 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 6

4 years ago
Comment on attachment 8355508 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily, but it may be possible for somebody with some work.

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

> Which older supported branches are affected by this flaw?
Firefox 22+.

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

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

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8355508 - Flags: sec-approval?
(Assignee)

Comment 7

4 years ago
(As far as we know, the fuzzers never found a testcase that breaks/crashes due to this bug. That means it may not be exploitable, but in general breaking TI invariants (producing Values with unexpected types) is scary and should be considered sec-high/sec-critical just to be safe, I think.)

Updated

4 years ago
tracking-firefox27: ? → +
tracking-firefox28: ? → +
tracking-firefox29: ? → +
status-firefox26: affected → wontfix
Comment on attachment 8355508 [details] [diff] [review]
Patch

sec-approval+ for trunk. We should get Aurora, Beta, and ESR24 patches made and nominated once it is in.
Attachment #8355508 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a585e70f28

(In reply to Eric Faust [:efaust] from comment #5)
> The only difference that I can see is that in this scheme, if we choose to
> go to the interpreter from the fallback path, and monitor a double result,
> we can then use that information to generate better stubs in the future for
> other typed arrays.
> 
> Is there some specific reason, other than simplicity, that we chose to check
> here, rather than in canAttachTypedArrayElement()?

I replied on IRC, but for posterity: stack/monitored TypeSets are frozen, so when we see a double the script will be invalidated anyway. The next time we compile it, we'll be able to attach the stub.

Setting needinfo for approval requests.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 10

4 years ago
Created attachment 8356074 [details] [diff] [review]
Patch for Aurora and Beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 840696.
User impact if declined: Security bugs.
Testing completed (on m-c, etc.): On m-i.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.
Attachment #8356074 - Flags: review+
Attachment #8356074 - Flags: approval-mozilla-beta?
Attachment #8356074 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jdemooij)
(Assignee)

Comment 11

4 years ago
Created attachment 8356083 [details] [diff] [review]
Patch for ESR24

[Approval Request Comment]
User impact if declined: Security issues.
Fix Landed on Version: 29, will be backported to 27 and 28.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8356083 - Flags: review+
Attachment #8356083 - Flags: approval-mozilla-esr24?
(Reporter)

Updated

4 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
(Reporter)

Comment 12

4 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 93fe56269382).

Updated

4 years ago
tracking-firefox-esr24: ? → 27+

Updated

4 years ago
Attachment #8356074 - Flags: approval-mozilla-beta?
Attachment #8356074 - Flags: approval-mozilla-beta+
Attachment #8356074 - Flags: approval-mozilla-aurora?
Attachment #8356074 - Flags: approval-mozilla-aurora+

Updated

4 years ago
Attachment #8356083 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
This was merged:
https://hg.mozilla.org/mozilla-central/rev/91a585e70f28

And uplifted:
https://hg.mozilla.org/releases/mozilla-aurora/rev/b0157bcff32c
https://hg.mozilla.org/releases/mozilla-beta/rev/d63b720379c4
https://hg.mozilla.org/releases/mozilla-esr24/rev/ef97beb65d7f

Jan, can you please make a patch for b2g26 as well? Thanks :)
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → fixed
status-firefox27: affected → fixed
status-firefox28: affected → fixed
status-firefox29: affected → fixed
status-firefox-esr24: affected → fixed
Flags: needinfo?(jdemooij)
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(Assignee)

Comment 14

4 years ago
Created attachment 8356187 [details] [diff] [review]
Patch for b2g26
Attachment #8356187 - Flags: review+
Flags: needinfo?(jdemooij)
(Assignee)

Updated

4 years ago
Flags: needinfo?(ryanvm)
(Reporter)

Updated

4 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 15

4 years ago
JSBugMon: This bug has been automatically verified fixed.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/21fd14e1c3d4
status-b2g-v1.2: affected → fixed
Flags: needinfo?(ryanvm)
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main27+][adv-esr24.3+]
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
Group: core-security
You need to log in before you can comment on or make changes to this bug.