AddressSanitizer: attempting free on address which was not malloc()-ed or Assertion failure: node, at memory/build/mozjemalloc.cpp:3895 or various crashes

RESOLVED FIXED in Firefox 61

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: decoder, Assigned: sfink)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla61
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox58 unaffected, firefox59- wontfix, firefox60- wontfix, firefox61- fixed)

Details

(Whiteboard: [fuzzblocker] [jsbugmon:])

Attachments

(3 attachments)

Flags: needinfo?(sphink)
Flags: needinfo?(jorendorff)
I've also seen variants of this with trashed base pointer:

==13584==ERROR: AddressSanitizer: BUS on unknown address 0x000000000000 (pc 0x00000046bf6f bp 0x16161616161615f0 sp 0x7fe56aff7d40 T5)

Probably easy to exploit.
Right, this is the sort of thing I've been worried about and never looked closely enough at.

We have a synthetic buffer with Transferables, which is a little crazy because normally Transferables embed pointers. We notice that and throw a JS error "clone buffer data is synthetic but may contain pointers". Which is great, but we still have this corrupt buffer stuck in our object, so eventually when we finalize it, we try to clean it up and free the pointers in it.

Very exploitable, *if* you are able to corrupt a buffer in a context that allows pointer-based Transferables to be used. Is that a case we're worried about? As in, a hacked content process breaking into its own privileged code? Or do we say that if they can manage to corrupt an internal clone buffer, then the game's up for that whole process and all we care about is preventing the chrome process from being hacked? I'll assume that's the case.

It's still a bug no matter what. We should drop the buffer on the floor in this case rather than freeing wild pointers.

And it's not as simple as saying that Transferable buffers have pointers and so shouldn't be tested or something. DifferentProcess buffers with Transferables won't have pointers, and *should* be tested. (Note that the testing function will demote any synthetic buffer to DifferentProcess, so that means the existing fuzzing is fine. It's totally fair for it to create buffers that say they have Transferable pointers in them; that's something a corrupted content process could do.)
Flags: needinfo?(sphink)
So I believe the problem here is that we're not propagating scopes properly, and we're freeing pointers in DifferentProcess buffers.

I guess it might be nice in fuzzing-unsafe builds to be able to set the scope when setting the clone buffer data, for testing the same-process formats, but I'm not sure if that's needed or useful -- you can't load an old version from disk or anything.

Note that this patch also carries around the structured clone version information, which doesn't seem to be used for anything, but I wanted the TestingFunction clone buffer object to match up to JSAutoStructuredCloneBuffer as much as possible.

And wow do some of these types need a rename. I noticed that the HTML5 spec has a whole set of new names; it probably doesn't fix this particular set, but it would be better to rename other of the internal functions to their names. A task for another day.
Attachment #8946556 - Flags: review?(jorendorff)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> 
> Very exploitable, *if* you are able to corrupt a buffer in a context that
> allows pointer-based Transferables to be used. Is that a case we're worried
> about? As in, a hacked content process breaking into its own privileged
> code? Or do we say that if they can manage to corrupt an internal clone
> buffer, then the game's up for that whole process and all we care about is
> preventing the chrome process from being hacked? I'll assume that's the case.

If a hacked content process can send this to the parent process (which I assume it can, because I specified "DifferentProcess" during the testing), then we consider this exploitable yes. That would be a sandbox escape. Since the hacked content process has full control over all of the clone buffer it wants to send, I don't see what would prevent it from sending this buffer.
(In reply to Christian Holler (:decoder) from comment #4)
> (In reply to Steve Fink [:sfink] [:s:] from comment #2)
> > 
> > Very exploitable, *if* you are able to corrupt a buffer in a context that
> > allows pointer-based Transferables to be used. Is that a case we're worried
> > about? As in, a hacked content process breaking into its own privileged
> > code? Or do we say that if they can manage to corrupt an internal clone
> > buffer, then the game's up for that whole process and all we care about is
> > preventing the chrome process from being hacked? I'll assume that's the case.
> 
> If a hacked content process can send this to the parent process (which I
> assume it can, because I specified "DifferentProcess" during the testing),
> then we consider this exploitable yes. That would be a sandbox escape. Since
> the hacked content process has full control over all of the clone buffer it
> wants to send, I don't see what would prevent it from sending this buffer.

I don't believe it can. You specified DifferentProcess, but internally the code forgot all about that when it came time to clean up the buffer. The scope wasn't properly propagated to everywhere that needed to use it, but that was a flaw in the testing functions. In the non-testing setup, JSAutoStructuredCloneBuffer is the container that remembers the scope, which is not forgeable via mangled data. For the testing setup, CloneBufferObject was sort of the equivalent except it didn't manage the scope. (I hadn't been thinking of them as equivalent until writing this patch. It would help to have better names, though I haven't come up with any.) This patch fixes that discrepancy.

In the browser, the scope is held in JSAutoStructuredCloneBuffer for the most part, although not everywhere. Where it is being read out of IndexedDB or similar, it should be set to DifferentProcess[1]. In other places, it is stored as a member field in StructuredCloneHolderBase (a dom class) and used when constructing the contained JSAutoStructuredCloneBuffer.

[1] Except it doesn't seem to be. https://searchfox.org/mozilla-central/source/dom/indexedDB/IDBObjectStore.cpp#1333 Filed bug 1434308
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
JSBugMon: Bisection requested, failed due to error (try manually).
As per comment 5, I believe this is a testing-only flaw. The browser code uses JSAutoStructuredCloneBuffer, which correctly tracks scopes. (Except when it does it wrong, but that's bug 1434308 and I don't think that's a big deal either.) As a result, I don't think this should be sec-high. I'm not sure who is supposed to lower those ratings. needinfo? decoder for that.
Flags: needinfo?(choller)
Group: javascript-core-security
Flags: needinfo?(choller)
Keywords: sec-high
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:]
JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test
Comment on attachment 8946556 [details] [diff] [review]
Trace structured clone buffer scopes properly for testing functions

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

sfink and I had a long video call and I think SCOPE_SLOT is unnecessary.
Attachment #8946556 - Flags: review?(jorendorff)
Flags: needinfo?(jorendorff) → needinfo?(sphink)
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/85839dc95172
user:        Steve Fink
date:        Mon Oct 09 17:41:01 2017 -0700
summary:     Bug 1406508 - Allow fuzzers to set binary clone buffer data, r=jorendorff

Steve, is bug 1406508 a likely regressor?
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> Steve, is bug 1406508 a likely regressor?

It enabled testing that this is a flaw in, so yes.
(In reply to Steve Fink [:sfink] [:s:] from comment #11)
> (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #10)
> > Steve, is bug 1406508 a likely regressor?
> 
> It enabled testing that this is a flaw in, so yes.

The issue was present before that though, so I wouldn't call that regressor. It just wasn't exposed in the JS shell.
Given that this is testing-only, I don't think we need to be tracking it.
This patch is now bitrotted and blocks all further StructuredClone testing. It is already marked as fuzzblocker and open for a month now. Can we please fix this now?
Flags: needinfo?(jorendorff)
Flags: needinfo?(sphink)
Flags: needinfo?(jorendorff)
The storedScope is an attractive nuisance. It cannot be relied upon because it could be forged. I think it's still ok to write it in and check against it, to catch non-malicious API misuse, but recording it in JSStructuredCloneReader is just an invitation for mistakes. Also, the comment describing the check (removed in this patch) is bogus.
Attachment #8969820 - Flags: review?(jorendorff)
Targeted fix for this bug, depending only on the isSynthetic check. I think this should be good enough to unblock fuzzing.
Attachment #8969821 - Flags: review?(jorendorff)
Priority: -- → P1
Comment on attachment 8969820 [details] [diff] [review]
Remove storedScope from structured clone

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

This is related to bug 1434308. In that particular case "it's still ok to write it in and check against it, to catch non-malicious API misuse" seems to be untrue: the check appears to prevent us landing the already-reviewed fix in that bug.

I would be OK with just deleting the check in readHeader (`if (storedScope < allowedScope)`) and treating `allowedScope` as a set of permissions, and it would be good to get that bug unstuck.

r=me either way.
Attachment #8969820 - Flags: review?(jorendorff) → review+
Comment on attachment 8969821 [details] [diff] [review]
Do not free transferables in synthetic clone buffers

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

::: js/src/builtin/TestingFunctions.cpp
@@ +2792,5 @@
>      void setData(JSStructuredCloneData* aData, bool synthetic) {
>          MOZ_ASSERT(!data());
>          setReservedSlot(DATA_SLOT, PrivateValue(aData));
>          setReservedSlot(SYNTHETIC_SLOT, BooleanValue(synthetic));
> +        // Temporary until the scope is moved into JSStructuredCloneData.

Style nit: Blank line before this comment, please.
Attachment #8969821 - Flags: review?(jorendorff) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #18)
> This is related to bug 1434308. In that particular case "it's still ok to
> write it in and check against it, to catch non-malicious API misuse" seems
> to be untrue: the check appears to prevent us landing the already-reviewed
> fix in that bug.
> 
> I would be OK with just deleting the check in readHeader (`if (storedScope <
> allowedScope)`) and treating `allowedScope` as a set of permissions, and it
> would be good to get that bug unstuck.

Ooh, I hadn't realized these were the same. You're right, that's another valid option for bug 1434308 -- just don't check.

I guess it seems like a nice check to have, since it's so easy to mess these things up. On the other hand, defending against API misuse that is already happening is kind of tricky.

Well, implementing the fancy fix for that bug (making a cutout to grandfather in the past misuse) shouldn't be that hard. So I think you're right -- I can remove the check, land bug 1434308, then put a patch up for review that re-adds the check. And if it turns out to be hard or weird, drop it on the floor.
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a31159fb6888
Remove storedScope from structured clone, r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7d7b8605ea
Do not free transferables in synthetic clone buffers, r=jorendorff
Backout by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bea67efaa0a
Backed out changeset 6b7d7b8605ea 
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7be222e99c6
Backed out changeset a31159fb6888 to fix a CLOSED TREE
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7bbc2e04472
Remove storedScope from structured clone, r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/17ca4a0eb913
Do not free transferables in synthetic clone buffers, r=jorendorff
https://hg.mozilla.org/mozilla-central/rev/d7bbc2e04472
https://hg.mozilla.org/mozilla-central/rev/17ca4a0eb913
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8969821 [details] [diff] [review]
Do not free transferables in synthetic clone buffers

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

Sorry for the awkward post-review review here, but I re-read this patch and had more to say.

::: js/public/StructuredClone.h
@@ +470,5 @@
>      size_t SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) {
>          return bufList_.SizeOfExcludingThis(mallocSizeOf);
>      }
>  
> +    // Temporary until the scope is moved into JSStructuredCloneData.

/**
 * Mark this clone buffer as containing data from another process. It can't 
 * legitimately contain pointers. If the buffer claims to have transferables,
 * it's a bug or an attack; don't free transferables when destroying the clone
 * buffer.
 *
 * This method is for testing only. It should go away in bug #######.
 */

Better yet, move the first paragraph to the declaration of OwnTransferablePolicy::IgnoreTransferablesIfAny.

::: js/src/builtin/TestingFunctions.cpp
@@ +2794,5 @@
>          setReservedSlot(DATA_SLOT, PrivateValue(aData));
>          setReservedSlot(SYNTHETIC_SLOT, BooleanValue(synthetic));
> +        // Temporary until the scope is moved into JSStructuredCloneData.
> +        if (synthetic)
> +            aData->IgnoreTransferables();

Same critique on this comment.

@@ +3082,5 @@
>                  JS_ReportErrorASCII(cx, "Invalid structured clone scope");
>                  return false;
>              }
>  
> +            if (fuzzingSafe && *maybeScope < scope) {

It seems like this should be banned whether we're `fuzzingSafe` or not.

`fuzzingSafe` is to rule out behavior that might be useful but is too potentially destructive for fuzzers. Does that apply here? I claim it's not useful.
Attachment #8969821 - Flags: review+
I'll file a new bug for those comments.
You need to log in before you can comment on or make changes to this bug.