1524970 - Update more front-end code to explicitly pass a CSP for new top-level loads

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

Bug 1518454 updates all the codepaths to explicitly pass a CSP where we have test coverage in our test suite. However, we need to do more testing and fix corner cases within this bug to make sure we cover all/most of the code paths.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: csp_principal_test.html

Hey Freddy, as discussed, we just landed Bug 1518454 to explicitly pass a CSP from frontend code to all docshell loads.

Within DoURILoad() in nsdocshell we added an assertion that the explicitly passed CSP is the same as the CSP on the principal.

I already did quite so much testing how we open new windows, e.g:

• regular click a link
• right-click -> open-link-in-new-window/tab
• drag and drop a link
• ...

I am attaching a testcase - could you please help me test opening that 'click-me' testlink in all variations you can imagine? If you have a testcase that hits the assertion, please let me know - because then we would have found a codepath we are not covering!

Comment 2

[Frederik Braun [:freddy]]

Having poked a bit, I couldn't find any additional cases that cause the assertion to fail.
I also tried a source code based approach, but the function is central to all kinds of loads: Going to callsites leads to InternalLoad, which then leads to everything, really.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Gijs, copying over your quote from
https://bugzilla.mozilla.org/show_bug.cgi?id=1518454#c14

The thing is, it looks to me like at least https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/toolkit/content/widgets/browser-custom-element.js#793 needs updating and isn't in this patch. I also would expect at least some of the callsites of openUILink(In) and openLink(In) to need updating... I realize quite a few use system principal or a newly minted null principal, but some also don't, right?

I looked extensively and updated all of the callsites of openLink(in) openUILink(in) within Bug 1518454. I guess it looks confusing because callsites only assemble an 'explicit' params object in case none has been provided by that caller. In most casses we pass aParams through and then openUILinkIn() just gets it here from the aIgnoreButton:
https://searchfox.org/mozilla-central/source/browser/base/content/utilityOverlay.js#101

Anyway, the browser-custom-element.js element bits were missing. I added them into a patch which I'll upload in a minute. Further me (and also Freddy) did quite some exhaustive testing to make sure we cover all codepaths.

Testing included:

regular click
ctrl-click
right-click->open-link in new tab
right-click->open-link in new container tab
drag and drop the link to a new tab
right-click->open-link in new window
right-click->open link in new private window
various other drag and drop operations (drag from one window into another window/tab, etc.).

If you have any more suggestions of what we could do test, or if you even want to test yourself, there are detailed instructions in comment 1.

Thanks!

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Summary: Bug 1524970: Explicity pass CSP for loadURI within browser-custom-element.js. r=gijs

Comment 5

[:Gijs (he/him)]

So I'm confused. As a random example that I dug up, without an exhaustive search, shouldn't pretty much all the callsites of openUILink in nsContextMenu.js be passing the CSP of the page?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #5)

So I'm confused. As a random example that I dug up, without an exhaustive search, shouldn't pretty much all the callsites of openUILink in nsContextMenu.js be passing the CSP of the page?

So in the cases of openLinkIn we get the CSP from within _openLinkInParameters. But you are right, openUILink within nsContextMenu.js should pass 'this.csp' as the CSP. I am confused why I havne't seen that myself. Let me look at that again. I'll clear the phab review until then.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1524970_questions.patch

Hey Gijs, so I performed a bunch of more testing, including opening images in new tab/window; view-image, etc and there were indeed some callsites missing.

Anyway, in the attached patch I equipped all of the callsites of openLinkIn() and openUILink() [1, 2] that don't pass a systemprincipal or also don't create a new nullPrincipal with a CSP argument.

In the patch, I left some 'todo:' where I guess I need to update nsIBrowser.idl [3] by a CSP argument, right? Question is, where do those argument get set. Put differently where is e.g. the nodePrincipal set, because I suppose I have to update those same callsites with the CSP argument, right?

Do you think I am missing something else? What other ways are there to open a new window/tab?

Thanks for your help!

[1] https://searchfox.org/mozilla-central/search?q=openLinkIn(&case=false&regexp=false&path=
[2] https://searchfox.org/mozilla-central/search?q=openUILink(&case=false&regexp=false&path=
[3] https://searchfox.org/mozilla-central/source/dom/interfaces/base/nsIBrowser.idl#94

Comment 8

[:Gijs (he/him)]

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

Created attachment 9050348 [details] [diff] [review]
bug_1524970_questions.patch

Hey Gijs, so I performed a bunch of more testing, including opening images in new tab/window; view-image, etc and there were indeed some callsites missing.

Anyway, in the attached patch I equipped all of the callsites of openLinkIn() and openUILink() [1, 2] that don't pass a systemprincipal or also don't create a new nullPrincipal with a CSP argument.

In the patch, I left some 'todo:' where I guess I need to update nsIBrowser.idl [3] by a CSP argument, right? Question is, where do those argument get set. Put differently where is e.g. the nodePrincipal set, because I suppose I have to update those same callsites with the CSP argument, right?

I assume you mean the contentPrincipal property on the browser instance. See https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/toolkit/content/widgets/browser-custom-element.js#597-599 and then https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/toolkit/modules/RemoteWebProgress.jsm#289 for where the e10s updates come from.

Do you think I am missing something else? What other ways are there to open a new window/tab?

Too many. :-(

Off-hand I think we need at least also:

https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/browser/base/content/nsContextMenu.js#882-885

everything in: https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/browser/components/pocket/content/main.js#479

https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/devtools/client/shared/link.js#64-67

https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/browser/base/content/browser.js#3722
https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/browser/base/content/browser.js#3687
(similar for the home button and the tabstrip and other drop areas that I'm probably forgetting)

https://searchfox.org/mozilla-central/rev/89414a1df52d06cfc35529afb9a5a8542a6e4270/browser/components/newtab/lib/ASRouter.jsm#1030 should actually be using about:newtab's own CSP, I suspect. (maybe the same for the other callsites in that document)

I think the easiest way to find things here would be to add assertions that cause us to fail to open things when loadURL and friends don't get a csp arg, perhaps holding such assertions to EARLY_BETA_OR_EARLIER initially, and landing that after soft freeze.

I think between the patch and the above set, we hopefully have enough that not everything breaks.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: frontend-csp.patch

Gijs, thanks for your comments and help. I incorporated all of your suggestions, but there are two remaining questions left before this can go into review:

1. Within "openTabWithUrl" within the file pocket/content/main.js we are expecting a 'contentPrincipal' but to me it seems we are not even setting it, I link to (what I think are) the two callsites. What do we do? Since we haven't set the contentPrincipal as of now, I think it's fine to file a follow up and let pocket folks handle it?

2. Within devtools/client/shared/link.js is target.csp correct? I mean can I just call that or is there some additional setup needed that I just don't see?

Finally, please note that there is an assertion within nsDocShell which checks that the explicit CSP and the CSP on the principal are equal. I guess there is no need for an additonal assertion somewhere else in the code. Agreed?

Once I have incorproated and fixed the issues above I think this is ready for review. Or is there still something else?

Ultimately: Again, thanks for all your help with that stuff - there are just way to many different ways to create a new tab/window, but I think we are getting there.

[1] https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9889-9916

Comment 10

[:Gijs (he/him)]

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

Created attachment 9050999 [details] [diff] [review]
frontend-csp.patch

Gijs, thanks for your comments and help. I incorporated all of your suggestions, but there are two remaining questions left before this can go into review:

1. Within "openTabWithUrl" within the file pocket/content/main.js we are expecting a 'contentPrincipal' but to me it seems we are not even setting it, I link to (what I think are) the two callsites. What do we do? Since we haven't set the contentPrincipal as of now, I think it's fine to file a follow up and let pocket folks handle it?

We do, it's passed to the listener at:

https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/browser/components/pocket/content/main.js#325-336

here:

https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/browser/components/pocket/content/main.js#588-597

. The other caller at https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/browser/components/pocket/content/main.js#97 passes an explicitly constructed null principal, as you noted.

2. Within devtools/client/shared/link.js is target.csp correct? I mean can I just call that or is there some additional setup needed that I just don't see?

OK, so for reference... I am not familiar with devtools. I tried chasing down references for a while and that was a bit of a nightmare. So then I took a step back and looked for the use of contentPrincipal in devtools:

https://searchfox.org/mozilla-central/search?q=contentPrincipal&case=false&regexp=false&path=devtools

This is quite manageable. Considering we know we're looking at something called "target", the obvious hit is target-mixin.js:

https://searchfox.org/mozilla-central/rev/f1c7ba91fad60bfea184006f3728dd6ac48c8e56/devtools/shared/fronts/targets/target-mixin.js#354

This seems to indicate (rather petrifyingly) that this is always null in e10s, and only returns something meaningful in non-e10s (see comment above the method). This seems to have been originally landed in bug 1466534. bug 1467945 is listed as wanting to improve this. I don't really understand why we don't use the contentPrincipal value for remote tabs, it's present then too. Anyway, as bug 1467945 explains, really this should be using the node's values, but we don't have access to those here because we're not same-process. We'd need to serialize/deserialize information to get it across. Or we could just always use a null principal, but again, as noted in bug 1467945, that'd break for file URIs. Either way, this is a CSP bypass - which might be OK given it's inside devtools... Needinfo :jdescottes to chime in if I'm missing something here.

TL;DR: no, just target.csp won't do. We'd need to add information to the devtools target mixin, and I actually don't know what it'd be getting.

Finally, please note that there is an assertion within nsDocShell which checks that the explicit CSP and the CSP on the principal are equal. I guess there is no need for an additonal assertion somewhere else in the code. Agreed?

Well, does the assertion fail if there is explicit CSP on the principal and no explicit CSP is passed? Last I checked, we just fall back on the principal's info. And if this is a regular, debug-only (as opposed to release) assertion, I'm willing to bet Actual Money that we're missing cases because nobody in frontend / none of our users run debug builds, they're too slow.

Comment 11

[Julian Descottes [:jdescottes]]

This seems to indicate (rather petrifyingly) that this is always null in e10s, and only returns something meaningful
in non-e10s (see comment above the method). This seems to have been originally landed in bug 1466534. bug 1467945 is
listed as wanting to improve this. I don't really understand why we don't use the contentPrincipal value for remote tabs,
it's present then too.

Local/Remote tabs in DevTools refers to remote debugging, not to e10s/non-e10s :) When we are doing remote debugging, the debugged tab is in another Firefox instance and we don't have access to the contentPrincipal of the tab. The typical usage for this method is when we linkify URLs coming from the content page and display them inside of the devtools UI.

I suppose adding a new csp getter on target-mixin that follows the same logic (null for remote debugging and this.tab.linkedBrowser.csp for local debugging) should be ok. On a sidenote, the only call site for target-mixin's contentPrincipal is link.js. It might be a good move to remove the method from target-mixin and have all this logic inside link.js, but we can handle this in a follow up.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Thanks Gijs and Julian for your help. For now I just added another get csp() to target-mixin.js - which we could fix in a follow up.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1524970: Update more frontend code to explicitly pass a csp. r=gijs

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Julian, I am not sure but I think the r+ from Gijs cleared you review request in phabricator, right? Just in case that is what I think is happening I am setting the ni? for you because I would like you to sign off on the /devtool changes within the patch - thanks!

Comment 15

[Julian Descottes [:jdescottes]]

thanks :)

If you want to avoid this you can use blocking reviewers (with a ! at the end of each reviewer name when you submit to phabricator). This way a first r+ will not clear the other requests.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Julian Descottes [:jdescottes] from comment #15)

If you want to avoid this you can use blocking reviewers (with a ! at the end of each reviewer name when you submit to phabricator). This way a first r+ will not clear the other requests.

Ah - I didn't know - thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3adf16249cb6
Update more frontend code to explicitly pass a csp. r=Gijs,jdescottes

Comment 18

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/3adf16249cb6

Comment 19

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4872