1663862 - Crash [@ strlen] through [@ FuncTypeToString]

Status: VERIFIED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200908-dc90a7a18c07 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

var lfLogBuffer = `
  function f(x) {}
  var module = new WebAssembly.Module(wasmTextToBinary(\`(module
    (import "env" "d_ffd" (func \$dffd (param f32) (param f32) (param f64) (result f64)))
  )\`));
  for (let desc of WebAssembly.Module.imports(module)) {}
`;
oomTest(function() { eval(lfLogBuffer) });

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x0000555557d8bbea in FuncTypeToString (cx=cx@entry=0x7ffff6027000, funcType=...) at js/src/wasm/WasmJS.cpp:1154
#2  0x0000555557d8970f in js::WasmModuleObject::imports (cx=<optimized out>, cx@entry=0x7ffff6027000, argc=<optimized out>, vp=<optimized out>) at js/src/wasm/WasmJS.cpp:1243
#3  0x0000555556cdc972 in CallJSNative (cx=0x7ffff6027000, native=native@entry=0x555557d88e90 <js::WasmModuleObject::imports(JSContext*, unsigned int, JS::Value*)>, reason=<optimized out>, reason@entry=js::CallReason::Call, args=...) at js/src/vm/Interpreter.cpp:507
#4  0x0000555556cdc0a9 in js::InternalCallOrConstruct (cx=0x0, cx@entry=0x7ffff6027000, args=..., construct=construct@entry=js::NO_CONSTRUCT, reason=reason@entry=js::CallReason::Call) at js/src/vm/Interpreter.cpp:599
#5  0x0000555556cdd872 in InternalCall (cx=0x7ffff6027000, args=..., reason=reason@entry=js::CallReason::Call) at js/src/vm/Interpreter.cpp:664
#6  0x0000555556cd1534 in js::CallFromStack (cx=0x0, args=...) at js/src/vm/Interpreter.cpp:668
#7  Interpret (cx=0x0, cx@entry=0x7ffff6027000, state=...) at js/src/vm/Interpreter.cpp:3336
#8  0x0000555556cc8185 in js::RunScript (cx=cx@entry=0x7ffff6027000, state=...) at js/src/vm/Interpreter.cpp:468
#9  0x0000555556cdecf5 in js::ExecuteKernel (cx=0x7ffff6027000, script=script@entry=..., envChainArg=envChainArg@entry=..., newTargetValue=newTargetValue@entry=..., evalInFrame=evalInFrame@entry=..., result=...) at js/src/vm/Interpreter.cpp:856
#10 0x0000555556d44883 in EvalKernel (cx=0x0, cx@entry=0x7ffff6027000, v=v@entry=..., evalType=<optimized out>, evalType@entry=DIRECT_EVAL, caller=..., env=env@entry=..., pc=<optimized out>, vp=...) at js/src/builtin/Eval.cpp:348
#11 0x0000555556d45637 in js::DirectEval (cx=<optimized out>, v=..., vp=...) at js/src/builtin/Eval.cpp:466
#12 0x000055555772c90c in js::jit::DoCallFallback (cx=0x0, frame=0x7fffffffb170, stub=<optimized out>, argc=1, vp=0x7fffffffb120, res=...) at js/src/jit/BaselineIC.cpp:3004
#13 0x00000c188977f3b3 in ?? ()
[...]
#36 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x0	0
rcx	0x0	0
rdx	0x7ffff7f16000	140737353179136
rsi	0x0	0
rdi	0x0	0
rbp	0x7fffffff9a60	140737488329312
rsp	0x7fffffff9978	140737488329080
r8	0x0	0
r9	0x2	2
r10	0x0	0
r11	0x7ffff6ed3f01	140737336131329
r12	0x1fffff7d	536870781
r13	0x7ffff5630060	140737310294112
r14	0x4	4
r15	0xc	12
rip	0x7ffff6dca746 <strlen+38>
=> 0x7ffff6dca746 <strlen+38>:	movdqu (%rax),%xmm4
   0x7ffff6dca74a <strlen+42>:	pcmpeqb %xmm0,%xmm4

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

That's a little distressing. Will look at this.

Comment 3

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200908215255-dc90a7a18c07.
The bug appears to have been introduced in the following build range:
> Start: 5439013bde201811d284837e703b3f6f0e96e297 (20200818172807)
> End: 048c2bf9129090d91f2111579e130d913c871cdd (20200818172922)
> Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5439013bde201811d284837e703b3f6f0e96e297&tochange=048c2bf9129090d91f2111579e130d913c871cdd

Comment 4

[Lars T Hansen [:lth]]

Oh, that thing. Ryan, can you take a look? A fix will need to be uplifted to beta.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1561521

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: Bug 1663862 - Check OOM from wasm::ToString(). r?lth

wasm::ToCString() couldn't OOM, but wasm::ToString() can, so the
all the use cases need to check for OOM and propagate it.

Comment 7

[Lars T Hansen [:lth]]

At most this is an NPE so not something that needs review for Nightly. I'm just going to land + request Beta uplift.

Comment 8

[Lars T Hansen [:lth]]

(Arguably not a security issue at all.)

Comment 9

[Lars T Hansen [:lth]]

Comment on attachment 9174790 [details]
Bug 1663862 - Check OOM from wasm::ToString(). r?lth

Beta/Release Uplift Approval Request

• User impact if declined: Out-of-memory condition in webassembly can result in dereference of null pointer; tab crashes.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This inserts correct guards following some string allocations and propagates an error that was previously left unpropagated.
• String changes made/needed:

Comment 10

[Jens Stutte [:jstutte]]

(In reply to Lars T Hansen [:lth] from comment #8)

(Arguably not a security issue at all.)

I was looking at this crash report and read EXCEPTION_ACCESS_VIOLATION_WRITE on 0x3599b450 suspecting a UAF. I did no further analysis, though.

Comment 11

[Lars T Hansen [:lth]]

(In reply to Jens Stutte [:jstutte] (REO for FF 81) from comment #10)

(In reply to Lars T Hansen [:lth] from comment #8)

(Arguably not a security issue at all.)

I was looking at this crash report and read EXCEPTION_ACCESS_VIOLATION_WRITE on 0x3599b450 suspecting a UAF. I did no further analysis, though.

No criticism intended! Just observing that this looks a benign NPE.

Comment 12

[Jens Stutte [:jstutte]]

No worries! I was just wondering, if the associated crashes are then relevant for the testcase. At least the one I linked above looks different, being the second frame js::jit::IonCompile. So maybe we should look at them, too?

Comment 13

[Lars T Hansen [:lth]]

Hm, all the ToCString calls in FuncTypeToString were removed by the regressing patch, so I don't know what might be going on there. Ryan, perhaps you can give it a look and make sure we're in the clear?

Comment 14

[Jens Stutte [:jstutte]]

The crash report I've seen is from FF 79 (before the regressing patch). Is that the same patch that removed ToCString ? Then we are probably ok.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/d9612ded4ee87528a9519a7e6c60480fcc7acf15
https://hg.mozilla.org/mozilla-central/rev/d9612ded4ee8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9174790 [details]
Bug 1663862 - Check OOM from wasm::ToString(). r?lth

Approved for 81.0b9.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f295c71e4abd

Comment 18

[Ryan Hunt [:rhunt] (on leave until early May)]

Took a look at that crash report and I'm not sure what's going on there. The call stack seems incorrect to me, the FuncTypeToString function is being called directly by IonCompile, which is not possible. That taken along with the crash happening before ToCString was removed leads me to think that we don't have to worry about this.

Comment 19

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200912092623-6f8fba692420.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.