1070131 - Replace JSErrorReport::originPrincipals with a 'muted errors flag'

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

I have patches for this which move the subsumes check between the origin principal and the execution into the JS engine at the time the error is raised. Doing so has three advantages:

(1) We're closer to the relevant principals, and are more likely to compare the right ones.
(2) It simplifies the Gecko-side handling of exceptions.
(3) it aligns us more with HTML5.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Why the time the error is raised and not the time when we first compile the script?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #1)
> Why the time the error is raised and not the time when we first compile the
> script?

Mostly because that would have been a more involved refactoring and wasn't necessary for my immediate goals. This bug will get us part of the way there, and we can file a followup to do the rest if we want to.

https://tbpl.mozilla.org/?tree=Try&rev=ce6b1dd3e015

Comment 3

[Bobby Holley (:bholley)]

Oh also, it doesn't help in the situation where the JSScript doesn't exist yet (i.e. when the error occurs during script parsing).

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=d2a3a1878ea5

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Reduce the cases where we dispatch nsScriptErrorEvent. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add a helper on JSPrincipals to compute subsumes. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Simplify script error muting and make our implementation look more like the spec's. v1

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8492903 [details] [diff] [review]
Part 2 - Add a helper on JSPrincipals to compute subsumes. v1

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

::: js/public/Principals.h
@@ +15,5 @@
>  
>  #include "jspubtd.h"
>  
> +struct JSPrincipals;
> +// Return whether the first principal subsumes the second. The exact meaning of

Can you put a \n between the decl and the comment?

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8492904 [details] [diff] [review]
Part 3 - Simplify script error muting and make our implementation look more like the spec's. v1

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

So this patch changes originPrincipals from having the simple opaque meaning "propagate this to child scripts" to something with meaning that we check (in two places).  Ideally, I think, we'd have some "JSIsErrorMutedOp" that we call, passing the current and origin principals (and maybe JSSubsumeOp) so that the embedding (who decides what originPrincipals means anyways) gets to decide.  If you think that is a bad idea, could we have a single inline check in public/Principals.h with a big beefy comment explaining why we are checking subsumption the way we are?

Comment 10

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #9)
> So this patch changes originPrincipals from having the simple opaque meaning
> "propagate this to child scripts" to something with meaning that we check
> (in two places).

Yeah. The upside is that we don't have to hang onto (and refcount) these opaque objects in as many places.

> Ideally, I think, we'd have some "JSIsErrorMutedOp" that
> we call, passing the current and origin principals (and maybe JSSubsumeOp)
> so that the embedding (who decides what originPrincipals means anyways) gets
> to decide.

There's certainly some logic to that, but I think it's a bit pedantic. The reintroduction of JSSubsumesOp is a recognition that the practicality of a single security callback for a few one-off checks outweighs the (somewhat hollow) conceptual win of removing all security decisions from the JS engine. Hoisting this decision into a callback is a local simplification but a global increase in complexity. And in practice, everyone but Gecko will have a null subsumes callback, and this stuff won't affect them.

> If you think that is a bad idea, could we have a single inline
> check in public/Principals.h with a big beefy comment explaining why we are
> checking subsumption the way we are?

I don't follow. Can you clarify how you think the patch should be refactored?

Comment 11

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #10)
> There's certainly some logic to that, but I think it's a bit pedantic. The
> reintroduction of JSSubsumesOp is a recognition that the practicality of a
> single security callback for a few one-off checks outweighs the (somewhat
> hollow) conceptual win of removing all security decisions from the JS
> engine.

Yeah, but the difference is that it's pretty darn hard to understand what exactly it is we're checking here.  I don't even really remember.

> I don't follow. Can you clarify how you think the patch should be refactored?

This patch adds two places that calls subsumes that don't have comments.  Ideally (if we don't hoist via a hook) there would be only one predicate that was well-commented.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8492902 [details] [diff] [review]
Part 1 - Reduce the cases where we dispatch nsScriptErrorEvent. v1

Please do a better job of preserving the comments about why we don't dispatch the event for OOM.

r=me with that.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8492904 [details] [diff] [review]
Part 3 - Simplify script error muting and make our implementation look more like the spec's. v1

The DOM/XPConnect changes seem ok.  r=me on those.

>+                     !originPrincipals->subsumes(subsumesOp, cx->compartment()->principals);

Isn't this subsumes check backwards?

Also, are we guaranteed that cx->compartment()->principals() is the same as the principal of the window the error reporter will end up dispatching the event to?  Or at least the same as the principals of the JSScript in question?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Comment on attachment 8492904 [details] [diff] [review]
> Part 3 - Simplify script error muting and make our implementation look more
> like the spec's. v1
> 
> The DOM/XPConnect changes seem ok.  r=me on those.
> 
> >+                     !originPrincipals->subsumes(subsumesOp, cx->compartment()->principals);
> 
> Isn't this subsumes check backwards?

Err, yes - good catch. It probably doesn't matter in practice (given that this stuff only really matters for codebase URIs where subsumes and !subsumes each imply their converse). But I will absolutely fix it.
 
> Also, are we guaranteed that cx->compartment()->principals() is the same as
> the principal of the window the error reporter will end up dispatching the
> event to?

Definitely not.

> Or at least the same as the principals of the JSScript in question?

That is the aim of this patch, yes. The assumption is that cx->compartment()->principals() is also going to be the compartment where the exception is created, and thus the canonical unfiltered view. Other compartments get the appropriate security wrapper (and any stringification should be done via the same security wrappers that the string consumer would have seen for the original exception object).

Our general security model doesn't handle the case where a compartment may not be allowed to see its own objects, which is why we need this custom security check.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 3 - Simplify script error muting and make our implementation look more like the spec's. v3 r=bz

This is a bit nicer. Luke, can you make sure that my usage of compartment() in
Stack.cpp makes sense per comment 14?

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8493330 [details] [diff] [review]
Part 3 - Simplify script error muting and make our implementation look more like the spec's. v3 r=bz

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

Much nicer, two questions:

::: js/src/jscompartment.h
@@ +202,5 @@
> +        if (!subsumesOp || !originPrincipals)
> +            return false;
> +        MOZ_ASSERT(principals, "If we have security callbacks, every compartment"
> +                   "that loads potentially-cross-origin script should have a principal");
> +        return principals->subsumes(subsumesOp, originPrincipals);

I'm confused why this return value isn't negated: if principals == originPrincipals, then principals->subsumes(originPrincipals) is true and we don't want to mute.

::: js/src/vm/Stack.cpp
@@ +991,5 @@
>  
>  bool
> +FrameIter::shouldMuteErrors(JSRuntime *rt) const
> +{
> +    return compartment()->shouldMuteErrors(originPrincipals());

Can FrameIter::compartment()->principals ever != cx->compartment()->principals()?
I'm guessing 'no' (no cross-origin calls and we sever frame chains on nested event loops n things).  If 'no', then this seems fine.  If 'yes'... then I don't even know what the right thing to do is -- the exception can be caught and inspected at any frame -- I'm assuming the answer is 'no'.

Also, 'rt' is unused.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Definitely not.

Hmm.  So in the old setup, we checked that the window's principal subsumes the exception's originPrincipal, right?  What are we really checking in the new setup then?  That the exception object's principal (which is the same as the principal of the script, except for parse errors, when it's the same as the principal we're compiling the script with) subsumes the script's originPrincipal?

Comment 18

[Boris Zbarsky [:bzbarsky]]

And the point is, we're explicitly passing information extracted from the exception (especially the source line) to the window's onerror, which is why we used to check that the window subsumes the real source of that information (the originPrincipal of the script that threw and whose source we're exposing).

Comment 19

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #16)
> I'm confused why this return value isn't negated: if principals ==
> originPrincipals, then principals->subsumes(originPrincipals) is true and we
> don't want to mute.

Yeah, you're absolutely right - thanks for catching that. This is why I shouldn't write security-sensitive patches when I'm tired and in a hurry. :-(

(In reply to Boris Zbarsky [:bz] from comment #17)
> Hmm.  So in the old setup, we checked that the window's principal subsumes
> the exception's originPrincipal, right?  What are we really checking in the
> new setup then?  That the exception object's principal (which is the same as
> the principal of the script, except for parse errors, when it's the same as
> the principal we're compiling the script with) subsumes the script's
> originPrincipal?

(In reply to Boris Zbarsky [:bz] from comment #18)
> And the point is, we're explicitly passing information extracted from the
> exception (especially the source line) to the window's onerror, which is why
> we used to check that the window subsumes the real source of that
> information (the originPrincipal of the script that threw and whose source
> we're exposing).

The spec says to do make the comparison between the script origin and the execution origin, and doesn't consider the possibility of an onerror handler that is cross-origin from the global in which the error was generated. This is for two reasons:
(1) In the spec's world, you can basically never trigger this case except for document.domain (where this case _is_ observable, if I may point out).
(2) The threat model assumes a malicious document that is trying to load sensitive cross-origin stuff. If the document is same-origin, there are a lot of easier ways to exfiltrate stuff anyway.

There are pessimal situations with addons where (1) is slightly different for Gecko, but I'm not really worried about them. So I think it's ok to do it the spec's way, especially because I realized that I was doing it all wrong and we should just nuke originPrincipals from js/src. I'll attach some new patches which I think luke will like a whole lot more.

Comment 20

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=e32c545f9acc

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 2 - Stop XDR-serializing origin principals. v1

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 3 - Switch originPrincipals to a mutedError flags. v4

Comment 23

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=27ee83d05467

Comment 24

[Boris Zbarsky [:bzbarsky]]

> and doesn't consider the possibility of an onerror handler that is cross-origin from the
> global in which the error was generated

Right, I'm not worried about that.

The real question is whether the script and window are guaranteed to be same-origin.  I'm not worrying about onerror handlers that are not same-origin with the window.

Comment 25

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #24)
> The real question is whether the script and window are guaranteed to be
> same-origin.

I don't understand what you mean here. The whole point here is that we set the muted flag for scripts that are not same-origin with the window. Are you referring to execution origin as opposed to the script origin? Can you clarify?

Comment 26

[Boris Zbarsky [:bzbarsky]]

I'm referring to the execution origin.  This is all not really relevant with the latest patch anyway.

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 8495827 [details] [diff] [review]
Part 2 - Stop XDR-serializing origin principals. v1

Definitely like.

Comment 28

[Luke Wagner [:luke]]

Comment on attachment 8495832 [details] [diff] [review]
Part 3 - Switch originPrincipals to a mutedError flags. v4

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

r+++++ would review again!

::: js/src/jsexn.cpp
@@ +329,1 @@
>          fop->free_(report);

No longer need { } around then branch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8495832 [details] [diff] [review]
Part 3 - Switch originPrincipals to a mutedError flags. v4

r=me

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8495827 [details] [diff] [review]
Part 2 - Stop XDR-serializing origin principals. v1

Er, don't we need to bump the XDR version or something?

Comment 31

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #30)
> Comment on attachment 8495827 [details] [diff] [review]
> Part 2 - Stop XDR-serializing origin principals. v1
> 
> Er, don't we need to bump the XDR version or something?

Yeah, you're right. I'd convinced myself that we didn't need to do that because we still serialize the (empty) flags, but I realized that we'd still have a problem when reading old serialized originPrincipals.

Comment 32

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4da0087b72
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b9bfd9b37fe8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ee03a0b3b037

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e4da0087b72
https://hg.mozilla.org/mozilla-central/rev/b9bfd9b37fe8
https://hg.mozilla.org/mozilla-central/rev/ee03a0b3b037