933574 - ReportPendingException is using wrong compartment

Status: RESOLVED DUPLICATE of bug 872273

(Core :: DOM: Core & HTML, defect)

Description

[Gregor Wagner [:gwagner]]

Attachment: patch

Errors from indexeddb are getting converted into:
E/GeckoConsole( 1626): [JavaScript Error: "Permission denied to access property 'toString'"]
E/GeckoConsole( 1626): [JavaScript Error: "Permission denied to access property 'message'"]
E/GeckoConsole( 1626): [JavaScript Error: "uncaught exception: unknown (can't convert to string)"]

Comment 1

[Gregor Wagner [:gwagner]]

mrbkap and bent fixed this one.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 825682 [details] [diff] [review]
patch

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

::: dom/base/nsJSUtils.cpp
@@ +140,5 @@
>  {
>    if (JS_IsExceptionPending(aContext)) {
>      bool saved = JS_SaveFrameChain(aContext);
>      {
> +      JS::RootedValue exn(aContext);

JS::Rooted<JS::Value>.

@@ +143,5 @@
>      {
> +      JS::RootedValue exn(aContext);
> +      JS_GetPendingException(aContext, &exn);
> +
> +      JS::RootedObject scope(aContext);

JS::Rooted<JSObject*>.

Comment 3

[Gregor Wagner [:gwagner]]

Attachment: patch

I think Bobby wrote this code.

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: patch

Its friday...

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 826068 [details] [diff] [review]
patch

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

/me has been eying this patch suspiciously for a few days now, and has decided that it's not kosher.

If someone throws |new Error()|, we compute a JSErrorReport on the spot, and stash it in a private slot on the object that bubbles up the stack. If it gets all the way up, we invoke JS_ReportPendingException(cx), which delegates to js_ReportUncaughtException(cx).

If the exception is a bonafide Error object (security-wrapped or not), we pull out the JSErrorReport, and pass it along to DOM script error reporter. Unwrapping the security wrapper is fine, because the JSErrorReport has an originPrincipals associated with it, which means it gets properly sanitized when passing to any any handler that is not same-origin with the origin of the _script_ (not the page) that threw the error.

If the exception is not bonafide Error object, we try to duck-type it, and generate an error report manually (with a null originPrincipals). This duck-typing is ok, because for cross-origin exception objects, we'll have a security wrapper that filters things appropriately.

But this all depends on the security wrappers encountered during duck-typing being the ones that would be seen by the scope in which the onerror handler might be running, which is that of the current inner window of the nsJSContext of whatever cx we're reporting the exception on. If we enter the compartment of the error object, we can leak information that the onerror handler isn't supposed to see. For example, if you did |throw crossOriginWindow.location|, and it passed duck-typing somehow, you'd report the stringification of the cross-origin location object to the onerror handler, which is a chemspill-worth offense.

Interestingly, this patch as it stands isn't actually always going to enter its intended compartment, because the pending exception is rewrapped on each compartment enter/leave. The only reason it's at all different in this case is that the JS_SaveFrameChain does cx->setCompartment(null), which means that the exception gets left in whatever scope we happened to be in before the call to nsJSUtils::ReportPendingException, which varies depending on the caller.

Boris, Waldo - does that all seem right?

Comment 7

[Bobby Holley (:bholley)]

And in terms of the bug you're trying to solve, I think you basically want bug 872273, which I never managed to land.

Comment 8

[Gregor Wagner [:gwagner]]

So what are the next steps here? Bobby, are you going to finish bug 872273?
We hit assertions during tests and we have no good way to get the actual content.
Like here for example: https://tbpl.mozilla.org/php/getParsedLog.php?id=30444652&tree=Cedar

Comment 9

[Bobby Holley (:bholley)]

I will fix bug 872273 if Waldo fixes bug 724768.

Comment 10

[Jeff Walden [:Waldo]]

Fixing that now that there's definite need sounds good to me.  It should be fairly quick, I'd guess on the order of a day or two.  I have some backporting of other patches to do, but after that I can pick that up again.  Guessing patches will be ready there by end of next week, if not sooner.

Comment 11

[Gregor Wagner [:gwagner]]

Is this now a dup of 872273?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Gregor Wagner [:gwagner] from comment #11)
> Is this now a dup of 872273?

I would think so, yes. If you run into any other problems in this area, please file. But I think this bug can be closed.