js_ErrorFromException handling of same-origin cross-compartment wrappers allows bypass of onerror sanitization

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({sec-moderate})

unspecified
mozilla24
x86
macOS
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox21 wontfix, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, firefox-esr17 wontfix, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 unaffected)

Details

(Whiteboard: [adv-main24-])

Attachments

(3 attachments)

This is great fun.

js_ErrorFromException returns null for a cross-compartment wrapper around an Error object.  When that happens we try to see whether the object quacks like an Error (which in the same-origin case it does), and if so manually synthesize a new JSErrorReport.  But this new JSErrorReport has no principals set, and ScriptErrorEvent::Run assumes no principals == same-origin, so we don't do any sanitization of the exception.

It's not clear to me whether the ScriptErrorEvent::Run behavior is what we need to change, but we should fix this somehow....

See attached testcase, which sees details about a cross-origin exception that it should not see.  You'll need to allow the non-https content or download it locally.
If the API involves the JS engine passing a non-compartmentalized representation of an error that occurred, and we want the sanitization to happen in DOM code, then I think the answer is probably to pass the principal more correctly. I wonder if there's a way we can do this in a more deliberate and verifiable fashion.
Assignee: general → bzbarsky
Group: dom-core-security
Attachment #751076 - Flags: review? → review?(jwalden+bmo)
Comment on attachment 751076 [details] [diff] [review]
Try harder to get a JSErrorReport out of exceptions.

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

r=bholley, but shouldn't we also be setting a principal on the duck-typed error report we generate to avoid the footgun of "no principal == same origin"?
Attachment #751076 - Flags: review?(bobbyholley+bmo) → review+
> shouldn't we also be setting a principal on the duck-typed error report we generate

That's not a bad idea, but which principal?  Just grab it from the compartment of the UncheckedUnwrapped object?
Flags: needinfo?(bobbyholley+bmo)
(In reply to Boris Zbarsky (:bz) from comment #4)
> > shouldn't we also be setting a principal on the duck-typed error report we generate
> 
> That's not a bad idea, but which principal?  Just grab it from the
> compartment of the UncheckedUnwrapped object?

Hm, I'm not really wild about at second UncheckedUnwrap here. Given that this only applies to the duck-typing case, and in the duck-typing case we see exactly what we're allowed to see, why not use use the principal of the object, wrapper or not?
Flags: needinfo?(bobbyholley+bmo)
Any suggestions on a security rating here? What are the implications of this?
Because the whole point of the principal thing here is to deal with cases where the page is throwing the exception itself but is NOT allowed to see its text because that comes from a cross-origin script.

I guess that really only matters for syntax errors and those can't really end up duck-typed anyway...

Given that, I'm not quite sure to what extent this bug is a security problem, actually, since obviously my testcase could just try/catch to grab the exceptions and interrogate them and there is _probably_ no way to get cross-compartment-wrapping behavior for script load syntax errors, right?
(In reply to Boris Zbarsky (:bz) from comment #7)
> Because the whole point of the principal thing here is to deal with cases
> where the page is throwing the exception itself but is NOT allowed to see
> its text because that comes from a cross-origin script.

But wouldn't such objects be wrapped in an opaque-enough wrapper that the caller can't see anything of interest? Or is the issue here that the compartment doing the duck-typing isn't necessarily the same compartment/origin that will eventually receive the error callback? If so, perhaps that's more the root of the problem...


> Given that, I'm not quite sure to what extent this bug is a security
> problem, actually, since obviously my testcase could just try/catch to grab
> the exceptions and interrogate them and there is _probably_ no way to get
> cross-compartment-wrapping behavior for script load syntax errors, right?

Define "cross-compartment-wrapping behavior"?
> But wouldn't such objects be wrapped in an opaque-enough wrapper that the caller can't
> see anything of interest? 

The principal of interest in the exception is the principal the script was loaded from.  It's not the principal the script is running as, and is completely unrelated to any other principal in the system or any compartments.  Therefore it's completely unrelated to wrappers too.

A better question is whether for the case that really matters (parse errors) the situation can arise at all that there's a cross-compatment wrapper here.  That depends on which exact cx the script loader ends up running the scripts on and in which compartments.  Which is fragile, but might be safe right now...

> Define "cross-compartment-wrapping behavior"?

"having a thing here which is not an actual Error object but has one inside".

Again, the principal in the JSErrorReport only matters for deciding what information to report to window.onerror.  Since onerror is called for parse errors, which are not otherwise catchable, we sanitize the information passed to it based on the originating principal of the script (which is different from the principal the script is running with: the latter is the principal of the page).

For any catchable exception there is no point in sanitizing onerror, since the caller can just catch it and examine it.
Whiteboard: [need review]
Comment on attachment 751076 [details] [diff] [review]
Try harder to get a JSErrorReport out of exceptions.

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

Please post a test that fails without this, and passes with, that we can push whenever this opens up.
Attachment #751076 - Flags: review?(jwalden+bmo) → review+
Attachment #752186 - Flags: review?(jwalden+bmo)
Comment on attachment 752186 [details] [diff] [review]
Test for this bug

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

Looks like it should work.  Man, that's a mess.  :-)
Attachment #752186 - Flags: review?(jwalden+bmo) → review+
I just realized I should have asked for sec-approval before pushing...  In any case, I'm not sure it's worth worrying about this for branches.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8290ccca36d1
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla24
Ah well. We never did narrow down a good security rating on this, right?
My gut feeling is sec:low, but I don't have a good feel for our security ratings, so I might be off.

My reasoning is as follows:

1)  No actual known exploit known, just a possible vector.
2)  Worst-case scenario is a cross-site information leak.

So for a problem to arise someone would need to find a way to use this vector, then find something that would leak something useful.
Sounds like a sec-moderate, maaybe, if not a sec-low. I'll err up.
Keywords: sec-moderate
https://hg.mozilla.org/mozilla-central/rev/8290ccca36d1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
How far back does this go? Does this need to be backported to 23 or other branches?
It goes way back (probably back to compartments landing). And I don't think this is worth backporting...
Whiteboard: [adv-main24+]
Whiteboard: [adv-main24+] → [adv-main24-]
Group: core-security
See Also: → 1184310
You need to log in before you can comment on or make changes to this bug.