Closed Bug 1361387 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → ckerschb
Priority: -- → P2
Whiteboard: [domsecurity-active]
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
Attachment #8863765 - Flags: review?(gijskruitbosch+bugs)
Attachment #8863765 - Flags: review?(bzbarsky)
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...)
Attachment #8863765 - Flags: review?(gijskruitbosch+bugs) → feedback+
(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?
(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 on attachment 8863765 [details] [diff] [review]
bug_1361387_update_browser_to_pass_triggeringpricnipal_to_loaduri.patch

Just delegating the whole thing to Gijs.
Attachment #8863765 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
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).
Attachment #8863765 - Attachment is obsolete: true
Attachment #8864023 - Flags: review?(gijskruitbosch+bugs)
Attachment #8864023 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a67d2537989c
Update callsites of loadURI() within browser/ to pass a triggeringPrincipal. r=gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a67d2537989c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.