Closed Bug 1452576 Opened 3 years ago Closed 3 years ago
Crash [@ get] with Structured
Clone Holder ending up in [@ mozilla::dom::Image Bitmap::Create From Clone Data] although Different Process
49 bytes, application/octet-stream
5.05 KB, patch
|Details | Diff | Splinter Review|
5.08 KB, patch
|Details | Diff | Splinter Review|
This is using the libfuzzer-based fuzzing target that uses dom::ipc::StructuredCloneData as its entry point to cover the StructuredCloneReader code that is browser-only, with the patch from bug 1442722 applied. The target code has not landed on mozilla-central yet, if you need it to reproduce (in case stack + test attached do not suffice), please let me know. The attached testcase crashes on mozilla-central revision ee6283795f41: Backtrace: ==11032==ERROR: AddressSanitizer: SEGV on unknown address 0x00207fff6060 (pc 0x7f772fdf47bf bp 0x7ffe40073370 sp 0x7ffe40073260 T0) ==11032==The signal is caused by a READ memory access. #0 0x7f772fdf47be in get gfx/gl/../../mfbt/RefPtr.h:287:27 #1 0x7f772fdf47be in operator mozilla::gfx::DataSourceSurface * gfx/gl/../../mfbt/RefPtr.h:300 #2 0x7f772fdf47be in mozilla::dom::ImageBitmap::CreateFromCloneData(nsIGlobalObject*, mozilla::dom::ImageBitmapCloneData*) dom/canvas/ImageBitmap.cpp:815 #3 0x7f772b9ce318 in mozilla::dom::StructuredCloneHolder::CustomReadTransferHandler(JSContext*, JSStructuredCloneReader*, unsigned int, void*, unsigned long, JS::MutableHandle<JSObject*>) dom/base/StructuredCloneHolder.cpp:1205:34 #4 0x7f7738bbc0ec in JSStructuredCloneReader::readTransferMap() js/src/vm/StructuredClone.cpp:2495:18 #5 0x7f7738ba04c6 in JSStructuredCloneReader::read(JS::MutableHandle<JS::Value>) js/src/vm/StructuredClone.cpp:2612:10 #6 0x7f7738ba00b8 in ReadStructuredClone(JSContext*, JSStructuredCloneData&, JS::StructuredCloneScope, JS::MutableHandle<JS::Value>, JSStructuredCloneCallbacks const*, void*) js/src/vm/StructuredClone.cpp:636:14 #7 0x7f772b9c2ee9 in ReadFromBuffer dom/base/StructuredCloneHolder.cpp:363:8 #8 0x7f772b9c2ee9 in mozilla::dom::StructuredCloneHolder::ReadFromBuffer(nsISupports*, JSContext*, JSStructuredCloneData&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) dom/base/StructuredCloneHolder.cpp:343 #9 0x7f77381b7e3a in FuzzingRunDomSC(unsigned char const*, unsigned long) dom/base/fuzztest/FuzzStructuredClone.cpp:68:10 [...] #22 0x421768 in _start (objdir-ff-libfuzzer/dist/bin/firefox+0x421768) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV gfx/gl/../../mfbt/RefPtr.h:287:27 in get ==11032==ABORTING The essential problem here is that we somehow end up in mozilla::dom::ImageBitmap::CreateFromCloneData which is same-process only, even if the data came under the different-process scope. As discussed with :baku, we either need better checks in StructuredCloneHolder to avoid entering any branches that we are not supposed to be in, under different-process scope, or, we somehow refactor something to improve the separation here. The first method is probably more viable but requires careful auditing of StructuredCloneHolder to not miss any checks. This is an exploitable sandbox escape, marking s-s and sec-high.
baku, is this something you could work on?
Priority: -- → P1
I'm not happy with bug 1444056. It breaks tests. Here a fix.
Assignee: nobody → amarchesini
Attachment #8973127 - Flags: review?(choller)
Comment on attachment 8973127 [details] [diff] [review] clone.patch Looks good to me as a for an intermediate fix until we can fix this properly. What about ReadWasmModule/WriteWasmModule? These methods assert about the clone scope apparently, but it isn't checked e.g. in CustomReadHandler.
Attachment #8973127 - Flags: review?(choller) → review+
Here a new patch with comments applied. [Security approval request comment] How easily could an exploit be constructed based on the patch? We need to have a compromised content process. Not so easy to reproduce at the moment (hopefully). About exploiting it, it's hard to say: a raw pointer is casted to a C++ object, so, I guess, it's possible to exploit it. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Just moved some assertions into if-stmts. Which older supported branches are affected by this flaw? all. If not all supported branches, which bug introduced the flaw? StructuredCloneHolder is a 'kind-of' old component. It was written with the idea of having a valid buffer to parse. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Easy to port. How likely is this patch to cause regressions; how much testing does it need? No regressions.
sec-approval+ to land on trunk. We'll want patches for Beta and both ESR branches made and nominated as well.
Attachment #8973224 - Flags: sec-approval? → sec-approval+
Comment on attachment 8973224 [details] [diff] [review] clone.patch Approval Request Comment [Feature/Bug causing the regression]: Structured Clone Algorithm [User impact if declined]: if the content process is hacked, this can trigger a crash on the parent side. [Is this code covered by automated tests?]: we have fuzzy tests. [Has the fix been verified in Nightly?]: n/a [Needs manual test from QE? If yes, steps to reproduce]: n/a [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: This patch converts assertions in if stmt. [String changes made/needed]: none
This was pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8d6bf455ae5
Comment on attachment 8973224 [details] [diff] [review] clone.patch Fixes a sec-high, approved for 61.0b4. We'll revisit the ESR60 request later in the cycle.
Attachment #8973224 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This also needs a rebased patch (and approval request) for ESR52.
Patch rebased for esr52.
Attachment #8975406 - Flags: approval-mozilla-esr52?
Comment on attachment 8975406 [details] [diff] [review] m-esr52 sec-high fix for 52.9esr
Attachment #8975406 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment on attachment 8973224 [details] [diff] [review] clone.patch sec-high fix for 60.1esr
Attachment #8973224 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [adv-main61+][adv-esr52.9+][adv-esr60.1+] → [adv-main61+][adv-esr52.9+][adv-esr60.1+][post-critsmash-triage]
You need to log in before you can comment on or make changes to this bug.