IonMonkey: Fix ARM moveValue so that GC pointers are traced

RESOLVED FIXED in Firefox 24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

({sec-high})

unspecified
mozilla25
sec-high
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox22 wontfix, firefox23 wontfix, firefox24+ fixed, firefox25+ fixed, firefox-esr17 unaffected, b2g18 disabled)

Details

(Whiteboard: [adv-main24+])

Attachments

(1 attachment)

(Assignee)

Description

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

I was reading some unrelated code earlier today and noticed this. I'm not sure it can cause crashes right now, but there are some callers that pass a Value that could be an object/string I think, so let's assume it's possible.

x86/x64 already do this and grep shows this is the only place that uses payload.i32 without an isMarkable() check.
Attachment #781644 - Flags: review?(mrosenberg)
(Assignee)

Updated

4 years ago
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
Comment on attachment 781644 [details] [diff] [review]
Patch

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

Nothing looks wrong with it, so r+, but I think we should poke a bit first.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +2672,5 @@
>  MacroAssemblerARMCompat::moveValue(const Value &val, Register type, Register data) {
>      jsval_layout jv = JSVAL_TO_IMPL(val);
>      ma_mov(Imm32(jv.s.tag), type);
> +    if (val.isMarkable())
> +        ma_mov(ImmGCPtr(reinterpret_cast<gc::Cell *>(val.toGCThing())), data);

Since without the ImmGCPtr, we'd never record the offset, we shouldn't crash due to the wrong instrutions during marking.  However, we could not trace through that pointer, and get the standard use-after-collect GC bug.  How does x86/x64 handle this case?
Attachment #781644 - Flags: review?(mrosenberg) → review+
(Assignee)

Comment 2

4 years ago
(In reply to Marty Rosenberg [:mjrosenb] from comment #1)
> Since without the ImmGCPtr, we'd never record the offset, we shouldn't crash
> due to the wrong instrutions during marking.  However, we could not trace
> through that pointer, and get the standard use-after-collect GC bug.

Yup, it'd be a use-after-collect GC bug.

> How does x86/x64 handle this case?

See comment 0; x86 already has exactly the same code. x64 has similar code (only difference is due to the different boxing format there).
Can we get a sec rating?  If this isn't something highly exploitable that we'd have to chemspill for - or something attributable to high crash volumes - we're probably not going to track this for FF23 since we're in the homestretch there and our final beta is going to build on Monday.
(Assignee)

Comment 4

4 years ago
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #3)
> Can we get a sec rating?  If this isn't something highly exploitable that
> we'd have to chemspill for - or something attributable to high crash volumes
> - we're probably not going to track this for FF23 since we're in the
> homestretch there and our final beta is going to build on Monday.

This is potentially sec-critical and can cause all kinds of crashes, but it's likely hard/impossible to trigger atm (fuzzers also didn't find it yet). So either we are lucky and for some reason it's not exploitable right now or maybe we have a sec-critical. Unfortunately I can't say for sure which one it is.
(Assignee)

Comment 5

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

lsblakk, I'm requesting sec-approval and aurora approval. Since we don't have a testcase and don't have any evidence this is causing problems right now, I'm fine with taking this on aurora only. If you disagree please a+ for beta too.

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
IonMonkey.

> User impact if declined: 
Maybe crashes, security bugs.

> Testing completed (on m-c, etc.): 
Didn't land yet.

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

> String or IDL/UUID changes made by this patch:
None.

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

> 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 18+.

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

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

> How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.
Attachment #781644 - Flags: sec-approval?
Attachment #781644 - Flags: approval-mozilla-aurora?
Comment on attachment 781644 [details] [diff] [review]
Patch

Giving sec-appoval+ to land in trunk. After this, please land on Aurora, which I have also set the approval flag for.
Attachment #781644 - Flags: sec-approval?
Attachment #781644 - Flags: sec-approval+
Attachment #781644 - Flags: approval-mozilla-aurora?
Attachment #781644 - Flags: approval-mozilla-aurora+
status-firefox23: affected → wontfix
tracking-firefox23: ? → ---
tracking-firefox24: ? → +
tracking-firefox25: ? → +
status-b2g18: --- → ?
status-firefox-esr17: --- → unaffected
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/affe224de780
https://hg.mozilla.org/mozilla-central/rev/affe224de780
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox25: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/releases/mozilla-aurora/rev/7fbedcc70869
status-firefox22: affected → wontfix
status-firefox24: affected → fixed
Whiteboard: [adv-main24+]
This shouldn't affect b2g18 because ionmonkey is disabled there.
status-b2g18: ? → disabled
Keywords: sec-high
Group: core-security
You need to log in before you can comment on or make changes to this bug.