AddressSanitizer: heap-buffer-overflow [@ __asan_memcpy] with arbitrary WRITE in JSStructuredCloneReader

RESOLVED FIXED in Firefox -esr52

Status

()

P1
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla59
x86_64
Linux
crash, jsbugmon, regression, sec-high, testcase
Points:
---

Firefox Tracking Flags

(firefox-esr5258+ fixed, firefox57 wontfix, firefox58+ fixed, firefox59+ fixed)

Details

(Whiteboard: [jsbugmon:update,bisect][adv-main58+][adv-esr52.6+])

Attachments

(3 attachments)

(Reporter)

Description

a year ago
The following testcase crashes on mozilla-central revision 5b1fdaa14d35 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2, run with --fuzzing-safe --cpu-count=2):

let data = new Uint8Array([
    ,,,,7,,255,255,34,,,128,5,1,255,255,
    ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
    ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
    ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
    ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,1
]);
let cloneBuffer = serialize(null);
cloneBuffer.clonebuffer = data.buffer;
try { deserialize(cloneBuffer) } catch{}


Backtrace:

==22005==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60d00000aea8 at pc 0x0000004f80ac bp 0x7ffeb9a09a20 sp 0x7ffeb9a091d0
WRITE of size 144 at 0x60d00000aea8 thread T0
    #0 0x4f80ab in __asan_memcpy /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:407
    #1 0x1b25030 in mozilla::BufferList<js::SystemAllocPolicy>::ReadBytes(mozilla::BufferList<js::SystemAllocPolicy>::IterImpl&, char*, unsigned long) const mozilla/BufferList.h:457:5
    #2 0x1b25030 in js::BufferIterator<unsigned long, js::SystemAllocPolicy>::readBytes(char*, unsigned long) js/src/vm/StructuredClone.cpp:218
    #3 0x1b13079 in bool js::SCInput::readArray<unsigned int>(unsigned int*, unsigned long) js/src/vm/StructuredClone.cpp:856:10
    #4 0x1aea476 in JSStructuredCloneReader::readV1ArrayBuffer(unsigned int, unsigned int, JS::MutableHandle<JS::Value>) js/src/vm/StructuredClone.cpp:2105:16
    #5 0x1ae994d in JSStructuredCloneReader::readTypedArray(unsigned int, unsigned int, JS::MutableHandle<JS::Value>, bool) js/src/vm/StructuredClone.cpp:1908:14
    #6 0x1aeb20d in JSStructuredCloneReader::startRead(JS::MutableHandle<JS::Value>) js/src/vm/StructuredClone.cpp:2311:20
    #7 0x1ada18b in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>) js/src/vm/StructuredClone.cpp:2585:14
    #8 0x1ad995a in ReadStructuredClone(JSContext*, JSStructuredCloneData&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:634:12
    #9 0x1aefbc0 in JS_ReadStructuredClone(JSContext*, JSStructuredCloneData&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:2664:12
    #10 0x11ed709 in Deserialize(JSContext*, unsigned int, JS::Value*) js/src/builtin/TestingFunctions.cpp:3050:10
    #11 0x841e49 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:291:15
[...]

0x60d00000aea8 is located 0 bytes to the right of 136-byte region [0x60d00000ae20,0x60d00000aea8)
allocated by thread T0 here:
    #0 0x50e35c in __interceptor_calloc compiler-rt/lib/asan/asan_malloc_linux.cc:66
    #1 0x160598b in js_calloc(unsigned long) dist/include/js/Utility.h:393:12
    #2 0x160598b in unsigned char* js_pod_calloc<unsigned char>(unsigned long) dist/include/js/Utility.h:587
    #3 0x160598b in unsigned char* js::MallocProvider<JS::Zone>::maybe_pod_calloc<unsigned char>(unsigned long) js/src/vm/MallocProvider.h:62
    #4 0x160598b in unsigned char* js::MallocProvider<JS::Zone>::pod_calloc<unsigned char>(unsigned long) js/src/vm/MallocProvider.h:132
    #5 0x16c66e2 in unsigned char* JS::Zone::pod_callocCanGC<unsigned char>(unsigned long) js/src/gc/Zone.h:697:16
    #6 0x16c66e2 in AllocateArrayBufferContents(JSContext*, unsigned int) js/src/vm/ArrayBufferObject.cpp:447
    #7 0x16c66e2 in js::ArrayBufferObject::create(JSContext*, unsigned int, js::ArrayBufferObject::BufferContents, js::ArrayBufferObject::OwnsState, JS::Handle<JSObject*>, js::NewObjectKind) js/src/vm/ArrayBufferObject.cpp:1192
    #8 0x16beb53 in js::ArrayBufferObject::create(JSContext*, unsigned int, JS::Handle<JSObject*>, js::NewObjectKind) js/src/vm/ArrayBufferObject.cpp:1230:12
    #9 0x1aea3d9 in JSStructuredCloneReader::readV1ArrayBuffer(unsigned int, unsigned int, JS::MutableHandle<JS::Value>) js/src/vm/StructuredClone.cpp:2087:21
    #10 0x1ae994d in JSStructuredCloneReader::readTypedArray(unsigned int, unsigned int, JS::MutableHandle<JS::Value>, bool) js/src/vm/StructuredClone.cpp:1908:14
    #11 0x1aeb20d in JSStructuredCloneReader::startRead(JS::MutableHandle<JS::Value>) js/src/vm/StructuredClone.cpp:2311:20
    #12 0x1ada18b in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>) js/src/vm/StructuredClone.cpp:2585:14
    #13 0x1ad995a in ReadStructuredClone(JSContext*, JSStructuredCloneData&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:634:12
    #14 0x1aefbc0 in JS_ReadStructuredClone(JSContext*, JSStructuredCloneData&, unsigned int, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:2664:12
    #15 0x11ed709 in Deserialize(JSContext*, unsigned int, JS::Value*) js/src/builtin/TestingFunctions.cpp:3050:10
    #16 0x841e49 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:291:15
[...]

SUMMARY: AddressSanitizer: heap-buffer-overflow asan_interceptors.cc:407 in __asan_memcpy
Shadow bytes around the buggy address:
  0x0c1a7fff95c0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c1a7fff95d0: 00 00 00 00 00[fa]fa fa fa fa fa fa fa fa 00 00
  0x0c1a7fff95e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 07 fa
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



This still reproduces with jorendorff's patch from bug 1425612 applied. Looks like an abitrary write, marking sec-critical.
Severity: critical → normal
status-firefox57: --- → affected
status-firefox58: --- → affected
Flags: needinfo?(jorendorff)
Priority: -- → P1
(Assignee)

Updated

a year ago
Assignee: nobody → jorendorff
Flags: needinfo?(jorendorff)
(Assignee)

Comment 1

a year ago
Attacker needs to take over a content process first, but then I believe this bug allows an attacker to write all over heap memory. Specifically, the attacker chooses a buffer size that is a multiple of 4; recipient is tricked into malloc-ing a buffer of that size and then writing any amount of data to it, and past the end of it. So if the allocation is adjacent to other malloc-allocated objects we can just overwrite them.

sec-high. Easy to fix, would backport.

baku, I'm assuming that content processes can send structured-clone-serialized data and have it deserialized in other processes. Is that correct?
Flags: needinfo?(amarchesini)
Keywords: sec-critical → sec-high
(Assignee)

Comment 2

a year ago
More importantly, I am assuming that we never deserialize structured-clone buffers from completely untrusted sources. If we do, this really is sec-critical.
> baku, I'm assuming that content processes can send
> structured-clone-serialized data and have it deserialized in other
> processes. Is that correct?

Yes. This can happen using BroadcastChannel, MessagePort and IndexedDB. In particular:

- BroadcastChannel could be used to send a buffer from content process A to content process B via parent process.
- MessagePort sends data from process A to parent process, and from there to process A again.
- IDB sends data from process A to parent process only. Reading data from IDB can send data from the parent to the content process.

Probably there are other APIs using structured clone cross processes.
Flags: needinfo?(amarchesini)
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> More importantly, I am assuming that we never deserialize structured-clone
> buffers from completely untrusted sources. If we do, this really is
> sec-critical.

Well, we need to define 'untrusted sources'. Structured clone data is always deserialized inside an IPDL protocol.
If the hacked content process is able to send data following IPDL protocols, it's hard to definite/detect it as 'untrusted' source.
Often, these IPDL protocols have an internal security validation: BroadcastChannel kills child processes if the message is coming from an invalid principal. Something similar happens for MessagePort and IDB.
(Assignee)

Comment 5

a year ago
What I meant is: this is sec-critical if an attacker can exploit it without having to first take over the content process (or, I guess, the backing store for IDB).

I believe that is not the case, so this is only sec-high.
If this is sec-high, we probably shouldn't land the test.
Comment on attachment 8940347 [details] [diff] [review]
Fix error handling in deserialization of invalid typed arrays

Review of attachment 8940347 [details] [diff] [review]:
-----------------------------------------------------------------

Yowch.
Attachment #8940347 - Flags: review?(sphink) → review+
(Assignee)

Comment 10

a year ago
Comment on attachment 8940834 [details] [diff] [review]
Fix error handling in deserialization of invalid typed arrays

Patch is identical except for removing the test. Carrying over sfink's review.
Attachment #8940834 - Flags: review+
(Assignee)

Comment 11

a year ago
Comment on attachment 8940834 [details] [diff] [review]
Fix error handling in deserialization of invalid typed arrays

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

    It would be easy, but by itself the exploit wouldn't be easy to use: the attacker has to take over a content process first.

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 supported branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

    The patch applies cleanly to beta, release, and esr52.

How likely is this patch to cause regressions; how much testing does it need?

    Our test coverage of this method should be sufficient.
Attachment #8940834 - Flags: sec-approval?
(Assignee)

Comment 12

a year ago
I can land once this and bug 1425612 both have approval.

(I could land the two patches in a single open bug about error handling, although I'm not sure it'll fool anyone when we backport that purportedly boring bug fix to all supported branches.)
sec-approval+ for trunk. We'll need this on Beta and ESR52 as soon as it lands too.
status-firefox57: affected → wontfix
status-firefox-esr52: --- → affected
tracking-firefox58: --- → +
tracking-firefox59: --- → +
tracking-firefox-esr52: --- → 58+
Attachment #8940834 - Flags: sec-approval? → sec-approval+
Gerry, over to you for the beta 16 build.
Jason can you request approval for beta uplift? Thanks!
Flags: needinfo?(jorendorff)
Flags: needinfo?(gchang)
(Assignee)

Comment 15

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f10263c3babef5f70e1e8fdb9e52c2de15cf22e1
Bug 1426783 - Fix error handling in deserialization of invalid typed arrays. r=sfink, a=abillings.
(Assignee)

Comment 17

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bda6eb9e8469ac4347bb9738d720ea81c358aea
Bug 1426783 - Fix error handling in deserialization of invalid typed arrays. r=sfink, a=abillings.
(Assignee)

Comment 18

a year ago
Comment on attachment 8940834 [details] [diff] [review]
Fix error handling in deserialization of invalid typed arrays

Approval Request Comment
[Feature/Bug causing the regression]: Structured cloning being used as a serialization format across trust boundaries.
[User impact if declined]: Sec-high risk as described in comment 1.
[Is this code covered by automated tests?]: No, it needs to be covered by fuzzing (decoder's fuzzing found this in the first place).
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Bug 1426783.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Changes are in error checking for invalid structured-clone data, which we do not generate. This code triggers only if we're attacked.
[String changes made/needed]: None.
Flags: needinfo?(jorendorff)
Attachment #8940834 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/3bda6eb9e846
status-firefox59: affected → fixed
Target Milestone: --- → mozilla59
Comment on attachment 8940834 [details] [diff] [review]
Fix error handling in deserialization of invalid typed arrays

Fix a sec-high. Beta58+.
Flags: needinfo?(gchang)
Attachment #8940834 - Flags: approval-mozilla-release+
Attachment #8940834 - Flags: approval-mozilla-beta?
Attachment #8940834 - Flags: approval-mozilla-beta+
(Assignee)

Comment 21

a year ago
The patch is now in beta due to today's central->beta merge. Will push to release.
(Assignee)

Updated

a year ago
status-firefox58: affected → fixed
Is there an ESR52 patch somewhere?
Flags: needinfo?(jorendorff)
(In reply to Jason Orendorff [:jorendorff] from comment #21)
> The patch is now in beta due to today's central->beta merge. Will push to
> release.

This and bug 1425612 still need to land on FIREFOX_58b_RELBRANCH on mozilla-beta for Fennec 58b17.
Flags: needinfo?(aryx.bugmail)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main58+]
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Hi Jason, since this was fixed in 58, I am wondering whether we ought to also uplift this fix to ESR52.6. Wdyt?

Al, Dan, I will gtb ESR52.6 build2 in ~2 hrs. Is this fix worth including in build2? I will if you agree it's a good idea.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
(Assignee)

Comment 28

a year ago
Comment on attachment 8943690 [details] [diff] [review]
Fix error handling in deserialization of invalid typed arrays (for esr52)

[Approval Request Comment]
User impact if declined: Sec-high risk as described in comment 1.
Fix Landed on Version: 58
Risk to taking this patch (and alternatives if risky): low, it affects an error path that neither content nor chrome should be able to trigger
String or UUID changes made by this patch: none
Attachment #8943690 - Attachment description: Fix error handling in deserialization of invalid typed arrays → Fix error handling in deserialization of invalid typed arrays (for esr52)
Flags: needinfo?(jorendorff)
Attachment #8943690 - Flags: review+
Attachment #8943690 - Flags: approval-mozilla-esr52?
Comment on attachment 8943690 [details] [diff] [review]
Fix error handling in deserialization of invalid typed arrays (for esr52)

Sec-high, in 58, Al said he's fine taking this one, ESR52+
Attachment #8943690 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
https://hg.mozilla.org/releases/mozilla-esr52/rev/fe271a2b9503
status-firefox-esr52: affected → fixed
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update,bisect][adv-main58+] → [jsbugmon:update,bisect][adv-main58+][adv-esr52.6+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.