Crash [@ js::jit::RValueAllocation::index() const] with heap-buffer-overflow

VERIFIED FIXED in Firefox 39, Firefox OS v2.2

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
3 years ago
2 months ago

People

(Reporter: decoder, Assigned: nbp)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla41
x86_64
Linux
crash, csectype-bounds, regression, sec-critical, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox38 wontfix, firefox38.0.5 wontfix, firefox39+ verified, firefox40+ verified, firefox41+ verified, firefox-esr31 unaffected, firefox-esr3839+ fixed, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [jsbugmon:update,ignore][adv-main39+][adv-esr38.1+], crash signature)

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
The following testcase crashes on mozilla-central revision 98820360ab66 (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2 --ion-eager):

function f() {
  return f.caller.p;
}
var lfGlobal = newGlobal();
for (lfLocal in this) { 
    if (!(lfLocal in lfGlobal)) { 
        lfGlobal[lfLocal] = this[lfLocal]; 
    }
}
function dummy() {
  test();
  function test() {
    function t(o, proplist) { var obj_12 = f("get"); }
    t();
    test()
  }
}
lfGlobal.offThreadCompileScript(dummy.toSource() + "dummy();");
lfGlobal.runOffThreadScript();



Backtrace:

==43399==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200003ad00 at pc 0x1093b32 bp 0x7fff0a7c3a00 sp 0x7fff0a7c39f8
READ of size 8 at 0x60200003ad00 thread T0
    #0 0x1093b31 in js::jit::RValueAllocation::index() const js/src/jit/JitFrames.cpp:2267
    #1 0x1093b31 in js::jit::SnapshotIterator::allocationValue(js::jit::RValueAllocation const&, js::jit::SnapshotIterator::ReadMethod) js/src/jit/JitFrames.cpp:1995
    #2 0x109a9f4 in js::jit::InlineFrameIterator::callee(js::jit::MaybeReadFallback&) const js/src/jit/JitFrames.cpp:2509
    #3 0xaf4dfc in js::FrameIter::callee(JSContext*) const js/src/vm/Stack.cpp:1108
    #4 0x14c4d67 in CallerGetterImpl(JSContext*, JS::CallArgs) js/src/jsfun.cpp:265
    #5 0x164b4a3 in bool JS::CallNonGenericMethod<&(IsFunction(JS::Handle<JS::Value>)), &(CallerGetterImpl(JSContext*, JS::CallArgs))>(JSContext*, JS::CallArgs) js/src/opt64asan/js/src/../../dist/include/js/CallNonGenericMethod.h:100
    #6 0x164b4a3 in CallerGetter(JSContext*, unsigned int, JS::Value*) js/src/jsfun.cpp:297
    #7 0xd88038 in js::jit::DoCallNativeGetter(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:1618

0x60200003ad00 is located 0 bytes to the right of 16-byte region [0x60200003acf0,0x60200003ad00)
allocated by thread T0 here:
    #0 0x4af3f7 in __interceptor_malloc /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x10c2cb8 in js_malloc(unsigned long) js/src/opt64asan/js/src/../../dist/include/js/Utility.h:126
    #2 0x10c2cb8 in _ZL13js_pod_mallocIN2js14RelocatablePtrIN2JS5ValueEEEEPT_m js/src/opt64asan/js/src/../../dist/include/js/Utility.h:281
    #3 0x10c2cb8 in js::RelocatablePtr<JS::Value>* js::SystemAllocPolicy::pod_malloc<js::RelocatablePtr<JS::Value> >(unsigned long) js/src/jsalloc.h:34
    #4 0x10c2cb8 in mozilla::VectorBase<js::RelocatablePtr<JS::Value>, 1ul, js::SystemAllocPolicy, mozilla::Vector<js::RelocatablePtr<JS::Value>, 1ul, js::SystemAllocPolicy> >::convertToHeapStorage(unsigned long) js/src/opt64asan/js/src/../../dist/include/mozilla/Vector.h:772
    #5 0x10c2cb8 in mozilla::VectorBase<js::RelocatablePtr<JS::Value>, 1ul, js::SystemAllocPolicy, mozilla::Vector<js::RelocatablePtr<JS::Value>, 1ul, js::SystemAllocPolicy> >::growStorageBy(unsigned long) js/src/opt64asan/js/src/../../dist/include/mozilla/Vector.h:863
    #6 0x1091fa1 in mozilla::UniquePtr<mozilla::Vector<js::RelocatablePtr<JS::Value>, 1ul, js::SystemAllocPolicy>, JS::DeletePolicy<mozilla::Vector<js::RelocatablePtr<JS::Value>, 1ul, js::SystemAllocPolicy> > >::operator bool() const js/src/opt64asan/js/src/../../dist/include/mozilla/Vector.h:924
    #7 0x1091fa1 in js::jit::RInstructionResults::init(JSContext*, unsigned int) js/src/jit/JitFrames.cpp:1711
    #8 0x10957d6 in js::jit::SnapshotIterator::computeInstructionResults(JSContext*, js::jit::RInstructionResults*) const js/src/jit/JitFrames.cpp:2222
    #9 0x10948b6 in js::jit::SnapshotIterator::initInstructionResults(js::jit::MaybeReadFallback&) js/src/jit/JitFrames.cpp:2199
    #10 0x1093fc8 in js::jit::SnapshotIterator::maybeRead(js::jit::RValueAllocation const&, js::jit::MaybeReadFallback&) js/src/jit/JitFrames.cpp:2026
    #11 0x109a9f4 in js::jit::InlineFrameIterator::callee(js::jit::MaybeReadFallback&) const js/src/jit/JitFrames.cpp:2509
    #12 0xaf4dfc in js::FrameIter::callee(JSContext*) const js/src/vm/Stack.cpp:1108
    #13 0x14c4d67 in CallerGetterImpl(JSContext*, JS::CallArgs) js/src/jsfun.cpp:265
    #14 0x164b4a3 in bool JS::CallNonGenericMethod<&(IsFunction(JS::Handle<JS::Value>)), &(CallerGetterImpl(JSContext*, JS::CallArgs))>(JSContext*, JS::CallArgs) js/src/opt64asan/js/src/../../dist/include/js/CallNonGenericMethod.h:100
    #15 0x164b4a3 in CallerGetter(JSContext*, unsigned int, JS::Value*) js/src/jsfun.cpp:297
    #16 0xd88038 in js::jit::DoCallNativeGetter(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) js/src/jit/BaselineIC.cpp:1618
    #17 0xe4134d in EnterBaseline(JSContext*, js::jit::EnterJitData&) js/src/jit/BaselineJIT.cpp:124
    #18 0xe40c8d in js::jit::EnterBaselineMethod(JSContext*, js::RunState&) js/src/jit/BaselineJIT.cpp:156
    #19 0x91e959 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:642
    #20 0x8ff70a in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:722
    #21 0x8b3986 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:759
    #22 0x16ad835 in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const js/src/proxy/DirectProxyHandler.cpp:77
    #23 0x16ad835 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const js/src/proxy/CrossCompartmentWrapper.cpp:289
    #24 0x16beb6e in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/proxy/Proxy.cpp:391
    #25 0x16c1d84 in js::proxy_Call(JSContext*, unsigned int, JS::Value*) js/src/proxy/Proxy.cpp:697
    #26 0x8ff14f in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235
    #27 0x8ff14f in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:695
    #28 0x8b3986 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:759
    #29 0x1315c6a in js::jit::InvokeFunction(JSContext*, JS::Handle<JSObject*>, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/jit/VMFunctions.cpp:75

SUMMARY: AddressSanitizer: heap-buffer-overflow js/src/jit/JitFrames.cpp:2267 js::jit::RValueAllocation::index() const
Shadow bytes around the buggy address:
  0x0c047ffff550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047ffff560: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047ffff570: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047ffff580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047ffff590: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 00 00
=>0x0c047ffff5a0:[fa]fa fd fa fa fa 00 05 fa fa fd fa fa fa fd fd
  0x0c047ffff5b0: fa fa 00 07 fa fa fd fa fa fa fd fd fa fa fd fa
  0x0c047ffff5c0: fa fa fd fd fa fa fd fa fa fa fd fd fa fa 00 01
  0x0c047ffff5d0: fa fa fd fd fa fa 00 fa fa fa fd fd fa fa fd fd
  0x0c047ffff5e0: fa fa fd fa fa fa fd fd fa fa 00 06 fa fa fd fd
  0x0c047ffff5f0: fa fa fd fa fa fa 00 07 fa fa 00 00 fa fa fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Contiguous container OOB:fc
  ASan internal:           fe
==43399==ABORTING


Marking s-s and sec-critical due to heap-buffer-overflow.
status-firefox40: --- → ?
tracking-firefox41: --- → +
Flags: needinfo?(nicolas.b.pierron)
(Reporter)

Updated

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

Comment 1

3 years ago
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/a50d660f09da
user:        Nicolas B. Pierron
date:        Fri Dec 19 15:28:30 2014 +0100
summary:     Bug 1073033 part 3 - Recover MLambda on bailouts. r=shu

This iteration took 115.171 seconds to run.
Ok, so far I noticed 2 issues:

1/ The first issue which is reported when compiling with a debug build.  The problem is that when we request "f.caller" we do recover the allocation of objects/lambda with the context of the introspecting compartment instead of the context of the introspected compartment.  This issue is a security issue which might open the door for privilege escalation.

2/ The second issue is the heap-buffer-overflow, which is apparently recycling the RecoverInstruction vector from a previous frame.  I still have not yet figured precisely what cause us to ignore the recovered instruction results.

In both cases, Bug 1073033 enabled this issue by adding a way to execute recover instruction from another compartment (1), and by not popping the recovered instruction when expected (2).
Assignee: nobody → nicolas.b.pierron
status-firefox38: --- → affected
status-firefox38.0.5: --- → affected
status-firefox39: --- → affected
status-firefox40: ? → affected
Flags: needinfo?(nicolas.b.pierron)
Created attachment 8617925 [details] [diff] [review]
Switch compartment before computing recover instruction results.

The heap-buffer-overflow was caused by the fact we tried to invalidate a
function from the wrong compartment, and thus we did not found the
CompileInfo corresponding to the RecompileInfo of the invalidated IonScript.

Using the AutoCompartment above the invalidate (2) call, and above the
computeInstructionResults (1) fixes both issues mentionned in comment 2.
Attachment #8617925 - Flags: review?(jdemooij)
Created attachment 8617927 [details] [diff] [review]
Assert that SnapshotIterator::initInstructionResults always match the recover instructions data.

(extra debug added to investigate this issue)
Attachment #8617927 - Flags: review?(jdemooij)
Created attachment 8617928 [details] [diff] [review]
Improve js::DumpBacktrace to include raw frame pointers and types.

(extra debug added to investigate this issue)
Attachment #8617928 - Flags: review?(jdemooij)
Does this affect ESR31?
status-firefox38: affected → wontfix
status-firefox38.0.5: affected → wontfix
status-firefox-esr31: --- → ?
status-firefox-esr38: --- → affected
tracking-firefox39: --- → +
tracking-firefox40: --- → +
tracking-firefox-esr38: --- → 39+

Updated

3 years ago
Attachment #8617925 - Flags: review?(jdemooij) → review+
Comment on attachment 8617927 [details] [diff] [review]
Assert that SnapshotIterator::initInstructionResults always match the recover instructions data.

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

Yay, assertions++.
Attachment #8617927 - Flags: review?(jdemooij) → review+

Updated

3 years ago
Attachment #8617928 - Flags: review?(jdemooij) → review+
(In reply to Al Billings [:abillings] from comment #6)
> Does this affect ESR31?

No, these issue are related to code added in Bug 1062869 (Gecko 35).

Requesting the "caller" attribute (as in the tests case comment 0) is just one way to introspect the stack, and evaluate code within the wrong compartment.
Comment on attachment 8617925 [details] [diff] [review]
Switch compartment before computing recover instruction results.

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

Building a function which can be used to gain privileges is doable, by reading the tests cases of the JavaScript engine. I think the hardest part would be to find the proper function which trigger the "f.caller" or request previously unobserved slots of the caller.

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

The second patch which adds assertions highlight that we can use a buffer overflow by using the same technique.  I suggests that we land it separately.

> Which older supported branches are affected by this flaw?

(see comment 8)

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

(see comment 8)

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

The code did not change (much?) since 38.  The same patch should apply cleaning or it would be trivial to make them.

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

This patch is unlikely to cause much trouble, as we switch to the compartment of the JSScript which is holding the IonScript, and thus the same compartment as where the object are supposed to live in when allocated.

As we do not yet have a way to inline across compartment yet, this line is correct and is likely to be a no-op most of the time.
Attachment #8617925 - Flags: sec-approval?
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
status-firefox-esr31: ? → unaffected
Comment on attachment 8617925 [details] [diff] [review]
Switch compartment before computing recover instruction results.

Sec-approval+ for trunk. We'll want this on Aurora, Beta, and ESR38 once it is on trunk.
Attachment #8617925 - Flags: sec-approval? → sec-approval+
(Reporter)

Updated

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

Comment 11

3 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision b7ee8e13145a).
Attachment #8617925 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/a59dc4748390

Please nominate this for Aurora/Beta/esr38 approval whenever you're comfortable doing so.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: affected → fixed
status-firefox41: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Reporter)

Updated

3 years ago
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
(Reporter)

Comment 14

3 years ago
JSBugMon: This bug has been automatically verified fixed.
Yes please nom for uplift. I would like to get this into beta 7 so would like to land this on beta before Thursday morning.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8617925 [details] [diff] [review]
Switch compartment before computing recover instruction results.

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

> User impact if declined: 
(see comment 2), potential privilege escalation.

> Fix Landed on Version:
Gecko 41

> Risk to taking this patch (and alternatives if risky): 
This should have little risks, as explained in comment 9.

> String or UUID changes made by this patch: 
N/A
Flags: needinfo?(nicolas.b.pierron)
Attachment #8617925 - Flags: approval-mozilla-release?
Attachment #8617925 - Flags: approval-mozilla-esr38?
Attachment #8617925 - Flags: approval-mozilla-beta?
Attachment #8617925 - Flags: approval-mozilla-aurora?
Comment on attachment 8617925 [details] [diff] [review]
Switch compartment before computing recover instruction results.

Taking it for 40 & esr38 as it is a critical issue.
However, we are not planning to do a dot release of 38 for this.
Attachment #8617925 - Flags: approval-mozilla-release?
Attachment #8617925 - Flags: approval-mozilla-release-
Attachment #8617925 - Flags: approval-mozilla-esr38?
Attachment #8617925 - Flags: approval-mozilla-esr38+
Attachment #8617925 - Flags: approval-mozilla-aurora?
Attachment #8617925 - Flags: approval-mozilla-aurora+
Comment on attachment 8617925 [details] [diff] [review]
Switch compartment before computing recover instruction results.

Approved for uplift to beta, small change, sec-critical.
Attachment #8617925 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Reporter)

Updated

3 years ago
status-firefox40: fixed → verified
(Reporter)

Comment 21

3 years ago
JSBugMon: This bug has been automatically verified fixed on Fx40
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main39+][adv-esr38.1+]
(Reporter)

Updated

3 years ago
status-firefox39: fixed → verified
(Reporter)

Comment 24

3 years ago
JSBugMon: This bug has been automatically verified fixed on Fx39

Updated

3 years ago
Group: core-security → core-security-release
(I should land these debug patches as well)
Flags: needinfo?(nicolas.b.pierron)
Debug patches are now landed on inbound, no need to backport them:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ed541d1ca7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee600e02101a
Flags: needinfo?(nicolas.b.pierron)
Group: core-security-release
Keywords: csectype-bounds
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.