1361387 - Update callsites of loadURI() within browser/ to pass a triggeringPrincipal

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1361387_update_browser_to_pass_triggeringpricnipal_to_loaduri.patch

Boris, Gijs, ultimately those callsites of loadURI() would fall back to using the systemPrincipal (here [1], since they don't pass any referrer. We have to be careful however, because those [1] if else statements perform more than just generating a fall back triggeringPrincipal. What do you think? Acceptable, or am I missing something?

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1488-1539

Comment 2

[:Gijs (out for now; he/him)]

Comment on attachment 8863765 [details] [diff] [review]
bug_1361387_update_browser_to_pass_triggeringpricnipal_to_loaduri.patch

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

The session restore thing looks sane. Less sure about web content converter...

::: browser/components/feeds/WebContentConverter.js
@@ +301,5 @@
>            channel.notificationCallbacks.getInterface(Ci.nsIWebNavigation);
>        webNavigation.loadURI(handler.getHandlerURI(channel.URI.spec),
>                              Ci.nsIWebNavigation.LOAD_FLAGS_NONE,
> +                            null, null, null,
> +                            Services.scriptSecurityManager.getSystemPrincipal());

I think this is problematic because these are used to deal with content types for which a web handler is registered. We should know what the triggering principal was for the content that we're trying to load here. Shouldn't that still be the triggering principal for loading this inside the handler?

That and/or we should make extra sure that these URIs (the one we're loading) never inherit principals.

::: browser/components/sessionstore/ContentRestore.jsm
@@ +25,5 @@
>    "resource:///modules/sessionstore/SessionStorage.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Utils",
>    "resource://gre/modules/sessionstore/Utils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services.",
> +  "resource://gre/modules/Services.jsm");

There's no need to lazy-load Services. Just Cu.import it (which I can't tell from the context, but I kind of assume is happening anyway? Maybe not...)

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #2)
> ::: browser/components/feeds/WebContentConverter.js
> @@ +301,5 @@
> >            channel.notificationCallbacks.getInterface(Ci.nsIWebNavigation);
> >        webNavigation.loadURI(handler.getHandlerURI(channel.URI.spec),
> >                              Ci.nsIWebNavigation.LOAD_FLAGS_NONE,
> > +                            null, null, null,
> > +                            Services.scriptSecurityManager.getSystemPrincipal());
> 
> I think this is problematic because these are used to deal with content
> types for which a web handler is registered. We should know what the
> triggering principal was for the content that we're trying to load here.
> Shouldn't that still be the triggering principal for loading this inside the
> handler?
> 
> That and/or we should make extra sure that these URIs (the one we're
> loading) never inherit principals.

In that case we could query the triggeringPrincipal from the 'request' that is passed to |  loadPreferredHandler(request) |. Probably more precise than the systemPrincipal. What do you think?

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> (In reply to :Gijs from comment #2)
> > ::: browser/components/feeds/WebContentConverter.js
> > @@ +301,5 @@
> > >            channel.notificationCallbacks.getInterface(Ci.nsIWebNavigation);
> > >        webNavigation.loadURI(handler.getHandlerURI(channel.URI.spec),
> > >                              Ci.nsIWebNavigation.LOAD_FLAGS_NONE,
> > > +                            null, null, null,
> > > +                            Services.scriptSecurityManager.getSystemPrincipal());
> > 
> > I think this is problematic because these are used to deal with content
> > types for which a web handler is registered. We should know what the
> > triggering principal was for the content that we're trying to load here.
> > Shouldn't that still be the triggering principal for loading this inside the
> > handler?
> > 
> > That and/or we should make extra sure that these URIs (the one we're
> > loading) never inherit principals.
> 
> In that case we could query the triggeringPrincipal from the 'request' that
> is passed to |  loadPreferredHandler(request) |. Probably more precise than
> the systemPrincipal. What do you think?

Assuming this works, that sounds right to me.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8863765 [details] [diff] [review]
bug_1361387_update_browser_to_pass_triggeringpricnipal_to_loaduri.patch

Just delegating the whole thing to Gijs.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1361387_update_browser_to_pass_triggeringpricnipal_to_loaduri.patch

Gijs, I updated WebContentConverter.js and query the triggeringPrincipal from the old channel which we can pass on to loadURI(). I tested locally and it seems to work. I think it still makes sense to keep the fallback to systemPrincipal in case a channel has no loadInfo attached. (Within Gecko that should never be the case though).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a67d2537989c
Update callsites of loadURI() within browser/ to pass a triggeringPrincipal. r=gijs

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a67d2537989c