1871442 - Potential sandbox escape in ContentParent::RecvScriptErrorInternal with malicious stack object

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Christian Holler (:decoder)]

There might be an issue in ContentParent::RecvScriptErrorInternal where a compromised content process can pass in an arbitrary JS object via the aStack argument, which is StructuredClone data. This data is deserialized and it is checked if it is a JSObject, however, the script error implementation later expects this to be a proper SavedFrame object. There is even a check in place, but it is a MOZ_ASSERT and should likely be a release assert instead, according to jandem:

https://searchfox.org/mozilla-central/rev/8461ad1fc943ac560414322b66de5929ef10f706/dom/ipc/ContentParent.cpp#5159

As far as I know, mismatching these object types can cause crashes, so I would assume this is dangerous in terms of sandbox security.

More about how this was found in a private follow-up comment.

Comment 2

[Andrew McCreight [:mccr8]]

That's an interesting find. js::UnwrapSavedFrame() (called by nsScriptErrorWithStack::ToString) does maybeUnwrapAs<SavedFrame>() which does a dynamic check so that's should be type safe at least.

The other use seems to be GetStack() which looks like it is only used from JS. I'd guess that avoids type safety issues, unless devtools passes it back into C++ somewhere, but I'd worry about stuff like an object where the line property is actually a proxy that runs unexpected code. I'm not sure what the principal of this object is.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Why is this sec-high? The object is from structured clone, so it is rather dummy, unless some proto hacks or similar are still allowed. (But this particular case wouldn't be any different to message passing through JS actor APIs)

Comment 4

[Christian Holler (:decoder)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #3)

Why is this sec-high? The object is from structured clone, so it is rather dummy, unless some proto hacks or similar are still allowed. (But this particular case wouldn't be any different to message passing through JS actor APIs)

It was unclear what exactly you could do if you can inject arbitrary objects here, in particular if you can somehow cause it to execute arbitrary JS code. I talked to :sfink about this and he took quite a while to convince himself that there is no way to achieve this with plain structured clone. However, he mentioned that "the most likely way something like that could happen is if there's some funky DOM object that has specific structured cloning support, and it has some host-implemented getter-like thing. But I don't know of any like that."

But I would be happy to hear from you what you think about it.

And yes, you are absolutely right, this question isn't specific anymore to this bug, it would also affect other JS actor APIs. I just would like to build more confidence around this. It feels like we could easily miss things with all of the things we can deserialize through StructuredClone.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Not sure what that host-implemented getter-like thing should do to be particularly bad.
Sure, DOM stuff may be structure-cloned, but in some sense that isn't too different to serialize stuff through IPC.

Is there anything specific here which hints actual sec-high/csectype-sandbox-escape issue? This feels more like a sec-audit level thing, no?

Comment 6

[Christian Holler (:decoder)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #5)

Not sure what that host-implemented getter-like thing should do to be particularly bad.
Sure, DOM stuff may be structure-cloned, but in some sense that isn't too different to serialize stuff through IPC.

The concern here is that the child somehow can inject arbitrary JS code to run in the parent. This came up as one of the potential concerns of arbitrary object being passed instead of a SavedFrame object.

Is there anything specific here which hints actual sec-high/csectype-sandbox-escape issue? This feels more like a sec-audit level thing, no?

Yes, comment 2 second paragraph mentions that we aren't certain that the given object isn't passed back into C++ somewhere. The underlying issue in this bug is an object of wrong type being passed, violating a MOZ_ASSERT expressing a requirement of nsScriptErrorWithStack.

In any case, the initial sec-high rating was based on the assumption that you could directly run JS code with this, which seems to be not possible. I'll remove the rating until we have concluded the investigation.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1871442 - better to handle an unexpected case rather than just debug-only assert about it, r=decoder

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9928e7691abc
better to handle an unexpected case rather than just debug-only assert about it, r=decoder

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/9928e7691abc