Closed Bug 965637 Opened 10 years ago Closed 5 years ago

Move CSP from nsIPrincipal into the Client

Categories

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

Product:
Core
Component:
DOM: Security
Platform:
x86_64
Linux
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: grobinson, Assigned: ckerschb)

References

(Depends on 3 open bugs, Blocks 13 open bugs, Regressed 1 open bug)

Details

(Whiteboard: [domsecurity-active][wptsync upstream])

Attachments

(4 files, 19 obsolete files)

Bug 965637: Move CSP from Principal into Client, part 1: backend changes. r=mccr8
47 bytes, text/x-phabricator-request
Details | Review
Bug 965637: Move CSP from Principal into Client, part 2: worker changes. r=baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 965637: Move CSP from Principal into Client, part 3: frontend changes. r=gijs
47 bytes, text/x-phabricator-request
Details | Review
Bug 965637: Move CSP from Principal into Client, part 4: test updates. r=mccr8,jkt
47 bytes, text/x-phabricator-request
Details | Review 
As part of the "get CSP to work on Chrome pages" strategy discussed today, we will need to move the CSP object from nsIPrincipal to the proposed new "document settings object" (once it's implemented), and update all of the call sites.
Blocks: 923902
Depends on: 965413
Assignee: nobody → grobinson
Note that you will need to fix up what session history stores and session restore saves/restores when you do this...
Summary: Move CSP from nsIPrincipal to new "document settings object" → Move CSP from nsIPrincipal to new "document settings object" (nsILoadInfo)
In bug 1038756 we add the requesting node/principal/contentPolicyType to nsILoadInfo and store the loadInfo on all channels relevant for CSP and MCB. Most likely there is no need to store the CSP seperately. In addition, bug 1038756 will allow us to remove nsiChannelPolicy because we have the needed information now stored in the loadInfo which allows evaluation of CSP after redirects.
Depends on: 1038756
The term nsILoadInfo is misleading, since nsILoadInfo is currently attached to very channel. In fact we should consider moving the CSP into the document or create some kind of object that hangs of a document where the CSP can live.
Summary: Move CSP from nsIPrincipal to new "document settings object" (nsILoadInfo) → Move CSP from nsIPrincipal to new "document settings object"
The point is that we need to hang the CSP off the _channel_ for cases that want to inherit the CSP.  Storing it in the document is also needed, of course, but the nsILoadInfo bit was there on purpose.
And in particular, if the CSP isn't in the principal, then it needs to explicitly be on the loadinfo whenever the loadinfo is inheriting the principal, no?
Unassigning Garrett to make this show up in backlog.
Assignee: garrett.f.robinson+mozilla → nobody
Component: Security → DOM: Security
Whiteboard: [domsecurity-backlog]
Priority: -- → P3
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog3]
FWIW, I'm adding code over in bug 1337543 to strip the CSP from the document nsIPrincipal used to register a service worker.  We don't want to inherit the CSP here.  I think it would be much better to move this stateful data off of our identity type.
See Also: → 1337543
(In reply to Garrett Robinson [:grobinson] from comment #0)
> As part of the "get CSP to work on Chrome pages" strategy discussed today,
> we will need to move the CSP object from nsIPrincipal to the proposed new
> "document settings object" (once it's implemented), and update all of the
> call sites.

Note, if we move this to another type, it needs to support Worker environments as well.  They have their own CSP.
The infrastructure I'm adding in bug 1293277 is creating a "Client" representation for each environment.  This will support both main thread documents and workers.  Perhaps one day CSP can be hung off of there.
Note, my clients work is landing.  The structure I'm proposing moving CSP to instead of the principal is the ClientSource and ClientInfo:

https://searchfox.org/mozilla-central/source/dom/clients/manager/ClientSource.h
https://searchfox.org/mozilla-central/source/dom/clients/manager/ClientInfo.h

I've already landed code to attached ClientInfo to an nsILoadInfo:

https://searchfox.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#851
I'm adding an NS_NewChannel() that takes ClientInfo over in bug 1231211.  I need this as part of the divorce between docshell and ServiceWorkerManager.

Note, I tried to make NS_NewChannel() just take the ClientInfo as a replacement for loading principal.  The ClientInfo's principal should be the same.  Unfortunately, the fact that CSP is mutable and added to the document/worker principal prevents that.  Using the ClientInfo's principal effectively stripped CSP.

So initiall the NS_NewChannel() API will have to take loading principal and ClientInfo until we fix this bug.
Depends on: 1231211
Assignee: nobody → ckerschb
Blocks: 1430748
Status: NEW → ASSIGNED
Priority: P3 → P2
Summary: Move CSP from nsIPrincipal to new "document settings object" → Move CSP from nsIPrincipal to nsILoadInfo
Whiteboard: [domsecurity-backlog3] → [domsecurity-active]
Attached patch bug_965637_move_csp_to_loadinfo.patch (obsolete) — — Splinter Review 
Boris, Ben. I did some initial testing and it seems possible to me to move the CSP from nsIPrincipal to nsILoadInfo. I haven't implemented the full inheritance model, inline styles and worker related parts. Ultimately I would like to apply a CSP to system privileged about: pages which this patch would be the groundwork for.

Are there any objections to moving forward with this idea of moving the CSP from nsIPrincipal to nsILoadInfo?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bkelly)
Moving this to loadinfo makes sense.

The other option would be to put this in the Client, right?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> The other option would be to put this in the Client, right?

Yes, I know that Ben suggested that at one point, hence I am making sure he is fine with my approach. I think the LoadInfo is the right place for the CSP to live though.
My main question is, how are you putting the CSP on the LoadInfo?

I'm not opposed to putting it on LoadInfo instead of ClientInfo.  I just want to make sure whatever we do here works equally well for workers as windows.  I just did the work of getting ClientInfo set on most (all?) of the worker nsIChannel LoadInfo's, so if we put the CSP in the ClientInfo then in theory it should just work for both.

I was kind of hoping in the long term that the LoadInfo ClientInfo could be universally set and replace things like mLoadingPrincipal, etc.
Flags: needinfo?(bkelly)
One question I have is where we actually need the CSP.  For example, do we need it on stylesheet loads?  I assume we only have ClientInfo on window and worker loads, right?
We set the ClientInfo whenever NS_NewChannel*() is called with a loading node.  So I believe this stylesheet path should have a ClientInfo:

https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/layout/style/Loader.cpp#1377

The "outside a document" path that uses a system principal will not have a ClientInfo currently.  I'm not sure I understand what that path is for.
To clarify when a ClientInfo is currently set, its either:

1. When NS_NewChannel*() is passed a ClientInfo via this API:

https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/netwerk/base/nsNetUtil.h#159

This is only currently done for APIs that workers can use.

2. When NS_NewChannel*() is passed a loading node, but not an explicit ClientInfo:

https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/netwerk/base/LoadInfo.cpp#138
(In reply to Ben Kelly [:bkelly] from comment #15)
> My main question is, how are you putting the CSP on the LoadInfo?

When you look at my attached patch, you'll see that I set a CSP on the Loadinfo whenever we would have set one on the principal. So, in theory we should only have the CSP on loadinfo's of type TYPE_DOCUMENT and TYPE_SUBDOCUMENT, which seems very similar to setting a client whenever you have a node available. To answer your question, it's not passed to the constructor of LoadInfo.

Within CSP we then query the channel from the loadingContext (basically the loading document) which is important, because we can't query the exact channel of the resource to be loaded due to the setup of nsIContentPolicy.
How does the CSP get set on a channel like this, then?

https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/dom/fetch/FetchDriver.cpp#530

A fetch() initiated from a worker thread.
(In reply to Ben Kelly [:bkelly] from comment #20)
> How does the CSP get set on a channel like this, then?
> 
> https://searchfox.org/mozilla-central/rev/
> b9f1a4ecba48b2d8c686669e32d109c40e927b48/dom/fetch/FetchDriver.cpp#530
> 
> A fetch() initiated from a worker thread.

I think we would have to explicitly add it. I assume same problem when we add it to the ClientInfo, right? But the bigger problem in that case is, how do we get it off the LoadInfo/ClientInfo when we need in within CSP code? As mentioned, nsIContentPolicy does not allow to pass/query the required channel.

Other than that I would imagine we face pretty much the exact same problems whether we add it to the LoadInfo or to the ClientInfo. I am fine either way, though I personally would prefer to add it to the LoadInfo. Seems like the semantically correct place to me.
Well if CSP was owned by ClientSource then it could be automatically included in ClientInfo and get passed along with the currently plumbing to set ClientInfo on the LoadInfo.  You could still have whatever "get CSP" API on the LoadInfo object, but it would be derived from the ClientInfo inside it.

Let me get at this in a different way.  Currently you just commented out all the CSP code in workers.  What do you plan to replace this with?

If you just pass it as a separate value to the LoadInfo you have to plumb it through a ton of API layers.  If you include the CSP value on ClientInfo you get that plumbing for free.

For example, we currently pass this test:

https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/testing/web-platform/tests/content-security-policy/connect-src/worker-connect-src-blocked.sub.html

Do we pass it with your current patch?  My guess is we don't.
(In reply to Ben Kelly [:bkelly] from comment #22)
> If you just pass it as a separate value to the LoadInfo you have to plumb it
> through a ton of API layers.  If you include the CSP value on ClientInfo you
> get that plumbing for free.

Yeah, the worker related stuff is a bit hairy and I haven't thought it completely through yet. Obviously you are in a better position for judgement since you know the worker code way better than I do. As I said, I am fine with having the CSP on the ClientInfo.

But then moving forward, what's the schedule? Are you actively working on the ClientInfo? If so, I would be fine with waiting and just add the LoadInfo bits for querying the CSP from the clientinfo through the LoadInfo. How long do you think it will take before ClientInfo gets landed?
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
> Within CSP we then query the channel from the loadingContext (basically the
> loading document) which is important, because we can't query the exact
> channel of the resource to be loaded due to the setup of nsIContentPolicy.

In bug 1407056, we talked about changing nsIContentPolicy to just pass LoadInfo in place of most of the current arguments. I haven't had a chance to go back and do that yet, but it would probably simplify this situation.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #23)
> But then moving forward, what's the schedule? Are you actively working on
> the ClientInfo? If so, I would be fine with waiting and just add the
> LoadInfo bits for querying the CSP from the clientinfo through the LoadInfo.
> How long do you think it will take before ClientInfo gets landed?

I'm not sure what you mean.  ClientInfo is in the tree today.  The ClientInfo from a worker gets set on the channel today.

Last we spoke the main blocker was that CSP lacked an .ipdl representation, so it could not yet be added to ClientInfo.  If that was added then I don't think it would be too hard to set the CSP stuff on the ClientSource and its internal ClientInfo.

But maybe I'm confused about one point here.  It seems there are a couple use cases:

1. Taking the CSP configured for a client and applying it to all of the nsIChannel objects that it creates so the policy can be forced.
2. Getting the initial CSP from the non-subresource load that creates the channel or its parent client.

I've mainly been talking about (1) here.  Maybe you have been more focused on (2)?

For (2) we would still need code in the document/worker cases to explicitly read the headers from the non-subresource channel after it loads and call something like ClientSource::SetCSP().

For inheriting CSP from its parent you would want to call this ClientSource::SetCSP() method when docshell creates the "initial ClientSource" here:

https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/docshell/base/nsDocShell.cpp#2790

You can see how it handles inheriting the service worker there.  I think it should be very similar.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #24)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
> > Within CSP we then query the channel from the loadingContext (basically the
> > loading document) which is important, because we can't query the exact
> > channel of the resource to be loaded due to the setup of nsIContentPolicy.
> 
> In bug 1407056, we talked about changing nsIContentPolicy to just pass
> LoadInfo in place of most of the current arguments. I haven't had a chance
> to go back and do that yet, but it would probably simplify this situation.

Yeah, now that legacy extensions are gone I think we can and should do that. Is there already a bug for that?
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26)
> Yeah, now that legacy extensions are gone I think we can and should do that.
> Is there already a bug for that?

I don't think I ever filed a bug for it, no. I've just been keeping it in mind. I can file one now, if you like, though.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #24)
> In bug 1407056, we talked about changing nsIContentPolicy to just pass
> LoadInfo in place of most of the current arguments. I haven't had a chance
> to go back and do that yet, but it would probably simplify this situation.

FWIW, I filed Bug 1439713 to get that done.
Baku already needed to add the serialization bits for the CSP, hence I mark bug 1438945 blocking this bug because it add the child/parent serialization bits of the CSP needed for this bug to work.
Depends on: 1438945
Ben, I am making good progress on moving the CSP into the loadInfo but I have a fundamental question regarding workers. Let's look at test_CSP.html which ultimately ends up calling file_main_worker.js [1] which ships with its own CSP which is applied to the principal here [2].

In the current world, we set the CSP on the principal which I assume is passed to NS_NewChannel() when we create a new channel. In the case of the test to DoXHR() and fetch(), right?

Now, here is the question. In the new world, no matter if the CSP actually lives in the Loadinfo itself or within ClientInfo inside the Loadinfo. How are we going to propagate the CSP from here [2] to the actual loadinfo of the channel?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/csp/file_main_worker.js#41-44
[2] https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#1480-1489
Flags: needinfo?(bkelly)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> Now, here is the question. In the new world, no matter if the CSP actually
> lives in the Loadinfo itself or within ClientInfo inside the Loadinfo. How
> are we going to propagate the CSP from here [2] to the actual loadinfo of
> the channel?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/security/test/csp/
> file_main_worker.js#41-44
> [2]
> https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.
> cpp#1480-1489

We already propagate the worker's ClientInfo to the fetch/xhr channel.  For example:

https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/dom/fetch/Fetch.cpp#539-540
https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/dom/fetch/Fetch.cpp#414
https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/dom/fetch/FetchDriver.cpp#529-542

So if you get the CSP into the ClientInfo, then the propagate to the channel is already done.
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [Part-time, not reviewing][:bkelly] from comment #31)
> So if you get the CSP into the ClientInfo, then the propagate to the channel
> is already done.

That sounds good, but again, to make sure. What is in you opinion the best way to propagate the CSP from here to the ClientInfo?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#1480-1489
Ben, please see comment 32!
Flags: needinfo?(bkelly)
Sorry, I misunderstood the previous question.

Basically we need to add a ClientSource::SetCSP() method:

https://searchfox.org/mozilla-central/source/dom/clients/manager/ClientSource.h

This will then update the ClientSource's mClientInfo with the value.  Its important to do it on the ClientSource's client info and not a copy.

Updating the ClientInfo is basically add a ClientInfo::SetCSP() that then updates the underlying IPCClientInfo.  This SetCSP method should probably have a comment warning that it should only be called by ClientSource.

The main hassle calling ClientSource::SetCSP() will be that you probably need to call it on the worker thread.  The code in comment 32 is on the main thread.  So you need to get the CSP back to the worker thread with the other script loading results and call it there.  You can look what I do with ScriptLoader.mController for an example:

https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/dom/workers/ScriptLoader.cpp#600

Does that help?
Flags: needinfo?(bkelly)
Oh, and then we should also call ClientSource::SetCSP() for nsGlobalWindowInner as well.
(In reply to Ben Kelly [Part-time, not reviewing][:bkelly] from comment #35)
> Oh, and then we should also call ClientSource::SetCSP() for
> nsGlobalWindowInner as well.

Sorry, this part is not related to workers.  I just mean that the window-path should use the same code as the worker-path.  Set the CSP on ClientSource and then it will get propagated to the channel auto-magically.
Finally I managed to make some good progress on the front of moving the CSP from the Principal into the LoadInfo. There are still some issues that need to be clarified and some bugs that we need to shakened out. Obviously the code needs to be simplified and cleaned up, but the overall logic is already there:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4df7fdf9dd12cbed424fc1585bcb3339dccf0d10
:kmag, within this bug we are moving the CSP from the Principal into the loadinfo. From what I can tell the AddonPolicy, which also lives on the Principal [1] is pretty much encapsulated from the changes we are performing regarding the 'regular' CSP.

Question is, since chnages here are already pretty complex, do we need to move AddonPolicy into the Loadinfo within this patch, or can it happen in a follow up bug (effort)?

[1] https://dxr.mozilla.org/mozilla-central/source/caps/nsIPrincipal.idl#278
Flags: needinfo?(kmaglione+bmo)
Attachment #8950227 - Attachment is obsolete: true
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #38)
> :kmag, within this bug we are moving the CSP from the Principal into the
> loadinfo. From what I can tell the AddonPolicy, which also lives on the
> Principal [1] is pretty much encapsulated from the changes we are performing
> regarding the 'regular' CSP.
> 
> Question is, since chnages here are already pretty complex, do we need to
> move AddonPolicy into the Loadinfo within this patch, or can it happen in a
> follow up bug (effort)?

We can't move AddonPolicy into LoadInfo, since it's needed in a lot of contexts other than loads. We could copy it, I suppose, assuming we can't just grab it from the LoadInfo principals.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #39)
> We can't move AddonPolicy into LoadInfo, since it's needed in a lot of
> contexts other than loads. We could copy it, I suppose, assuming we can't
> just grab it from the LoadInfo principals.

I guess for this bug we'll consider the AddonPolicy out of scope and decide in a different bug how to move forward. I guess there are some ways to get the AddonPolicy off the Principal as well, but let's leave that discussion for a different day.
Boris,

I spent quite so much time debugging why the iframe within this test [1] is not blocked by CSP, but it seems there is some disconnect in my brain and I can't figure out how Firefox internals work in that case.

Looking at the test:
a) The toplevel page ships with a CSP including frame-src 'none'
b) The toplevel page then loads an iframe using about:srcdoc (which correctly inherits the CSP of the toplevel page, and is not blocked because about: pages are excluded from CSP restrictions)
c) The toplevel page then does document.write('create new iframe') within the about:srcdoc iframe.
and here is where I can't follow what's happening anymore.

Within the CSP code the loading document for the iframe load within (c) is not the about:srcdoc iframe, but instead it's a document with a loadinfo holding a contenttype of TYPE_OTHER. Is this document created and inserted somehow by document.write() and does not go through docshell? Or is there anything else I am missing?

In the old world that test used to work because the CSP was on the principal and hence that inheritance model worked and the correct CSP was applied. Now that we are trying to move the CSP into the loadinfo we have to do CSP inheritance ourselves. For iframe loads that inheritance happens within docshell. Is there some other place (leaving workers aside) where we would have to do some explicit CSP inheritance? Like e.g in this particular case?

[1] https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/generic/policy-inherited-correctly-by-plznavigate.html#33
Flags: needinfo?(bzbarsky)
So a few things:

1)  This test is racy.  It doesn't wait for the srcdoc iframe to load before doing the write(), so it might be doing the write() on a srcdoc document or on the initial about:blank document, depending on the exact network timing and so forth.  That should probably fixed no matter what.

2)  The write() call, since it's not coming from a parser-inserted script (in either case) does a document.open.  How that interacts with CSP I thought was an open spec issue, but I can't find it right now.  That said, per the letter of the spec as written right now https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-document-open does not change the document's CSP list as defined at https://html.spec.whatwg.org/multipage/dom.html#concept-document-csp-list so should not affect the document's CSP.  As far as I can tell.

It looks like you are not actually storing the CSP on the document (instead it's stored on the channel?) so you are going to get mismatches wrt to spec any time we change the channel (or any time the spec changes out the CSP list, if it ever does that).  document.open certainly changes the channel.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #43)
> 1)  This test is racy.  It doesn't wait for the srcdoc iframe to load before
> doing the write(), so it might be doing the write() on a srcdoc document or
> on the initial about:blank document, depending on the exact network timing
> and so forth.  That should probably fixed no matter what.

FWIW, I filed bug 1466508 to get the race condition in the test fixed.
 
> 2)  The write() call, since it's not coming from a parser-inserted script
> (in either case) does a document.open.

Right, this was the part that I was missing - thanks. For now I fixed that by manually transferring the CSP from the old doc to the new document. Whether this is the absolutely right fix or not, I don't know. I just wanted to find all the places where potentially a manual transfer/inheritance of the CSP is necessary.

> It looks like you are not actually storing the CSP on the document (instead
> it's stored on the channel?)

Right, I am storing the CSP in the loadinfo of the channel of a document (modulo the worker bits where it works a little different). At this point I am not entirely convinced that the loadinfo is the right place. Nevertheless, every subresource stored it's loading document. So currently the CSP code for a subresource load, looks at the loadingDocument and queries the CSP from that channel.

I am the process of crafting a document which highlights the tradeoffs/pros/and also cons of the approach. I'll set up a meeting at the AllHands where I would like to discuss those tradeoffs.
Hey Ben,

I am having a hard time wrapping my head around ClientInfo. To avoid any unnecessary confusion I was wondering if you could affirm that my fundamental model about ClientInfo is correct. Leaving workers aside for now, every document should have a clientInfo, right? And the document's clientinfo should be the same as document->channel->loadInfo->clientinfo, right?

If my assumption is correct then I think we should be able to do the following:
* I assume we should be able to add the CSP to the ClientInfo within nsDocument.cpp [1] where we currently query the CSP from the header. Something like:
Maybe<ClientInfo> clientInfo = GetClientInfo();
if (clientInfo.isSome()) {
  clientInfo.ref().setCSP(csp);
}

* And within nsCSPService.cpp, where we actually perform the CSP check we do the inverse, we query the loadingDocument from the loadInfo an query the ClientInfo from that doc, right? Something like:

nsCOMPtr<nsINode> requestingNode = do_QueryInterface(loadinfo->requestContext);
if (requestingNode) {
  nsIDocument* ownerDoc = requestingNode->OwnerDoc();
  Maybe<mozilla::dom::ClientInfo> clientInfo = ownerDoc->GetClientInfo();
  if (clientInfo.isSome()) {
    nsIContentSecurityPolicy* csp = clientInfo.ref().getCSP();
  }
}

* CSP needs to be inherited, currently I am doing that in docshell when I create the new channel for the doc that is about to be loaded. Do we already have a clientInfo at that point? Or does the inheritance of CSP need to happen somewhere else now?

* Finally, can I add |nsCOMPtr<nsIContentSecurityPolicy> mCsp;| within the ClientInfo here [2] or is that the wrong approach?

* I think I am also a little confused because there is also GetInitialClientInfo() within loadinfo, when would I use that?

Thanks for your time and help!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2845-2983
[2] https://dxr.mozilla.org/mozilla-central/source/dom/clients/manager/ClientInfo.h#28
Flags: needinfo?(bkelly)
So, first, be aware ClientInfo is an inert snapshot structure representing the ClientSource.  So just keep in mind any changes to ClientInfo must also be applied to the ClientSource.

So your SetCSP() code is not correct.  Instead you need to:

1. Add a SetCSP() method on ClientSource
2. Make nsDocument call that SetCSP on the nsGlobalWindowInner's mClientSource
3. Make WorkerPrivate call SetCSP on its mClientSource

Then ClientInfo should probably have a read-only CSP getter method.

When performing your CSP check I think you should avoid using the requestingNode at all.  That won't be set for workers consistently.  Instead I would recommend something like this:

  Maybe<ClientInfo> clientInfo = loadInfo->GetClientInfo();
  if (clientInfo.isSome()) {
    nsIContentSecurityPolicy* csp = clientInfo.ref().getCSP();
  }

To your question about the different ClientInfo values on LoadInfo:

1. GetClientInfo() returns the ClientInfo for what you are calling the requestingNode.  Its represents the global that initiated the network load.
2. GetInitialClientInfo() should only return a value for document non-subresource requests (navigations) where there is an initial about:blank client.  The initial about:blank will be reused as the final Client for that navigation (unless its cross-origin).
3. GetReservedClientInfo() should only return a value document non-subresource requests without an initial about:blank *or* for any worker non-subresource requests (main script load).

So (1) is mainly concerned with subresource loads, although it can also be set for non-subresource loads if script does `self.location = foo`.  (2) and (3) are only set for non-subresource loads and represent the Client-to-be as a result of the result.

For the purposes of CSP I imagine you are mainly interested in GetClientInfo() since CSP is enforced on subresource requests, AIUI.

Does that help?
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [:bkelly] from comment #47)
> Does that help?

Yes, in fact that helps a lot - thanks!
Attached patch move_csp_clientinfo.patch (obsolete) — — Splinter Review 
Hey Ben,

thanks for your help. I did a bunch of tests and it seems it's working as expected. So far I only did the document parts (not the workers parts, and also not the meta csp parts).

Before I move on I wanted to quickly check if this is how it should work. Please note that the attached patch based on top of the patch where I move the CSP into the loadinfo, which we kind of need anyway because we can only attach the CSP to the ClientInfo within EnsureClientSource().

The main question I have. Do we have to deserialize the CSP every time we load a subresource load? That seems suboptimal to me, can we cache that somehow? If so, where and how would we do that? Within the patch you see that within nsCSPService I have to create a new CSP object feeding the CSP-Policy-String into that object.

Finally, I also have to create some kind of CSPInfo[] which we can add to IPCClientInfo, because a CSP contains of the string itself, whether it is a report only policy and whether it was delivered through a meta tag or not. I imagine this to be very similar to PrincipalInfo.

Other than that, does that patch match your mental model of how it should all work together or am I missing something?
Attachment #8987810 - Flags: feedback?(bkelly)
Comment on attachment 8987810 [details] [diff] [review]
move_csp_clientinfo.patch

Clearing the ni? - that all seems to work out - making really good progress on moving everything into the loadinfo - soon should have a patch that is ready to be sent to try.
Attachment #8987810 - Flags: feedback?(bkelly)
Summary: Move CSP from nsIPrincipal to nsILoadInfo → Move CSP from nsIPrincipal into the Client
Sorry, its been on my list to look at.

In regards to caching, I think the problem is where to cache it?  You could try caching it on ClientInfo, but that object has copy and move constructors to support passing between threads, etc.  Its not clear to me how useful a cache would be there.

Ideally the thread-safe, serializable form of CSP would be closer to the parsed output so its less expensive to access.  Or even ideally make the normal CSP type thread-safe and serializable itself.  There are plans to do that for nsIPrincipal which will remove the need for PrincipalInfo.

Personally I would not worry about caching the CSP value for now.  If it shows up in profiles then you can figure out how to address that particular use case.
Attached patch move_csp_clientinfo.patch (obsolete) — — Splinter Review 
In case someone is following that bug, I was able to write all the serialization code for CSP and also move CSP into the Client for regular document loads and worker loads (not working for any kind of meta csp yet).

The good news is, this all seems to work as expected and in the end is way cleaner than having it into the loadinfo itself, especially in the worker code.

(In reply to Ben Kelly [:bkelly] from comment #51)
> Sorry, its been on my list to look at.
> 
> In regards to caching, I think the problem is where to cache it?  You could
> try caching it on ClientInfo, but that object has copy and move constructors
> to support passing between threads, etc.  Its not clear to me how useful a
> cache would be there.

Yeah, I tried to cache it in the ClientInfo, but I get all sorts of errors when accessing that pointer. For this first version I gave up on any caching. Probably that string parsing is pretty fast anyway and I just worried too much.


Using the attached patch we are passing over 711 of our own CSP mochitests (27 are failing), where I am pretty sure all failing tests are related to meta CSPs which I am going to look at next week.
Attachment #8987810 - Attachment is obsolete: true
Comment on attachment 8988731 [details] [diff] [review]
move_csp_clientinfo.patch

In theory I think we got the right setup now. I guess it's time to fix corner cases:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d687caff4bc09b0024d6f2360ac5579ac97a7b82
Attachment #8988731 - Attachment is obsolete: true
Attached patch bug_965637_move_csp_to_client.patch (obsolete) — — Splinter Review 
Boris, Kris,

I am attaching a patch which moves the CSP from the Principal into the Client (also providing getters through the loadinfo and the document itstelf). Anyway, this approach seems to work just fine (though there are some minor problems that need to be resolved [2]).

The bigger problem I am facing however, are addons and in particular AddonPolicy(). I already asked that question before [2] but I thought I might get away without touching it, but apparently we have to update that part as well. Test [3] visualizes the problem. Now that the CSP does not live on the Principal anymore, BasePrincipal::OverridesCSP() basically becomes a no-op.

My suggestion is the following. Currently we have Client::GetCSP() and Client::GetPreloadCSP() - so probably we should also add Client::GetExtensionCSP() and within nsDocument::InitCSP() we just treat the addonPolicy slightly different and move it into the new client::ExtensionCSP(). That part should be fine, right?

Now, the next question is, how do we identify an Extension principal/document? Formerly, we used the Principal::AddonPolicy() to do that. Is there are better way of distinguishing? Based on that distinguishing factor we could then just apply whatever CSP (either page csp or extension csp) which 'hopefully' fixes the incorrectly appying CSP problem in a broader way.

What do you think?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1b3bb4481d5bbbf7cc4e2cb5830eaf1366552ff&selectedJob=188330776
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=965637#c39
[3] toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(bzbarsky)
Sorry for the lag; I wasn't actually familiar with the addonpolicy bits.

So OverridesCSP doesn't seem like a problem per se.  The problem is that we need access to the csp associated with the "thing doing the request", not just the node, right?

I'm not sure what the Client::ExtensionCSP proposal is really proposing.  There can be different CSPs for the different extensions that are touching a page, I would think.  Right now, we capture the principal of the extension in various cases (see the work tracked in bug 1267027 and its dependencies).  I guess we'd need to capture more information here?

Alternately, we could keep storing a CSP in expanded principals and use that in CSPService::ShouldLoad as needed?  I'm not sure what to do about the IsSystemPrincipal(requestPrincipal) case; if we want to do CSP for system bits, do we want to apply that CSP even if the system bits are messing with page content?  If we do, we need some way of getting to that info from the ShouldLoad code...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #62)
> So OverridesCSP doesn't seem like a problem per se.  The problem is that we
> need access to the csp associated with the "thing doing the request", not
> just the node, right?

Yes, that is correct. Probably we should just keep storing the AddonCSP on the principal for now and just move all the other CSP bits into the node.

> Alternately, we could keep storing a CSP in expanded principals and use that
> in CSPService::ShouldLoad as needed? 

That might work - I'll give that a try actually.

> I'm not sure what to do about the
> IsSystemPrincipal(requestPrincipal) case; if we want to do CSP for system
> bits, do we want to apply that CSP even if the system bits are messing with
> page content?  If we do, we need some way of getting to that info from the
> ShouldLoad code...

I guess we do actually. So we need to figure some way round that problem. Give me some time to think about that and run a few experiments. Apart from that problem we have a pretty green TRY run; the only 3 failing tests that are concerning are:
* toolkit/components/extensions/test/mochitest/test_ext_contentscript_about_blank.html
* toolkit/components/extensions/test/mochitest/test_ext_cookies.html
* toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js
which all touch the addonPolicy bits - so this is the last problem we have to overcome for this bug.

Thanks!
It seems the failing performance tests (see underneath) are failing because of Bug 1462879 hence marking Bug 1462879 as blocking this bug. Baku mentioned he might be able to land Bug 1462879 soonish.

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

and in particular:
* navigation-timing/nav2_test_document_open.html
* navigation-timing/test_timing_server_redirect.html
Depends on: 1462879
Boris, storing the CSP on ExpandedPrincipal seems like a plausible and actually quite good solution for me, at least for now. It still allows us to move the CSP to the Client in all the cases besides addons. The only remaining question is: When do we create a moz-extension principal?

It seems that in the majority of cases we only enter this if [1] where the principal is an ExpandedPrincipal, but we also enter that if-clause when the principal is a 'moz-extension://e386a224-9ba0-4d44-9368-00849c44cfbe/' principal. Is that something we need to worry about? Probably yes, but I don't know when we create a moz-extension principal and what we should do in that case.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2900-2908

Local testing seems promising, let's say if TRY feels the same way:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de222d00f3ed9fcf216f6fdfb722fca1df44c1a9
Flags: needinfo?(bzbarsky)
I don't actually know.  Hoping Kris does.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #67)
> I don't actually know.  Hoping Kris does.

While we wait, adding the CSP to the ExpandedPrincipal seems like the best path forward here. Since this patch is already pretty big, I think we should file a follow up bug to remove the SystemPrincipal case from BasePrincipal::OverridesCSP().

Once baku gets the updates to the performance timing in (see Bug 1462879) then this Bug should be ready for review (module the cleanup parts).

See latest try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa4871fb4df1500f0f12d5bb2ce1d725633b6f4
Flags: needinfo?(bzbarsky)
Comment on attachment 8992342 [details] [diff] [review]
bug_965637_move_csp_to_client.patch

Marking this patch as obsolete, for the purpose of review we have to split those patches up anyway.

Here is the latest TRY:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a58c16664bda88881ba6bf7b7d82555bbba2e0b
Attachment #8992342 - Attachment is obsolete: true
Hey Kmag,

this patch is almost ready for review (if needed find the latest patch using the treeherder link in comment 71). Anyway, there is one last test failing related to web extensions and I simply don't know why.

In particular, test_ext_contentscript_about_blank.html does not trigger the content script for about:blank [1]. I tried to trace the code and while the code currently in mozilla-central receives a call to Matches() [2] with a URL of about:blank I don't receive a call to Matches() [2] when my patches are applied.

Any idea how I could debug or what might potentially go wrong here with the content script URL matching?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_contentscript_about_blank.html#81
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/WebExtensionPolicy.cpp#600
Flags: needinfo?(kmaglione+bmo)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #72)
> In particular, test_ext_contentscript_about_blank.html does not trigger the
> content script for about:blank [1]. I tried to trace the code and while the
> code currently in mozilla-central receives a call to Matches() [2] with a
> URL of about:blank I don't receive a call to Matches() [2] when my patches
> are applied.

I eventually figured the problem. It was some code I added within nsDocshell which called GetDocument() which causes a new content viewer to be set up because GetDocument() internally calls EnsureContentViewer(). Thanks Anyway!
Flags: needinfo?(kmaglione+bmo)
Hey Baku, Boris, Kmag,

let me try to summarize the changes within this bug. Ultimately we want to move the CSP away from the Principal into the Client (within the LoadInfo). The only exception is the CSP for web extensions which (for now) we are going to keep on ExpandedPrincipals which allows us to keep current semantics of BasePrincipal::OverridesCSP(). I added helper functions within the LoadInfo to query the CSP which returns the CSP for that particular load, either from the Client repsonsible for the load, or the ExpandedPrincipal in case OverridesCSP returns true.

* Parsing the CSP and putting it into the Client:
When CSP is delivered through an http header then nsDocument::InitCSP() performs the task of getting the CSP off the response header and generating the object representation of CSP. We want to keep that part of generating the CSP object generation there, because e.g. if the CSP includes sandbox flags then we want to set up the document using the right flags. However, at this point the Client has not been created yet, hence we temporarily store the CSP on the document and propagate the CSP into the Client within nsGlobalWindowInner::EnsureClientSource.

* Serializing the CSP:
Current code in mozilla-central does not allow to serialize a CSP. Since we are moving the CSP into the Client however, we need a way of serializing the CSP. We have options to stringify CSP by calling the ::toString() methods. For deserialzing the CSP we just feed that string into the CSP Parser again. Further, to replicate the CSP correclty, we also need the requestPrincipal, selfURI, referrer, and innerWindowID - hence I added functionality for that.

* Special cases for propagating the CSP:
Not having the CSP on the principal anymore complicates a few corner cases where in the old world the principal inhertiance code basically took care of the CSP inheritance. That is not the case anymore in the new world and hence we have to write a little special code e.g. for about:blank loads and further for upgrade-insecure-requests. I tried to document that hacky part of the patch as good as possible, if we have better options, I am happy to hear and incorporate suggestions.

* Adding CSP to System Privileged Pages:
Ultimately we want to apply CSP to system privileged pages, which should be doable once the CSP lives on the Client. However, we need some special code, e.g. within OverridesCSP() and some other places within our codebase. This patch is *not* meant to incorporate that change and I added an assertion to make sure the Principal is never SystemPrincipal when parsing a CSP. Let's do the required work for that in a follow up bug.


I know it's quite a change and I am happy to provide answers inline or also hop on a vidyo call if needed!

Thanks,
  Christoph
Attached patch bug_965637_part_1_principal_changes.patch (obsolete) — — Splinter Review 
Please see notes about the patch within comment 75!
Attachment #9007726 - Flags: review?(kmaglione+bmo)
Attached patch bug_965637_part_2_move_csp_into_client.patch (obsolete) — — Splinter Review 
Please see notes about the patch within comment 75!
Attached patch bug_965637_part_3_workers.patch (obsolete) — — Splinter Review 
Please see notes about the patch within comment 75!
Attachment #9007728 - Flags: review?(amarchesini)
Attached patch bug_965637_part_4_addoncontentpolicy.patch (obsolete) — — Splinter Review 

        Attachment #9007730 -
        Flags: review?(kmaglione+bmo)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_5_test_updates.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Please see notes about the patch within comment 75!

    
    

    
  
Comment on attachment 9007728 [details] [diff] [review]
bug_965637_part_3_workers.patch

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

::: dom/serviceworkers/ServiceWorkerPrivate.cpp
@@ +1864,4 @@
>  #ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
> +   nsCOMPtr<nsIContentSecurityPolicy> csp;
> +   if (info.mChannel) {
> +     nsCOMPtr<nsILoadInfo> loadinfo = info.mChannel->GetLoadInfo();

Security check here?

::: dom/workers/ScriptLoader.cpp
@@ +1353,5 @@
>  
>        nsCOMPtr<nsIContentSecurityPolicy> csp;
> +      if (parentDoc) {
> +        csp = parentDoc->GetCSP();
> +      }

This is wrong. parentDoc can be null (in subworkers, for instance). In this case csp is null and the assertion below fails.

More important: why do you want to take the CSP From the parent doc? For subworkers, do you want to take it from the parent worker thread?

::: dom/workers/WorkerLoadInfo.cpp
@@ +288,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  if (CSP_ShouldChannelInheritCSP(aChannel)) {
> +    nsCOMPtr<nsILoadInfo> loadinfo = aChannel->GetLoadInfo();

return value check?

::: dom/workers/WorkerPrivate.cpp
@@ +2498,3 @@
>  {
> +  if (mClientSource && CSP_ShouldChannelInheritCSP(aChannel)) {
> +    nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();

if (!loadInfo) ?

@@ +2498,4 @@
>  {
> +  if (mClientSource && CSP_ShouldChannelInheritCSP(aChannel)) {
> +    nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
> +    nsCOMPtr<nsIContentSecurityPolicy> csp = nullptr;

no needs to have "= nullptr" here.

        Attachment #9007728 -
        Flags: review?(amarchesini) → review-

    
    

    
  
(In reply to Andrea Marchesini [:baku] from comment #81)
> > +     nsCOMPtr<nsILoadInfo> loadinfo = info.mChannel->GetLoadInfo();
> Security check here?

I am fine with adding a null check for loadinfo in those cases, but ultimately there should be no more channel within our codebase that does not have a loadinfo attached. We only added those checks since we were gradually moving towards having a loadinfo everywhere.
 
> ::: dom/workers/ScriptLoader.cpp
> @@ +1353,5 @@
> >  
> >        nsCOMPtr<nsIContentSecurityPolicy> csp;
> > +      if (parentDoc) {
> > +        csp = parentDoc->GetCSP();
> > +      }
> 
> This is wrong. parentDoc can be null (in subworkers, for instance). In this
> case csp is null and the assertion below fails.

This entire code block here lives behind MOZ_DIAGNOSTIC_ASSERT_ENABLED and I think (I am not sure) was added to make sure that there is no CSP at this point, hence the assertion checks !csp. Ultimately I just want to replicate whatever repsonsePrincipal->GetCSP() was and I thought responsePrincipal in that case should reflect the parentDocument. I am not entirely sure but maybe we can remove that entire assertion now that the csp is on the client anyway? What do you think?
Flags: needinfo?(amarchesini)

    
    

    
  
Comment on attachment 9007728 [details] [diff] [review]
bug_965637_part_3_workers.patch

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

Oh right!

        Attachment #9007728 -
        Flags: review- → review+
Flags: needinfo?(amarchesini)

    
    

    
  
Comment on attachment 9007727 [details] [diff] [review]
bug_965637_part_2_move_csp_into_client.patch

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

Please see notes about the patch within comment 75!

        Attachment #9007727 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 9007726 [details] [diff] [review]
bug_965637_part_1_principal_changes.patch

Please see notes about the patch within comment 75!

        Attachment #9007726 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 9007731 [details] [diff] [review]
bug_965637_part_5_test_updates.patch

Please see notes about the patch within comment 75!

        Attachment #9007731 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 9007730 [details] [diff] [review]
bug_965637_part_4_addoncontentpolicy.patch

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

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp
@@ +428,5 @@
>    RefPtr<BasePrincipal> principal =
>      BasePrincipal::CreateCodebasePrincipal(NS_ConvertUTF16toUTF8(url));
>  
> +  nsCOMPtr<nsIContentSecurityPolicy> csp =
> +    do_CreateInstance("@mozilla.org/cspcontext;1", &rv);

Can we construct this directly without the XPCOM gunk?

        Attachment #9007730 -
        Flags: review?(kmaglione+bmo) → review+

    
    

    
  
(In reply to Kris Maglione [:kmag] from comment #88)
> > +  nsCOMPtr<nsIContentSecurityPolicy> csp =
> > +    do_CreateInstance("@mozilla.org/cspcontext;1", &rv);
> 
> Can we construct this directly without the XPCOM gunk?

I don't think we can. Neither I think we should. What would be the advantage in this particular case?

    
    

    
  
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #89)
> (In reply to Kris Maglione [:kmag] from comment #88)
> > > +  nsCOMPtr<nsIContentSecurityPolicy> csp =
> > > +    do_CreateInstance("@mozilla.org/cspcontext;1", &rv);
> > 
> > Can we construct this directly without the XPCOM gunk?
> 
> I don't think we can. Neither I think we should. What would be the advantage
> in this particular case?

The same as in any other case. The XPCOM createInstance logic is expensive and error prone, and we should move away from it wherever possible. Concrete constructors give us compile-time error checking, and are much more efficient at runtime.

    
    

    
  
(In reply to Kris Maglione [:kmag] from comment #91)
> The same as in any other case. The XPCOM createInstance logic is expensive
> and error prone, and we should move away from it wherever possible. Concrete
> constructors give us compile-time error checking, and are much more
> efficient at runtime.

Ok, I'll check the code and update the createinstance here and possibly in other cases where possible.
QA Contact: ckerschb
Blocks: 1391994

    
    

    
  
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #96)
> Rebased Patches, making sure we still got a green TRY:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c7006bf8a0af37600979e4340c839359f64877b2

It seems there has been a merge conflict in the workers file, this TRY run should look much better:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d3588043a818426819b68fb43a37553340f1701

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_1_principal_changes.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Putting up rebased patches - please see comment 75 for summary of the changes performed within this bug.

        Attachment #9007726 -
        Attachment is obsolete: true

        Attachment #9007726 -
        Flags: review?(kmaglione+bmo)

        Attachment #9007726 -
        Flags: review?(bzbarsky)

        Attachment #9021484 -
        Flags: review?(kmaglione+bmo)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_2_move_csp_into_client.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Putting up rebased patches - please see comment 75 for summary of the changes performed within this bug.

        Attachment #9007727 -
        Attachment is obsolete: true

        Attachment #9007727 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_5_test_updates.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Putting up rebased patches - please see comment 75 for summary of the changes performed within this bug.

        Attachment #9007731 -
        Attachment is obsolete: true

        Attachment #9007731 -
        Flags: review?(bzbarsky)
Flags: needinfo?(bzbarsky)

    
    

    
  
Comment on attachment 9021484 [details] [diff] [review]
bug_965637_part_1_principal_changes.patch

It worries me a bit to change the serialization of principals, but hopefully it'll be ok...

>+++ b/caps/ExpandedPrincipal.cpp
>+ExpandedPrincipal::GetCSP()
>+  nsCOMPtr<nsIContentSecurityPolicy> csp = mCSP;
>+  return csp.forget();

  return do_AddRef(mCSP);

r=me.  I assume this will get folded with the later patches, so we don't have non-compiling changesets in the tree...

        Attachment #9021484 -
        Flags: review+

    
    

    
  
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #102)
> >+++ b/caps/ExpandedPrincipal.cpp
> >+  return csp.forget();
> 
>   return do_AddRef(mCSP);

Sure - done!
 
> r=me.  I assume this will get folded with the later patches, so we don't
> have non-compiling changesets in the tree...

Yes, I will fold everything together into one patch. My idea was to get some parts factored out into different patches so we don't have to look at them anymore, because the overall size of those patches is quite massive.

    
    

    
  
Comment on attachment 9021485 [details] [diff] [review]
bug_965637_part_2_move_csp_into_client.patch

Overall architecture question:  Why do we need the new CSPInfo representation and the back-and-forth serializing and reparsing it entails?

>+++ b/caps/nsScriptSecurityManager.cpp
> nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext *cx,
>+    // Query the associated document with that context

How about:

  // Get the document, if any, corresponding to the current global

>+    JSObject* global = JS::CurrentGlobalOrNull(cx);
>+    nsGlobalWindowInner* win = xpc::WindowGlobalOrNull(global);

   nsGlobalWindowInner* win = xpc::CurrentWindowOrNull(cx);

>+    nsIDocument* doc = win->GetDocument();
>+    if (!doc) {
>+      return true;

Hmm.  If the window is partially torn down and has had its document nulled out, do we really want to return true here?  There might have been a CSP on that document...

I think what we should do is have getter on nsGlobalWindowInner for the CSP and implement it similarly to nsGlobalWindowInner::GetPrincipal: forward to mDoc if we have one, else use the value we snapshot in nsGlobalWindowInner::FreeInnerObjects.

>+++ b/docshell/base/nsDocShell.cpp
>+      // Hack: Manually copy the CSP for the new about:blank document since
>+      // about:blank inherits the CSP but this about:blank load does not call
>+      // nsDocument::StartDocumentLoad() where we usually do CSP inheritance!

This looks dangerous to me.  For example, in the toplevel case, there are situations where we CreateAboutBlankContentViewer() but do _not_ want to pick up the opener's principal/CSP, if any.  We handle that for principal by making it an explicit argument to this function.  I feel like we should do the same with CSP.  It will require looking at the callers and deciding what CSP they all want, but at that point we will know that we're using the CSP that really matches the "who is responsible for this about:blank?" information.

>@@ -10467,38 +10500,59 @@ nsDocShell::DoURILoad(nsIURI* aURI,
>+  // Let's do CSP inheritance here before InitCSP() is called, because
>+  // if mLoadedAsData is true, then InitCSP() is not called, but data: URIs
>+  // need to inherit the CSP.

I don't understand this comment.  mLoadedAsData has nothing to do with data: URIs.  It's about "data documents" like document clones, document objects created via DOMImplementation, documents created via XHR responseXML or DOMParser, etc.   Documents that are not "in a browsing context" in spec terms.

I would expect that things that have mLoadedAsData true do not need a CSP and should not force creation of one.  So all this logic could live in InitCSP, I suspect, and probably should.

>+  if (aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT) {
>+    nsCOMPtr<nsPIDOMWindowOuter> curWin = mScriptGlobal->AsOuter();
>+    if (curWin) {
>+      nsCOMPtr<mozIDOMWindowProxy> targetOpener = curWin->GetOpener();

Say page A without CSP window.open()s page B, which has CSP with upgrade-insecure-requests.  Page B then navigates to page C.  I believe in the old world we'd upgrade the navigation to C, but now we won't.  Is my understanding of the behaviors correct?  If so, is this a desired behavior change?

>+    // for frame replacements we can still query the current document which holds
>+    // that bit of information.
>+    if (mContentViewer) {
>+      openerDoc = mContentViewer->GetDocument();
>+    }

For the initial load in a frame, mContentViewer could be null.  Shouldn't we still do the upgrade if the parent has the upgrade-insecure-requests CSP?

>+++ b/docshell/base/nsDocShell.h
>+public:
>   already_AddRefed<nsDocShell> GetParentDocshell();

I don't think we'll want this change once we fix the bits in nsDocument::StartDocumentLoad.

>+++ b/dom/base/nsDocument.cpp
>@@ -2783,29 +2784,76 @@ nsDocument::StartDocumentLoad(const char
>+        nsCOMPtr<mozIDOMWindowProxy> targetOpener = curWin->GetOpener();

This looks wrong to me.  If page A window.open()s page B, which then navigates to data: URI C, then C should pick up the CSP of B, not A.  This code will pick up the CSP of A, as far as i can tell.

I would have thought we would get the "CSP to inherit" from the channel (via the loadinfo and client bits hanging off it) here, not look for it via opener/parent relationships.

>+    csp = do_CreateInstance("@mozilla.org/cspcontext;1", &rv);

Kris is right.  This file already includes mozilla/dom/nsCSPContext.h, so we can just |new nsCSPContext| instead of going through XPCOM bits.  Is there a reason to not do that?

For that matter, we should strongly consider a followup to replace all uses of nsIContentSecurityPolicy  in C++ with nsCSPContext, and expose nicer APIs on it.

>+nsIDocument::GetCSP()
>+  mozilla::Maybe<mozilla::dom::ClientInfo> clientInfo = GetClientInfo();

Why is that "mozilla::" there?

>+  mozilla::ipc::OptionalCSPInfo cspInfo = clientInfo.ref().GetCspInfo();

And here.

>+  nsCOMPtr<nsIContentSecurityPolicy> csp = CSPInfoToCSP(cspInfo, this);

This creates a new nsCSPContext every time this function is called, right?  Why is this desirable?  Why can't we cache the nsCSPContext in the Client or the document after it's created?  As far as I can tell CSPInfoToCSP involves reparsing all the CSP stuff every time it's called, which doesn't seem fast.

It's also not clear to me how this function works if called before we've moved mTmpCSP to the client.  This function isn't using mTmpCSP, right?

>+nsIDocument::ApplySettingsFromCSP(bool aSpeculative, bool aFromMetaTag)
>+    // 1) apply settings from regular CSP, but not that the Client

s/not/note/?

>+    // is not ready when we call ApplySettingsFromCSP for a CSP delivered
>+    // through the http header, hence we *have* to use the tmp CSP stored
>+    // on the document in that case.

I'm being a bit confused about this.  My understanding was that the whole point of using Client here is that it's a thing that exists all along (on the channel to start with).  So it would be available at all points.  Is that not the case?

>+  nsCOMPtr<nsIContentSecurityPolicy> preloadCsp = nullptr;

No need for the "= nullptr;" bit.

>+  Maybe<mozilla::dom::ClientInfo> clientInfo = GetClientInfo();

No need for "mozilla::dom::".

>+    mozilla::ipc::OptionalCSPInfo preloadCSPInfo = clientInfo.ref().GetPreloadCspInfo();

No need for "mozilla::".

>+    if (preloadCSPInfo.type() != mozilla::ipc::OptionalCSPInfo::Tvoid_t) {

No need for "mozilla::".

>@@ -2938,29 +3021,32 @@ nsIDocument::InitCSP(nsIChannel* aChanne
>+    nsCOMPtr<nsIExpandedPrincipal> ep = do_QueryInterface(principal);

Is there a reason to not use GetIsExpandedPrincipal()?

Or better yet, do something more like this:

  auto* basePrin = BasePrincipal::Cast(principal);
  if (basePrin->Is<ExpandedPrincipal>()) {
    basePrin->As<ExpandedPrincipal>()->SetCSP(csp);
  }

or so.

>@@ -4910,20 +4995,18 @@ nsIDocument::SetScriptGlobalObject(nsISc
>+  if (mTmpCSP) {

We're guaranteed that this happens before we transfer mTmpCSP to the Client?   Or does that not matter?

>+    static_cast<nsCSPContext*>(mTmpCSP.get())->flushConsoleMessages();

See, this is why mTmpCSP should be a RefPtr<nsCSPContext>.  ;)  Then you could just:

   mTmpCSP->flushConsoleMessages();

>+nsIDocument::GetCspJSON(nsString& outCSPinJSON)

File style is more like aCSPinJSON for the outparam.  But really, I'd call it "aJSON".

>+  Maybe<ClientInfo> clientInfo = GetClientInfo();

This seems to be reimplementing all of GetCSP.  Why?

>+++ b/dom/base/nsGlobalWindowInner.cpp
>@@ -1881,16 +1882,22 @@ nsGlobalWindowInner::EnsureClientSource(
>+    nsCOMPtr<nsIContentSecurityPolicy> csp = mDoc->GetTmpCSP();
>+    mClientSource->SetCsp(csp);

So we're exposing a getter for mTmpCSP on document just so we can do this part, right?

It might be better to have a method on document that takes a ClientSource and sets up a CSP on it.

I think it would be pretty helpful to have some documentation (on mTmpCSP, or on the client source's CSP member, with other places pointing to it) that describes the "when does the CSP live where?" setup and how the various storage locations interact.  That includes header-delivered CSP, <meta>-delivered CSP, normal vs preload CSP, etc.

>+++ b/dom/base/nsGlobalWindowInner.h
>+  void SetCsp(nsIContentSecurityPolicy* aCsp);
>+  void SetPreloadCsp(nsIContentSecurityPolicy* aPreloadCsp);

Need documentation.  Possibly by reference to the above overall documentation.

>+++ b/dom/base/nsIDocument.h
>+   * Returns the CSP for that document

I have a hard time reconciling this claim with the fact that GetSCP ignores mTmpCSP, even in situations where that's the "CSP for the documeent", no?

>+   * The CSP of a document lives in the Client, but the Client
>+   * is not available at the time of CSP parsing.

I thought the whole point of using a client was that it's available all along.

>+   * Hands off! This method should only be called from within
>+   * CreateAboutBlankContentViewer

Might be good to enforce that by making it private and making CreateAboutBlankContentViewer a friend, in addition to having a comment here about not calling this from other document code?

>+  void ApplySettingsFromCSP(bool aSpeculative, bool aFromMetaTag);

Would be good to document the args.

>+++ b/dom/base/nsPIDOMWindow.h
>+  void SetCsp(nsIContentSecurityPolicy* aCsp);
>+  void SetPreloadCsp(nsIContentSecurityPolicy* aPreloadCsp);

Should point to the lifetime documentation.

Though really, we may want a getter for Client on nsIDocument or something....

>+++ b/dom/base/nsStyleLinkElement.cpp
>+    if (!nsStyleUtil::CSPAllowsInlineStyle(thisContent->AsElement(), doc,

Why do we need the extra arg?  doc == thisContent->OwnerDoc() here, so the callee can just get it from  the element.

Same thing for the other callsites.

>+++ b/dom/clients/manager/ClientIPCTypes.ipdlh
>+  OptionalCSPInfo cspInfo;
>+  OptionalCSPInfo preloadCspInfo;

Please document, possibly by reference to the overall lifetime doc.

>+++ b/dom/clients/manager/ClientInfo.cpp
>+                                    mozilla::dom::FrameType::None,
>+                                    mozilla::void_t(), mozilla::void_t()))

All this code is in the mozilla::dom namespace.  So I don't see why the prefixing is needed.

I guess FrameType needs to be disambiguated because ClientInfo has a FrameType method, so it needs to be written as "dom::FrameType".  But none of the "mozilla::" bits should be here.

>+const mozilla::ipc::OptionalCSPInfo&

No "mozilla::".

>+ClientInfo::SetCspInfo(const mozilla::ipc::CSPInfo& aCSPInfo)

And here.

>+const mozilla::ipc::OptionalCSPInfo&

And here.

>+ClientInfo::SetPreloadCspInfo(const mozilla::ipc::CSPInfo& aPreloadCSPInfo)

And here.

>+++ b/dom/clients/manager/ClientInfo.h
>+  const mozilla::ipc::OptionalCSPInfo& GetCspInfo() const;
>+  void SetCspInfo(const mozilla::ipc::CSPInfo& aCSPInfo);
>+
>+  const mozilla::ipc::OptionalCSPInfo& GetPreloadCspInfo() const;
>+  void SetPreloadCspInfo(const mozilla::ipc::CSPInfo& aPreloadCSPInfo);

And all of these: no "mozilla::".

>+++ b/dom/clients/manager/ClientSource.h
>+  void SetCsp(nsIContentSecurityPolicy* aCsp);
>+  void SetPreloadCsp(nsIContentSecurityPolicy* aPreloadCSP);

These need to be documented.  In particular, the fact that they copy the passed-in CSP instead of referencing it is entirely non-obvious.

It also seems to me like the copying could be done more efficiently than via serializing (and then deserializing whenever you want to use it), which is what the setup here is, right?

>+++ b/dom/html/HTMLMetaElement.cpp
>+        csp = do_CreateInstance("@mozilla.org/cspcontext;1");

As above, we shouldn't be going through XPCOM goop here.  Especially if we want to assume that we didn't get null out (e.g. because the XPCOM registration failed)...

>+      // overwrite the request context, otherwise 'self' does not translate correctly
>+      csp->SetRequestContextWithDoc(aDocument);

We only need this in the case when we just created it, right?

>+      nsPIDOMWindowInner* inner = aDocument->GetInnerWindow();
>+      if (inner) {
>+        inner->SetCsp(csp);
>       }

The asymmetry here between getting the CSP from the document and setting it on the window is odd.  Why are the getter and setter not on the same object?

>+++ b/dom/html/nsHTMLDocument.cpp
>+  // document.open inherits the CSP from the parent document

It does?  What spec says that where?  I just looked a bit and found nothing about this.  Per spec,  document.open seems to not change the CSP of the target page at all, afaict.

In any case, the code is not inheriting from the "parent" document....

>+  nsCOMPtr<nsIContentSecurityPolicy> csp = callerDoc->GetCSP();
>+  if (csp) {
>+    mTmpCSP = csp;
>+  }

This should document why setting mTmpCSP is the right thing to do and why not doing anything when callerDoc->GetCSP() returns null is correct, even if we already have a non-null mTmpCSP.   Which seems dubious.  But again, I can't tell what behavior we're really aiming for here, so it's hard to tell.

>+++ b/dom/interfaces/security/nsIContentSecurityPolicy.idl
>+  void setRequestContextWithDoc(in Document aDocument);

setRequestContextWithDocument would be no longer than setRequestContextWithPrincipal... WithDoc is probably OK, but we could go either way.

>+  [noscript] readonly attribute nsIPrincipal requestPrincipal;
>+  [noscript] readonly attribute nsIURI selfURI;
>+  [noscript] readonly attribute AString referrer;
>+  [noscript] readonly attribute unsigned long long innerWindowID;

These should probably be [notxpcom,nostdcall] too, as long as we're here.  Definitely for requestPrincipal, selfURI, and innerWindowID.   I'm not sure what signature we would generate for referrer in the [notxpcom] case.

>+++ b/dom/jsurl/nsJSProtocolHandler.cpp
>+    // for document navigations we need to check the CSP of the original document.

Which original document?   Which part of which spec is this implementing?  We've already run the script,  so doing a script-src check for 'inline' at this point is rather odd.

>+++ b/dom/script/ScriptLoader.cpp
>+  csp->GetAllowsInline(nsIContentPolicy::TYPE_SCRIPT,

This would benefit greatly from working with nsCSPContext and having a C++-friendly getter.  For one thing, it would be obvious why ignoring the success-or-failure bit is OK.

Followup is OK for this.

>+++ b/dom/security/FramingChecker.cpp
>+ShouldIgnoreFrameOptions(nsIChannel* aChannel, nsIContentSecurityPolicy* aCSP)
>+  NS_ENSURE_TRUE(aCSP, false);

Isn't "no CSP" the common case here?  Why do we want that case to warn?

I suspect we don't want the NS_ENSURE_* bit here.

>+++ b/dom/security/nsCSPContext.cpp
>+nsCSPContext::SetRequestContextWithPrincipal(nsIPrincipal* aRequestPrincipal,
>+  // sending messages to the browser conolse instead of the web console in that case.

Preexisting, but s/conolse/console/

>+++ b/dom/security/nsCSPParser.cpp
>+#ifdef Debug

Should be DEBUG?  I don't think we ever define "Debug".

>+      return nullptr;

This return is debug-only.  Changing control flow like that based on debug or not seems odd.  I expect this is not doing what this code meant to do.

>+++ b/dom/security/nsCSPService.cpp
>+    rv = aLoadInfo->GetPreloadCsp(getter_AddRefs(preloadCsp));

Again, should be [notxpcom, nostdcall], since it never actually fails.

>@@ -283,16 +265,25 @@ CSPService::AsyncOnChannelRedirect(nsICh
>+  if (XRE_IsE10sParentProcess() && triggeringPrincipal->GetIsCodebasePrincipal()) {
>+    // Please note that for redirects using a CodebasePrincipal we already enforce CSP
>+    // in the child. The parent process does not have access to the node and hence we
>+    // can't query the nonce which causes us to add that early return here.

s/that early return/the early return/?

That said, why was this not needed before?  Commit message should explain.

That said, is the nonce really supposed to be examined at redirect time, not snapshotted at load start time?   That is, if we start the load, then change the nonce, is the CSP check during redirects supposed to pick up the new nonce??  https://fetch.spec.whatwg.org/#concept-request-nonce-metadata makes me think the nonce is captured at request start time, so we should not need an element to do a nonce check.   And in particular, we should be able to do this enforcement completely in the parent process.  Please file a followup bug on this.  I bet there is no test coverage for this.  :(

>+++ b/dom/security/nsCSPUtils.cpp
>+bool CSP_ShouldChannelInheritCSP(nsIChannel *aChannel)

I don't know why we're prefixing instead of namespacing, but I guess that's file style....

>+  nsAutoCString spec;
>+  channelURI->GetSpec(spec);
>+  bool isSrcDoc = spec.EqualsLiteral("about:srcdoc");
>+  bool isAboutBlank = spec.EqualsLiteral("about:blank");

What about "about:blank#foo" or "about:blank?aaa"?  What is the actual standard here?  This function could really benefit from a link to the standard.

I _think_ the relevant standard is https://w3c.github.io/webappsec-csp/#initialize-document-csp step 1, which is a much looser check than what's being done here.

The standard doesn't have anything for javacript: there, but presumably that's handled somewhere else in  the specs?  Would be good to link to that too.

>+  if (!isData && !isBlob && !isJS && !isSrcDoc && !isAboutBlank) {
>+    return false;
>+  }
>+  return true;

Why not:

  return isData || isBlob || ....

>+++ b/dom/security/nsCSPUtils.h
>+bool CSP_ShouldChannelInheritCSP(nsIChannel *aChannel);

This is really "ShouldResponseInheritCSP", right?  Would be good to clearly document what actual question this predicate is answering (possibly with a spec link).

>+++ b/dom/webidl/Document.webidl
>+  readonly attribute DOMString cspJSON;

This is being exposed to the web.  That does not seem desirable.

It looks like this is only used in tests, right?  If so, we should probably expose it only if IsInAutomation and some pref is set, set the pref on the relevant tests, and have a test without the pref set that checks this is not exposed.

Alternately, you could make this ChromeOnly  and access it only via SpecialPowers or something.

But the upshot should be that this is not exposed to the web.

>+++ b/ipc/glue/BackgroundUtils.cpp
>+CSPInfoToCSP(const CSPInfo& aCSPInfo,
>+CSPToCSPInfo(nsIContentSecurityPolicy* aCSP, CSPInfo* aCSPInfo)

I did not review these carefully for now, because it's not clear to me whether they will even remain in their current form.  For example, the argument to CSPToCSPInfo may become an nsCSPContext based on the  other comments above.

It's also not completely clear to me that we need this serialize-and-reparse thing at all, per above.

>+++ b/layout/style/nsStyleUtil.cpp
>+    nsCOMPtr<nsIExpandedPrincipal> ep = do_QueryInterface(aTriggeringPrincipal);

As above, we should be able to directly ask the BasePrincipal for this information, with no QI.

>-    if (aRv)
>-      *aRv = rv;

At this point aRv is always set to NS_OK.  Please file a followup to just remove it.

>+++ b/netwerk/base/LoadInfo.cpp
>+    nsCOMPtr<nsIContentSecurityPolicy> csp;
>+    GetCsp(getter_AddRefs(csp));

Why are we using XPCOM-style getters for internal code?  Especially infallible getters?

We should have an internal getter that returns already_AddRefed<nsIContentSecurityPolicy> or nsIContentSecurityPolicy* or some equivalent with nsCSPContext, then build the xpcom getter on top of  that.

>+LoadInfo::GetCsp(nsIContentSecurityPolicy** aCsp)
>+    nsCOMPtr<nsIExpandedPrincipal> ep = do_QueryInterface(mTriggeringPrincipal);

Again, let's do this with BasePrincipal and without QI.

>+      nsCOMPtr<nsIContentSecurityPolicy> addonCSP =
>+      static_cast<ExpandedPrincipal*>(ep.get())->GetCSP();

Please fix the indent.

>+      NS_IF_ADDREF(*aCsp = addonCSP);

  addonCSP.forget(aCsp);

In general, NS_IF_ADDREF and NS_ADDREF should be avoided.

>+  if (mClientInfo.isNothing()) {
>+    return NS_OK;

This returns NS_OK but never sets *aCsp, so now the caller has a random-valued pointer.

This problem will go away when this is rewritten to not be all XPCOM-y per above. :)

I didn't read the rest of the details carefully so far, because I'm not convinced on the serialization/reparsing bits, per above.

>+LoadInfo::GetPreloadCsp(nsIContentSecurityPolicy** aPreloadCsp)

All the same issues apply here.

>+++ b/netwerk/base/nsILoadInfo.idl
>+   * A helper function that returns the CSP associated with this load. 

This needs to be spelled out a lot more.  Is this the CSP of the thing that started the load, the CSP of the document the load is happening in, the CSP of the response, or something else?

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

If these are noscript and infallible, why do we have XPCOM signatures at all?  These should be [notxpcom, nostdcall] and look like normal C++ methods returning useful types.

>+++ b/parser/html/nsHtml5TreeOpExecutor.cpp
>+  nsCOMPtr<nsIContentSecurityPolicy> preloadCsp =
>+    do_CreateInstance("@mozilla.org/cspcontext;1", &rv);

Should create nsCSPContext directly, imo.
Flags: needinfo?(bzbarsky)

        Attachment #9021485 -
        Flags: review-

    
    

    
  
Comment on attachment 9021486 [details] [diff] [review]
bug_965637_part_5_test_updates.patch

Review for these will need to happen after the cspJSON exposure bits are fixed.

    
    

    
  
Hey Yaron,

within this bug we are trying to move the CSP from the Principal into the Client (within the Loadinfo) which better reflects the security context of either a document or a worker. In order to do so we made some adjustments to the worker code [1], e.g. please take a look at code like |mClientSource->SetCsp(aCSP);| within WorkerPrivate.cpp.

Now, after the changes of Bug 1504638 that is not doable anymore because that code happened on the main thread but |MOZ_ACCESS_THREAD_BOUND(mWorkerThreadAccessible, data);| basically asserts the opposite.

After discussing things with Baku we thought we could move the CSP related code into ScriptExecutorRunnable::PreRun() and within there do something like aWorkerPrivate->SetCSPOnClient(csp);. Now that works for accessing mClientSource, but now we are facing a different problem - namely that the serialization code which we added to BackgroundUtils.cpp, namely CSPToCSPInfo [2] (but also the Principal serialization code within BackgroundUtils.cpp) assert |MOZ_ASSERT(NS_IsMainThread());|. So it seems we are spinning in circles, kind of.

Anyway, at this point it's unclear to me what the best path forward here is and which code should be remoted and where. Since you recently rewrote a lot of those things within Bug 1504638 I was wondering if you have any insights/suggestions on how to move forward! Thanks for your help!

[1] https://bugzilla.mozilla.org/attachment.cgi?id=9007728&action=diff
[2] https://bugzilla.mozilla.org/attachment.cgi?id=9021485&action=diff
Flags: needinfo?(ytausky)

    
    

    
  
Let me preface by saying that I don't yet fully understand the underlying object model, so please do point out any wrong assumptions I might make.

If I understand correctly, it seems like serialization code is somewhat coupled with the worker domain logic, i.e. ClientSource is calling CSPToCSPInfo, which is tied to a specific thread. From a brief look it looks like ClientSource::SetSCP could be made to accept a CSPInfo instead of an nsIContentSecurityPolicy, so storing a CSPInfo in a WorkerRunnable and setting it on the worker thread seems like a viable solution. Is there a reason not to do that?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_3_workers.patch (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to Yaron Tausky [:ytausky] from comment #107)
> If I understand correctly, it seems like serialization code is somewhat
> coupled with the worker domain logic, i.e. ClientSource is calling
> CSPToCSPInfo, which is tied to a specific thread. From a brief look it looks
> like ClientSource::SetSCP could be made to accept a CSPInfo instead of an
> nsIContentSecurityPolicy

Right, this was also what baku and I had in mind. Just wanted to make sure we are not missing anything - thanks for your answer and thanks for adding the thread boundaris - I like it a lot!

@Baku: As discussed, we store not only a CSP but also a CSPInfo in the workerLoadInfo so we can do the serialization of the CSP on the main thread. Later within ScriptExecutorRunnable::PreRun() we move the CSPInfo into the Client (now that we are on the worker thread).

In the end I like that solution more than the previous version we had. Can you please sign off on this version again?

Final question: I don't necessarily like that we have to use nsAutoPtr to store the CSPInfo within WorkerLoadInfo.h, but it seems we can't include |#include "mozilla/ipc/PBackgroundSharedTypes.h"| within the .h file but only in the .cpp file hence using the autoPtr so we can use the forward declaration. Can you think of anything better? I am little worried that we might leak memory.

        Attachment #9007728 -
        Attachment is obsolete: true
Flags: needinfo?(ytausky)

        Attachment #9026013 -
        Flags: review?(amarchesini)

    
    

    
  
Comment on attachment 9026013 [details] [diff] [review]
bug_965637_part_3_workers.patch

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

::: dom/clients/manager/ClientSource.cpp
@@ +756,5 @@
>  }
>  
>  void
> +ClientSource::SetCspInfo(const CSPInfo& aCSPInfo)
> +{

MOZ_ASSERT_OWNINGHTREAD(ClientSource);

same for SetCSP.

@@ +765,2 @@
>  ClientSource::SetPreloadCsp(nsIContentSecurityPolicy* aPreloadCsp)
>  {

here as well

::: dom/clients/manager/ClientSource.h
@@ +180,5 @@
>  
>    void SetCsp(nsIContentSecurityPolicy* aCsp);
>    void SetPreloadCsp(nsIContentSecurityPolicy* aPreloadCSP);
>  
> +  // Please use SetCsp is possible! This helper only supports

if possible :)

@@ +182,5 @@
>    void SetPreloadCsp(nsIContentSecurityPolicy* aPreloadCSP);
>  
> +  // Please use SetCsp is possible! This helper only supports
> +  // thread boundaries for workers!
> +  void SetCspInfo(const mozilla::ipc::CSPInfo& aCSPInfo);

I think this comment is wrong. We should incorauge people to use SetCSPInfo if they have a CSPInfo.
The most important thing is to call this (and the other method) on the owning thread.

        Attachment #9026013 -
        Flags: review?(amarchesini) → review+

    
    

    
  
(In reply to Andrea Marchesini [:baku] from comment #109)
> I think this comment is wrong. We should incorauge people to use SetCSPInfo
> if they have a CSPInfo.
> The most important thing is to call this (and the other method) on the
> owning thread.

OK - updated. So I think the most important thing here is do add (which I just did):
  NS_ASSERT_OWNINGTHREAD(ClientSource);

Thanks!

    
    

    
  
It seems the changes within Bug 1438945 cause us to do some refactoring for the worker patches within this bug, hence marking Bug 1438945 as blocking this bug.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_6_shared_worker_updates.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Hey Baku, as discussed over slack, it seems that Bug 1438945 propagated a lot of redundant CSP information. E.g. the workers do not need to know about preloadCSP. The workerLoadInfo (as in the other worker patch within this bug) does not even hold a preloadCSP member.

Anyway, just looking at this patch it seems that the ClientInfo makes things so much easier and nicer when propagating CSP info.

Anything I missed?

        Attachment #9026325 -
        Flags: review?(amarchesini)

        Attachment #9026325 -
        Flags: review?(amarchesini) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_3_workers.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Uploading a rebased version of the worker changes. Please note that I qfolded the shared worker related changes into this patch.

        Attachment #9026013 -
        Attachment is obsolete: true

        Attachment #9026325 -
        Attachment is obsolete: true

        Attachment #9027442 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_2_move_csp_into_client.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Boris, sorry this took a while but in the meantime some worker related changes landed on mozilla-central (workers are now thread bound (Bug 1504638) and also the RemoteWorker code landed (Bug 1438945)). However, in the end that allows us to incorporate some additonal assertions to make sure we access the client on the correct thread - so I guess that's a win.

Anyway, here is a summary of the changes since the last version (also with regards to our meeting the other day):

* Q: Why do we need the new CSPInfo representation and the back-and-forth serializing and reparsing it entails?
ClientInfo itself does not hold any pointers - only data, hence we need to serialize CSP data because we send it across processes.

* Q: Why not store the CSP in the Client all along?
Unfortunately the Client is not available within nsDocument::StartDocumentLoad which is when we need to parse the CSP. Unfortunately we can also not move the CSP parsing code, because if CSP contains sandbox flags, then we need to set up the document using those sandbox flags at this point. Hence we have to store/cache the CSP on the doucment and propagate the CSP into the Client within nsGlobalWindowInner::EnsureClientSource.

* Caching the CSP on the document
As discussed, we now cache the CSP on the document which we can query to avoid constant serializaton/deserialization of the CSP. I added assertions in the loadinfo to make sure the CSP of document matches the CSP within the client whenever we access it.

* Querying the CSP from within ContentSecurityPolicyPermitsJSAction
I added a helper function nsGlobalWindowInner::GetCsp. As suggested I added a snapshot variable mDocumentCsp which gets updated within FreeInnerObjects(). I hope this is what you meant.

* CSP inheritance function in nsCSPUtils
I updated the implementation of ShouldResponseInheritCSP() to reflect the specification more closely and further added a link to the spec. Please note that the spec was unclear regarding javascript:, hence I filed https://github.com/w3c/webappsec-csp/issues/368 to get that bit resolved.

* Inheriting the CSP for about:blank
As suggested, I extended CreateAboutBlankContentViewer with another argument - aCSP. To set that up correclty I also had to extend SetInitialPrincipalToSubject with a CSP argument so we can query and pass the CSP from the previous window into the new window.

* Exposing cspJSON within the Document
As suggested, I maded that ChromeOnly so only test code using SpecialPowers can access.

* Regarding the mozilla::ipc:: namespace. It seems we can't remove the mozilla:: from nsDocument, compiler says it's ambigious, because of mozilla::ipc:: or mozilla::dom::ipc::


Finally I have two questions of things I have not incorporated yet:
a) You mentioned you would like to have nsILoadInfo.idl use [noscript,notxpcom,nostdcall] for csp which would generate a function signature of |nsIContentSecurityPolicy* LoadInfo::GetCSP()|, but the problem is that in case we have to query and deserialize the CSP from the client, we generate an nsCOMPtr. Returning the raw pointer would mean that the GC cleans up that csp at any point. I see two options:
* We keep what we have and keep doing the getter_AddRefs(csp)-dance everywhere, or
* we return an already addrefed pointer, doing something like the following in the nsILoadInfo.idl
%{C++
 already_AddRefed<nsICSP> GetCSP();
%}


b) I haven't incporated the changes for the actual CSP inheritance as of now (the code around nsDocument::StartDocumentLoad()) because I don't know of a better way of doing that. Your suggestions in the review imply you have an idea, but at the moment I think using |GetParentDocshell| is the best option we have - happy to hear suggestions and incorporate those changes.

Feel free to ping me on slack if there is anything I can be helpful with!

Thanks,
  Christoph

        Attachment #9021485 -
        Attachment is obsolete: true

    
    

    
  
> but the problem is that in case we have to query and deserialize the CSP from the client, we generate an nsCOMPtr

OK.  And we don't cache it, right?

One option is to do something like:

  native CSPRef(already_AddRefed<nsIContentSecurityPolicy>);

or

  native CSPRef(already_AddRefed<nsCSPContext>);

at the top of the idl file, and then you can do:

  [notxpcom,nostdcall] CSPRef GetCSP();

You don't want to mess with "%{C++" blocks if you can avoid it; too easy to add virtual methods inside them and cause footguns.

    
    

    
  
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #115)
> > but the problem is that in case we have to query and deserialize the CSP from the client, we generate an nsCOMPtr
> 
> OK.  And we don't cache it, right?

Right, we don't cache the deserialized CSP, because it would be only used once (well maybe a few times if the channel gets redirected). So I guess it doesn't really make sense to cache it only for that.

Given that I think your suggestion makes sense to me - I'll incorporate that change which will clean up the C++ code quite a bit!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_2_move_csp_into_client.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Hey Boris,

as discussed on Slack I am providing a new iteration of the patch which already has incorporated the suggestions for question (a) from comment 114 - C++ friendly getters for the CSP within the Loadinfo. Please see comment 114 for a general overview of the patch, but here are some additional details:

* CSPAllowsInlineStyle() got extended by an additonal argument 'aDocument', and while I agree that one could query the Document from the first argument 'aElement', that first argument is not always passed, hence I guess we need the additonal 'aDocument' argument.

* The remaining question I have is (as already put in comment 114 (b)) what is the best way to inherit/query the CSP from the previous document. Those bits need to be updated within nsDocshell.cpp where we query the CSP needed for upgrade-insecure-requests and also within nsDocument.cpp where we need to inherit the CSP into the new Document. Once that is resolved I guess we can also make GetParentDocshell() within nsDocshell.h private again.

* I'll wait with putting up a new patch for tests updates till we have resolved all the issues within this main patch here.

Thanks,
  Christoph

        Attachment #9027479 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #117)
> * CSPAllowsInlineStyle() got extended by an additonal argument 'aDocument',
> and while I agree that one could query the Document from the first argument
> 'aElement', that first argument is not always passed, hence I guess we need
> the additonal 'aDocument' argument.

FWIF, I filed Bug 1510652 to simplify and update CSPAllowsInlineStyle. It seems we can pass aElement in all the cases (at least from a first glance) and do further optimizations. Let's not pollute that bug with that but do the that cleanup in the follow up I filed.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_2_move_csp_into_client.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Boris,

I am uploading a rebased version for feedback (f?) now that the tree has been reformatted. For details about the patch please see:
* comment 114
* comment 117, and
* comment 118

Thanks!

        Attachment #9028277 -
        Attachment is obsolete: true

    
    

    
  
> I haven't incporated the changes for the actual CSP inheritance as of now (the code around nsDocument::StartDocumentLoad()) because I don't know of a better way of doing that.

I thought the whole point of passing the CSP around in the loadinfo was so we could inherit it here.  So why are we not getting the CSP from the loadinfo?  The CSP in the loadinfo is a clone of the CSP of the document that started the load, right?

Sorry for the lag on this; I had missed the question about this above.  :(

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_965637_part_2_move_csp_into_client.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Boris, I updated the inheritance bits within nsDocument::InitCSP() the way I think it should work. For iframe loads you are right, we have the CSP in the loadinfo of the channel and can simply set it to the newly created document. When creating a new window however, we have to query the CSP from the previous document.

Anyway, I think overall we are pretty close to get this out the door. Obviously there are a few things that could potentially be simplified. What do you think?

        Attachment #9028943 -
        Attachment is obsolete: true

        Attachment #9029199 -
        Flags: feedback?(bzbarsky)

    
    

    
  
Comment on attachment 9029199 [details] [diff] [review]
bug_965637_part_2_move_csp_into_client.patch

It would have been really useful to have an interdiff here...

If all the scripted callers are just going to pass null for the second arg of createAboutBlankContentViewer(), why not just make that arg optional?

>+++ b/caps/nsScriptSecurityManager.cpp
>+  // Get the document, if any, corresponding to the current global

This code is not working with documents at all....  Please fix the comment.

>+++ b/docshell/base/nsDocShell.cpp

As we discussed at the all-hands, the logic in nsDocShell::DoURILoad needs fixing.

>+++ b/dom/base/nsContentPolicy.cpp

In nsContentPolicy::CheckPolicy I don't understand the new code.  "doc" will only be set if requestingLocation is null.  That only happens in the system principal case, I expect.  That means the EnsureEventTarget code is generally not reached with this patch, which seems wrong.  Do we have tests for this code?  (This was a problem with the previous patch iteration too, apparently....)

>+++ b/dom/base/nsDocument.cpp
>+nsIContentSecurityPolicy* nsIDocument::GetCsp() {

Any reason this isn't just in the header?  Similar for the setter and the preload CSP bits.

> nsresult nsIDocument::InitCSP(nsIChannel* aChannel) {

Again, as discussed at the all-hands this needs fixing.

Should we assert !mCSP on entry into this function?  We certainly seem to be assuming it...

>+  nsresult myrv = CSPToCSPInfo(mCSP, &cspInfo);

OK, so if we're not inheriting this will leave cspInfo empty, right?

>+  mCSP = CSPInfoToCSP(cspInfo, this);

And this will create an empty CSP?  This is preferable to having mCSP null when there is no CSP?

>+  if (!mCSP) {

This can only happen if CSPInfoToCSP fails, right?  Do we think that the do_CreateInstance/SetRequestContextWithDocument will succeed even in that case?

>+    mCSP->SetRequestContextWithDocument(this);

Why are we ignoring the return value?

>+void nsIDocument::GetCspJSON(nsString& aJSON)
>+  dom::CSPPolicies jsonPolicies;

Why do we need this declared outside the !mCSP block?

>+++ b/dom/base/nsGlobalWindowInner.cpp
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocumentCsp)

Document did not traverse/unlink its mCSP.  Should it have?

>+++ b/dom/base/nsGlobalWindowInner.h

Could use more documentation here.

>+++ b/dom/base/nsIDocument.h
>+  // initiated subresource loads we can use that cached version of the CSP so we do not

"that cached version" means "the document's cached version", right?

I didn't re-read every single line for the moment, because I assume you're mostly looking for feedback on the caching setup, right?  That part looks good to me.

        Attachment #9029199 -
        Flags: feedback?(bzbarsky) → feedback+

    
    

    
  
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #122)
> I didn't re-read every single line for the moment, because I assume you're
> mostly looking for feedback on the caching setup, right?  That part looks
> good to me.

Yes, mostly was looking for feedback on the caching mechanism. Next step is to get Bug 1513241 out of the way so we can clean up the docshell bits. Once that dependency has cleared we can come back to this bug. Thanks for your feedback!

    
    

    
  
Hmm.  So as far as the caching on the loadinfo, I have a few questions, actually.

1) The first "if" in LoadInfo::AssertCachedCSPAndClientCSPAreEqual seems like it should be an assert, not a test.

2) Given that mClientInfo stores a snapshot but the document stores a mutable thing (due to <meta>), I don't see how AssertCachedCSPAndClientCSPAreEqual can hold in general.

    
    

    
  
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #124)
> Hmm.  So as far as the caching on the loadinfo, I have a few questions,
> actually.
> 
> 1) The first "if" in LoadInfo::AssertCachedCSPAndClientCSPAreEqual seems
> like it should be an assert, not a test.

Agreed, since we only cache the CSP if there is a document and actually only call AssertCachedCSPAndClientCSPAreEqual if there is a doc. To sum it up, we can replace the early return within AssertCachedCSPAndClientCSPAreEqual with the following assertion:

+ MOZ_ASSERT(doc && mClientInfo->Type() == ClientType::Window, "...");


> 2) Given that mClientInfo stores a snapshot but the document stores a
> mutable thing (due to <meta>), I don't see how
> AssertCachedCSPAndClientCSPAreEqual can hold in general.

Well, the clientInfo gets updated with every <meta csp>, see the last few lines within HTMLMetaElement.cpp, in particular:

+  nsPIDOMWindowInner* inner = aDocument->GetInnerWindow();
+  if (inner) {
+    inner->SetCsp(csp);
+  }

Further, nsGlobalWindowInner::SetCsp() updates the client and further stores a cached version on the document.
In other words, the clientinfo does not hold a snapshot that is immutable - so that assertion should hold true, or are we facing a more conceptual problem that I am are not aware of?
Flags: needinfo?(bzbarsky)

    
    

    
  
> Well, the clientInfo gets updated with every <meta csp>

Which ClientInfo, though?  We update the one on the window.  But the ClientInfo on the LoadInfo is a snapshot of the window's ClientInfo as of the moment the LoadInfo was created, as far as I can tell.  After that point, the two ClientInfos can have different CSPs.

Or in other words, the sequence of events I'm thinking about here is:

1) Load starts.
2) Document CSP is changed by adding a <meta>
3) Load gets a redirect response, goes to check its CSP.

We should be able to add a test for this...
Flags: needinfo?(bzbarsky)

    
    

    
  
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #126)
> > Well, the clientInfo gets updated with every <meta csp>
> 
> Which ClientInfo, though?  We update the one on the window.  But the
> ClientInfo on the LoadInfo is a snapshot of the window's ClientInfo as of
> the moment the LoadInfo was created, as far as I can tell.  After that
> point, the two ClientInfos can have different CSPs.
> 
> Or in other words, the sequence of events I'm thinking about here is:
> 
> 1) Load starts.
> 2) Document CSP is changed by adding a <meta>
> 3) Load gets a redirect response, goes to check its CSP.
> 
> We should be able to add a test for this...

Ok, so this part definitely needs some tests - I suppose we can just modify test_meta_header_dual.html [1] by a redirect case.

[1] https://searchfox.org/mozilla-central/source/dom/security/test/csp/test_meta_header_dual.html
Blocks: 1228985

    
    

    
  
OK, so I rebased all those patches, fixed remaining wpt-tests and it seems we have a green TRY run now:

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


I still need to do some code cleanups but then this should be ready for review.



    
    

    
  
This is ready for review (finally:

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


I'll remove all splinter patches and upload new phab patches.



        Attachment #9007730 -
        Attachment is obsolete: true

        Attachment #9021484 -
        Attachment is obsolete: true

        Attachment #9021484 -
        Flags: review?(kmaglione+bmo)

        Attachment #9021486 -
        Attachment is obsolete: true

        Attachment #9027442 -
        Attachment is obsolete: true

        Attachment #9029199 -
        Attachment is obsolete: true

    
    

    
  
Hey Andrew,


thanks for taking on the work and helping review the backend changes within that bug! Let me provide a quick summary which hopefully makes the changes digestible for you.


Let's start with the major benefits:


    

  • Currently the CSP hangs off the Principal and since the SystemPrincipal is a singleton instance which is shared for all system privileged pages it does not allow us to secure chrome privileged pages with a custom CSP for each of those about pages.
    • 

  • Moving the CSP away from the Principal is a requirement to make nsIPrincipal objects threadsafe.
    • 

  • We are expecting a performance boost since we do not need to serialize and deserialize a CSP whenever serializing a Principal through IPC.
    • 

  • The Client better maps a security context than a Principal (at least in terms of CSP execution context).
    • 



Let's move on to the actual changes within this bug:


    

  • 

    Overall idea
    
Ultimately we want to move the CSP away from the Principal into the Client (within the LoadInfo). The only exception is the CSP for web extensions which (for now) we are going to keep on ExpandedPrincipals which allows us to keep current semantics of BasePrincipal::OverridesCSP(). I added helper functions within the LoadInfo to query the CSP which returns the CSP for that particular load, either from the Client responsible for the load, or the ExpandedPrincipal in case OverridesCSP returns true. Please note that we keep a cached copy of the CSP on the current document, because the Client itself only holds a serialized version of the CSP. To avoid unnecessary deserialization we simply check if we can query the CSP from the document, if not, then we have to take the slow patch and deserialize the CSP. Since the LoadInfo represents the source of truth in terms of security parameters for every network load, the method LoadInfo::GetCSP() encapsulates all of that functionality.
    

    • 

  • 

    Parsing the CSP and putting it into the Client:
    
When CSP is delivered through an http header then nsDocument::InitCSP() performs the task of getting the CSP off the response header and generating the object representation of CSP. We want to keep that part of generating the CSP object generation there, because e.g. if the CSP includes sandbox flags then we want to set up the document using the right flags. However, at this point the Client has not been created yet, hence we only store the CSP on the document and propagate/sync the CSP with the Client whenever it is generated, which happens within nsGlobalWindowInner::EnsureClientSource.
    

    • 

  • 

    Inheriting the CSP
    
In case the CSP needs to be inherited (e.g. a data: URI iframe) the docshell is the entry point into the new document load. We added a member variable to docshell which only is non null in case the CSP needs to be inherited. Within Document::InitCSP() we then query the CSP that needs to be inherited and move it over to the new document. Not having the CSP on the principal anymore complicates a few corner cases where in the old world the principal inhertiance code basically took care of the CSP inheritance. That is not the case anymore in the new world and hence we have to write a little special code e.g. for about:blank loads where we manually have to propagate the CSP because such loads do not go through ::LoadURI.
    

    • 

  • 

    Serializing the CSP:
    
We have options to stringify CSP by calling the ::toString() methods. For deserialzing the CSP we just feed that string into the CSP Parser again. Further, to replicate the CSP correctly, we also need the requestPrincipal, selfURI, referrer, and innerWindowID - hence I added functionality for that.
    

    • 

  • 

    Adding CSP to System Privileged Pages:
    
As mentioned previously, we want to apply CSP to system privileged pages, which should be doable once the CSP lives on the Client. However, we need some special code, e.g. within OverridesCSP() and some other places within our codebase. This patch is not meant to incorporate that change and I added assertions to make sure the Principal is never SystemPrincipal when parsing a CSP. All of that work which allows to apply a CSP to system privileged loads will happen later in Bug 1496418.
    

    • 



I know it's quite a change and I am happy to provide more insights in our video call next week, but I hope this summary provides a basic overview of what we are trying to accomplish!


Thanks,

Christoph



    
    

    
  


We added a member variable to docshell




Christoph, why do we need this?  Generally speaking, a docshell does not correspond to a single load.  Furthermore, in the fission world, the docshell where the load starts may not even be the thing the new document is loaded into.


I was figuring the "CSP to inherit" would come from the loadinfo, which is attached to the channel and therefore is available at the point when we set up the document from the channel.  Does that not work for some reason?


Flags: needinfo?(ckerschb)

    
    

    
  
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #135)




Christoph, why do we need this?  Generally speaking, a docshell does not correspond to a single load.  Furthermore, in the fusion world, the docshell where the load starts may not even be the thing the new document is loaded into.




Boris, this was more of a semantic choice - let me explain:


a) For iframe loads the member variable cspToInherit on the docshell is in fact not needed at all, because we could simply query the CSP to inherit from the embedding document within document::initCSP().


b) For new window loads we explicitly pass the CSP of the 'triggering' document to ::LoadURI(). If the CSP needs to be inherited then I thought it makes sense to temporarily store the CSP which needs to be inherited on the docshell and then pass it on to the document within document::InitCSP(). It felt semantically right to do that (at least to me). Now that you bring that up we could already store the CSP which needs to be inherited on the "new" document channel/loadinfo we create within docshell so that the channel for the new document already holds the new CSP. I guess we need to add clear and more documentation that there is already a CSP within the loadinfo/client at the time we call document::InitCSP(), but not a big deal in the end.


I assume that is what you are suggesting, right? If that is preferred I am happy to incorporate that change - should be a quick fix.


Flags: needinfo?(ckerschb) → needinfo?(bzbarsky)

    
    

    
  


because we could simply query the CSP to inherit from the embedding document within document::initCSP()




No, for several reasons:


    

  1. With fission the embedding document can be in a different process and hence not reachable.
    2. 

  2. This is not right semantically.  The CSP to inherit comes from the request's client in https://w3c.github.io/webappsec-csp/#initialize-document-csp.  For a load in an iframe the client may NOT be the embedding document.  A simple example is if the load in the iframe is done via setting its location.href.  In that case the client is whatever global the code that did the href set is running in, which may not be the embedding document at all.
    3. 



Please make sure there are tests for this that would have caught that bug.




I assume that is what you are suggesting, right?




I am suggesting that we should generally do what the spec says.  The spec stores the CSP on the load, effectively (though it gets it from the load's client dynamically instead of storing a copy; I thought there was a known spec issue filed on this, but I'm not seeing one; I will get one filed).  That's what we should do.


By the way, I strongly suggest testing CSP inheritance across different-process toplevel loads if that's not already tested; you should be able to do that right now with the "noopener" bits for window.open.


Flags: needinfo?(bzbarsky)
Type: defect → task

        Attachment #9058525 -
        Attachment description: Bug 965637: Move CSP from Principal into Client, part 4: test updates. r=bz → Bug 965637: Move CSP from Principal into Client, part 4: test updates. r=mccr8

        Attachment #9058522 -
        Attachment description: Bug 965637: Move CSP from Principal into Client, part 1: backend changes. r=bz → Bug 965637: Move CSP from Principal into Client, part 1: backend changes. r=mccr8
Blocks: 1547271
No longer depends on: 1547271

    
    

    
  
Mike,


within this bug we are going to move the CSP from the Principal into the Client. In turn that causes some changes to the current serialization format of: (a) the Principal, and (b) the ContentSecurityPolicy:


Ad (a) ContentPrincipal::Read() and Write() [see part 1 of the changesets attached]

For now we are going to continue to write NS_WriteOptionalCompoundObject() with a 'nullptr' as the CSP, so that the format itself does not change - only the CSP itself is not serialized to disk anymore. On the way back we read that information but do not consume it. FWIW, the actual CSP is serialized separately within nsISHEntry (we started writing and explicit CSP into the history entry within Bug 1518454).


Ad (b) nsCSPContext::Read() and Write() [see part 1 of the changesets attached]

We start serializing a Principal within the CSP.


To make sure we are not missing anything I wanted to check with you the following things:

(1) If someone would update to the latest Firefox version, serialize something to disc and then revert to an older Firefox version we would be in trouble, because for (a) we would not have a CSP to read back in the Principal and (b) the serialized Principal within the CSP would corrupt the deserialization of the CSP.


(2) If we would have to back out that changeset, we would face the same problems. This second problem can be somewhat mitigated by the fact that we are planning to land this changes next week (or at least as early in the new cycle as possible) and rather fix problems than backing changes out.


FWIW, :jkt is working on a better serialization format for Principals within Bug 1508939 which would help to mitigate the problems outlined in (a) but not in (b).


Can we take the risks outlined or is that a no-go?


Flags: needinfo?(mdeboer)

        Attachment #9058525 -
        Attachment description: Bug 965637: Move CSP from Principal into Client, part 4: test updates. r=mccr8 → Bug 965637: Move CSP from Principal into Client, part 4: test updates. r=mccr8,jkt

    
    

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




Can we take the risks outlined or is that a no-go?




FWIW, :jkt and I worked out a testing plan that provides some confidence we are not breaking principal serialization for the two cases outlined within comment 139. We added:


    

  • browser/base/content/test/caps/browser_principalSerialization_version1.js
    • 

  • browser/base/content/test/caps/browser_principalSerialization_csp.js
    • 



where the latter uses a hardcoded serialized principal that includes a CSP and ensures that the remaining bits can be read back correctly (even after this bug) so that we have a deserialized principal that is identical to the input modulo the CSP bits. Given that that works and people would just 'loose' the CSP, that seems like a good compromise to me in case people are messing with Firefox versions.



    
    

    
  
Hi Chrisptoph, thanks for reaching out!


So if I read comment 140 correctly, this means that current serialized sessions with v1 principals will be deserialized properly when v2 lands, right? So that's the basic thing that should continue to work at the very least.

That first session running v2 will be serialized to disk using v2 principals, thus subsequent restores will work (this is kinda obvious, but still).

The interesting bit, usually, is indeed backward compat. So can sessions serialized using v2 be restored using v1 - a previous Firefox version? If not, that'd be problematic, since we try to be non-destructive in this case at the very least. The way we usually solve that is to introduce a new property name, like triggeringPrincipal_base64v2, which will be ignored by older clients and thus be treated as there being no principal data available for that tab. I believe that will not break things as badly, right?


But if read comment 140 correctly, we don't even need to worry about the latter scenario, because v2 already takes care of it transparently - which is fantastic :-)


Flags: needinfo?(mdeboer)

    
    

    
  
(In reply to Mike de Boer [:mikedeboer] from comment #141)




The interesting bit, usually, is indeed backward compat. So can sessions serialized using v2 be restored using v1 - a previous Firefox version? If not, that'd be problematic, since we try to be non-destructive in this case at the very least. The way we usually solve that is to introduce a new property name, like triggeringPrincipal_base64v2, which will be ignored by older clients and thus be treated as there being no principal data available for that tab. I believe that will not break things as badly, right?




To clarify, are you suggesting if I install Firefox 69 it should be serializing both formats in case I load stable 67? Or simply that the key shouldn't collide?


So are you expecting the history entry to look something like:


  entries: [{
    url: "https://example.com",
    title: "my title",
    triggeringPrincipal_base64: Base64Principal,
    triggeringPrincipal_base64v2: Base64Encode({"1":{"0":"https://example.com","2":"^privatebrowsing=2"}}),
  }



Or like:


  entries: [{
    url: "https://example.com",
    title: "my title",
    triggeringPrincipal_base64v2: Base64Encode({"1":{"0":"https://example.com","2":"^privatebrowsing=2"}}),
  }



I would prefer if we didn't have to store that duplication.


triggeringPrincipal_base64 currently is used for both as I handle the JSON and downgrade if it's not JSON in the deserialization. The legacy code in older browsers simply won't be able to deserialize either and will effectively not have a principal either.


My current plan in Bug 1508939 was to be able to continue implementing Read for v1 format such that older profiles can be migrates and then write all new principals with v2.


Flags: needinfo?(mdeboer)
Depends on: 1550414

    
    

    
  
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/ddf4012a7652
Move CSP from Principal into Client, part 1: backend changes. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/3399e7c51942
Move CSP from Principal into Client, part 2: worker changes. r=baku
https://hg.mozilla.org/integration/autoland/rev/63587b402c35
Move CSP from Principal into Client, part 3: frontend changes. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/c5554e93087a
Move CSP from Principal into Client, part 4: test updates. r=mccr8,jkt

    
    

    
  
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16944 for changes under testing/web-platform/tests
Whiteboard: [domsecurity-active] → [domsecurity-active][wptsync upstream]

    
    

    
  
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Regressions: 1553440
Regressions: 1461642
Regressions: 1553742
Regressions: 1553652

    
    

    
  
Christoph can you please take a look at the regressions linked here?


Flags: needinfo?(ckerschb)

    
    

    
  
(In reply to Andreea Pavel [:apavel] from comment #148)




Christoph can you please take a look at the regressions linked here?




Yes, me, Jonathan and Basti are on it. Clearing this ni? but keeping others on individual bugs.


Flags: needinfo?(ckerschb)
Regressions: 1555043
Regressions: 1555050
Flags: needinfo?(mdeboer)
Regressions: CVE-2019-17001
Regressions: CVE-2019-17020

    
  
Regressions: 1582115

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: