1444056 - Tighten the scope access controls in StructuredCloneHolder

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Christian Holler (:decoder)]

While working on the structured clone fuzzing, I noticed that the StructuredCloneHolder code has assertions like this in place:

https://searchfox.org/mozilla-central/rev/33cc9e0331da8d9ff3750f1e68d72d61201176cb/dom/base/StructuredCloneHolder.cpp#1009-1010

After talking to :baku it turned out that we don't check the clone scope when deserializing some things that should only be possible to deserialize in either SameProcessSameThread or SameProcessDifferentThread. Essentially this means, that in the DifferentProcess case, the IPC child can access code it is not supposed to be able to access.

While I haven't found any severe crashes or other vulnerabilities yet, related to this fact, we certainly should restrict access here as good as possible. Every piece of code that the child can reach that it shouldn't be able to reach in the first place is attack surface that can be used for sandbox escapes.

Bug 1442722 could also be related to this: In that bug, mutated data is reaching DiscardTransferables although that should never be possible in DifferentProcess scenarios. It is yet to be determined if that bug is a security problem or not.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: algorithm.patch

Just to make the code a bit stronger and secure, I split StructuredCloneHolder in 3 classes:

StructuredCloneHolderSameProcessSameThread
StructuredCloneHolderSameProcessDifferentThread
StructuredCloneHolderDifferentProcess

The first 2 share the same base class: StructuredCloneHolderSameProcess with custom methods to read/write/transfer objects.

I also cleaned up the throwing of exception, giving the control of throwing the DATA_CLONE error to the JS engine. Here I need Steve to take a look.

I also removed a couple of assertions related to transferring/cloning support and replace them with if() stmt.

Comment 2

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

Comment on attachment 8966519 [details] [diff] [review]
algorithm.patch

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

r=me for the part relevant to me, just some minor suggestions to consider.

::: dom/base/StructuredCloneHolder.h
@@ +35,5 @@
>  namespace dom {
>  
> +// Probably, this is not the class you should use. This is a low level
> +// Structured Clone class that requires custom callback handlers.
> +// Consider the use of one of the StructuredCloneHolder* classes.

Is this ever something you would want? If not, wouldn't it be safer to make this an abstract base class? Maybe by making the destructor pure virtual or something and having the subclasses' destructors call some protected cleanup() function.

@@ +147,5 @@
>  class BlobImpl;
>  class MessagePort;
>  class MessagePortIdentifier;
> +class StructuredCloneHolderSameProcess;
> +class StructuredCloneHolderDifferentProcess;

I guess the set of collections doesn't exactly line up, but which sounds better? StructuredCloneHolderDifferentProcess or StructuredCloneHolder<DifferentProcess>?

I guess that would be StructuredCloneHolder<JS::StructuredCloneScope::DifferentProcess> which is pretty long.

You may end up repeating all the code anyway, I guess.

@@ +366,5 @@
>  
> +class StructuredCloneHolderSameProcessSameThread;
> +class StructuredCloneHolderSameProcessDifferentThread;
> +
> +// Please, don't use this class. This is just to avoid duplicated close.

"close"? Did you mean "code", maybe?

::: js/src/vm/StructuredClone.cpp
@@ +1614,5 @@
> +        if (callbacks && callbacks->write) {
> +            if (callbacks->write(context(), this, obj, closure))
> +                return true;
> +
> +            // If the writing fails, we should throw an exception if we don't have already one.

s/have already/already have/

@@ +1616,5 @@
> +                return true;
> +
> +            // If the writing fails, we should throw an exception if we don't have already one.
> +            if (!context()->isExceptionPending())
> +                return reportDataCloneError(JS_SCERR_UNSUPPORTED_TYPE);

I don't know what the significance of NS_ERROR_DOM_DATA_CLONE_ERROR is, but it looks like this is changing the exact error reported? I'm fine with that if you are.

@@ +1624,4 @@
>          // else fall through
>      }
>  
>      return reportDataCloneError(JS_SCERR_UNSUPPORTED_TYPE);

I guess they're both a little weird, but given that the error is the same, I *think* it looks a little better as

  {
    ...
    if (callbacks && callbacks->write) {
      if (callbacks->write(...))
        return true;

      if (context()->isExceptionPending())
        return false;
    }
    // fall through
  }

  // Not successfully handled.
  return reportDataCloneError(...);

That's assuming you definitely want JS_SCERR_UNSUPPORTED_TYPE instead of some error that indicates the handler failed.

Comment 3

[Christian Holler (:decoder)]

Comment on attachment 8966519 [details] [diff] [review]
algorithm.patch

Looks good to me. The separation introduced here should avoid accidentally entering code that isn't meant to be reachable in e.g. DifferentProcess scope and is probably more solid than adding a lot of if statements guarding the code.

There are a few "don't use this class" comments in there. Have we checked if anyone is using them directly? And I guess there is no good way to avoid them being used outside of this area easily?

Comment 4

[Andrea Marchesini [:baku]]

> There are a few "don't use this class" comments in there. Have we checked if
> anyone is using them directly? And I guess there is no good way to avoid
> them being used outside of this area easily?

I make the CTOR/DTOR private. So those classes cannot be used directly.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: algorithm2.patch

I haven't investigated this too much, but the issue is that, with the new approach, here we receive a DataCloneError. This seems totally correct, but without this patch, the message will be "An unexpected error occurred" because it would not be taken from the DataCloneError object.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: algorithm2.patch

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8968843 [details] [diff] [review]
algorithm2.patch

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

Hi Andrea,
Follows a couple of feedback comments.

I think that :kmag should also take a quick look at this, he may have additional ideas/opinions (and he may also notice other possible regressions that could be introduced by this change and not explicitly covered by an existent test case).

::: toolkit/components/extensions/ExtensionCommon.jsm
@@ +341,5 @@
>          message = error.message;
>          fileName = error.fileName;
>        }
> +
> +      if ("type" in error && error.type == "DataCloneError") {

If I'm not wrong this should actually check `error.name` instead of error.type.
I guess that by checking error.type this is still failing on the test_ext_runtime_sendMessage_errors.js xpcshell test, am I right?

@@ +342,5 @@
>          fileName = error.fileName;
>        }
> +
> +      if ("type" in error && error.type == "DataCloneError") {
> +        message = error.message;

When the DataCloneError is coming from a message that an extension is trying to send using browser.runtime.sendMessage/browser.tabs.sendMessage(or port.sendMessage) and it can't be sent across processes because it is not "structured clonable", I would also expect that we have a `caller` parameter available, which should be a SavedFrame which contains the stacktrace of the original extension API call (introduced by Bug 1441333 in Firefox 60), and in that case it would be nicer to create an error object which would contain that as its stacktrace, similarly to the one returned by lines 334 and 335 of this file (the ones that clone the caller object into the target extension global and than create the new Error object using `ChromeUtils.createError(error.message, caller);`).

Comment 8

[Kris Maglione [:kmag]]

(In reply to Andrea Marchesini [:baku] from comment #5)
> Created attachment 8968820 [details] [diff] [review]
> algorithm2.patch
> 
> I haven't investigated this too much, but the issue is that, with the new
> approach, here we receive a DataCloneError. This seems totally correct, but
> without this patch, the message will be "An unexpected error occurred"
> because it would not be taken from the DataCloneError object.

Where is this happening? If there are places where we want to expose these errors to extensions, we should probably explicitly handle those places, rather than whitelisting DataCloneErrors in general.

Also, I'd expect the cases we'd want to expose would mostly be when we're constructing or deserializing StructuredCloneHolders for extension scopes... I'm not sure whether the errors belong to the scope we're cloning for/from in those cases (I'd expect them to be, since we enter the object compartment before cloning), but if not, we should probably make it so they are.

Comment 9

[Kris Maglione [:kmag]]

comment 8

Comment 10

[Andrea Marchesini [:baku]]

toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_errors.js This is the test failing. Is this what you are asking?

Comment 11

[Christian Holler (:decoder)]

:baku, is this bug still relevant and being worked on? We fixed the assertions in bug 1452576 and I assume this bug is about a more generic refactoring to avoid this class of errors?

Comment 12

[Andrea Marchesini [:baku]]

Let's close this bug. The current assertions are good enough.

Comment 13

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Clear old pending requests.