Closed Bug 1626382 Opened 11 months ago Closed 11 months ago

AddressSanitizer: use-after-poison [@ __asan_memmove] with READ of size 16

Categories

(Core :: Web Audio, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox-esr68 76+ fixed
firefox75 --- wontfix
firefox76 + fixed
firefox77 + fixed

People

(Reporter: jkratzer, Assigned: padenot)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main76+r][adv-ESR68.8+r])

Attachments

(3 files)

Found while fuzzing mozilla-central rev e1672b3231e9. I'm currently attempting to reduce the testcase and will update once complete.

==64677==ERROR: AddressSanitizer: use-after-poison on address 0x3ee69b133c10 at pc 0x563e1ba23bff bp 0x7ffefc280230 sp 0x7ffefc27f9f8
READ of size 16 at 0x3ee69b133c10 thread T0 (Web Content)
    #0 0x563e1ba23bfe in __asan_memmove /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:30:3
    #1 0x7f6c794f66c0 in PodMove<float> /builds/worker/workspace/obj-build/dist/include/mozilla/PodOperations.h:140:3
    #2 0x7f6c794f66c0 in mozilla::dom::AudioBuffer::CopyToChannel(JSContext*, mozilla::dom::TypedArray<float, &(js::UnwrapFloat32Array(JSObject*)), &(JS_GetFloat32ArrayData(JSObject*, bool*, JS::AutoRequireNoGC const&)), &(js::GetFloat32ArrayLengthAndData(JSObject*, unsigned int*, bool*, float**)), &(JS_NewFloat32Array(JSContext*, unsigned int))> const&, unsigned int, unsigned int, mozilla::ErrorResult&) /gecko/dom/media/webaudio/AudioBuffer.cpp:403:3
    #3 0x7f6c76645bd5 in mozilla::dom::AudioBuffer_Binding::copyToChannel(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/AudioBufferBinding.cpp:475:24
    #4 0x7f6c77efeb98 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /gecko/dom/bindings/BindingUtils.cpp:3205:13
    #5 0x7f6c7e426bbb in CallJSNative /gecko/js/src/vm/Interpreter.cpp:477:13
    #6 0x7f6c7e426bbb in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:569:12
    #7 0x7f6c7e4289da in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:632:10
    #8 0x7f6c7f2cbcaf in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /gecko/js/src/jit/BaselineIC.cpp:2926:10
    #9 0x1c4430888e27  (<unknown module>)

Address 0x3ee69b133c10 is a wild pointer.
SUMMARY: AddressSanitizer: use-after-poison /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:30:3 in __asan_memmove
Shadow bytes around the buggy address:
  0x07dd5361e730: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x07dd5361e740: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x07dd5361e750: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x07dd5361e760: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x07dd5361e770: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
=>0x07dd5361e780: f7 f7[f7]f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x07dd5361e790: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x07dd5361e7a0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x07dd5361e7b0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x07dd5361e7c0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x07dd5361e7d0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==64677==ABORTING
Group: core-security → media-core-security
Priority: -- → P1
Attached file testcase.zip

The attached testcase is not fully reduce. Further, it only reproduces in our testcase reducer. I'm attaching with the hope that this may still help to identify the cause of the bug.

To those looking at the testcase above, the fuzzer implements a proxy object 'o' that if an index which points to undefined is requested, it will search the contents of 'o' looking for an object of a matching type. A cleaned up version of that testcase would look like the following:

async function start () {
    const context = new AudioContext({'sampleRate':17457})
    const xhr = new XMLHttpRequest()
    xhr.open('GET', '278d3423471a6c778572009e35da5c5cec38ca78.wav', false)
    xhr.send(null)
    arr_0 = new Uint8Array(null)
    for (let i = 0; i < xhr.response.length; i++) {
        arr_0[i] = xhr.response.charCodeAt(i)
    }
    arr_1 = new Float32Array(4)
    buffer = context.createBuffer(14, new Int32Array([119126391])[0], 63805)
    buffer.copyToChannel(arr_1, (1824350050 % buffer.numberOfChannels), (3411677103 % buffer.numberOfChannels))
    setTimeout(window.close, 500)
}

Paul, assigning to you so that a P1 sec bug has an owner. Feel free to re-assign if others are better suited to take a look at this please.

Assignee: nobody → padenot

I'm having a really hard time believing this is an invalid read of 16 bytes when asan has a scalar memmove. This would make sense with a vectored load. So I don't know if I can trust the fact that it's an invalid load.

If it's really an invalid load, then we're trying to read 16 bytes from invalid memory that comes from a float32 array, which are supposed to be pretty solid these days, and I don't think we're doing anything wrong.

Waldo, this is surprising to me, I don't think we're doing anything wrong, do you have an idea? Code is this function.

Flags: needinfo?(jwalden)

The Float32Array::ComputeState() call would implicitly use js::GetFloat32ArrayLengthAndData().

Presumably, in the same way as JS_GetFloat32ArrayData(), the pointer can become invalid on GC. RestoreJSChannelData() can GC.

This seems to be a foot-gun that comes with ComputeState().

I assume the call can be moved to before aSource.Length(), within the JS::AutoCheckCannotGC.

Aha! I stared at this earlier and nothing jumped out, probably partly because last time I looked at this ComputeState had a different name, so I didn't realize what was happening. It appears to me that simply moving that function call down to just before the JS::AutoCheckCannotGC ought do the trick -- obviously if it can't GC it can't move past that. :-)

Flags: needinfo?(jwalden)
Attached file Bug 1626382 - r?karlt

Comment on attachment 9142702 [details]
Bug 1626382 - r?karlt

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: You'd have to time a GC very precisely just after the previous location of ComputeState, so you'd have to try for a very long time (the fuzzer has a really hard time reproducing it). For someone that knows about this kind of issue, I suppose it's quite easy.

I don't know how easy it is to change this read to something useful though.

  • 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?: all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: I said no because it's technically true, but not risky at all, it's just a matter of renaming a function call: ComputeState was called ComputeLengthAndData in ESR68.
  • How likely is this patch to cause regressions; how much testing does it need?: No risk, no need to test, the problem is clear.
Attachment #9142702 - Flags: sec-approval?

To help expedite things, please attach a rebased patch for ESR68 too when you get a chance.

Comment on attachment 9142702 [details]
Bug 1626382 - r?karlt

sec-approval+ and a=dveditz for beta uplift. I think the RC is Monday so please land soon.

Attachment #9142702 - Flags: sec-approval?
Attachment #9142702 - Flags: sec-approval+
Attachment #9142702 - Flags: approval-mozilla-beta+
See Also: → 1632717
See Also: → 1632719

It seems that this bug has existed since even before the hint of documentation was introduced.

https://bugzilla.mozilla.org/show_bug.cgi?id=1061288#c4 suggests the issue temporarily went away when ArrayBuffers ceased being nursery-allocatable and returned when compacting GC was introduced.

Some of the foot-guns were addressed when Bug 1061288 was fixed. I'm not aware of a follow-up for others. I've filed bug 1632714 for some of the remainder.

I've audited dom::TypedArray_base::ComputeState() calls in dom/media and filed bug 1632719 and bug 1632717.

Comment on attachment 9142933 [details]
Bug 1626382 uplift to ESR68

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
  • User impact if declined:
  • Fix Landed on Version: on 76, approved for 77
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Simple change in ordering of existing operations, to avoid changes in pointers during GC. Automated test coverage for GC-independent behavior is good.
  • String or UUID changes made by this patch: None.
Attachment #9142933 - Flags: approval-mozilla-esr68?
Group: media-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Flags: needinfo?(padenot)

Comment on attachment 9142933 [details]
Bug 1626382 uplift to ESR68

Approved for 68.8esr.

Attachment #9142933 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main76+r]
Whiteboard: [post-critsmash-triage][adv-main76+r] → [post-critsmash-triage][adv-main76+r][adv-ESR68.8+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.