AddressSanitizer: TRAP on unknown address in js::wasm::Instance::callExport
Categories
(Core :: JavaScript: WebAssembly, defect, P1)
Tracking
()
People
(Reporter: eternalsakuraalpha, Assigned: bvisness)
References
(Regression)
Details
(Keywords: regression, reporter-external, sec-moderate, Whiteboard: [client-bounty-form][adv-main135+][adv-ESR128.7+])
Attachments
(5 files, 1 obsolete file)
Reproduce
- Download linux-asan-debug spidermonkey
- run poc.js
ASAN LOG
/home/sakura/.cache/autobisect/builds/js-m-c-linux-asan-debug-b16c09f16ef5/dist/bin/js poc.js
AddressSanitizer:DEADLYSIGNAL
=================================================================
==1915891==ERROR: AddressSanitizer: TRAP on unknown address 0x000000000000 (pc 0x1ef308b5010d bp 0x7ffe78c211d0 sp 0x7ffe78c21150 T0)
/home/sakura/.cache/autobisect/builds/js-m-c-linux-asan-debug-b16c09f16ef5/dist/bin/llvm-symbolizer: error: '[anon:js-executable-memory]': No such file or directory
#0 0x1ef308b5010d ([anon:js-executable-memory]+0x2010d)
#1 0x1ef308b50b3e ([anon:js-executable-memory]+0x20b3e)
#2 0x557cdc9c01df in js::wasm::Instance::callExport(JSContext*, unsigned int, JS::CallArgs const&, js::wasm::CoercionLevel) /builds/worker/checkouts/gecko/js/src/wasm/WasmInstance.cpp:3695:10
#3 0x557cdc9be4c3 in WasmCall(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/js/src/wasm/WasmInstance.cpp:3479:19
#4 0x557cd986c122 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:532:13
#5 0x557cd986ada6 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:628:12
#6 0x557cd988cfcc in js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:700:10
#7 0x557cd988cfcc in js::Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3338:16
#8 0x557cd98690a5 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:502:13
#9 0x557cd98714e4 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:893:13
#10 0x557cd9872179 in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:926:10
#11 0x557cd9bb9b0a in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/CompilationAndEvaluation.cpp:601:10
#12 0x557cd9bb9f8f in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) /builds/worker/checkouts/gecko/js/src/vm/CompilationAndEvaluation.cpp:625:10
#13 0x557cd95ddbae in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) /builds/worker/checkouts/gecko/js/src/shell/js.cpp:1307:10
#14 0x557cd95dc555 in Process(JSContext*, char const*, bool, FileKind) /builds/worker/checkouts/gecko/js/src/shell/js.cpp
#15 0x557cd9501512 in ProcessArgs(JSContext*, js::cli::OptionParser*) /builds/worker/checkouts/gecko/js/src/shell/js.cpp:11741:10
#16 0x557cd9501512 in Shell(JSContext*, js::cli::OptionParser*) /builds/worker/checkouts/gecko/js/src/shell/js.cpp:11993:12
#17 0x557cd94ef02e in main /builds/worker/checkouts/gecko/js/src/shell/js.cpp:12408:12
#18 0x7fc795a29d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
==1915891==Register values:
rax = 0x000033fc75300080 rbx = 0x00002241d35710e0 rcx = 0x0000507000000480 rdx = 0x0000507000004310
rdi = 0x0000000000000000 rsi = 0x0000000000000000 rbp = 0x00007ffe78c211d0 rsp = 0x00007ffe78c21150
r8 = 0x00000fffcf1842dd r9 = 0x00007ffe78c21700 r10 = 0x00007ffe78c21428 r11 = 0x00000000ffffffff
r12 = 0x00007ffe78c21248 r13 = 0x00007ffe78c21428 r14 = 0x0000519000086b80 r15 = 0x00007fc58b3ab000
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: TRAP ([anon:js-executable-memory]+0x2010d)
==1915891==ABORTING
Updated•1 year ago
|
Updated•1 year ago
|
Comment 1•1 year ago
|
||
Here's the same test using the text format. I wasn't able to repro on OSX debug ASAN locally, going to switch to my linux machine.
Updated•1 year ago
|
Comment 3•1 year ago
|
||
struct.new_default 1
local.set 0
loop
local.get 0
ref.cast i31ref
i31.get_s
i32.const 0
i32.store8
br 0
end
The issue here is that we're somehow hoisting the i31.get_s operation to happen before the cast operation (which should always fail). I do not know why that is happening.
Updated•1 year ago
|
| Assignee | ||
Comment 4•1 year ago
•
|
||
It seems to simply be that MWasmI31RefGet generated by i31.get_s is marked as movable, and gets hoisted above both the ref.cast and the MWasmTrapIfNull that is also generated. This is obviously wrong, but since the MWasmI31RefGet doesn't depend on either of those two instructions, and it's movable, LICM will move it up.
A quick fix is to make MWasmI31RefGet immovable like other ref accesses. A better fix would be to change how casting works, and how MIR is generated for i31.get_s, to ensure that we have explicit dependencies between these nodes in MIR.
As for security impact, it seems that i31.get_s and i31.get_u are the only ref-loading operations that are marked as movable. Struct/array loads and stores are not movable, table.set is not movable, call_indirect and call_ref are not movable, and other ref operations do nothing but treat it as an opaque value. This bug could possibly allow you to load a pointer value from outside the sandbox, but I don't believe it would allow you to actually load or call anything from the host.
It's also worth noting that I haven't yet come up with a way to actually do anything with the i31 value that was prematurely loaded. WebAssembly validation ensures that we must ref.cast before actually performing an operation such as a load, store, or call, and such operations are not also movable. So while the load can incorrectly occur too early, the cast will still fail, correctly, before the bad value can be exfiltrated.
| Assignee | ||
Comment 5•1 year ago
|
||
The following factors all make this bug less severe, in my opinion:
- It's somewhat difficult to repro even with the knowledge that
i31.getshould not be movable - There's no clear way to exfiltrate the data because:
- Exploiting this requires you to use
anyrefandref.castto prevent validation from failing oni31.get - The
ref.castwill still run correctly and trap before the result ofi31.getcan be used
- Exploiting this requires you to use
- Loading a host pointer value as an integer still does not directly allow you to load or call anything from the host
So, I think we would be ok to just ship the easy fix, even though removing movable: true makes the nature of the bug somewhat obvious.
| Assignee | ||
Comment 6•1 year ago
|
||
Updated•1 year ago
|
Comment 7•1 year ago
|
||
This was added by i31ref support which was shipped in Fx120 through bug 1845373.
Comment 8•1 year ago
|
||
Set release status flags based on info from the regressing bug 1692065
| Assignee | ||
Updated•1 year ago
|
Updated•1 year ago
|
| Assignee | ||
Comment 9•1 year ago
|
||
Original Revision: https://phabricator.services.mozilla.com/D232110
Updated•1 year ago
|
Comment 10•1 year ago
|
||
beta Uplift Approval Request
- User impact if declined: A bug with bad codegen could possibly be exploited in ways we currently cannot imagine.
- Code covered by automated testing: yes
- Fix verified in Nightly: no
- Needs manual QE test: no
- Steps to reproduce for manual QE testing: -
- Risk associated with taking this patch: Low to none
- Explanation of risk level: Because this patch makes an instruction immovable, it makes the code more predictable.
- String changes made/needed: n/a
- Is Android affected?: yes
Comment 11•1 year ago
|
||
Comment 12•1 year ago
|
||
Updated•1 year ago
|
Updated•1 year ago
|
Comment 13•1 year ago
|
||
Comment on attachment 9444088 [details]
Bug 1936454: Make i31.get immovable. r=rhunt
We're past the point of taking security uplifts for 134 this cycle, but I've repurposed this revision for the ESR128 uplift next cycle that we'll need to do still.
Updated•1 year ago
|
Updated•1 year ago
|
Comment 14•1 year ago
|
||
| uplift | ||
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Comment 15•1 year ago
|
||
Updated•1 year ago
|
Updated•8 months ago
|
Description
•