1333030 - (require-triggering-principal) [meta] Assert nsDocShell::loadURIWithOptions receives a non null triggeringPrincipal

Status: ASSIGNED

(Core :: DOM: Security, task, P3)

Description

[Christoph Kerschbaumer [:ckerschb]]

Using the extended API added within Bug 1329032 and already used within Bug 1331686, we should add an assertion within nsDocShell::loadURIWIthOptions to make sure we receive a non null triggeringPrincipal.

Once that happenend, we can do the same for LoadURI (Bug  1316275).

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1333030_explicitly_pass_triggeringprincipal_to_loaduriwithoptions.patch

Boris, in order to pass a valid trigeringPrincipal to nsIWebNavigation.loadURIWIthOptions() I was wondering if we could kill of nsIWebNavigation.LoadURI() and use LoadURIWithOptions() instead. Within this patch you would see all the changes needed within Gecko (modulo the principal bits), but I am not sure if my approach could be a no-go because of addons. What do you think?

If you agree, we could then add an assertion within loadUriWithOptions and make sure we always get a non-null triggeringPrincipal.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> but I am not sure if my approach could be a no-go because of addons

I'm pretty sure it's no-go because of addons as long as we support addons that can touch XPCOM directly.  https://dxr.mozilla.org/addons/search?q=loadURI shows 16000+ hits, and while some might not be this API some certainly are just based on trivial inspection of the dxr results.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1333030_explicitly_pass_triggeringprincipal_to_loaduriwithoptions.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #2)
> > but I am not sure if my approach could be a no-go because of addons
> 
> I'm pretty sure it's no-go because of addons as long as we support addons
> that can touch XPCOM directly. 

Boris, I thought about this bug a little more. What do you think about the following proposal: I think ultimately we would like to pass the correct principal to ::LoadURI(..., loadinfo,...), right?

So, what if we rewrite loadURI() to loadURIWIthOptions(..., triggeringPrincipal) within Gecko? While working on it we could have an assertion within ::LoadURIWithOptions() to make sure all calls to LoadURIWithOptions pass a valid triggeringPrincipal. Before landing we would take out that assertion.

I agree we still need to support loadURI() as well as loadURIWithOptions() for addons not passing a triggeringPrincipal, hence we keep the rest of the code including all fallbacks as is, but at least for Gecko code we could pass the right principal. In the attached patch are all the places we would need to update. Is that idea something worth moving forward?

Comment 4

[Boris Zbarsky [:bzbarsky]]

That seems like a possible first step.  That said, why do we need to make people call loadURIWithOptions instead of just adding a triggeringPrincipal arg to loadURI as well?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #4)
> That seems like a possible first step.  That said, why do we need to make
> people call loadURIWithOptions instead of just adding a triggeringPrincipal
> arg to loadURI as well?

Adding an optional argument triggeringPrincipal to loadURI makes sense to me - agreed.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1333030_pass_principal_from_tabbrowser_to_backend.patch

Gijs, ultimately we would like to add an assertion within nsDocShell::LoadURI() to make sure we pass a valid triggeringPrincipal from the frontend to the backend. Within this (very raw) patch I tried to identify what code we have to touch to make this happening.

The backend infrastructure is in place, but it seems there is a lot of front end work that needs to happen. As a first step I think it makes sense to extend "addTab" within tabbrowser.xml with an argument 'triggeringPrincipal'. See underneath:
       <method name="addTab">
         <parameter name="aURI"/>
         <parameter name="aReferrerURI"/>
         <parameter name="aCharset"/>
         <parameter name="aPostData"/>
         <parameter name="aOwner"/>
         <parameter name="aAllowThirdPartyFixup"/>
         <parameter name="aIsPrerendered"/>
+        <parameter name="aTriggeringPrincipal"/>


That would require to update 853 tests [1] to add that additional argument. To be more precise, an update would look like this:
-  gBrowser.selectedTab = gBrowser.addTab(rootDir + "test-form_sjis.html");
+  gBrowser.selectedTab = gBrowser.addTab(rootDir + "test-form_sjis.html", {triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal()});

Does that make sense? Or can we query the triggeringPrincipal within "addTab" for all the testcases somehow? Also, in case we would go down that route, can we add some kind check that we always get a valid triggeringPrincipal? Something like:

if (!aTriggeringPrincipal) {
  Components.utils.reportError("no triggeringPricnipal: " + Components.stack.caller);
}

What do you think? Should we file a bug and get that work started, or should we start somewhere else?

[1] https://dxr.mozilla.org/mozilla-central/search?q=gBrowser.addTab&redirect=false

Comment 7

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Created attachment 8864483 [details] [diff] [review]
> bug_1333030_pass_principal_from_tabbrowser_to_backend.patch
> 
> Gijs, ultimately we would like to add an assertion within
> nsDocShell::LoadURI() to make sure we pass a valid triggeringPrincipal from
> the frontend to the backend. Within this (very raw) patch I tried to
> identify what code we have to touch to make this happening.
> 
> The backend infrastructure is in place, but it seems there is a lot of front
> end work that needs to happen. As a first step I think it makes sense to
> extend "addTab" within tabbrowser.xml with an argument
> 'triggeringPrincipal'. See underneath:
>        <method name="addTab">
>          <parameter name="aURI"/>
>          <parameter name="aReferrerURI"/>
>          <parameter name="aCharset"/>
>          <parameter name="aPostData"/>
>          <parameter name="aOwner"/>
>          <parameter name="aAllowThirdPartyFixup"/>
>          <parameter name="aIsPrerendered"/>
> +        <parameter name="aTriggeringPrincipal"/>
> 
> 
> That would require to update 853 tests [1] 

This query is a bit inaccurate, in that it captures some other method calls like addTabsProgressListener. But there's still >800 hits. The other thing to note, though, is that not all callsites are tests... we would have to update the other non-test in-tree callsites.

Finally, there's a lot of options that are only available if you pass an (non-URI) object as the second parameter. I think this can be the same - we can just convert the in-house consumers that use the other syntax if necessary, and not add a XBL <parameter>.

> to add that additional argument.
> To be more precise, an update would look like this:
> -  gBrowser.selectedTab = gBrowser.addTab(rootDir + "test-form_sjis.html");
> +  gBrowser.selectedTab = gBrowser.addTab(rootDir + "test-form_sjis.html",
> {triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal()});
> 
> Does that make sense? Or can we query the triggeringPrincipal within
> "addTab" for all the testcases somehow? Also, in case we would go down that
> route, can we add some kind check that we always get a valid
> triggeringPrincipal? Something like:

Under what circumstances should the triggering principal *not* be system principal, and what codepaths does that correspond to?

> if (!aTriggeringPrincipal) {
>   Components.utils.reportError("no triggeringPricnipal: " +
> Components.stack.caller);
> }

Cu.reportError is effectively the same as > /dev/null . It won't fail tests, it won't get reported back via telemetry, it won't stop the load.

What would be the aim of the warning? Depending on the aim, there are other avenues we could consider.

Comment 8

[:Gijs (he/him)]

Oh, also, you can call addTab without any URI to load at all. In that case we should probably not do anything to complain about the lack of triggering principal...

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #7)
> Finally, there's a lot of options that are only available if you pass an
> (non-URI) object as the second parameter. I think this can be the same - we
> can just convert the in-house consumers that use the other syntax if
> necessary, and not add a XBL <parameter>.

Agreed. No need to add an XBL parameter. We can just to addTab(uri, {...,triggeringprincpal} everywhere.
 
> > to add that additional argument.
> > To be more precise, an update would look like this:
> > -  gBrowser.selectedTab = gBrowser.addTab(rootDir + "test-form_sjis.html");
> > +  gBrowser.selectedTab = gBrowser.addTab(rootDir + "test-form_sjis.html",
> > {triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal()});
> > 
> > Does that make sense? Or can we query the triggeringPrincipal within
> > "addTab" for all the testcases somehow? Also, in case we would go down that
> > route, can we add some kind check that we always get a valid
> > triggeringPrincipal? Something like:
> 
> Under what circumstances should the triggering principal *not* be system
> principal, and what codepaths does that correspond to?

As far as I can tell (not having inspected all the code in detail) we could also end up here from within loadOneTab (within tabbrowser.xml). Or e.g. also from within BrowserTestUtils.jsm, but in that case we want to use SystemPrincipal anyway.

> > if (!aTriggeringPrincipal) {
> >   Components.utils.reportError("no triggeringPricnipal: " +
> > Components.stack.caller);
> > }
> 
> Cu.reportError is effectively the same as > /dev/null . It won't fail tests,
> it won't get reported back via telemetry, it won't stop the load.
> 
> What would be the aim of the warning? Depending on the aim, there are other
> avenues we could consider.

I think we want to fail tests. That code can only be reached from within Gecko anyway right? Or can addons somehow end up here? In that case we don't want to fail hard, right?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #8)
> Oh, also, you can call addTab without any URI to load at all. In that case
> we should probably not do anything to complain about the lack of triggering
> principal...

Right, we can special case that scenario within addTab to not raise a warning (or fail hard).

Comment 11

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> (In reply to :Gijs from comment #7)
> > > Or can we query the triggeringPrincipal within
> > > "addTab" for all the testcases somehow?
> > Under what circumstances should the triggering principal *not* be system
> > principal, and what codepaths does that correspond to?
> 
> As far as I can tell (not having inspected all the code in detail) we could
> also end up here from within loadOneTab (within tabbrowser.xml). Or e.g.
> also from within BrowserTestUtils.jsm, but in that case we want to use
> SystemPrincipal anyway.

Sorry, my implicit question here was: why do callers need to pass a principal? Can we just assume system principal inside addTab() ? For which callers would that *not* be correct, if any?

> > > if (!aTriggeringPrincipal) {
> > >   Components.utils.reportError("no triggeringPricnipal: " +
> > > Components.stack.caller);
> > > }
> I think we want to fail tests. 

I'm not aware of any way to do that from non-test code other than throwing (uncaught) exceptions. So that'd require throwing, but...

> That code can only be reached from within
> Gecko anyway right? Or can addons somehow end up here? In that case we don't
> want to fail hard, right?

Until 57, add-ons can do whatever they like, so they can (and do) call this directly.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #11)
> Sorry, my implicit question here was: why do callers need to pass a
> principal? Can we just assume system principal inside addTab() ? For which
> callers would that *not* be correct, if any?

While for testing purposes that would be ok, there is, for example all the |ctrl+click| or also |right-click-open-in-new-tab| that needs a different triggeringPricnipal than System (e.g. [1]). I suppose we could add a fallback, if aTriggiringPrincipal is not passed explicitly, then fall back to using the SystemPrincipal, but that entails that we are confident about our changes. In other words, not missing any cases. The thing that worries me about using systemPrincipal is that in the error case (meaning we miss something) we allow everything by default instead of blocking everything by default, which would be better in terms of security.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8828775&action=diff
 
> Until 57, add-ons can do whatever they like, so they can (and do) call this
> directly.

I suppose for testing purposes we could just throw and exception when pushing to try which we then take out before landing because ultimately we want to have the assertion within docshell::LoadURI() anyway.

Comment 13

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> (In reply to :Gijs from comment #11)
> > Sorry, my implicit question here was: why do callers need to pass a
> > principal? Can we just assume system principal inside addTab() ? For which
> > callers would that *not* be correct, if any?
> 
> While for testing purposes that would be ok, there is, for example all the
> |ctrl+click| or also |right-click-open-in-new-tab| that needs a different
> triggeringPricnipal than System (e.g. [1]). I suppose we could add a
> fallback, if aTriggiringPrincipal is not passed explicitly, then fall back
> to using the SystemPrincipal, but that entails that we are confident about
> our changes. In other words, not missing any cases. The thing that worries
> me about using systemPrincipal is that in the error case (meaning we miss
> something) we allow everything by default instead of blocking everything by
> default, which would be better in terms of security.

Right. Equally, it seems like the number of cases where we need a different triggering principal is low, and the number of cases where system principal is correct is high... But I don't really have any better ideas. :-(

> [1] https://bugzilla.mozilla.org/attachment.cgi?id=8828775&action=diff
>  
> > Until 57, add-ons can do whatever they like, so they can (and do) call this
> > directly.
> 
> I suppose for testing purposes we could just throw and exception when
> pushing to try which we then take out before landing because ultimately we
> want to have the assertion within docshell::LoadURI() anyway.

That would WFM.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #13)
> That would WFM.

Thanks Gijs for your feedback. I filed Bug 1362034 to get that ready.