ReportPendingException is using wrong compartment

RESOLVED DUPLICATE of bug 872273

Status

()

Core
DOM
RESOLVED DUPLICATE of bug 872273
4 years ago
4 years ago

People

(Reporter: gwagner, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

1.12 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review-
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Created attachment 825682 [details] [diff] [review]
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)"]
(Reporter)

Comment 1

4 years ago
mrbkap and bent fixed this one.
(Reporter)

Updated

4 years ago
Attachment #825682 - Flags: review?(bzbarsky)
(Reporter)

Updated

4 years ago
Blocks: 921721
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*>.
(Reporter)

Comment 3

4 years ago
Created attachment 826047 [details] [diff] [review]
patch

I think Bobby wrote this code.
Attachment #825682 - Attachment is obsolete: true
Attachment #825682 - Flags: review?(bzbarsky)
Attachment #826047 - Flags: review?(bobbyholley+bmo)
(Reporter)

Comment 4

4 years ago
Created attachment 826049 [details] [diff] [review]
patch
Attachment #826047 - Attachment is obsolete: true
Attachment #826047 - Flags: review?(bobbyholley+bmo)
Attachment #826049 - Flags: review?(bobbyholley+bmo)
(Reporter)

Comment 5

4 years ago
Created attachment 826068 [details] [diff] [review]
patch

Its friday...
Attachment #826049 - Attachment is obsolete: true
Attachment #826049 - Flags: review?(bobbyholley+bmo)
Attachment #826068 - Flags: review?(bobbyholley+bmo)
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?
Attachment #826068 - Flags: review?(bobbyholley+bmo) → review-
And in terms of the bug you're trying to solve, I think you basically want bug 872273, which I never managed to land.
(Reporter)

Comment 8

4 years ago
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
Flags: needinfo?(bobbyholley+bmo)
(Reporter)

Updated

4 years ago
Blocks: 933355
I will fix bug 872273 if Waldo fixes bug 724768.
Flags: needinfo?(bobbyholley+bmo) → needinfo?(jwalden+bmo)
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.
Flags: needinfo?(jwalden+bmo)
(Reporter)

Updated

4 years ago
Depends on: 872273
(Reporter)

Comment 11

4 years ago
Is this now a dup of 872273?
Flags: needinfo?(bobbyholley)
(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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(bobbyholley)
Resolution: --- → DUPLICATE
Duplicate of bug: 872273
You need to log in before you can comment on or make changes to this bug.