Closed Bug 1797685 Opened 2 years ago Closed 1 year ago

AddressSanitizer: heap-use-after-free [@ (anonymous namespace)::FunctionCompiler::delegatePadPatches] with READ of size 8

Categories

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

Product:
Core
Component:
JavaScript: WebAssembly
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
109 Branch
Tracking Flags:
Tracking Status
firefox-esr102 108+ fixed
firefox106 --- wontfix
firefox107 --- wontfix
firefox108 + fixed
firefox109 + fixed

People

(Reporter: decoder, Assigned: rhunt)

References

(Regression)

Details

(6 keywords, Whiteboard: [jsbugmon:update,bisect][post-critsmash-triage][adv-main108+r][adv-esr102.6+r])

Attachments

(4 files)

Detailed Crash Information
15.12 KB, text/plain
Details
test.zip
14.13 KB, application/zip
Details
Bug 1797685 - wasm: Fix validation condition in decodeDelegate. r?jseward
48 bytes, text/x-phabricator-request
diannaS
: approval-mozilla-beta+
dmeehan
: approval-mozilla-esr102+
tjr
: sec-approval+
Details | Review
Bug 1797685 - wasm: Add test. r?jseward
48 bytes, text/x-phabricator-request
Details | Review

The attached testcase crashes on mozilla-central revision 20221027-68d82e6cb0c9 (build with asan-opt, run with --no-threads test.js).

Backtrace:

    ==32005==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000021a20 at pc 0x565483e5deeb bp 0x7ffd54b50b70 sp 0x7ffd54b50b68
    READ of size 8 at 0x60c000021a20 thread T0
        #0 0x565483e5deea in (anonymous namespace)::FunctionCompiler::delegatePadPatches(mozilla::Vector<js::jit::MControlInstruction*, 8ul, js::SystemAllocPolicy> const&, unsigned int) js/src/wasm/WasmIonCompile.cpp:2836:35
        #1 0x565483e2e15f in EmitDelegate((anonymous namespace)::FunctionCompiler&) js/src/wasm/WasmIonCompile.cpp:3800:12
        #2 0x565483de82f8 in EmitBodyExprs((anonymous namespace)::FunctionCompiler&) js/src/wasm/WasmIonCompile.cpp:5891:14
        #3 0x565483de6d56 in js::wasm::IonCompileFunctions(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/WasmIonCompile.cpp:7103:12
        #4 0x565483da372d in ExecuteCompileTask(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmGenerator.cpp:664:12
        #5 0x565483da398d in js::wasm::ModuleGenerator::locallyCompileCurrentTask() js/src/wasm/WasmGenerator.cpp:719:8
        #6 0x565483da4795 in js::wasm::ModuleGenerator::finishFuncDefs() js/src/wasm/WasmGenerator.cpp:850:24
        #7 0x565483d7b421 in bool DecodeCodeSection<js::wasm::Decoder>(js::wasm::ModuleEnvironment const&, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:686:13
        #8 0x565483d7addc in js::wasm::CompileBuffer(js::wasm::CompileArgs const&, js::wasm::ShareableBytes const&, mozilla::UniquePtr<char [], JS::FreePolicy>*, mozilla::Vector<mozilla::UniquePtr<char [], JS::FreePolicy>, 0ul, js::SystemAllocPolicy>*, JS::OptimizedEncodingListener*) js/src/wasm/WasmCompile.cpp:708:8
        #9 0x565483dfa5da in js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*) js/src/wasm/WasmJS.cpp:1713:7
        #10 0x56548270ee19 in CallJSNative js/src/vm/Interpreter.cpp:459:13
        [...]
        #22 0x56548237127c in main js/src/shell/js.cpp:12320:12
    
    0x60c000021a20 is located 32 bytes inside of 128-byte region [0x60c000021a00,0x60c000021a80)
    freed by thread T0 here:
        #0 0x56548231e656 in __interceptor_realloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:85:3
        #1 0x565483e5c176 in js_arena_realloc dist/include/js/Utility.h:402:10
        #2 0x565483e5c176 in js_pod_arena_realloc<js::jit::MControlInstruction *> dist/include/js/Utility.h:608:26
        #3 0x565483e5c176 in maybe_pod_arena_realloc<js::jit::MControlInstruction *> dist/include/js/AllocPolicy.h:42:12
        #4 0x565483e5c176 in pod_arena_realloc<js::jit::MControlInstruction *> dist/include/js/AllocPolicy.h:55:12
        #5 0x565483e5c176 in pod_realloc<js::jit::MControlInstruction *> dist/include/js/AllocPolicy.h:80:12
        #6 0x565483e5c176 in growTo dist/include/mozilla/Vector.h:305:21
        #7 0x565483e5c176 in mozilla::Vector<js::jit::MControlInstruction*, 8ul, js::SystemAllocPolicy>::growStorageBy(unsigned long) dist/include/mozilla/Vector.h:1066:10
        #8 0x565483e5bede in growByUninitialized dist/include/mozilla/Vector.h:1172:9
        #9 0x565483e5bede in bool mozilla::Vector<js::jit::MControlInstruction*, 8ul, js::SystemAllocPolicy>::emplaceBack<js::jit::MControlInstruction*&>(js::jit::MControlInstruction*&) dist/include/mozilla/Vector.h:765:10
        #10 0x565483e5bda8 in (anonymous namespace)::FunctionCompiler::addPadPatch(js::jit::MControlInstruction*, unsigned long) js/src/wasm/WasmIonCompile.cpp:2813:23
        #11 0x565483e5ddf6 in (anonymous namespace)::FunctionCompiler::delegatePadPatches(mozilla::Vector<js::jit::MControlInstruction*, 8ul, js::SystemAllocPolicy> const&, unsigned int) js/src/wasm/WasmIonCompile.cpp:2837:12
        #12 0x565483e2e15f in EmitDelegate((anonymous namespace)::FunctionCompiler&) js/src/wasm/WasmIonCompile.cpp:3800:12
        #13 0x565483de82f8 in EmitBodyExprs((anonymous namespace)::FunctionCompiler&) js/src/wasm/WasmIonCompile.cpp:5891:14
        #14 0x565483de6d56 in js::wasm::IonCompileFunctions(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/WasmIonCompile.cpp:7103:12
        #15 0x565483da372d in ExecuteCompileTask(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmGenerator.cpp:664:12
        #16 0x565483da398d in js::wasm::ModuleGenerator::locallyCompileCurrentTask() js/src/wasm/WasmGenerator.cpp:719:8
        #17 0x565483da4795 in js::wasm::ModuleGenerator::finishFuncDefs() js/src/wasm/WasmGenerator.cpp:850:24
        #18 0x565483d7b421 in bool DecodeCodeSection<js::wasm::Decoder>(js::wasm::ModuleEnvironment const&, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:686:13
        #19 0x565483d7addc in js::wasm::CompileBuffer(js::wasm::CompileArgs const&, js::wasm::ShareableBytes const&, mozilla::UniquePtr<char [], JS::FreePolicy>*, mozilla::Vector<mozilla::UniquePtr<char [], JS::FreePolicy>, 0ul, js::SystemAllocPolicy>*, JS::OptimizedEncodingListener*) js/src/wasm/WasmCompile.cpp:708:8
        #20 0x565483dfa5da in js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*) js/src/wasm/WasmJS.cpp:1713:7
        #21 0x56548270ee19 in CallJSNative js/src/vm/Interpreter.cpp:459:13
        [...]
        #33 0x56548237127c in main js/src/shell/js.cpp:12320:12
    
    previously allocated by thread T0 here:
        #0 0x56548231e38e in malloc /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
        #1 0x565483e5c25c in js_arena_malloc dist/include/js/Utility.h:366:10
        #2 0x565483e5c25c in js_pod_arena_malloc<js::jit::MControlInstruction *> dist/include/js/Utility.h:576:26
        #3 0x565483e5c25c in maybe_pod_arena_malloc<js::jit::MControlInstruction *> dist/include/js/AllocPolicy.h:33:12
        #4 0x565483e5c25c in pod_arena_malloc<js::jit::MControlInstruction *> dist/include/js/AllocPolicy.h:46:12
        #5 0x565483e5c25c in pod_malloc<js::jit::MControlInstruction *> dist/include/js/AllocPolicy.h:72:12
        #6 0x565483e5c25c in mozilla::Vector<js::jit::MControlInstruction*, 8ul, js::SystemAllocPolicy>::convertToHeapStorage(unsigned long) dist/include/mozilla/Vector.h:1025:30
        #7 0x565483e5bede in growByUninitialized dist/include/mozilla/Vector.h:1172:9
        #8 0x565483e5bede in bool mozilla::Vector<js::jit::MControlInstruction*, 8ul, js::SystemAllocPolicy>::emplaceBack<js::jit::MControlInstruction*&>(js::jit::MControlInstruction*&) dist/include/mozilla/Vector.h:765:10
        #9 0x565483e5bda8 in (anonymous namespace)::FunctionCompiler::addPadPatch(js::jit::MControlInstruction*, unsigned long) js/src/wasm/WasmIonCompile.cpp:2813:23
        #10 0x565483e5ddf6 in (anonymous namespace)::FunctionCompiler::delegatePadPatches(mozilla::Vector<js::jit::MControlInstruction*, 8ul, js::SystemAllocPolicy> const&, unsigned int) js/src/wasm/WasmIonCompile.cpp:2837:12
        #11 0x565483e2e15f in EmitDelegate((anonymous namespace)::FunctionCompiler&) js/src/wasm/WasmIonCompile.cpp:3800:12
        #12 0x565483de82f8 in EmitBodyExprs((anonymous namespace)::FunctionCompiler&) js/src/wasm/WasmIonCompile.cpp:5891:14
        #13 0x565483de6d56 in js::wasm::IonCompileFunctions(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/WasmIonCompile.cpp:7103:12
        #14 0x565483da372d in ExecuteCompileTask(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmGenerator.cpp:664:12
        #15 0x565483da398d in js::wasm::ModuleGenerator::locallyCompileCurrentTask() js/src/wasm/WasmGenerator.cpp:719:8
        #16 0x565483da4795 in js::wasm::ModuleGenerator::finishFuncDefs() js/src/wasm/WasmGenerator.cpp:850:24
        #17 0x565483d7b421 in bool DecodeCodeSection<js::wasm::Decoder>(js::wasm::ModuleEnvironment const&, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:686:13
        #18 0x565483d7addc in js::wasm::CompileBuffer(js::wasm::CompileArgs const&, js::wasm::ShareableBytes const&, mozilla::UniquePtr<char [], JS::FreePolicy>*, mozilla::Vector<mozilla::UniquePtr<char [], JS::FreePolicy>, 0ul, js::SystemAllocPolicy>*, JS::OptimizedEncodingListener*) js/src/wasm/WasmCompile.cpp:708:8
        #19 0x565483dfa5da in js::WasmModuleObject::construct(JSContext*, unsigned int, JS::Value*) js/src/wasm/WasmJS.cpp:1713:7
        #20 0x56548270ee19 in CallJSNative js/src/vm/Interpreter.cpp:459:13
        #21 0x56548270ee19 in CallJSNativeConstructor js/src/vm/Interpreter.cpp:475:8
        [...]
        #33 0x56548237127c in main js/src/shell/js.cpp:12320:12
    
    SUMMARY: AddressSanitizer: heap-use-after-free js/src/wasm/WasmIonCompile.cpp:2836:35 in (anonymous namespace)::FunctionCompiler::delegatePadPatches(mozilla::Vector<js::jit::MControlInstruction*, 8ul, js::SystemAllocPolicy> const&, unsigned int)
    Shadow bytes around the buggy address:
      0x0c187fffc330: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
    =>0x0c187fffc340: fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd
      0x0c187fffc350: 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
      Freed heap region:       fd
    ==32005==ABORTING

Missing testcase.

Attached file test.zip —

From an initial scan, it looks like Control::tryPadPatches is being modified
at the same time that we're iterating through it, hence causing its contents
array to be realloc'd somewhere else, while the iteration is still reading
the old version.

EmitDelegate
-> delegatePadPatches(control.tryPadPatches)
   -> [iterates over the vector of patches]
      -> addPadPatch
         -> tryControl.tryPadPatches.emplaceBack(..)

I downloaded the test case and I keep getting 'illegal opcode: 0xff'. I've tried it in SM and also dumping out the buffer that would be compiled and disassembling it with wasm-tools. Did you attach the correct test case or am I missing something? The opcode 0xff is an illegal one, so I don't think I'm missing a feature flag.

Flags: needinfo?(choller)

(In reply to Ryan Hunt [:rhunt] from comment #5)

I downloaded the test case and I keep getting 'illegal opcode: 0xff'. I've tried it in SM and also dumping out the buffer that would be compiled and disassembling it with wasm-tools. Did you attach the correct test case or am I missing something? The opcode 0xff is an illegal one, so I don't think I'm missing a feature flag.

Did you run this through an ASan build?

Flags: needinfo?(choller) → needinfo?(rhunt)

I got the message about the illegal opcode too.

I didn't even bother with an ASan build; on Linux you can get the UAF reports merely by running on valgrind. I reproduced it like that. You do need to configure with --enable-valgrind --disable-jemalloc though.

Assignee: nobody → rhunt

I'm traveling today, and my ASAN build on my MacBook doesn't seem to terminate on the example, which is strange. I may need to try this again when I get back. Looking into it still. The illegal opcode makes it difficult to understand what is going on in the module, as I can't disassemble it to reconstruct what Ion is doing without running it.

Flags: needinfo?(rhunt)

Have been booked with meetings so far this week, but finally got time to run it in ASAN on my linux desktop and can reproduce it now.

The issue is with integer overflow causing wraparound to 0, causing a routine to mutate the vector it is iterating over.

We use 'relativeDepth' as the index for the control stack when validating.
A relative depth of 0 is equal to the last element in the control stack.

The target depth immediate for delegate starts at the enclosing block
of the try block, so we add 1 to get the relativeDepth. However, this
can overflow when it is provided as UINT32_MAX. This results in
confusion in the outside code where we can attempt to move pad patches
from a try block to itself, resulting in iterating over a vector that
we are also mutating.

This commit fixes the issue by ensuring the delegateDepth is valid
before incrementing it to a relativeDepth.

Severity: -- → S2
Priority: -- → P1

It looks like this is a regression from the initial implementation of wasm-exceptions that rode the trains in version 100.

status-firefox106: --- → affected
status-firefox107: --- → affected
status-firefox-esr102: --- → affected
Regressed by: 1759217

Comment on attachment 9301647 [details]
Bug 1797685 - wasm: Fix validation condition in decodeDelegate. r?jseward

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Assuming we remove the test case and commit message before landing, I still think the patch is suspicious for integer overflow but it's not obvious. From there some investigation would be required to discover that it can lead to growing a vector while iterating it, and then an attacker would need to find a way to turn the read-after-free into a general exploit.
  • 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?: Regressed in Fx100, so ESR, beta, and release
  • If not all supported branches, which bug introduced the flaw?: None
  • 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?: The patch is simple, and we have good tests for the semantics for 'delegate' instructions.
  • Is Android affected?: Yes
Attachment #9301647 - Flags: sec-approval?

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

status-firefox109: --- → affected

Comment on attachment 9301647 [details]
Bug 1797685 - wasm: Fix validation condition in decodeDelegate. r?jseward

I'll approve it assuming you remove the test and commit message, but in the future please prepare the patch for approval as it would land. (So put the comment in the bug instead of the description, and separate the test out into a separate patch.)

Attachment #9301647 - Flags: sec-approval? → sec-approval+
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][reminder-test 2023-02-01]
Attachment #9301647 - Attachment description: Bug 1797685 - wasm: Don't overflow relativeDepth when validating decodeDelegate. r?jseward → Bug 1797685 - wasm: Fix validation condition in decodeDelegate. r?jseward

Okay, will do that in the future. I've now removed the test and made the commit message to be generic, (just stating we are fixing something in validation, not what). Also removed the extended commit message and will post it here for posterity.

The commit without the tests should now be ready to land.

We use 'relativeDepth' as the index for the control stack when validating.
A relative depth of 0 is equal to the last element in the control stack.

The target depth immediate for `delegate` starts at the enclosing block
of the try block, so we add 1 to get the relativeDepth. However, this
can overflow when it is provided as UINT32_MAX. This results in
confusion in the outside code where we can attempt to move pad patches
from a try block to itself, resulting in iterating over a vector that
we are also mutating.

This commit fixes the issue by ensuring the delegateDepth is valid
before incrementing it to a relativeDepth.

wasm: Fix validation condition in decodeDelegate. r=jseward
https://hg.mozilla.org/integration/autoland/rev/b69c4c118ebcc88e9b2c36a6baa9feaa4c6a8364
https://hg.mozilla.org/mozilla-central/rev/b69c4c118ebc

Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox109: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 109 Branch

The patch landed in nightly and beta is affected.
:rhunt, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox108 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(rhunt)

Comment on attachment 9301647 [details]
Bug 1797685 - wasm: Fix validation condition in decodeDelegate. r?jseward

Beta/Release Uplift Approval Request

  • User impact if declined: Easily triggerable overflow may lead to UaF in code compilation. The UaF appears to be only reads, but as this code is compiling code, this corruption could lead to malformed code being generated, leading to other issues.
  • Is this code covered by automated tests?: No
  • 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): Simple change to avoid overflow, with a test case.
  • String changes made/needed:
  • Is Android affected?: Yes
Flags: needinfo?(rhunt)
Attachment #9301647 - Flags: approval-mozilla-release?
Attachment #9301647 - Flags: approval-mozilla-beta?

Comment on attachment 9301647 [details]
Bug 1797685 - wasm: Fix validation condition in decodeDelegate. r?jseward

Approved for 108.0b5

Attachment #9301647 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9301647 [details]
Bug 1797685 - wasm: Fix validation condition in decodeDelegate. r?jseward

Switching release uplift request to ESR uplift request.

Attachment #9301647 - Flags: approval-mozilla-release? → approval-mozilla-esr102?

Comment on attachment 9301647 [details]
Bug 1797685 - wasm: Fix validation condition in decodeDelegate. r?jseward

Approved for 102.6esr.

Attachment #9301647 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect][reminder-test 2023-02-01] → [jsbugmon:update,bisect][reminder-test 2023-02-01][post-critsmash-triage]
Whiteboard: [jsbugmon:update,bisect][reminder-test 2023-02-01][post-critsmash-triage] → [jsbugmon:update,bisect][reminder-test 2023-02-01][post-critsmash-triage][adv-main108+r]
Whiteboard: [jsbugmon:update,bisect][reminder-test 2023-02-01][post-critsmash-triage][adv-main108+r] → [jsbugmon:update,bisect][reminder-test 2023-02-01][post-critsmash-triage][adv-main108+r][adv-esr102.6+r]

2 months ago, Tom Ritter [:tjr] placed a reminder on the bug using the whiteboard tag [reminder-test 2023-02-01] .

rhunt, please refer to the original comment to better understand the reason for the reminder.

Flags: needinfo?(rhunt)
Whiteboard: [jsbugmon:update,bisect][reminder-test 2023-02-01][post-critsmash-triage][adv-main108+r][adv-esr102.6+r] → [jsbugmon:update,bisect][post-critsmash-triage][adv-main108+r][adv-esr102.6+r]

I believe enough time has passed now to land the test.

Flags: needinfo?(rhunt)
Group: core-security-release

Unable to reproduce bug 1797685 using build mozilla-central 20230420092216-6ca54f5a4a1b. 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.

Keywords: bugmon
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: