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

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla35
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Blocks: 1070049
No longer blocks: 1067574
Why the time the error is raised and not the time when we first compile the script?
Assignee

Comment 2

5 years ago
(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
Assignee

Comment 3

5 years ago
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).
Assignee

Updated

5 years ago
Blocks: 1070842
Assignee

Updated

5 years ago
No longer blocks: 1070049
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?
Attachment #8492903 - Flags: review?(luke) → review+
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?
Assignee

Comment 10

5 years ago
(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?
(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 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.
Attachment #8492902 - Flags: review?(bzbarsky) → review+
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?
Attachment #8492904 - Flags: review?(bzbarsky) → review+
Assignee

Comment 14

5 years ago
(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.
Assignee

Comment 15

5 years ago
This is a bit nicer. Luke, can you make sure that my usage of compartment() in
Stack.cpp makes sense per comment 14?
Attachment #8492904 - Attachment is obsolete: true
Attachment #8492904 - Flags: review?(luke)
Attachment #8493330 - Flags: review?(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.
> 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?
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).
Assignee

Comment 19

5 years ago
(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.
Assignee

Comment 21

5 years ago
Attachment #8492903 - Attachment is obsolete: true
Attachment #8493330 - Attachment is obsolete: true
Attachment #8493330 - Flags: review?(luke)
Attachment #8495827 - Flags: review?(luke)
Attachment #8495827 - Flags: review?(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.
Assignee

Comment 25

5 years ago
(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?
I'm referring to the execution origin.  This is all not really relevant with the latest patch anyway.
Comment on attachment 8495827 [details] [diff] [review]
Part 2 - Stop XDR-serializing origin principals. v1

Definitely like.
Attachment #8495827 - Flags: review?(luke) → review+
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.
Attachment #8495832 - Flags: review?(luke) → review+
Attachment #8495827 - Flags: review?(bzbarsky) → review+
Comment on attachment 8495832 [details] [diff] [review]
Part 3 - Switch originPrincipals to a mutedError flags. v4

r=me
Attachment #8495832 - Flags: review?(bzbarsky) → review+
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?
Assignee

Comment 31

5 years ago
(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.
Summary: Replace JSErrorReport::originPrincipals with a 'muted error flag' → Replace JSErrorReport::originPrincipals with a 'muted errors flag'
https://hg.mozilla.org/mozilla-central/rev/1e4da0087b72
https://hg.mozilla.org/mozilla-central/rev/b9bfd9b37fe8
https://hg.mozilla.org/mozilla-central/rev/ee03a0b3b037
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.