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

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Jason Kratzer [:jkratzer]]

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

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: 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.

Comment 2

[Karl Tomlinson (:karlt)]

Thank you, Jason. Pasting the likely triggers here:

async function start () {
    try { o6[3] = new AudioContext({'sampleRate':17457}) } catch (e) { }
    try { o6[56] = new XMLHttpRequest() } catch (e) { }
    try { o6[56].open('GET', '278d3423471a6c778572009e35da5c5cec38ca78.wav', false) } catch (e) { }
    try { o6[56].send(null) } catch (e) { }
    try { o6[58] = new Uint8Array(o6[57]) } catch (e) { }
    try { for (let i = 0; i < o6[56].response.length; i++) o6[58][i] = o6[56].response.charCodeAt(i) } catch (e) { }
    try { o6[60] = new Float32Array(4) } catch (e) { }
    try { o6[80] = o6[3].createBuffer(14, new Int32Array([119126391])[0], 63805) } catch (e) { }
    try { o6[80].copyToChannel(o6[85], (1824350050 % o6[80].numberOfChannels), (3411677103 % o6[80].numberOfChannels)) } catch (e) { }
    setTimeout(window.close, 500)
}

Comment 3

[Jason Kratzer [:jkratzer]]

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)
}

Comment 4

[Nils Ohlmeier [:drno]]

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.

Comment 5

[Paul Adenot (: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.

Comment 6

[Karl Tomlinson (:karlt)]

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.

Comment 7

[Jeff Walden [:Waldo]]

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. :-)

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 1626382 - r?karlt

Comment 9

[Paul Adenot (:padenot)]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Daniel Veditz [:dveditz]]

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.

Comment 12

[Karl Tomlinson (:karlt)]

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 13

[Karl Tomlinson (:karlt)]

Attachment: Bug 1626382 uplift to ESR68

Comment 14

[Karl Tomlinson (:karlt)]

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.

Comment 15

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

https://hg.mozilla.org/integration/autoland/rev/5e0fa8d4f6c14e149af7d1ae9ceab72cab58a420
https://hg.mozilla.org/mozilla-central/rev/5e0fa8d4f6c1

Comment 16

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

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Approved for 68.8esr.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/d787223017e1