1767177 - AddressSanitizer: heap-buffer-overflow [@ new_<js::wasm::Stk>] with WRITE of size 4

Status: RESOLVED FIXED

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

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision 20220427-139c89a60b72 (build with fuzzing-asan-opt, run with --no-threads --wasm-compiler=baseline).

Backtrace:

    ==13340==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6250000278f8 at pc 0x55587aaa61df bp 0x7ffd201f9990 sp 0x7ffd201f9988
    WRITE of size 4 at 0x6250000278f8 thread T0
        #0 0x55587aaa61de in new_<js::wasm::Stk> /builds/worker/workspace/obj-build/dist/include/mozilla/Vector.h:58:30
        #1 0x55587aaa61de in infallibleEmplaceBack<js::wasm::Stk> /builds/worker/workspace/obj-build/dist/include/mozilla/Vector.h:707:5
        #2 0x55587aaa61de in push<js::wasm::Stk> /js/src/wasm/WasmBCStkMgmt-inl.h:44:8
        #3 0x55587aaa61de in pushF64 /js/src/wasm/WasmBCStkMgmt-inl.h:544:3
        #4 0x55587aaa61de in js::wasm::BaseCompiler::emitCatch() /js/src/wasm/WasmBaselineCompile.cpp:3958:9
        #5 0x55587aad92b0 in js::wasm::BaseCompiler::emitBody() /js/src/wasm/WasmBaselineCompile.cpp:8465:9
        #6 0x55587ab0e844 in emitFunction /js/src/wasm/WasmBaselineCompile.cpp:10192:8
        #7 0x55587ab0e844 in js::wasm::BaselineCompileFunctions(js::wasm::ModuleEnvironment const&, js::wasm::CompilerEnvironment const&, js::LifoAlloc&, mozilla::Vector<js::wasm::FuncCompileInput, 8ul, js::SystemAllocPolicy> const&, js::wasm::CompiledCode*, mozilla::UniquePtr<char [], JS::FreePolicy>*) /js/src/wasm/WasmBaselineCompile.cpp:10364:12
        [...]
        #26 0x555878fc50ee in main /js/src/shell/js.cpp:12702:12
    
    0x6250000278f8 is located 0 bytes to the right of 8184-byte region [0x625000025900,0x6250000278f8)
    allocated by thread T0 here:
        #0 0x555878f7333e in __interceptor_malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
        #1 0x55587aba3dc9 in mozilla::detail::VectorImpl<js::wasm::Stk, 0ul, js::SystemAllocPolicy, false>::growTo(mozilla::Vector<js::wasm::Stk, 0ul, js::SystemAllocPolicy>&, unsigned long) /builds/worker/workspace/obj-build/dist/include/mozilla/Vector.h:123:29
        #2 0x55587aa862fa in reserve /builds/worker/workspace/obj-build/dist/include/mozilla/Vector.h:1071:9
        #3 0x55587aa862fa in js::wasm::BaseCompiler::pushResults(js::wasm::ResultType, js::wasm::StackHeight) /js/src/wasm/WasmBaselineCompile.cpp:1154:15
        #4 0x55587aaada2f in js::wasm::BaseCompiler::emitCall() /js/src/wasm/WasmBaselineCompile.cpp:4627:10
        #5 0x55587aad9435 in js::wasm::BaseCompiler::emitBody() /js/src/wasm/WasmBaselineCompile.cpp:8507:9
        #6 0x55587ab0e844 in emitFunction /js/src/wasm/WasmBaselineCompile.cpp:10192:8
        [...]
        #26 0x555878fc50ee in main /js/src/shell/js.cpp:12702:12
    
    SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/workspace/obj-build/dist/include/mozilla/Vector.h:58:30 in new_<js::wasm::Stk>
    Shadow bytes around the buggy address:
      0x0c4a7fffcef0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
      0x0c4a7fffcf00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    =>0x0c4a7fffcf10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00[fa]
      0x0c4a7fffcf20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
      0x0c4a7fffcf30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    Shadow byte legend (one shadow byte represents 8 application bytes):
      Addressable:           00
      Heap left redzone:       fa

This was found by wasm-smith.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Lars T Hansen [:lth]]

This usually means somebody forgot to reserve space on the eval stack before pushing an unbounded number of items. I'll take a look. We'll want to uplift a fix for this.

Comment 4

[Lars T Hansen [:lth]]

Attachment: Bug 1767177 - Accommodate catch values. r?yury

Comment 5

[Lars T Hansen [:lth]]

Caught by an assert in a debug build, too.

It's not the first time we've made this mistake. It's definitely not desirable to have release asserts in the value pusher because that code is white-hot; at the same time, we don't want to keep making this mistake. I'm not sure how to resolve the tension of that.

I've uploaded the obvious fix for review, and I may strip comments before landing. The TC can land some other time.

Comment 6

[Lars T Hansen [:lth]]

Exception handling is riding the trains for FF100 so I think we're affected back to that. This bug may be used to scribble past the limit of the evaluation stack, a compiler data structure, and may be used to overwrite other compiler data structures. Exploit potential is unclear, as the data being overwritten are hard to control - memory allocation will tend to be chaotic - and the data being written are not quite within attacker control, though they are deterministic.

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 9274607 [details]
Bug 1767177 - Accommodate catch values. r?yury

Security Approval Request

• How easily could an exploit be constructed based on the patch?: A DOS is probably trivial, as an attacker should be able to scribble on other compiler data structures, but the attacker is very constrained in the values being written, and the heap is chaotic in a multi-threaded compilation scenario, so it may be quite difficult to break into the process. Luck will be a factor.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: 100, 101, 102
• If not all supported branches, which bug introduced the flaw?: Bug 1335652
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Very low risk, needs no special testing.
• Is Android affected?: Yes

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 9274607 [details]
Bug 1767177 - Accommodate catch values. r?yury

Beta/Release Uplift Approval Request

• User impact if declined: Minimally some DOS potential, possibly something more serious.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): It only reserves space in a compiler-internal data structure, no logic change beyond that.
• String changes made/needed:
• Is Android affected?: Yes

Comment 9

[Lars T Hansen [:lth]]

Comment on attachment 9274607 [details]
Bug 1767177 - Accommodate catch values. r?yury

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Minimally some DOS potential, possibly something worse.
• Fix Landed on Version: 102
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Reserves space in an internal data structure; no logic change.

Comment 10

[Lars T Hansen [:lth]]

I'm of two minds about uplifting to release; this may be worth taking if a dot release is created for other reasons, but it likely does not warrant a dot release on its own, absent other information.

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 9274607 [details]
Bug 1767177 - Accommodate catch values. r?yury

Approved to land and uplift

Comment 12

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

Accommodate catch values. r=yury
https://hg.mozilla.org/integration/autoland/rev/2aa2440bdc55743b40ba02481820a47a4dfa11cc
https://hg.mozilla.org/mozilla-central/rev/2aa2440bdc55

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Landed for 101.0b3.
https://hg.mozilla.org/releases/mozilla-beta/rev/8c2b581462ff

Comment 14

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 1767177 using build mozilla-central 20220427094429-139c89a60b72. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.