Closed Bug 1412420 Opened 2 years ago Closed 2 years ago

Crash [@ js::TypeSet::GetValueType] with invalid read

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 58+ fixed
firefox57 --- wontfix
firefox58 + fixed
firefox59 + fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:][adv-main58+][adv-esr52.6+][post-critsmash-triage])

Crash Data

Attachments

(1 file, 1 obsolete file)

The following testcase crashes on mozilla-central revision a80d568a417e (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe):

function f(j) {
    var i = Math.pow(j++, j) | 0;
    var obj = {
        i: i,
    }
}
var max = 150;
for (var j = 0; j <= max; ++j) {
    f(j);
}


Backtrace:

received signal SIGSEGV, Segmentation fault.
js::TypeSet::GetValueType (val=...) at js/src/vm/TypeInference-inl.h:182
#0  js::TypeSet::GetValueType (val=...) at js/src/vm/TypeInference-inl.h:182
#1  js::AddTypePropertyId (cx=cx@entry=0x7ffff6952000, group=0x7ffff467f8b0, obj=0x0, id=..., value=...) at js/src/vm/TypeInference.cpp:2850
#2  0x000000000062133c in js::jit::DoTypeUpdateFallback (cx=0x7ffff6952000, frame=<optimized out>, stub=0x7ffff432f3c8, objval=..., value=...) at js/src/jit/BaselineIC.cpp:329
#3  0x00000289dd30058e in ?? ()
#4  0x00007fffffffd5e0 in ?? ()
#5  0x00007fffffffd4f8 in ?? ()
#6  0x0000000000000000 in ?? ()
rax	0x7fffffffffff	140737488355327
rbx	0x7ffff6952000	140737330356224
rcx	0x7ffff4600d60	140737293323616
rdx	0x0	0
rsi	0x7ffff467f8b0	140737293842608
rdi	0x7ffff6952000	140737330356224
rbp	0x7fffffffd420	140737488344096
rsp	0x7fffffffd3f0	140737488344048
r8	0x40ffe8000000	71467853152256
r9	0xfff80000ffffffff	-2251795518717953
r10	0x7fffffffd518	140737488344344
r11	0x7ffff4674bc8	140737293798344
r12	0x7fffffffd440	140737488344128
r13	0x7fffffffd530	140737488344368
r14	0x7ffff432f3c8	140737290367944
r15	0x7fffffffd480	140737488344192
rip	0xc856ed <js::AddTypePropertyId(JSContext*, js::ObjectGroup*, JSObject*, jsid, JS::Value const&)+125>
=> 0xc856ed <js::AddTypePropertyId(JSContext*, js::ObjectGroup*, JSObject*, jsid, JS::Value const&)+125>:	mov    (%r8),%rax
   0xc856f0 <js::AddTypePropertyId(JSContext*, js::ObjectGroup*, JSObject*, jsid, JS::Value const&)+128>:	or     $0x1,%r8


I was only able to reproduce this on the GCOV builds we made for JS shell coverage analysis. The difference to our normal builds is that the GCOV builds use gcc 6. So this could be a compiler issue.

Marking s-s and sec-high because this looks like an invalid read from a bad address.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Hi Naveed, please reassigned these bugs to developers who can investigate and fix them. Thanks!
Assignee: nobody → nihsanullah
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Interesting. This reminds me of some crashes that have been reported on unsupported ARM configurations.

How can we repro this locally?
Flags: needinfo?(choller)
(In reply to Jan de Mooij [:jandem] from comment #3)
>
> How can we repro this locally?

I've used this compiler to build, so maybe try that as a start:

gcc-6 (Ubuntu/Linaro 6.3.0-18ubuntu2~16.04) 6.3.0 20170519


Also, the builds we use for coverage are here:

https://build.fuzzing.mozilla.org/builds/jsshell-mc-64-opt-gcov.zip
https://build.fuzzing.mozilla.org/builds/jsshell-mc-64-debug-gcov.zip

I only tried the opt build here because that is what we currently use in testing gcov.
Flags: needinfo?(choller)
Flags: needinfo?(jdemooij)
Priority: -- → P1
I can reproduce this with GCC 6.4 when I add the --coverage flag to CC and CXX.

Our Baseline IC for JSOP_BITOR calls JS::ToInt32(double) and this GCC build does *not* zero the upper half of the return register (it contains garbage). Then we use masm.orPtr and masm.boxValue => we now have a corrupt Value.

A few things to do here:

* We should use masm.or32 et al instead of orPtr.

* We have storeCallWordResult and storeCallBoolResult - we probably need storeCallInt32Result.

Patch next week.
Assignee: nihsanullah → jdemooij
Status: NEW → ASSIGNED
Oh and this is very build specific. I doubt it affects our official Tier-1 builds (we would have heard by now).
Attached patch Patch (obsolete) — Splinter Review
Fixes described in comment 5.

We now use masm.or32/xor32/and32 instead of orPtr/xorPtr/andPtr and I split storeCallWordResult into storeCallInt32Result and storeCallPointerResult.
Flags: needinfo?(jdemooij)
Attachment #8925532 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8925532 [details] [diff] [review]
Patch

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

::: js/src/irregexp/NativeRegExpMacroAssembler.cpp
@@ +878,5 @@
>          if (!unicode) {
>              int (*fun)(const char16_t*, const char16_t*, size_t) = CaseInsensitiveCompareStrings;
>              masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, fun));
>          } else {
>              int (*fun)(const char16_t*, const char16_t*, size_t) = CaseInsensitiveCompareUCStrings;

follow-up nit: change the return type to int32_t.

::: js/src/jit/CodeGenerator.cpp
@@ +10682,5 @@
>      masm.passABIArg(obj);
>      masm.movePtr(ImmPtr(gen->runtime), output);
>      masm.passABIArg(output);
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, jit::TypeOfObject));
> +    masm.storeCallPointerResult(output);

nit: static_assert(sizeof(JSType) == sizeof(void*));

::: js/src/jit/shared/CodeGenerator-shared.h
@@ +726,5 @@
>  
>      inline void generate(CodeGeneratorShared* codegen) const {
> +        // It's okay to use storePointerResultTo here - the VMFunction wrapper
> +        // ensures the upper bytes are zero for bool/int32 return values.
> +        codegen->storePointerResultTo(out_);

Thanks for this comment.

Sean, can you double check if this comment is accurate on arm64?
http://searchfox.org/mozilla-central/rev/5a60492a53667fc61a62af1847d005a210b7a4f6/js/src/jit/arm64/Trampoline-arm64.cpp#719,724
Attachment #8925532 - Flags: review?(nicolas.b.pierron)
Attachment #8925532 - Flags: review+
Attachment #8925532 - Flags: feedback?(sstangl)
There's a considerable amount of crashes with this signature in Crash Stats. Could this be triggered on Windows somehow or is this strictly an issue with GCC?
Flags: needinfo?(jdemooij)
The comment does hold on ARM64: LDRB zero-extends, and LDR (really, any instruction) with a W-reg always zeroes the upper 32 bits of the X-reg so as to enable better micro-optimization.
Flags: needinfo?(jdemooij)
Flags: needinfo?(jdemooij)
Jan, is this something we can land in 59 and uplift to 58? Unsure of the risk of this patch.
(In reply to Christian Holler (:decoder) from comment #9)
> There's a considerable amount of crashes with this signature in Crash Stats.
> Could this be triggered on Windows somehow or is this strictly an issue with
> GCC?

In theory we could trigger this on Windows/MSVC, but the crashes with this signature look unrelated (ObjectValue with null payload or something).

Not sure what to do here since we only see this with GCOV builds. I'll just request sec-approval.
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> > +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, jit::TypeOfObject));
> > +    masm.storeCallPointerResult(output);
> 
> nit: static_assert(sizeof(JSType) == sizeof(void*));

It's confusing but js::TypeOfObject returns JSType, jit::TypeOfObject returns JSString*. We call the latter here so this should be okay.
Attached patch PatchSplinter Review
[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Official builds are most likely not affected. Requesting sec-approval just in  case... I think we should land this on beta and Nightly.

> 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?
All, in theory (see above).

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

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8925532 - Attachment is obsolete: true
Attachment #8925532 - Flags: feedback?(sstangl)
Flags: needinfo?(jdemooij)
Attachment #8933331 - Flags: sec-approval?
Attachment #8933331 - Flags: review+
I'm confused... 99% of the crashes with this signature are Windows (mostly with wildptr addresses).  How would this be a GCOV issue?
Flags: needinfo?(jdemooij)
Attachment #8933331 - Flags: sec-approval? → sec-approval+
sec-approval+ to land on trunk.
(In reply to Randell Jesup [:jesup] from comment #15)
> I'm confused... 99% of the crashes with this signature are Windows (mostly
> with wildptr addresses).  How would this be a GCOV issue?

JS crashes often have a very generic signature. Note that this bug is 64-bit only and a lot of these crashes are on 32-bit, so they're definitely unrelated. That said, I thought the 64-bit crashes were unrelated because of the location where we crash in the function, but after thinking about it more, this bug could trigger similar crashes.

Still okay to land this, Al?
Flags: needinfo?(abillings)
Naveed, can you ask someone to look at the Windows crashes here?
Flags: needinfo?(nihsanullah)
(In reply to Jan de Mooij [:jandem] from comment #17)

> Still okay to land this, Al?

Yes.
Flags: needinfo?(abillings)
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Comment on attachment 8933331 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Potential crashes or security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Very mechanical/simple changes.
[String changes made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8933331 - Flags: approval-mozilla-esr52?
Attachment #8933331 - Flags: approval-mozilla-beta?
Comment on attachment 8933331 [details] [diff] [review]
Patch

Sec-high, Beta58+
Attachment #8933331 - Flags: approval-mozilla-esr52?
Attachment #8933331 - Flags: approval-mozilla-esr52+
Attachment #8933331 - Flags: approval-mozilla-beta?
Attachment #8933331 - Flags: approval-mozilla-beta+
(In reply to Jan de Mooij [:jandem] from comment #21)
> Ehm this landed:
> 
> https://hg.mozilla.org/mozilla-central/rev/
> 26d3df5b58fb60facb9057dc1e86576127c9e1f5
Also: https://hg.mozilla.org/mozilla-central/rev/0b42ea549d67
This needs a rebased patch for ESR52.
Flags: needinfo?(jdemooij)
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main58]
Whiteboard: [jsbugmon:][adv-main58] → [jsbugmon:][adv-main58+]
Whiteboard: [jsbugmon:][adv-main58+] → [jsbugmon:][adv-main58+][adv-esr52.6+]
Flags: qe-verify-
Whiteboard: [jsbugmon:][adv-main58+][adv-esr52.6+] → [jsbugmon:][adv-main58+][adv-esr52.6+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.