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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase. Either allow the http content or download locally.

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.

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Try harder to get a JSErrorReport out of exceptions.

Comment 3

[Bobby Holley (:bholley)]

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"?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 5

[Bobby Holley (:bholley)]

(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?

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Any suggestions on a security rating here? What are the implications of this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Bobby Holley (:bholley)]

(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"?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 10

[Jeff Walden [:Waldo]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Test for this bug

Comment 12

[Jeff Walden [:Waldo]]

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.  :-)

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Ah well. We never did narrow down a good security rating on this, right?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Sounds like a sec-moderate, maaybe, if not a sec-low. I'll err up.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8290ccca36d1

Comment 18

[Al Billings [:abillings - ex-MoCo]]

How far back does this go? Does this need to be backported to 23 or other branches?

Comment 19

[Boris Zbarsky [:bzbarsky]]

It goes way back (probably back to compartments landing). And I don't think this is worth backporting...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 873229 test. r=waldo

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71a189bb56b3
test.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/506535e6646f
Fix ESlint failure. CLOSED TREE

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/71a189bb56b3
https://hg.mozilla.org/mozilla-central/rev/506535e6646f