Closed
Bug 1433642
Opened 6 years ago
Closed 6 years ago
AddressSanitizer: attempting free on address which was not malloc()-ed or Assertion failure: node, at memory/build/mozjemalloc.cpp:3895 or various crashes
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | wontfix |
firefox58 | --- | unaffected |
firefox59 | - | wontfix |
firefox60 | - | wontfix |
firefox61 | - | fixed |
People
(Reporter: decoder, Assigned: sfink)
References
(Blocks 1 open bug)
Details
(5 keywords, Whiteboard: [fuzzblocker] [jsbugmon:])
Attachments
(3 files)
11.58 KB,
patch
|
Details | Diff | Splinter Review | |
5.43 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
5.42 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
Reporter | ||
Updated•6 years ago
|
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.
Assignee | ||
Comment 2•6 years ago
|
||
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)
Assignee | ||
Comment 3•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•6 years ago
|
||
(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.
Assignee | ||
Comment 5•6 years ago
|
||
(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
Updated•6 years ago
|
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
Comment 6•6 years ago
|
||
JSBugMon: Bisection requested, failed due to error (try manually).
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 7•6 years ago
|
||
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)
Reporter | ||
Updated•6 years ago
|
Updated•6 years ago
|
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:]
Comment 8•6 years ago
|
||
JSBugMon: Cannot process bug: Error: Failed to isolate original revision for test
Updated•6 years ago
|
Comment 9•6 years ago
|
||
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)
Updated•6 years ago
|
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?
Assignee | ||
Comment 11•6 years ago
|
||
(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.
Reporter | ||
Comment 12•6 years ago
|
||
(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.
Comment 13•6 years ago
|
||
Given that this is testing-only, I don't think we need to be tracking it.
status-firefox61:
--- → affected
tracking-firefox61:
--- → -
Reporter | ||
Comment 14•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(sphink)
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 16•6 years ago
|
||
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)
Assignee | ||
Comment 17•6 years ago
|
||
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)
Updated•6 years ago
|
Priority: -- → P1
Comment 18•6 years ago
|
||
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 19•6 years ago
|
||
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+
Assignee | ||
Comment 20•6 years ago
|
||
(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.
Comment 21•6 years ago
|
||
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
Comment 22•6 years ago
|
||
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
Comment 23•6 years ago
|
||
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
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d7bbc2e04472 https://hg.mozilla.org/mozilla-central/rev/17ca4a0eb913
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 25•6 years ago
|
||
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+
Comment 26•6 years ago
|
||
I'll file a new bug for those comments.
Updated•6 years ago
|
Attachment #8969821 -
Flags: review+
Updated•6 years ago
|
Updated•4 years ago
|
Blocks: asan-maintenance
You need to log in
before you can comment on or make changes to this bug.
Comment 1
•