Downgrade system principal loads inside non-system-principal docshell frames to null principal / codebase principal

RESOLVED FIXED in Firefox 55

Status

()

Core
Document Navigation
P2
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
STR:

0. non-e10s + devtools.chrome.enabled set to true, for ease of reproduction
1. open a website with a frame (data:text/html,<iframe%20src='about:blank'> will do).
2. open the browser console
3. eval: content.frames[0].location.href = "about:preferences";
4. eval content.frames[0].document.nodePrincipal

Actual:

system principal

Expected:
something lower than system principal. Probably null principal, or a unique codebase principal, or something. Or refuse to load/construct the document based on the principal it'd get, that'd also wfm.
We could do this, yeah.  In particular, in nsDocument::Reset we could check the parent's principal, if any, if GetChannelResultPrincipal() returns system, and if so change to a nullprincipal.  That doesn't seem terribly likely to break things.  (Famous last words, I know.)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
I had to poke the XULDocument::StartDocumentLoad method as well to make the test in comment #0 fail. nsDocument::Reset wasn't enough. For a non-XUL doc, try e.g. chrome://pocket/content/panels/signup.html .

Speaking of tests, assuming this doesn't break the world, I guess it should have tests.

Let's see about breaking the world first:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10853bb598c06b45979e70b4d676182a35443e84
(Assignee)

Comment 4

a year ago
mozreview-review
Comment on attachment 8815291 [details]
Bug 1320124 - don't allow privileged loads inside unprivileged loads,

https://reviewboard.mozilla.org/r/96282/#review96484

::: dom/base/nsDocument.cpp:2142
(Diff revision 1)
>      nsGlobalWindow::Cast(win)->RefreshCompartmentPrincipal();
>    }
>  }
>  
> +bool
> +nsDocument::HasNonSystemPrincipalParent()

Hm, maybe this would be better off on nsContentUtils with a docshell parent? Or on docshell directly? OTOH, those are both dumping grounds already... :-\
Priority: -- → P2
(Assignee)

Comment 5

a year ago
Ugh. So that trypush has a bundle of orange. :-(
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 6

11 months ago
Boris, a lot of that orange is related to the background page thumbnail service using the hidden window to create a frame to the chrome URL of about:mozilla (so it gets chrome privileges) and then create a <browser> in there in which to load content. Is there a way to avoid doing that and still effectively load a web page "normally" and render the result to a canvas/image?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bzbarsky)
(Assignee)

Comment 7

11 months ago
(this stuff is presumably also (part of) the reason we still specialcase the hidden window in nsScriptSecurityManager... :-( )
> create a frame to the chrome URL of about:mozilla (so it gets chrome privileges)

Ugh.  Talk about fragile.  :(  That said, are you sure it's about:mozilla?  about:mozilla doesn't get chrome privileges, afaict.  Oh, I guess they're actually loading "chrome://global/content/mozilla.xhtml".  <sigh>

> Is there a way to avoid doing that and still effectively load a web page "normally" and
> render the result to a canvas/image?

Well, if they really want to keep using this hack they could Services.appShell.createWindowlessBrowser(true) to get themselves a chrome-type docshell, load "chrome://global/content/mozilla.xhtml" into it and then press on.

Or they could Services.appShell.createWindowlessBrowser() to create a content-type docshell and just load their thing into it directly.

I _think_ the result should still be drawable via drawWindow, but should be checked, of course.
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

9 months ago
Depends on: 1338215
(Assignee)

Comment 9

9 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #8)
> > create a frame to the chrome URL of about:mozilla (so it gets chrome privileges)
> 
> Ugh.  Talk about fragile.  :(  That said, are you sure it's about:mozilla? 
> about:mozilla doesn't get chrome privileges, afaict.  Oh, I guess they're
> actually loading "chrome://global/content/mozilla.xhtml".  <sigh>
> 
> > Is there a way to avoid doing that and still effectively load a web page "normally" and
> > render the result to a canvas/image?
> 
> Well, if they really want to keep using this hack they could
> Services.appShell.createWindowlessBrowser(true) to get themselves a
> chrome-type docshell, load "chrome://global/content/mozilla.xhtml" into it
> and then press on.
> 
> Or they could Services.appShell.createWindowlessBrowser() to create a
> content-type docshell and just load their thing into it directly.

I think it's not possible to create a root *remote* windowless browser, so I went with the least-invasive thing of just splicing this stuff out of the hidden window and into a windowless browser, which then creates a remote=true subframe . Still also requires a fix for bug 1287004 as well, but at least the patch was fairly straightforward.

> I _think_ the result should still be drawable via drawWindow, but should be
> checked, of course.

It seems to work, yes. Split this bit off to bug 1338215. New trypush with that + 1287004 + rebased copy of this patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8f6b50167cbbdcefc220220bfa6bf655c10b8ca

(I still anticipate jetpack failures based on the last trypush, but one can hope...)
Comment hidden (mozreview-request)
> I think it's not possible to create a root *remote* windowless browser

Hmm... Maybe we should make it possible...
(Assignee)

Comment 12

9 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)
> > I think it's not possible to create a root *remote* windowless browser
> 
> Hmm... Maybe we should make it possible...

I wouldn't be opposed, but I'm not sure how many assumptions in our codebase that breaks, and wouldn't want to block this bug on it. :-)

Anyway, plenty more stuff in the trypush I need to look at first (some of which I would swear is new since last time...), though some of it looks like it might have already got fixed elsewhere (bug 1339144)
Depends on: 1339144
(Assignee)

Comment 13

9 months ago
(In reply to :Gijs from comment #12)
> Anyway, plenty more stuff in the trypush I need to look at first (some of
> which I would swear is new since last time...), though some of it looks like
> it might have already got fixed elsewhere (bug 1339144)

Another trypush to check:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=3546dfb192daee7f3fbb4b6675f9648ae0ce9e5d
(In reply to :Gijs (out until Monday 20th) from comment #12)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)
> > > I think it's not possible to create a root *remote* windowless browser
> > 
> > Hmm... Maybe we should make it possible...
> 
> I wouldn't be opposed, but I'm not sure how many assumptions in our codebase
> that breaks, and wouldn't want to block this bug on it. :-)

I'm not sure it would break many assumptions, but I don't think it would be very practical. The remote <browser> bindings make a lot of things practical that wouldn't be practical with a bare frameloader.
(Assignee)

Updated

8 months ago
Depends on: 1351300
(Assignee)

Updated

8 months ago
Depends on: 1351991
(Assignee)

Comment 15

8 months ago
Let's try again now that bug 1351300 is fixed and I have patches for bug 1351991...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95b6c073174679350e338fb873d61a58883d0dff
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 16

8 months ago
(In reply to Kris Maglione [:kmag] from comment #14)
> (In reply to :Gijs (out until Monday 20th) from comment #12)
> > (In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)
> > > > I think it's not possible to create a root *remote* windowless browser
> > > 
> > > Hmm... Maybe we should make it possible...
> > 
> > I wouldn't be opposed, but I'm not sure how many assumptions in our codebase
> > that breaks, and wouldn't want to block this bug on it. :-)
> 
> I'm not sure it would break many assumptions, but I don't think it would be
> very practical. The remote <browser> bindings make a lot of things practical
> that wouldn't be practical with a bare frameloader.

This makes sense to me. On the other hand, we now have a number of different consumers that end up creating their own chrome docshells just to add remote <browser>s to. I wonder what would break if we tried to get those consumers to share the chrome docshell - docshells aren't exactly free, so now that there's at least 2 consumers even on Firefoxen without any add-ons, that might be worth looking at.
Yeah, if we're going to rewrite those bindings in WebIDL, anyway, it might make sense to base them on mozbrowser, or something, and allow them to be remote.
(Assignee)

Updated

8 months ago
Depends on: 1353104
Comment hidden (mozreview-request)
(Assignee)

Comment 19

8 months ago
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=df430ad09d478b6947a3b35f196cef842dffe011
(Assignee)

Updated

7 months ago
Attachment #8815291 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 months ago
Attachment #8815291 - Flags: review?(bzbarsky)
Comment hidden (mozreview-request)
Comment on attachment 8815291 [details]
Bug 1320124 - don't allow privileged loads inside unprivileged loads,

https://reviewboard.mozilla.org/r/96282/#review131582

::: dom/base/nsDocument.cpp:2246
(Diff revision 4)
>  
> +bool
> +nsDocument::HasNonSystemPrincipalParent()
> +{
> +  bool rv = false;
> +  if (mDocumentContainer) {

Why can't we use GetParentDocument() here?  Seems like we should be able to, since we're not just considering same-type parents here.  At least as long as mParentDocument is set up by this point...

If there's a reason we're not using GetParentDocument, we should document that reason here.  And it there isn't this whole function becomes:

    nsIDocument* parent = GetParentDocument();
    return parentDoc && !nsContentUtils::IsSystemPrincipal(parentDoc->NodePrincipal());

::: dom/xul/XULDocument.cpp:4451
(Diff revision 4)
>          nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
>          if (channel && secMan) {
>              nsCOMPtr<nsIPrincipal> principal;
>              secMan->GetChannelResultPrincipal(channel, getter_AddRefs(principal));
>  
> +            if (!sChromeInContentPrefCached) {

OK, this code duplication is getting annoying.  ;)

We should make sChromeInContentPrefCached and sChromeInContentAllowed static nsDocument members, then have a static nsDocument function for getting the sChromeInContentAllowed value, setting up the pref cache as needed in the process.  Then there will only be one place that has this logic.

::: dom/xul/XULDocument.cpp:4457
(Diff revision 4)
> +                sChromeInContentPrefCached = true;
> +                Preferences::AddBoolVarCache(&sChromeInContentAllowed,
> +                                             kChromeInContentPref, false);
> +            }
> +
> +            if (nsContentUtils::IsSystemPrincipal(principal) &&

This is forgetting to check sChromeInContentAllowed.  Maybe this check should also be factored out into a non-static nsDocument function taking nsIPrincipal*?
Attachment #8815291 - Flags: review?(bzbarsky) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 23

7 months ago
Thanks for all the feedback! Much better this way. I tried using GetParentDocument(), but it returns null at the point where we call it, so I added a comment instead. I created a method that takes a principal and may return a different one, and put the 'hasnonsystemprincipalparent' stuff in there instead. Hopefully this is easier to follow / less-duplicate-y.
Comment hidden (mozreview-request)
Comment on attachment 8815291 [details]
Bug 1320124 - don't allow privileged loads inside unprivileged loads,

https://reviewboard.mozilla.org/r/96282/#review132230

::: dom/base/nsDocument.h:517
(Diff revision 6)
>  
>    virtual void Reset(nsIChannel *aChannel, nsILoadGroup *aLoadGroup) override;
>    virtual void ResetToURI(nsIURI *aURI, nsILoadGroup *aLoadGroup,
>                            nsIPrincipal* aPrincipal) override;
>  
> +  already_AddRefed<nsIPrincipal> MaybeDowngradePrincipal(nsCOMPtr<nsIPrincipal> aPrincipal);

The arg type should be nsIPrincipal*.
Attachment #8815291 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 27

7 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #25)
> Comment on attachment 8815291 [details]
> Bug 1320124 - don't allow privileged loads inside unprivileged loads,
> 
> https://reviewboard.mozilla.org/r/96282/#review132230
> 
> ::: dom/base/nsDocument.h:517
> (Diff revision 6)
> >  
> >    virtual void Reset(nsIChannel *aChannel, nsILoadGroup *aLoadGroup) override;
> >    virtual void ResetToURI(nsIURI *aURI, nsILoadGroup *aLoadGroup,
> >                            nsIPrincipal* aPrincipal) override;
> >  
> > +  already_AddRefed<nsIPrincipal> MaybeDowngradePrincipal(nsCOMPtr<nsIPrincipal> aPrincipal);
> 
> The arg type should be nsIPrincipal*.

Fixed. However, I'm a little confused because, AIUI, returning the nsIPrincipal* aPrincipal here (as the 'default' case), because of the already_AddRefed return type, now means constructing a new nsCOMPtr in order to be able to call .forget() and pass back the already_addrefed ref. Is that correct and/or is there a better way to do this (should I not be using already_AddRefed as a return type, either)?
Flags: needinfo?(bzbarsky)
> now means constructing a new nsCOMPtr in order to be able to call .forget()

Yep.

> or is there a better way to do this

Not really.

> should I not be using already_AddRefed as a return type, either

already_AddRefed as a return type is correct, because in the nullprincipal case we need that.
Flags: needinfo?(bzbarsky)

Comment 29

7 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/14c22e5dbf01
don't allow privileged loads inside unprivileged loads, r=bz

Comment 30

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/14c22e5dbf01
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
\o/
status-firefox53: affected → ---
You need to log in before you can comment on or make changes to this bug.