Closed Bug 1452576 Opened 3 years ago Closed 3 years ago

Crash [@ get] with StructuredCloneHolder ending up in [@ mozilla::dom::ImageBitmap::CreateFromCloneData] although DifferentProcess


(Core :: DOM: Core & HTML, defect, P1)




Tracking Status
firefox-esr52 61+ fixed
firefox-esr60 61+ fixed
firefox60 --- wontfix
firefox61 + fixed
firefox62 + fixed


(Reporter: decoder, Assigned: baku)



(Keywords: crash, sec-high, testcase, Whiteboard: [adv-main61+][adv-esr52.9+][adv-esr60.1+][post-critsmash-triage])

Crash Data


(3 files, 1 obsolete file)

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:


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

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.
Attached file Testcase
I think this is an example for the checks that we want to add in bug 1444056.
See Also: → 1444056
Group: core-security → dom-core-security
baku, is this something you could work on?
Flags: needinfo?(amarchesini)
Priority: -- → P1
Attached patch clone.patch (obsolete) — Splinter Review
I'm not happy with bug 1444056. It breaks tests. Here a fix.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8973127 - Flags: review?(choller)
Comment on attachment 8973127 [details] [diff] [review]

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.
Flags: needinfo?(amarchesini)
Attachment #8973127 - Flags: review?(choller) → review+
Attached patch clone.patchSplinter 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?


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.
Attachment #8973127 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8973224 - Flags: sec-approval?
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]

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
Attachment #8973224 - Flags: approval-mozilla-esr60?
Attachment #8973224 - Flags: approval-mozilla-beta?
Comment on attachment 8973224 [details] [diff] [review]

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+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
This also needs a rebased patch (and approval request) for ESR52.
Flags: needinfo?(amarchesini)
Attached patch m-esr52Splinter Review
Patch rebased for esr52.
Flags: needinfo?(amarchesini)
Attachment #8975406 - Flags: approval-mozilla-esr52?
Attachment #8975406 - Attachment is patch: true
Comment on attachment 8975406 [details] [diff] [review]

sec-high fix for 52.9esr
Attachment #8975406 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment on attachment 8973224 [details] [diff] [review]

sec-high fix for 60.1esr
Attachment #8973224 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [adv-main61+][adv-esr52.9+][adv-esr60.1+]
Flags: qe-verify-
Whiteboard: [adv-main61+][adv-esr52.9+][adv-esr60.1+] → [adv-main61+][adv-esr52.9+][adv-esr60.1+][post-critsmash-triage]
Component: DOM → DOM: Core & HTML
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.