1362329 - Have loadURIWithFlags() provide the correct triggeringPrincipal

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

Similar to what we do within Bug 1362034, we have to make sure loadURIWithFlags() passes a valid triggeringprincipal.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1362034_addtab_triggeringPrincipal.patch

Gijs, this is going to be quite an exercise :-) To make sure that we are on the same page I wanted to verify with you that the approach is correct:
* Ulimately we want to make sure that we can pass a valid and accurate triggeringPrincipal from within addTab to loadURIWithFlags. (All of the callsites for loadURIWithFlags will be updated and examined within Bug 1362329).
* Within Gecko we make sure that callsites of addTab() pass a 'params' object as the second argument which contains a valid triggeringPrincipal.
* Tests are allowed to *not* pass a params object, in which case we explicitly fall back to using the systemPrincipal as the triggeringPrincipal.
* Tests that are passing a params object need to contain a valid triggeringPrincipal (those are only a few).
* In order to pass a triggeringPricnipal within the params object to addTab() we also have to update loadOneTab() as well as loadtabs().

I think within this first iteration of the patch I traced down all the callsites. If you want I can extract all the tests into a separate patch. Now the important part is to make sure we use the right principal.

Approach makes sense, or did I miss something?

One final question, is it possible that delayedOpenTab() does not have any callsite? In that case I think we can remove that function instead of tracing callistes :-)

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 3

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8864825 [details] [diff] [review]
> bug_1362034_addtab_triggeringPrincipal.patch
> 
> Gijs, this is going to be quite an exercise :-) To make sure that we are on
> the same page I wanted to verify with you that the approach is correct:
> * Ulimately we want to make sure that we can pass a valid and accurate
> triggeringPrincipal from within addTab to loadURIWithFlags. (All of the
> callsites for loadURIWithFlags will be updated and examined within Bug
> 1362329).
> * Within Gecko we make sure that callsites of addTab() pass a 'params'
> object as the second argument which contains a valid triggeringPrincipal.
> * Tests are allowed to *not* pass a params object, in which case we
> explicitly fall back to using the systemPrincipal as the triggeringPrincipal.
> * Tests that are passing a params object need to contain a valid
> triggeringPrincipal (those are only a few).
> * In order to pass a triggeringPricnipal within the params object to
> addTab() we also have to update loadOneTab() as well as loadtabs().

I think that separating out the tests as you suggest would make this simpler, but it opens the door to non-test consumers calling addTab without a params object and, with that, doing the wrong thing. I'm not sure if in that case it wouldn't be better to remove the possibility of doing so entirely... Florian might be able to help with automatically rewriting the tests such that they pass a system principal as the triggering principal.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #3)
> I think that separating out the tests as you suggest would make this
> simpler, but it opens the door to non-test consumers calling addTab without
> a params object and, with that, doing the wrong thing. I'm not sure if in
> that case it wouldn't be better to remove the possibility of doing so
> entirely... Florian might be able to help with automatically rewriting the
> tests such that they pass a system principal as the triggering principal.

Gijs, I agree. But just to make sure that we talk about the same thing: Let's rewrite all callsites (in this case addTab, loadOneTab, and loadTabs) to actually pass a params object where within that object the triggeringPrincipal is set explicitly. And to be precise not only in Gecko code but also for all of our tests [1], agreed? From a security point of view that's what I really would like to do. The fallback to using the systemPrincipal if no principal is passed explicitly is one of the practices we should actually avoid having. In that case we can really find all the callsites (with the help of TRY) to pass a principal to addTab() - sounds good to me.

@Florian: Gijs mentioned (see comment 3) that you might be able to help to automatically rewrite tests [1] to pass a triggeringPrincipal explicitly. Within the attached patch I am already doing so in some cases where addTab() already uses a params object but did not include. From a birds eye of view we have to do the following:
* addTab() -> addTab(null, {triggeringPrincipal: Services.scriptSecurityManager.getSystemprincipal() }
* addTab(uri) -> addTab(uri, {triggeringPrincipal: Services.scriptSecurityManager.getSystemprincipal() }
* addTab(uri, {foo}) -> addTab(uri, {foo, triggeringPrincipal: Services.scriptSecurityManager.getSystemprincipal() }
Potentially I am missing a corner case, but ultimately we have to do something like the above. Any suggestions how to accomplish that?

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

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Fixed a couple more issues (still a lot to cover):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=52c216bd539b3b3b303753075de88722dafad538

Comment 6

[Florian Quèze [:florian]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)

> @Florian: Gijs mentioned (see comment 3) that you might be able to help to
> automatically rewrite tests [1] to pass a triggeringPrincipal explicitly.
> Within the attached patch I am already doing so in some cases where addTab()
> already uses a params object but did not include. From a birds eye of view
> we have to do the following:
> * addTab() -> addTab(null, {triggeringPrincipal:
> Services.scriptSecurityManager.getSystemprincipal() }
> * addTab(uri) -> addTab(uri, {triggeringPrincipal:
> Services.scriptSecurityManager.getSystemprincipal() }
> * addTab(uri, {foo}) -> addTab(uri, {foo, triggeringPrincipal:
> Services.scriptSecurityManager.getSystemprincipal() }
> Potentially I am missing a corner case, but ultimately we have to do
> something like the above. Any suggestions how to accomplish that?

This seems easy to script. Do you want to learn how to do it, or do you just want it done and would prefer me to write the script for you next time I'm stuck in a plane?

But... I tend to prefer when scripted rewrites reduce the size of the code, and the rewrite suggested above tends to make the code uglier.
How about instead we rewrite gBrowser.addTab to BrowserTestUtils.addTab, and have that method in BrowserTestUtils.jsm add a system triggering principal automatically if it's missing? (this file isn't shipped with the browser; only with tests, so no there's no risk of this behavior leaking to real consumers) That becomes easy enough that it could just be a search&replace (although you may still want a smarter script to fix the indentation automatically).

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> This seems easy to script. Do you want to learn how to do it, or do you just
> want it done and would prefer me to write the script for you next time I'm
> stuck in a plane?

Since I have a thousand things on my plate it would be great if you can help me out providing the script. If you have the time obviously.
 
> But... I tend to prefer when scripted rewrites reduce the size of the code,
> and the rewrite suggested above tends to make the code uglier.
> How about instead we rewrite gBrowser.addTab to BrowserTestUtils.addTab, and
> have that method in BrowserTestUtils.jsm add a system triggering principal
> automatically if it's missing? (this file isn't shipped with the browser;
> only with tests, so no there's no risk of this behavior leaking to real
> consumers) That becomes easy enough that it could just be a search&replace
> (although you may still want a smarter script to fix the indentation
> automatically).

Rewriting gBrowser.addTab() to BrowserTestUtls.addTab() sounds like a great path forward to me actually. Since there is a lot of Gecko code that needs to be changed, I will focus on that part for now.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

^^

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Ah, Florian, I forgot to mention it's not only addTab(), it's also loadOneTab(), and loadTabs() and potentially one ore two more of that family.

Comment 10

[Florian Quèze [:florian]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #6)
> > This seems easy to script. Do you want to learn how to do it, or do you just
> > want it done and would prefer me to write the script for you next time I'm
> > stuck in a plane?
> 
> Since I have a thousand things on my plate it would be great if you can help
> me out providing the script. If you have the time obviously.

Ok, give me the exact specifications of what needs rewriting and I'll see what I can do for you :).

> Rewriting gBrowser.addTab() to BrowserTestUtls.addTab() sounds like a great
> path forward to me actually. Since there is a lot of Gecko code that needs
> to be changed, I will focus on that part for now.

So if I understand correctly, the next step is for you to get that BrowserTestUtils.addTab method added/reviewed, before the rewriting can begin.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> So if I understand correctly, the next step is for you to get that
> BrowserTestUtils.addTab method added/reviewed, before the rewriting can
> begin.

Yes, that sounds like a plan. I'll add the BrowserUtils.addTab() function and whatever else is needed. Once I have set everything up I'll ping you again. I would also like to get a green TRY run before moving any further to see how much tweaking of the code is actually needed, otherwise we might have to many follow up bugs. Anyway, thanks for your help - I'll ping you once everything is ready.

Comment 12

[:Gijs (he/him)]

The plan you and Florian have worked out here sounds fine to me. Clearing needinfo.

Comment 13

[Jonathan Kingston [:jkt] he/him]

Pretty much all cases here are covered with the addTab work and other lands. I'm double checking we have all the erroring in place from the original patch and with try:

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

Comment 14

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1362329 - Throw errors for openUriInFrame, loadTabs and openURIInNewTab when no triggeringPrincipal passed.

Comment 15

[Jonathan Kingston [:jkt] he/him]

This is mostly covered with addTab asserts however there are at least one edge case in these three I can see that would permit loadTabs to be used without a triggeringPrincipal. I don't think there is much harm in adding these until the final work is done for this project.

Comment 16

[:Gijs (he/him)]

(In reply to Jonathan Kingston [:jkt] from comment #15)
> This is mostly covered with addTab asserts however there are at least one
> edge case in these three I can see that would permit loadTabs to be used
> without a triggeringPrincipal. I don't think there is much harm in adding
> these until the final work is done for this project.

Can you elaborate? I'm not sure I understand what you mean.

Comment 17

[Jonathan Kingston [:jkt] he/him]

> Can you elaborate? I'm not sure I understand what you mean.

I only found one path through these functions where it would be possible to ignore existing triggeringPrincipal throws or asserts. However until we are at our goal of no assumptions about triggering principals I don't think these harm.

The case I found which would prermit no triggering principal was a call to _openUTIInNewTab() with aIsExternal or !aURI set.

I think it's useful to have the loadTabs exception for now as it's a larger function and I copied all three of these from the previous patch as none of the rest of it was needed.

Comment 18

[Jonathan Kingston [:jkt] he/him]

This is no longer needed as was covered in Bug 1508654 further down the call stack.