1434384 - AddressSanitizer: BUS on unknown address 0x000000000000 [@ __asan::asan_free] with clobbered bp involving StructuredClone

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 8a2584063e19+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --enable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --enable-fuzzing --disable-debug, run with --fuzzing-safe):

let data = new Uint8Array([
  0x0,0x0,0x0,0x0,0x0,0x2,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x4,0x0,0x2,0x0,
  0x0,0x0,0x2,0x2,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x40,0x0,0x20,0x0,0x0,0x0,0x0,
  0x0,0x0,0x0,0x0
]);
let cloneBuffer = serialize(null);
cloneBuffer.clonebuffer = data.buffer;
try {
  let obj = deserialize(cloneBuffer);
  print(uneval(obj));
} catch(exc1) { print(exc1); }


Backtrace:

==15174==ERROR: AddressSanitizer: BUS on unknown address 0x000000000000 (pc 0x00000046a7ef bp 0x20003ffffffffff0 sp 0x7ffe32e839a0 T0)
    #0 0x46a7ee in __asan::asan_free(void*, __sanitizer::BufferedStackTrace*, __asan::AllocType) (js/src/opt64lf/js/src/shell/js+0x46a7ee)
    #1 0x52129c in __interceptor_cfree.localalias.0 (js/src/opt64lf/js/src/shell/js+0x52129c)
    #2 0x15a622d in js::Class::doFinalize(js::FreeOp*, JSObject*) const js/src/opt64lf/dist/include/js/Class.h:874:5
    #3 0x15a622d in JSObject::finalize(js::FreeOp*) js/src/jsobjinlines.h:108
    #4 0x15a622d in unsigned long js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) js/src/jsgc.cpp:552
    #5 0x158ac45 in bool FinalizeTypedArenas<JSObject>(js::FreeOp*, js::gc::Arena**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&, js::gc::ArenaLists::KeepArenasEnum) js/src/jsgc.cpp:610:33
    #6 0x151f3b8 in FinalizeArenas(js::FreeOp*, js::gc::Arena**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&, js::gc::ArenaLists::KeepArenasEnum) js/src/jsgc.cpp:644:1
    #7 0x151c325 in js::gc::ArenaLists::backgroundFinalize(js::FreeOp*, js::gc::Arena*, js::gc::Arena**) js/src/jsgc.cpp:3041:5
    #8 0x1527b6e in js::gc::GCRuntime::sweepBackgroundThings(js::gc::ZoneList&, js::LifoAlloc&) js/src/jsgc.cpp:3434:21
    #9 0x154b70b in js::gc::GCRuntime::endSweepingSweepGroup(js::FreeOp*, js::SliceBudget&) js/src/jsgc.cpp:5671:9
    #10 0x15c39bc in sweepaction::SweepActionSequence<js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run(js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&) js/src/jsgc.cpp:6202:29
    #11 0x15c493e in sweepaction::SweepActionRepeatFor<js::gc::SweepGroupsIter, JSRuntime*, js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&>::run(js::gc::GCRuntime*, js::FreeOp*, js::SliceBudget&) js/src/jsgc.cpp:6263:25
    #12 0x1551527 in js::gc::GCRuntime::performSweepActions(js::SliceBudget&, js::AutoLockForExclusiveAccess&) js/src/jsgc.cpp:6418:26
    #13 0x15588d5 in js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason, js::AutoLockForExclusiveAccess&) js/src/jsgc.cpp:6980:13
    #14 0x155c56a in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) js/src/jsgc.cpp:7317:5
    #15 0x1560255 in js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) js/src/jsgc.cpp:7460:25
    #16 0x1525368 in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:7530:5
[...]

Another crash where the base pointer looks clobbered, so this might again be easily exploitable if it can be triggered by a child process. Marking sec-high due to potential sandbox escape.

Comment 1

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

Attachment: Mark v1 structured clone data as cross-process

I had already created a patch for this a while back, but didn't think it through to figure out if it actually mattered; I was thinking of it as a sanity type thing.

As this test proves, it matters! Er, maybe.

The problem is that the structured clone code needs to support reading old-format data from disk (IndexedDB), and old-format data lacks a header giving what scope it was generated in. So if it sees anything other than the header marker, it doesn't bother with scopes, on the assumption that you only ever read those off of disk.

But that skips the scope check -- with a v2+ clone, you have an allowed scope and a stored scope, and if you aren't allowed to read what is stored, you throw. A v1 clone not only skips the check, but it never initializes the stored scope, so it tends to be zero (SameProcessSameThread).

If it did the check, it would probably fail because only DifferentProcess is allowed here.

Or, if it set storedScope to DifferentProcess (which it always will be; nothing will ever create anything new with v1 format data), then it will still try to read the data in but it'll throw if it finds anything that isn't allowed for a DifferentProcess scope. That is what this patch does.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8946768 [details] [diff] [review]
Mark v1 structured clone data as cross-process

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

::: js/src/vm/StructuredClone.cpp
@@ +2378,5 @@
>          return in.reportTruncated();
>  
> +    if (tag == SCTAG_HEADER) {
> +        MOZ_ALWAYS_TRUE(in.readPair(&tag, &data));
> +        storedScope = JS::StructuredCloneScope(data);

Move the if-block that checks the value of `data` in here, before the line `storedScope = JS::StructuredCloneScope(data);`.

If we take the else branch, it's wrong to check `data` that way.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Are we going to land this before soft-freeze?  And should we uplift it to 59?  (it might be too late, or would it be a candidate for a ride-along if it's too late, and we want it in 59).  We can nominate it for 59 if it makes sense, and let releng decide

Comment 4

[Jason Orendorff [:jorendorff]]

This is sec-high. We should definitely land and probably uplift.

Comment 5

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

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Comment on attachment 8946768 [details] [diff] [review]
> Mark v1 structured clone data as cross-process
> 
> Review of attachment 8946768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/StructuredClone.cpp
> @@ +2378,5 @@
> >          return in.reportTruncated();
> >  
> > +    if (tag == SCTAG_HEADER) {
> > +        MOZ_ALWAYS_TRUE(in.readPair(&tag, &data));
> > +        storedScope = JS::StructuredCloneScope(data);
> 
> Move the if-block that checks the value of `data` in here, before the line
> `storedScope = JS::StructuredCloneScope(data);`.
> 
> If we take the else branch, it's wrong to check `data` that way.

The else branch does an early-return. But clearly this is unclear, so I'll invert the condition and do an early-return, and unindent the above. That reads much better.

Comment 6

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

Attachment: Mark v1 structured clone data as cross-process

Refactored as above.

Comment 7

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

Comment on attachment 8955201 [details] [diff] [review]
Mark v1 structured clone data as cross-process

Carrying over r+.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I don't think it's exploitable.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? back to esr52

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

How likely is this patch to cause regressions; how much testing does it need? This is safe. It only applies to old-format clones, which means it'll only be used for IndexedDB, and it doesn't change the interpretation of disk-serializable data from past versions. As far as I can tell, this change will only apply to testing.

Comment 8

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

Sorry, I'm wrong, as decoder pointed out to me.

(In reply to Steve Fink [:sfink] [:s:] from comment #7)
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch? I don't think
> it's exploitable.

It appears that this *is* exploitable, because it crashes even when reading as a DifferentProcess scope.

As for how easy it is to exploit, I'm not sure, but it's the sort of thing where once you realize that you can corrupt structured clone messages, there'll be some way to make use of 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

I'd still say no.

> How likely is this patch to cause regressions; how much testing does it
> need? This is safe. It only applies to old-format clones, which means it'll
> only be used for IndexedDB, and it doesn't change the interpretation of
> disk-serializable data from past versions. As far as I can tell, this change
> will only apply to testing.

This is still safe in terms of causing regressions. It only matters when an attacker is corrupting data.

Comment 9

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

Comment on attachment 8955201 [details] [diff] [review]
Mark v1 structured clone data as cross-process

sec-approval+.
We'll want Beta and ESR52 patches made and nominated ASAP as well.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d85679eb427513cb18650f3d4e7d37a6ccbefbab

Please request release and ESR52 approval on this ASAP. It grafts cleanly as-landed.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/d85679eb4275

Comment 12

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

Comment on attachment 8955201 [details] [diff] [review]
Mark v1 structured clone data as cross-process

Approval Request Comment
[Feature/Bug causing the regression]: structured cloning. Longstanding bug.
[User impact if declined]: compromised content process can exploit the main process
[Is this code covered by automated tests?]: no, we should additionally land the test in comment 0
[Has the fix been verified in Nightly?]: manually with the JS shell only
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: no behavioral change; requires an adversary to send corrupt messages
[String changes made/needed]: none

Comment 13

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

Attachment: corrupt v1 structured clone data

Imported fuzz test.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8955201 [details] [diff] [review]
Mark v1 structured clone data as cross-process

Approved for Fx59rc1 and ESR 52.7.0.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/52551848cfcf (FIREFOX_59b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/5c286cc709df
https://hg.mozilla.org/releases/mozilla-esr52/rev/62b831df8269