Assertion failure: act->asJit()->hasWasmExitFP(), at js/src/wasm/WasmBuiltins.cpp:68

VERIFIED FIXED in Firefox 60

Status

()

defect
P1
critical
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: decoder, Assigned: bbouvier)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla61
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(thunderbird_esr52 unaffected, firefox-esr52 unaffected, firefox59 unaffected, firefox60+ verified, firefox61+ verified)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
The following testcase crashes on mozilla-central revision 8063b0c54b88 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

function wasmEvalText(str, imports) {
  let binary = wasmTextToBinary(str);
  m = new WebAssembly.Module(binary);
  return new WebAssembly.Instance(m, imports);
}
var { table } = wasmEvalText(`(module
    (func $add (result i32) (param i32) (param i32)
     get_local 0
    )
    (table (export "table") 10 anyfunc)
    (elem (i32.const 0) $add)
)`).exports;
for (var i = 0; i < 100; i++) {
  table.get(0)((true).get++, i*2+1);
}


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000d67730 in CallingActivation () at js/src/wasm/WasmBuiltins.cpp:68
#0  0x0000000000d67730 in CallingActivation () at js/src/wasm/WasmBuiltins.cpp:68
#1  0x0000000000d6b5ee in CoerceInPlace_JitEntry (funcExportIndex=0, tlsData=0x7ffff4947000, argv=0x7fffffffc4a8) at js/src/wasm/WasmBuiltins.cpp:345
#2  0x000038b192d9e23a in ?? ()
[...]
#18 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff5f15000	140737319620608
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffc420	140737488340000
rsp	0x7fffffffc410	140737488339984
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7fffffffc4a8	140737488340136
r13	0x7ffff4947000	140737296756736
r14	0x0	0
r15	0x7ffff486e028	140737295867944
rip	0xd67730 <CallingActivation()+144>
=> 0xd67730 <CallingActivation()+144>:	movl   $0x0,0x0
   0xd6773b <CallingActivation()+155>:	ud2


Not sure what this assert exactly means but it is jit-related and the stack doesn't have a lot of useful information either, marking s-s until investigated.
Flags: needinfo?(bbouvier)
(Assignee)

Comment 1

a year ago
Embarrassing: a lazy jit entry stub should use the builtin thunk for the symbolic address, not the actual function itself, so FP can be marked as wasm.

I think that in the worst case, the jit frame iterator (if used from an inline conversion) will be confused and misinterpret wasm frames as jit frames when it starts unwinding. This should result in infinite stack unwinding, which always ends up with a crash in debug builds (probably in release builds too).

Will make a patch.
Flags: needinfo?(bbouvier)
(Assignee)

Comment 2

a year ago
Posted patch 1.tests.patchSplinter Review
Three changes here:

- one test for CoerceInPlace, which is the one from comment 0.
- another test for ReportI64, which can also be called from the jit entry, and exposed a crash for the reason explained in the previous comment (infinite unwinding, then an assert blows up or we exhaust the stack when unwinding it).
- tweaks to the existing test that reports i64 from the jit entry; it wasn't used anymore, probably due to changes in ion. With this change, I can confirm it does expose it again (thanks to a printf in the C++ ReportI64 function).

Putting tests in a separate patch just in case we uplift/land in two times.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8964617 - Flags: review?(luke)
(Assignee)

Comment 3

a year ago
Posted patch 2.fix.patch (obsolete) — Splinter Review
When generating a lazy jit entry, we need to use the builtin thunk of the symbolic address we're calling, and not the raw pointer to the function itself (otherwise we lose FP information).
Attachment #8964619 - Flags: review?(luke)
Attachment #8964617 - Flags: review?(luke) → review+
Comment on attachment 8964619 [details] [diff] [review]
2.fix.patch

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

I wonder if there are different names that make the "what you call from a module" / "what you call from the builtin thunk" distinction more clear?
Attachment #8964619 - Flags: review?(luke) → review+
(Assignee)

Comment 5

a year ago
Thanks for the reviews!

(In reply to Luke Wagner [:luke] from comment #4)
> Comment on attachment 8964619 [details] [diff] [review]
> 2.fix.patch
> 
> Review of attachment 8964619 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wonder if there are different names that make the "what you call from a
> module" / "what you call from the builtin thunk" distinction more clear?

Your question showed me that we can remove wasm::AddressOf from WasmBuiltins.h, and have it static only in WasmBuiltins.cpp, which makes it clearer that's it private. Will fold this change in and carry forward r+; please let me know if you disagree about this change.
Keywords: sec-high
(Assignee)

Updated

a year ago
Blocks: 1422043
(Assignee)

Comment 6

a year ago
Posted patch 2.fix.patchSplinter Review
Carrying forward r+ from Luke.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's a bit hard to understand all the implications from this patch, but one could make a test case that triggers the issue with some effort (read the code and think about possible interactions). Any wasm fuzzer might hit the crash in a debug build quickly, though.

Once the issue has been triggered, it's not quite obvious how it could be exploited: the VM thinks it's looking at JIT frames while it's actually looking at WASM frames (I think it would lead to a crash very quickly). Jan told me this kind of problem is generally sec-high, so being cautious here.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Tests stand in a separate patch. The changeset message has been redacted to just contain the bug number.

Which older supported branches are affected by this flaw? If not all supported branches, which bug introduced the flaw?
Bug 1422043 introduced the issue, so 60 is affected.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No backports, but the same patch should hopefully properly apply.

How likely is this patch to cause regressions; how much testing does it need?
Very very low risk (~one liner). Tests contained in the other patch all passed with this patch, on all platforms. Local testing showed no other issues with this patch.
Attachment #8964619 - Attachment is obsolete: true
Attachment #8964627 - Flags: sec-approval?
Attachment #8964627 - Flags: review+
Comment on attachment 8964627 [details] [diff] [review]
2.fix.patch

sec-approval+ for trunk. Please nominate for beta so we can avoid ever shipping the flaw.
Attachment #8964627 - Flags: sec-approval? → sec-approval+
Priority: -- → P1
(Assignee)

Comment 8

a year ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/37fe16ee889e1810b7350e5fe7932d4a98e974dd

Thanks for sec approval. Fix landed. When would be a good time to land the tests?
Flags: needinfo?(abillings)
(Assignee)

Comment 9

a year ago
Comment on attachment 8964627 [details] [diff] [review]
2.fix.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1422043
[User impact if declined]: see sec-approval comment
[Is this code covered by automated tests?]: yes, in separate patch
[Has the fix been verified in Nightly?]: not yet, but locally verified in shell
[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?]: not much
[Why is the change risky/not risky?]: the meaningful change is a one-liner and the patch has been well tested
[String changes made/needed]: none
Attachment #8964627 - Flags: approval-mozilla-beta?
Comment on attachment 8964627 [details] [diff] [review]
2.fix.patch

wasm sec fix, approved for 60.0b11
Attachment #8964627 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

a year ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 12

a year ago
JSBugMon: Bisection requested, failed due to error (try manually).
(In reply to Benjamin Bouvier [:bbouvier] from comment #8)
> remote:  
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 37fe16ee889e1810b7350e5fe7932d4a98e974dd
> 
> Thanks for sec approval. Fix landed. When would be a good time to land the
> tests?

Probably three or four weeks after we ship a release with the fix. We want it out and in public before we check in a test that points to the problem. 60 ships on May 9 so we're looking at early June.
Flags: needinfo?(abillings)
https://hg.mozilla.org/mozilla-central/rev/37fe16ee889e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

Updated

a year ago
Status: RESOLVED → VERIFIED

Comment 15

a year ago
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release

Comment 16

a year ago
JSBugMon: This bug has been automatically verified fixed on Fx60
Group: core-security-release
Flags: in-testsuite?
(Assignee)

Updated

11 months ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.