1456604 - Guard against structured clone scope API misuse

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Description

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

This bug is the spawn of bug 1433642 and bug 1434308. I removed the storedScope checking because it would fail for IndexedDB if it were corrected to use DifferentProcess scope rather than its current (incorrect) SameProcessSameThread scope, where said incorrect scope is now stored on disk.

We should re-add the storedScope check using a special value to grandfather in IndexedDB, as discussed in bug 1434308.

Comment 1

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

Attachment: Add a SerializeForStorage scope and use it for IndexedDB

Comment 2

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

Attachment: Add a SerializeForStorage scope and use it for IndexedDB

I believe this was the idea? This patch only adds the scope, it doesn't actually remove any DifferentProcess-serializable things. But it adds back the error-checking, with the IndexedDB cutout.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8970758 [details] [diff] [review]
Add a SerializeForStorage scope and use it for IndexedDB

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

::: js/src/vm/StructuredClone.cpp
@@ +1700,5 @@
>                  JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_WASM_NO_TRANSFER);
>                  return false;
>              }
>  
> +            if (scope >= JS::StructuredCloneScope::DifferentProcess) {

I prefer to have here scope == JS::StructuredCloneScope::DifferentProcess || scope == S::StructuredCloneScope::SerializeForStorage

@@ +2404,5 @@
> +        // essentially DifferentProcess data, but labeled as
> +        // SameProcessSameThread (because that's what old code wrote.)
> +        return true;
> +    }
> +    

extra spaces

@@ +2451,5 @@
>          if (!in.read(&extraData))
>              return false;
>  
>          if (tag == SCTAG_TRANSFER_MAP_ARRAY_BUFFER) {
> +            if (allowedScope >= JS::StructuredCloneScope::DifferentProcess) {

same here

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8970758 [details] [diff] [review]
Add a SerializeForStorage scope and use it for IndexedDB

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

::: js/public/StructuredClone.h
@@ +150,2 @@
>       * When writing, this means we're writing for an audience in a different
>       * process. Produce serialized data that can be sent to other processes,

While you're here, please change this comment to mention that the HTML spec calls DifferentProcess mode "ForStorage", as in "StructuredSerializeForStorage", but we use it for IPC as well as storage.

https://html.spec.whatwg.org/multipage/structured-data.html#structuredserializeforstorage

@@ +160,5 @@
> +
> +    /**
> +     * Allow only data that is marked [Serializable] in the HTML spec. This is
> +     * also used to handle a backwards-compatibility case with IndexedDB -- see
> +     * bug 1434308.

I found this really misleading.

How about:

/**
 * When reading, this means: treat input as DifferentProcess, but as a
 * special backward-compatibility hack, allow reading a buffer written
 * with SameProcessDifferentThread, as long as it has no transferables.
 * See bug 1434308.
 *
 * This setting is not allowed for writing; use DifferentProcess instead.
 */
DifferentProcessForIndexedDB

(Below, I suggest treating it as an error if users try to write using this mode.)

My key review comment here is: if we're going to have a compatibility hack here, it must look like a hack. Make it clear to future readers that there's no other mysterious difference in behavior.

::: js/src/vm/StructuredClone.cpp
@@ +1700,5 @@
>                  JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_WASM_NO_TRANSFER);
>                  return false;
>              }
>  
> +            if (scope >= JS::StructuredCloneScope::DifferentProcess) {

I agree!

And if we can ban the new scope setting for writing, we should.

Comment 5

[Jason Orendorff [:jorendorff]]

OK, my thinking about how "scope" should work in this API has evolved. Or, some old news has finally gotten through my skull.

Here's my new complete answer, with rationale:

- StructuredCloneScope should be renamed to SerializeMode, and should have 3 values
- we should use it only when writing
- when reading, we can't rely on the stored scope, so we can
  use it to detect errors early, but must otherwise ignore it
- when reading, we only *really* care whether or not the buffer is
  trusted+SameProcess (i.e.: may contain pointers and own objects)
- that's a property of the clone buffer, not an option in the read API
  (we know this because the clone buffer destructor needs to know it)
- so the read functions should not take a scope argument at all!
- paranoid readers can assert, *before* reading, that the buffer is
  or is not trusted, as desired, but if they're not already setting
  that flag correctly when they create the buffer, they're already pwnt

Comment 6

[Jason Orendorff [:jorendorff]]

Comment 5 is meant as general commentary, not a review comment on the patch in this bug. ...It probably belongs somewhere else.

Comment 7

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

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> My key review comment here is: if we're going to have a compatibility hack
> here, it must look like a hack. Make it clear to future readers that there's
> no other mysterious difference in behavior.

Ok, I guess I didn't comment this at all, but my intention was to use the new scope for two different purposes: (1) to support the new behavior desired by bug 1350254 related to webidl [Serializable], and (2) for the backwards-compatibility hack. I actually just kind of made that assumption at some point without questioning it, but you're right, it's not at all obvious nor necessarily a good idea.

The implementation here is just the backwards-compatibility hack. I figured that once someone actually starts looking at the Serializable flag, they'd use SerializeForStorage wherever appropriate.

Come to think of it... I'm not sure SerializeForStorage is appropriate for IndexedDB. If we *do* have different modes for "just [Serializable]" vs "whatever we might want to support writing to disk for internal purposes", then IndexedDB is likely to be the latter.

Ugh. I'm going to redo this patch to only cover IndexedDB.

Comment 8

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

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> - StructuredCloneScope should be renamed to SerializeMode, and should have 3
> values
> - we should use it only when writing

(I think I argued myself into agreement below.)

> - when reading, we can't rely on the stored scope, so we can
>   use it to detect errors early, but must otherwise ignore it

That should be the case now, after the storedScope field removal.

> - when reading, we only *really* care whether or not the buffer is
>   trusted+SameProcess (i.e.: may contain pointers and own objects)

I'll admit I'm still a little fuzzy on the exact requirements here, partly as a hangover from thinking that "can contain Transferables" is the relevant thing. And it really isn't.

 - DifferentProcess + Transferable is ok, it just means the buffer has to have a copy of the data instead of a pointer.

> - that's a property of the clone buffer, not an option in the read API
>   (we know this because the clone buffer destructor needs to know it)
> - so the read functions should not take a scope argument at all!

Hm. My thinking has been that when you're reading, you really ought to know what you're capable of handling. Having an API that allows reading something that you're not consciously aware you need to be able to handle (eg a pointer with thread restrictions) is a footgun. Though...

> - paranoid readers can assert, *before* reading, that the buffer is
>   or is not trusted, as desired, but if they're not already setting
>   that flag correctly when they create the buffer, they're already pwnt

...that seems like it ought to be good enough -- but despite patching it a bunch, I'm still not clear on the full set of API users. It seems remarkably difficult to know the right scope/mode upfront, which is why I created the gross Unassigned state to allow its value to be set later. That may just be a symptom of me not understanding the using code well enough.

And come to think of it, the Unassigned thing suggests that you are correct, and it's good enough to have the assertions that say the scope can't be Unassigned by the time you have a nonempty buffer.

So now I'm thinking that perhaps I should finish fixing this here, then strip things down to specify

 - trusted, can contain pointers
 - with Transferables (used during writing only; the resulting buffer will end up storing either pointers or data copies, depending on the previous setting, but in any case the write will have a side effect of detaching the source)
 - can contain SharedArrayBuffers (implies trusted)

That eliminates the SameProcessSameThread/SameProcessDifferentThread distinction that we haven't used yet, and keeps a single untrusted mode that will only include [Serializable] objects, unless someone comes up with a use case where we want to serlialize non-[Serializable] objects to IndexedDB or over IPC or something.

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #7)
> I'll admit I'm still a little fuzzy on the exact requirements here,
> partly as a hangover from thinking that "can contain Transferables"
> is the relevant thing. And it really isn't.

We're in the same boat. Here are my logical starting points right now:

1. The serialization procedure in the spec only has two modes: forStorage and not forStorage.

2. The deserialization procedure in the spec has no modes. It just does whatever the buffer tells it to do.

3. The restrictions on DifferentProcess buffers are exactly the same as those on untrusted, presumed-hostile buffers, a.k.a. synthetic buffers. Such buffers can't contain pointers because there's no safe way to use them or even discard them (meaning: unlike spec-ese, C++ has destructors, and embedded pointers have to be freed or decref'd, which is not going to be safe).

> (1) to support the new behavior
> desired by bug 1350254 related to webidl [Serializable]

Oh, cool. That might be fine! But if we're lucky, [Serializable] won't require a new mode (see #1 above).

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b6f6fcaaa17
Add DifferentProcessForIndexedDB scope to handle backwards compatibility with SameProcessSameThread clones, r=jorendorff

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7b6f6fcaaa17