Closed Bug 1026460 Opened 10 years ago Closed 10 years ago

Crash [@ js::jit::SnapshotIterator::fromInstructionResult] with warning "Tried to access unreadable value allocation (possible f.arguments)"

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla33
blocking-b2g 2.0+
Tracking Status
firefox31 --- unaffected
firefox32 + verified
firefox33 + verified
firefox-esr24 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: decoder, Assigned: nbp)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(1 file)

The following testcase crashes on mozilla-central revision bb35d1b73634 (run with --fuzzing-safe --ion-eager):


(function() {
  function boo() {
    return foo.arguments[0];
  }
  function foo(a,b,c) {
    if (a == 0) {
      var x1 = (a /= -1);
      return boo();
    }
  }
  function inlined() {
    return foo.apply({}, arguments);
  }
  inlined(1,2,3);
  inlined(0,2,3);
})();
Debug backtrace:


Program received signal SIGSEGV, Segmentation fault.
js::jit::SnapshotIterator::fromInstructionResult (this=this@entry=0xffff8d98, index=0) at js/src/jit/IonFrames.cpp:1679
1679        MOZ_ASSERT(!(*instructionResults_)[index].isMagic(JS_ION_BAILOUT));
#0  js::jit::SnapshotIterator::fromInstructionResult (this=this@entry=0xffff8d98, index=0) at js/src/jit/IonFrames.cpp:1679
#1  0x08263cb3 in js::jit::SnapshotIterator::allocationValue (this=0xffff8d98, alloc=...) at js/src/jit/IonFrames.cpp:1611
#2  0x085ac572 in read (this=0xffff8d98) at js/src/jit/JitFrameIterator.h:390
#3  js::jit::InlineFrameIterator::readFrameArgsAndLocals<js::CopyToHeap, js::jit::InlineFrameIterator::Nop> (this=this@entry=0xffff9440, cx=cx@entry=0x94c6ac0, argOp=..., localOp=..., scopeChain=scopeChain@entry=0x0, rval=rval@entry=0x0, argsObj=argsObj@entry=0x0, thisv=thisv@entry=0x0, behavior=behavior@entry=js::jit::ReadFrame_Actuals) at js/src/jit/JitFrameIterator.h:592
#4  0x085ac86d in js::jit::InlineFrameIterator::unaliasedForEachActual<js::CopyToHeap> (this=this@entry=0xffff9440, cx=cx@entry=0x94c6ac0, op=..., op@entry=..., behavior=behavior@entry=js::jit::ReadFrame_Actuals) at js/src/jit/JitFrameIterator.h:600
#5  0x085ac933 in js::FrameIter::unaliasedForEachActual<js::CopyToHeap> (this=0xffff93e0, cx=cx@entry=0x94c6ac0, op=op@entry=...) at js/src/vm/Stack-inl.h:343
#6  0x085ac99f in CopyScriptFrameIterArgs::copyArgs (this=this@entry=0xffff931c, cx=cx@entry=0x94c6ac0, dstBase=dstBase@entry=0x95a30f8, totalArgs=totalArgs@entry=3) at js/src/vm/ArgumentsObject.cpp:138
#7  0x085acda4 in js::ArgumentsObject::create<CopyScriptFrameIterArgs> (cx=0x94c6ac0, script=0xf6634230, callee=(JSFunction * const) 0xf663b820 [object Function "foo"], numActuals=3, copy=...) at js/src/vm/ArgumentsObject.cpp:214
edx     0x0     0
=> 0x82638c9 <js::jit::SnapshotIterator::fromInstructionResult(unsigned int) const+41>: cmpb   $0x0,0x60(%edx)


Marked s-s because of the warning. Should this warning maybe be an assert or even a forced crash if it has security implications?
Whiteboard: [jsbugmon:update,bisect]
Needinfo on nbp, requested :)
Flags: needinfo?(nicolas.b.pierron)
I will take this bug later today or tomorrow.

This is a known issue that I did not get time to fix yet. This is caused by the addition of recover instruction in the Dead Code Elimination phase (landed in Gecko 32), we need to check if the resume point operand is observable or not.

What is happeninng in this test case of comment 0 is that "a"'s mutation is DCE'd and boo() tries to read the mutated content of "a".  Checking for the isObservableOperand should prevent DCE optimizations on "a", while keeping any on "x1".

(In reply to Christian Holler (:decoder) from comment #1)
> Marked s-s because of the warning. Should this warning maybe be an assert or
> even a forced crash if it has security implications?

Currently the warning is here to warn when the instruction is not readable and it should return Undefined instead.  This is a known incorrect JS Semantic, as we wanted to check if a rare case could happen in the wild.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4fa4ba576464
user:        Sankha Narayan Guria
date:        Fri Jun 06 17:32:15 2014 +0200
summary:     Bug 1011541 - Implement Div Recover instruction. r=nbp

This iteration took 508.949 seconds to run.
This patch is changing the way hasLiveDefUses consider resume point as live
definitions, in the case f.arguments (or others) can be used to observe the
operand of the resume point.

hasLiveDefUses is used by both the DCE pass and the graph coherency pass
which is checking that no recover instructions are used if they have live
uses.
Attachment #8441410 - Flags: review?(shu)
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
QA Contact: nicolas.b.pierron
The bug is coming from Bug 990106 part 4 (attachment 8408953 [details] [diff] [review]).  The patch attached in comment 5 should fix this issue, and the isObservable() function got added at the end of Gecko 32, so hopefully the above patch should also work on top of aurora.

The following testcase should crash on mozilla-central revision bb35d1b73634 (run with --fuzzing-safe --ion-eager):


(function() {
  function boo() {
    return foo.arguments[0];
  }
  function foo(a,b,c) {
    if (a == 0) {
      a += 1; // Should bisect to Bug 990106 part 4.
      return boo();
    }
  }
  function inlined() {
    return foo.apply({}, arguments);
  }
  inlined(1,2,3);
  inlined(0,2,3);
})();
Blocks: 990106
Nicolas, can you suggest a security rating for this?
Comment on attachment 8441410 [details] [diff] [review]
Consider observable MIR nodes as live uses.

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

LGTM
Attachment #8441410 - Flags: review?(shu) → review+
(In reply to Christian Holler (:decoder) from comment #1)
> Program received signal SIGSEGV, Segmentation fault.
> js::jit::SnapshotIterator::fromInstructionResult
> (this=this@entry=0xffff8d98, index=0) at js/src/jit/IonFrames.cpp:1679
> 1679       
> MOZ_ASSERT(!(*instructionResults_)[index].isMagic(JS_ION_BAILOUT));

This SEGV is related to the fact that instructionResults_ is NULL, and calling the operator[] on it might index anything from 0 to N, where N is an index of a recover instruction of the current frame, and the vector is a vector of Values (sizeof == 8 bytes).

Currently there is no direct upper bound on the number of recover instruction which might be encoded, even if this is indirectly restricted by the size of the code, we might be able to store 512 recover instruction in one Snapshot.

Even if this sounds like an awful way to reach the second page of the memory, it is quite reliable.
This provides a read access to a part of the memory which can be forged to fool the baseline Jit into making a bad jump.

=> sec-critical.

Note that this would have only be a JS semantic issue if frame #2 of comment 1 was "maybeRead" instead of "read", which produces the warning and returns Undefined.
Keywords: sec-critical
Assignee: nobody → nicolas.b.pierron
Comment on attachment 8441410 [details] [diff] [review]
Consider observable MIR nodes as live uses.

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

See comment 9.

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

Yes, but as much as the code it-self.

Which older supported branches are affected by this flaw?

Gecko 32 & 33.

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

Bug 990106 part 4 (attachment 8408953 [details] [diff] [review] [diff] [review])

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

The same patch should apply cleanly.

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

This patch prevents recover instruction optimizations.  It should not need much testing and is unlikely to cause additional regressions.
Attachment #8441410 - Flags: sec-approval?
Comment on attachment 8441410 [details] [diff] [review]
Consider observable MIR nodes as live uses.

sec-approval+ for trunk. 

We should have an Aurora patch created and nominated too since that's the only branch that's affected.
Attachment #8441410 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/b33274d43434
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
blocking-b2g: --- → 2.0?
blocking-b2g: 2.0? → 2.0+
https://hg.mozilla.org/releases/mozilla-aurora/rev/6e08edd34e5b

FYI, in the future, you don't need a=abillings in the commit message for the sec-approval when landing on trunk. In fact, it's preferred you not ;)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/6e08edd34e5b

Thanks for landing :)

> FYI, in the future, you don't need a=abillings in the commit message for the
> sec-approval when landing on trunk. In fact, it's preferred you not ;)

Ok.
Reproduced the original issue using the information from comment #0, received the following error:

"Warning! Tried to access unreadable value allocation (possible f.arguments).
Segmentation fault (core dumped)"

Once reproduced, I verified that it's not occurring under fx32 using the "200568:fd1a2d08ca92" changeset. Compiled/Ran the js shell using "-enable-arm-simulator" and the options that are mentioned under comment #0. Used Ubuntu 13.10 x86.
Group: core-security
Keywords: regression
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: