1518454 - Add CSP to loadURIOptions dictionary

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Hey Nika, if I would want to extend CreateWindow() [1] by an 'nsIContentSecurityPolicy' argument, what is the right way of doing that these days? I looked at IPC::Princpial [2] and I would have implemented ParamTraits for nsIContentSecurityPolicy, but the comment on line 24 [2] says 'Legacy IPC::Princpal type' so I am not sure what do if I want to pass a CSP through the IPC layer.

Thanks!

[1] https://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#4814
[2] https://searchfox.org/mozilla-central/source/dom/ipc/PermissionMessageUtils.h#24

Comment 2

[Nika Layzell [:nika] (ni? for response)]

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

Hey Nika, if I would want to extend CreateWindow() [1] by an 'nsIContentSecurityPolicy' argument, what is the right way of doing that these days? I looked at IPC::Princpial [2] and I would have implemented ParamTraits for nsIContentSecurityPolicy, but the comment on line 24 [2] says 'Legacy IPC::Princpal type' so I am not sure what do if I want to pass a CSP through the IPC layer.

So, the 'Legacy IPC::Principal type' is the legacy wrapper type, because there is now a ParamTraits<> implementation directly on the nsIPrincipal object. It's totally fine & recommended to implement ParamTraits (or IPDLParamTraits) for nsIContentSecurityPolicy :-).

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Hey :mossop,

I would like to also send a CSP within that JSON here [1]. I tried to figure out how the principal is serialized there, but unfortunately I couldn't. If I do:

json.csp = ownerDoc.nodePrincipal.csp;
then automagically the ::toJSON() implementation of nsIContentSecurityPolicy [2] is called - but I guess that's rather some consistence than happening on purpose, right?

Either way, if I would like to send a CSP there, what would I have to do?

Thanks!

[1] https://searchfox.org/mozilla-central/source/browser/actors/ClickHandlerChild.jsm#89-91
[2] https://searchfox.org/mozilla-central/source/dom/security/nsCSPContext.cpp#1494

Comment 4

[Dave Townsend [:mossop]]

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

Hey :mossop,

I would like to also send a CSP within that JSON here [1]. I tried to figure out how the principal is serialized there, but unfortunately I couldn't. If I do:

json.csp = ownerDoc.nodePrincipal.csp;
then automagically the ::toJSON() implementation of nsIContentSecurityPolicy [2] is called - but I guess that's rather some consistence than happening on purpose, right?

JSON.stringify atuomatically calls an object's toJSON if it exists. I guess maybe the code that serialises for IPC does the same?

Either way, if I would like to send a CSP there, what would I have to do?

Now that https://phabricator.services.mozilla.com/D15728 is landed doesn't including the principal just include the CSP?

Comment 5

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

(In reply to Dave Townsend [:mossop] (he/him) from comment #4)

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

Either way, if I would like to send a CSP there, what would I have to do?

Now that https://phabricator.services.mozilla.com/D15728 is landed doesn't including the principal just include the CSP?

It does, but the plan is to move the CSP off the principal...

Comment 6

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] (he/him) from comment #4)

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

Hey :mossop,

I would like to also send a CSP within that JSON here [1]. I tried to figure out how the principal is serialized there, but unfortunately I couldn't. If I do:

json.csp = ownerDoc.nodePrincipal.csp;
then automagically the ::toJSON() implementation of nsIContentSecurityPolicy [2] is called - but I guess that's rather some consistence than happening on purpose, right?

JSON.stringify atuomatically calls an object's toJSON if it exists. I guess maybe the code that serialises for IPC does the same?

Actually this is really strange. I can't see the structured cloning code calling toJSON methods anywhere, and JS shouldn't be able to access the csp attribute of principals since it is marked as [noscript]. So I have no idea how that worked.

So I think you have two options:

Assuming JS can get the nsIContentSecurityPolicy for the policies, add that to the JSON and write the code for structured cloning it (most of that already exists in https://phabricator.services.mozilla.com/D15728).

Or send something that represents the CSP across and make it possible to recreate on the other side. nsIPrincipal.cspJSON seems to be something you could use for this.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dave Townsend [:mossop] (he/him) from comment #6)

Actually this is really strange. I can't see the structured cloning code calling toJSON methods anywhere, and JS shouldn't be able to access the csp attribute of principals since it is marked as [noscript]. So I have no idea how that worked.

Right, I should have mentioned that I changed nsIPrincipal

• [noscript] attribute nsIContentSecurityPolicy csp;

• attribute nsIContentSecurityPolicy csp;

so that JS can access the CSP.

So I think you have two options:

Assuming JS can get the nsIContentSecurityPolicy for the policies, add that to the JSON and write the code for structured cloning it (most of that already exists in https://phabricator.services.mozilla.com/D15728).

Yeah that makes sense - I'll try that - thanks!

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1518454_add_csp_to_loadURIOptions_backend_changes.patch

Boris, I am attaching a patch which provides the backend bits needed to pass the CSP around explicitly for top-level loads. The patch passes TRY but I am guessing we still miss some corner cases - but I would like to follow up on those in separate bugs as we encounter those corner cases. Please note that this patch still queries the CSP to be used from the principal within nsDocshell.cpp, but I added an assertion that the explicit CSP is identical to the CSP on the principal.

Regarding the inclusion of |#include "nsIXULRuntime.h"|. I couldn't figure out what I changed, but all of sudden I encountered that compiler error:

0:16.83 In file included from /home/ckerschb/source/mc-dbg/dom/ipc/Unified_cpp_dom_ipc1.cpp:2:
0:16.83 /home/ckerschb/source/mc/dom/ipc/PreallocatedProcessManager.cpp:157:16: error: no member named 'BrowserTabsRemoteAutostart' in namespace 'mozilla'
0:16.83 if (mozilla::BrowserTabsRemoteAutostart() &&
0:16.83 ~~~~~~~~~^
0:20.90 1 error generated.
hence I included nsIXULRuntime.h to make that compile error go away.

Finally, if you can review the backend part then Gijs can help with the frontend bits.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1518454_add_csp_to_loadURIOptions_frontend_changes.patch

Gijs, as you are aware, we are trying to get the CSP off the principal. To do so, we need to pass the CSP explicitly from the frontend to docshell. Now, ultimately the CSP will hang off the document, hence the new entry points are wherever we still have a document. At the moment, I query the CSP from the principal, but ultimately I will query it from that document.

I am pretty sure we will encounter places in the codebase where we don't pass the CSP correctly. This patch at least passes TRY and we can follow up on individual code paths later, because the CSP used at the moment is still the one from the principal.

One case however I am unsure, which is within browser/base/content/nsContextMenu.js. Is it possible to query the document somewhere there. If so, please let me know. Otherwise we have to pass the CSP as an argument into that function. (Not sure if within that patch or as a follow up).

Comment 10

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

Comment on attachment 9037524 [details] [diff] [review]
bug_1518454_add_csp_to_loadURIOptions_frontend_changes.patch

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

This seems incomplete, e.g. what happens with the other callers of browser.loadURI (e.g. webext-panels) and the browser binding / custom element implementation(s)?

::: browser/base/content/browser.js
@@ +4650,5 @@
>      return BrowserUtils.onBeforeLinkTraversal(originalTarget, linkURI, linkNode, isAppTab);
>    },
>  
>    // Check whether this URI should load in the current process
> +  shouldLoadURI(aDocShell, aURI, aReferrer, aHasPostData, aTriggeringPrincipal, aCsp) {

see idl remarks elsewhere.

::: browser/base/content/nsContextMenu.js
@@ +229,5 @@
>      // Everything after this isn't sent directly from ContextMenu
>      this.ownerDoc = this.target.ownerDocument;
>  
> +    // Bug 965637, query the CSP from the doc instead of the Principal
> +    this.csp = context.principal.csp;

You can do this in https://searchfox.org/mozilla-central/source/browser/actors/ContextMenuChild.jsm and then deserialize it here. That'll prep getting it from the doc instead of the principal, though tbf I'm pretty sure we use the nodePrincipal for the context-clicked thing so I doubt there'd be a difference...

::: browser/base/content/tab-content.js
@@ +34,5 @@
>      return BrowserUtils.onBeforeLinkTraversal(originalTarget, linkURI, linkNode, isAppTab);
>    },
>  
>    // Check whether this URI should load in the current process
> +  shouldLoadURI(aDocShell, aURI, aReferrer, aHasPostData, aTriggeringPrincipal, aCsp) {

This is an IDL method so needs changing in https://searchfox.org/mozilla-central/source/toolkit/components/browser/nsIWebBrowserChrome3.idl#56 and/or https://searchfox.org/mozilla-central/source/xpfe/appshell/nsIXULBrowserWindow.idl#67 as well.

@@ +49,5 @@
>    },
>  
>    // Try to reload the currently active or currently loading page in a new process.
> +  reloadInFreshProcess(aDocShell, aURI, aReferrer, aTriggeringPrincipal, aLoadFlags, aCsp) {
> +    E10SUtils.redirectLoad(aDocShell, aURI, aReferrer, aTriggeringPrincipal, true, aLoadFlags, aCsp);

Ditto

::: mobile/android/chrome/geckoview/GeckoViewNavigationChild.js
@@ +66,5 @@
>      return BrowserUtils.onBeforeLinkTraversal(aOriginalTarget, aLinkURI, aLinkNode, aIsAppTab);
>    }
>  
>    // nsIWebBrowserChrome
> +  shouldLoadURI(aDocShell, aURI, aReferrer, aHasPostData, aTriggeringPrincipal, aCsp) {

Please find a mobile reviewer for the portion in this file, I don't really know enough about mobile to help here.

::: toolkit/actors/WebNavigationChild.jsm
@@ +43,5 @@
>          this.loadURI(message.data.uri, message.data.flags,
>                       message.data.referrer, message.data.referrerPolicy,
>                       message.data.postData, message.data.headers,
> +                     message.data.baseURI, message.data.triggeringPrincipal,
> +                     message.data.csp);

While we're here and need to change the signature anyway, can we just pass `message.data` directly to `loadURI` ?

::: toolkit/modules/sessionstore/Utils.jsm
@@ +145,5 @@
> +   *
> +   * @param {nsIContentSecurity} csp. The csp to serialize.
> +   * @return {String} The base64 encoded csp data.
> +   */
> +  serializeCSP(csp) {

I know that the principal (de)serialization helper lives on this object, but IMO it's not a very good place for it because it's a sessionstore module, and sessionstore is definitely not the only or even primary consumer of these methods. Especially in this case, E10SUtils would be better. We can move the principal one in a separate bug.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1518454_add_csp_to_loadURIOptions_frontend_changes.patch

Gijs,

thanks for your suggestions. Please note that the various *.idl changes are in the other patch where I incorporated all the backend changes [1]. After review, I'll merge the patches together anyway - sorry for not mentioning this explicitly in the initial review. Further, I moved the (de)serializeCSP bits into E10SUtils and updated the interface of loadURI() within WebNavigationChild.jsm as suggested.

To answer your question if this update is incomplete or not. I updated all the codepaths so this implementation runs cleanly on TRY. In addition, I did quite intensive testing (see list underneath) to make sure we are covering all the highly executed codepaths. Please note that (within the other patch) we have an assertion within nsDocShell.cpp to make sure the passed in CSP matches the CSP from the principal. More importantly for now however, we do not use the passed in CSP - we just assert. Probably there might be a follow up, but I would like to get that fundamental implementation of the explicit CSP landed. Again it doesn't affect any of our production code. Finally, I manually inspected callsites, e.g. you mentioned webext-panels, there for example we use a systemPrincipal as the loadingPrincipal hence we don't have a CSP.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=9037523&action=diff

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.).

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1518454_add_csp_to_loadURIOptions_mobile_changes.patch

Hey :snorp, I am not sure if you are the right reviewer. If not, maybe you can suggest someone who can take a look at the changes. In short, we are trying to explicitly pass a CSP from the frontend to the backend (docshell) because ultimately we would like to remove the CSP from the principal.

The *.idl changes for the change are in the other patch [1].

Please feel free to ping me on slack in case you have any questions - thanks!

[1] https://bugzilla.mozilla.org/attachment.cgi?id=9037523&action=diff

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9037952 [details] [diff] [review]
bug_1518454_add_csp_to_loadURIOptions_mobile_changes.patch

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

::: mobile/android/chrome/geckoview/GeckoViewNavigationChild.js
@@ +73,5 @@
>      // We currently only support one remoteType, "web", so we only need to bail out
>      // if we want to load this URI in the parent.
>      // const remoteType = E10SUtils.getRemoteTypeForURIObject(aURI, true);
>      // if (!remoteType) {
> +    //   E10SUtils.redirectLoad(aDocShell, aURI, aReferrer, aTriggeringPrincipal, false, null, aCsp);

This whole comment block should just be removed. I think I accidentally left it in a patch.

Comment 14

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

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

To answer your question if this update is incomplete or not. I updated all the codepaths so this implementation runs cleanly on TRY. In addition, I did quite intensive testing (see list underneath) to make sure we are covering all the highly executed codepaths. Please note that (within the other patch) we have an assertion within nsDocShell.cpp to make sure the passed in CSP matches the CSP from the principal. More importantly for now however, we do not use the passed in CSP - we just assert. Probably there might be a follow up, but I would like to get that fundamental implementation of the explicit CSP landed. Again it doesn't affect any of our production code. Finally, I manually inspected callsites, e.g. you mentioned webext-panels, there for example we use a systemPrincipal as the loadingPrincipal hence we don't have a CSP.

So let me get this straight... we don't use the new parameter? And "failure" in this case would imply not passing it, as that's the status quo. So the only way any behavior change would trip things on try is if we had failing tests on debug, right?

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?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

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

So let me get this straight... we don't use the new parameter? And "failure" in this case would imply not passing it, as that's the status quo. So the only way any behavior change would trip things on try is if we had failing tests on debug, right?

Yes, your assumptions are correct. Please note that we have hundreds of CSP tests, and also quite so many wpt tests for CSP. I did quite so much manual testing additionally. If you have more suggestions of how I can exercise code-paths I am happy to do so.

Please don't get me wrong, I will invest even more time testing and updating after this initial version has landed - but I think we should land this bug rather sooner than later to get more debug time. I would hope that the community (our own engineers) will file bugs and we can follow up on them individually. More or less get code-paths updated iteratively. Similar to what we did for passing the right triggeringPrincipal for top-level loads.

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 agree, would you be fine with me doing that in a follow up bug?

Comment 16

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

Comment on attachment 9037950 [details] [diff] [review]
bug_1518454_add_csp_to_loadURIOptions_frontend_changes.patch

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

r=me on the understanding that there will need to be follow-ups to update any remaining codepaths, and that we won't land this until after merge day.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

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

r=me on the understanding that there will need to be follow-ups to update
any remaining codepaths, and that we won't land this until after merge day.

Yes for sure - as agreed we will land after the merge day and we will update remaining codepaths in that upcoming cycle. Thanks!

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

FWIW, here is a green TRY run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e3878bc7a3791decb9db4a26846f2c03ef6fdae

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9037523 [details] [diff] [review]
bug_1518454_add_csp_to_loadURIOptions_backend_changes.patch

>+++ b/caps/nsIPrincipal.idl
>-    [noscript] attribute nsIContentSecurityPolicy csp;
>+    attribute nsIContentSecurityPolicy csp;

I assume this is a temporary measure, since we plan to move the CSP out of principals?

Do we need to _set_ this from script, or can we have a scriptable getter and a noscript setter?

>-    [noscript] readonly attribute nsIContentSecurityPolicy preloadCsp;
>+    readonly attribute nsIContentSecurityPolicy preloadCsp;

Likewise.

>+++ b/docshell/base/nsDocShell.cpp
>@@ -6869,16 +6882,21 @@ nsresult nsDocShell::EndPageLoad(nsIWebP
>+          // Bug 965637, query the CSP from the doc instead of the Principal

Which "the doc", exactly?  Seems like this needs to come from the loadinfo or something...

>@@ -9142,33 +9161,38 @@ nsresult nsDocShell::InternalLoad(nsDocS
>+        // Bug 965637, query the CSP from the doc instead of the Principal
>+        nsresult rv = doc->NodePrincipal()->GetCsp(getter_AddRefs(newCsp));

What this really needs is a discussion about where the CSP should be coming from: the current document, or the document that caused the load to start.  Or does it not matter?  There's absolutely no documentation for what the aCsp argument to aNewURI means, so I can't tell what should get passed there.

>@@ -9349,17 +9373,17 @@ nsresult nsDocShell::InternalLoad(nsDocS
>+        aLoadState->Csp(), &shouldLoad);

Why is that the right CSP?  More on this below.

>+++ b/docshell/base/nsDocShell.h
>   NS_IMETHOD OnLinkClick(nsIContent* aContent, nsIURI* aURI,
...
>+                         nsIContentSecurityPolicy* aCsp) override;

What are the semantics of this argument?  It's not at all clear how callers should decide what to pass here.

>   NS_IMETHOD OnLinkClickSync(
...
>+      nsIContentSecurityPolicy* aCsp = nullptr) override;

Likewise.

>   nsresult AddToSessionHistory(nsIURI* aURI, nsIChannel* aChannel,
>+                               nsIContentSecurityPolicy* aCsp,

And here.

>   bool OnNewURI(nsIURI* aURI, nsIChannel* aChannel,
>+                nsIContentSecurityPolicy* aCsp, bool aFireOnLocationChange,

And here.

>+++ b/docshell/base/nsDocShellLoadState.h
>+  // The CSP of the load, that is, the CSP of the document that started the
>+  // load.
>+  nsCOMPtr<nsIContentSecurityPolicy> mCsp;

What if the load was not started by a document?  Can this be null?  Is this the CSP that gets applied to redirects?  Or is that affected by headers?

This could use a somewhat clearer comment.

>+++ b/docshell/base/nsILinkHandler.h
>+   * @param aCsp, the CSP to be used.

Used for what?

Ideally, this would be described in terms of what spec sections have behavior that is affected by the value of this argument....

>+   * @param aCsp, the CSP to be used.

And here.


>+++ b/docshell/shistory/nsISHEntry.idl
>+     * Get the csp, if any, that was associated with the channel
>+     * that the document that was loaded to create this history entry
>+     * came from.

A channel has at least two CSPs associated with it that I can think of: the CSP of the Client that started the load (which can get inherited, and maybe affect redirects?) and the CSP from the channel's response headers.  Which one is this?

>+++ b/dom/base/nsOpenURIInFrameParams.h
>+  nsCOMPtr<nsIContentSecurityPolicy> mCsp;

Document.

>+++ b/dom/bindings/Bindings.conf
>+addExternalIface('ContentSecurityPolicy', nativeType='nsIContentSecurityPolicy',
>+                 headerFile='nsIContentSecurityPolicy.h', notflattened=True)

You don't need the headerFile bit.  It defaults to just replacing "::" with "/" in the nativeType and appending ".h" on the end.

>+++ b/dom/interfaces/base/nsIBrowserDOMWindow.idl
>+  attribute nsIContentSecurityPolicy csp;

Document.

>+++ b/dom/ipc/CSPMessageUtils.cpp

I guess this is no different from what we do now with principals, but longer-term we'll want a better solution here, it feels like.

>+++ b/dom/ipc/DOMTypes.ipdlh
>+  nsIContentSecurityPolicy Csp;

Document.

>+++ b/dom/ipc/PContent.ipdl
>+                       nsIContentSecurityPolicy aCsp,

Document.

>     async CreateWindowInDifferentProcess(
>+      nsIContentSecurityPolicy aCsp,

Document.

>+++ b/dom/plugins/base/nsPluginInstanceOwner.cpp
>+                       /* isTrusted */ true, triggeringPrincipal, nullptr);

This at least needs documentation explaining why we're not passing the document's CSP, no?

>+++ b/dom/webidl/LoadURIOptions.webidl
>+   * The CSP to be used for the load.

Again, should document what this means in practice.

>+++ b/toolkit/components/browser/nsIWebBrowserChrome3.idl
>+   * @param aCsp
>+   *        The CSP to be applied to aURI.

CSP is not applied to URIs, right?  It applies to loads and to documents and whatnot, but not really URIs... what does this mean?

>   bool reloadInFreshProcess(in nsIDocShell aDocShell,
>+                            in nsIContentSecurityPolicy aCsp);

Document.

>+++ b/xpfe/appshell/nsIXULBrowserWindow.idl
>+   * @param aCsp
>+   *        The CSP to be applied to aURI.

Again, what does that mean?

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Summary: Bug 1518454: Part 1, backend changes, add CSP to loadURIOptions dictionary and pass CSP explicitly from frontend to docshell. r=bz

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b18d986ef29
Part 1, backend changes, add CSP to loadURIOptions dictionary and pass CSP explicitly from frontend to docshell. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/530bf099ee9d
Part 2, frontend changes, add CSP to loadURIOptions dictionary and pass CSP explicitly from frontend to docshell. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c794488399f7
Part 3, mobile changes, add CSP to loadURIOptions dictionary and pass CSP explicitly from frontend to docshell. r=snorp

Comment 22

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9b18d986ef29
https://hg.mozilla.org/mozilla-central/rev/530bf099ee9d
https://hg.mozilla.org/mozilla-central/rev/c794488399f7