Closed Bug 1320124 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Navigation, defect, P2)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file)

Bug 1320124 - don't allow privileged loads inside unprivileged loads,
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
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.)
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
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
Ugh. So that trypush has a bundle of orange. :-(
Flags: needinfo?(gijskruitbosch+bugs)
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)
(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)
Depends on: 1338215
(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...)
> I think it's not possible to create a root *remote* windowless browser

Hmm... Maybe we should make it possible...
(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
(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.
Depends on: 1351300
Depends on: 1351991
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
(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.
Depends on: 1353104
Attachment #8815291 - Flags: review?(bzbarsky)
Attachment #8815291 - Flags: review?(bzbarsky)
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-
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 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+
(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)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/14c22e5dbf01
don't allow privileged loads inside unprivileged loads, r=bz
https://hg.mozilla.org/mozilla-central/rev/14c22e5dbf01
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: