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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

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?

Comment 2

[Jason Orendorff [:jorendorff]]

More importantly, I am assuming that we never deserialize structured-clone buffers from completely untrusted sources. If we do, this really is sec-critical.

Comment 3

[Andrea Marchesini [:baku]]

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

Comment 4

[Andrea Marchesini [:baku]]

(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.

Comment 5

[Jason Orendorff [:jorendorff]]

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.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Fix error handling in deserialization of invalid typed arrays

Comment 7

[Jan de Mooij [:jandem]]

If this is sec-high, we probably shouldn't land the test.

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8940347 [details] [diff] [review]
Fix error handling in deserialization of invalid typed arrays

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

Yowch.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Fix error handling in deserialization of invalid typed arrays

Comment 10

[Jason Orendorff [:jorendorff]]

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.

Comment 11

[Jason Orendorff [:jorendorff]]

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.

Comment 12

[Jason Orendorff [:jorendorff]]

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

Comment 13

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We'll need this on Beta and ESR52 as soon as it lands too.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Gerry, over to you for the beta 16 build.
Jason can you request approval for beta uplift? Thanks!

Comment 15

[Jason Orendorff [:jorendorff]]

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

Comment 16

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

Bug 1426783 and bug 1425612 got backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/600bee353e155608a5832f7eb9d5e42987d66591 for failing non262/extensions/sharedtypedarray.js:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a8e2b4cf8e26b900983e33cc2fb6b48f1e5747b2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
TEST-UNEXPECTED-FAIL | non262/extensions/sharedtypedarray.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so")

Comment 17

[Jason Orendorff [:jorendorff]]

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

Comment 18

[Jason Orendorff [:jorendorff]]

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.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/3bda6eb9e846

Comment 20

[Gerry Chang [:gchang]]

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

Fix a sec-high. Beta58+.

Comment 21

[Jason Orendorff [:jorendorff]]

The patch is now in beta due to today's central->beta merge. Will push to release.

Comment 22

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/releases/mozilla-release/rev/d70260efa901

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Is there an ESR52 patch somewhere?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 25

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

https://hg.mozilla.org/releases/mozilla-beta/rev/9624359659f033a34985be234cec79efb045bb87 (FIREFOX_58b_RELBRANCH)

Comment 26

[Jason Orendorff [:jorendorff]]

Attachment: Fix error handling in deserialization of invalid typed arrays (for esr52)

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 28

[Jason Orendorff [:jorendorff]]

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

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/fe271a2b9503