[meta] Call Content Policies after a channel is created

NEW
Unassigned

Status

()

defect
5 years ago
6 months ago

People

(Reporter: tanvi, Unassigned)

Tracking

(Depends on 3 bugs, Blocks 3 bugs, {addon-compat, meta})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Reporter

Description

5 years ago
We would like to call content policies (and other security checks like checksameorigin) after a channel has been created instead of before (this way we can take redirects into account, among other things).  In order to do this, we need some information about who initiated a given load and what type of load it is.  Namely, the principal and content-type.

Because of the way content policies are written today, they depend on the context and get the principal from the context.  Hence, it is debatable whether we want to store the RequestingContext or the RequestingPrincipal in the channel, or both.  

We need to store this information to the channel (similar work is going on in bug 965413).  We will pass this info into NS_NewChannel and then set it on the channel.

We will attempt to move the content policy call to AsyncOpen() and see how it goes.

I have some WIP patches that I will post to this bug a bit later.
Reporter

Comment 1

5 years ago
Note that the channelPolicy is a parameter to NS_NewChannel and contains the content type.  Can we rely on channelPolicy?  Is it always set?  Was channelPolicy created for CSP redirects?  If that is the only consumer, we can remove channelPolicy as part of this bug and instead store the content type directly on the channel.  Will need to talk to Sid more about this to get some background info on channelPolicy.
I think we should definitely store the requesting principal.

As for context, I think avoiding storing the window is a good thing because that avoids the whole inner/outer window mess. Instead I'd recommend storing either the requesting node (in cases like <img> and <link rel=stylesheet>), or the requesting document (in cases like API calls like XHR and sendBeacon).
(In reply to Jonas Sicking (:sicking) from comment #2)
> I think we should definitely store the requesting principal.
> 
> As for context, I think avoiding storing the window is a good thing because
> that avoids the whole inner/outer window mess. Instead I'd recommend storing
> either the requesting node (in cases like <img> and <link rel=stylesheet>),
> or the requesting document (in cases like API calls like XHR and sendBeacon).

For CSP, we need to be able to query the attribute 'nonce' from any nsIDOMHTMLElement.
Currently we perform the following two queries to get that information from aRequestContext in ShouldLoad:

> nsCOMPtr<nsIDOMHTMLDocument> doc = do_QueryInterface(aRequestContext);
and further
> nsCOMPtr<nsIDOMHTMLElement> htmlElement = do_QueryInterface(aRequestContext);

To sum it up, I think what Jonas recommends should work and we can store the requesting document along with the requesting principal.
Reporter

Comment 4

5 years ago
For now, I am passing both the context and the principal so we don't have to change the individual content policies right now.  Once we have a proof of concept working, we can remove the context and update each content policy.
If we pass in a node/document, we can still grab the context window from that and pass the context window to nsIContentPolicy.
Reporter

Comment 6

5 years ago
Here is a work in progress patch.  It does the following so far:

* Creates AsyncOpen2(), which calls AsyncOpen().  Call AsyncOpen2 from nsURILoader::OpenURI() (the only call site so far).  Call content policies in AsyncOpen2 (note this is a duplicate call because we have not removed the original call to the content policies).

* Creates NS_NewChannel2(), which calls NS_NewChannel().  Pass in content-type, context, and principal to NS_NewChannel2.  Right now, nsIURILoader:DoURILoad() is the only call site for NS_NewChannel2().

* Takes Christoph's patch for Bug 418354 and tweaks it a bit -
Content Type, RequestingContext and RequestingPrincipal all become members of the channel.
Attempt to set Content Type and RequestingContext on http channels.  But RequestingPrincipal has not been set anywhere except in nsDocShell::DoURILoad.  Note that in many cases they are set after a call to NS_NewChannel.  This will change and instead we will pass it into NS_NewChannel2 and let NS_NewChannel set it on the channel.  This will happen one call site at a time.

Note that the patch is full of comments/notes/prints and will need a lot of cleaning up.  Right now it is just a wip / proof of concept.

Jonas/Christoph - this consolidates the patches I sent you earlier today, fixes the bug I mentioned, as well as some other followups we discussed.

Next steps -
* Change a few other call sites to NS_NewChannel2 and appropriately pass in the context/principal/content-type.
* Instead of passing in the principal and the context to NS_NewChannel2, try passing in the node and getting the context and principal from it.
Reporter

Comment 7

5 years ago
Change calls in nsScriptLoader.cpp to AsyncOpen2 and NS_NewChannel2.  Remove calls to content policies from nsScriptLoader.  Note I could only remove one call (the call at preload time).  If I remove the second call, content policies would only get called once instead of twice (bug 839235).  We need to resolve whether or not we need to call them twice.
Reporter

Comment 8

5 years ago
Instead of passing in both the requestingPrincipal and an nsISupports requestingContext to NS_NewChannel2 and setting them both on the channel, we should just pass in an nsINode.  The nsINode would be used to extract the principal.  And is more specific than nsISupports.

However, NS_CheckContentLoadPolicy currently takes the requestingContext as an nsISupports, so it may not be an nsINode.  It might be an nsIDOMWindow.
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentPolicy.cpp#86
https://bugzilla.mozilla.org/show_bug.cgi?id=782654#c92

For NS_NewChannel2, we can just start specifying that the context is a node instead of allowing callers to pass in an nsIDOMwindow.  In the future, we might be able to change shouldLoad so it takes an nsINode instead of an nsISupports.  But we will have to check for compatibility and whether or not an addon passes in an nsIDOMWindow from someone else.  (Can we add an assert to nsContentPolicy::CheckPolicy to see if we ever get an nsIDOMWindow?)

Looking at the 6 content policies we have, CSP is the only one that assumes requestingContext is an nsINode.
My thinking for what to do is something like this:

Always pass a node and a "load type" (img/script/etc) to NS_NewChannel2 if we can get to any context at all. Make the implementation of AsyncOpen2 check the "load type" and then grab an element, a window, a document, or whatever as the nsIContentPolicy "context".

This way we can pass the exact same context *to nsIContentPolicy* as we do today. I.e. we can pass a window where we are currently passing a window, or pass an element where we are currently passing an element.

But other callers that nsIContentPolicy could always rely on having a node available on the channel, rather than sometimes a node, sometimes a window etc.
The reason I think it's better that we to nsIContentPolicy keep sending what we're currently sending is that trying to "clean up" nsIContentPolicy is going to be more work than it's worth given that we're deprecating it. Any time we change what context we pass to nsIContentPolicy we likely will break some addons that rely on the current weird behavior.

Let's ask them to move to nsINetworkObserver rather than ask them to adjust to nsIContentPolicy cleanup.
Reporter

Comment 11

5 years ago
(In reply to Jonas Sicking (:sicking) from comment #10)
> The reason I think it's better that we to nsIContentPolicy keep sending what
> we're currently sending is that trying to "clean up" nsIContentPolicy is
> going to be more work than it's worth given that we're deprecating it. Any
> time we change what context we pass to nsIContentPolicy we likely will break
> some addons that rely on the current weird behavior.
> 
> Let's ask them to move to nsINetworkObserver rather than ask them to adjust
> to nsIContentPolicy cleanup.

You are right, we shouldn't worry about changing the Content Policy API.  Anyway, it is going to go away and the 6 individual content policies we have today will fire during the appropriate observer notification.  addons that use content policy will have to rewrite their code to use the observers as well.  So we don't have to worry about whether requestingContext is a node or a nsIDOMWindow.
Well.. We need to keep compatibility with existing addons and existing use of nsIContentPolicy for some time going forward. We should certainly stop using it ourselves, but can't expect that addons will do the same very quickly.

So sadly we will still need to care about sending the "right" context.
In case someone is tracking progress, we push our patches to:
https://github.com/ckerschb/gecko-dev/tree/revampGeckoSecHook
Having changed all the callsites that create a channel through nsNetUtil.h (also calling contentPolicies within AsyncOpen), I think it's worth posting what we have experienced so far and what direction we are going forwards:
Jonas initially proposed the following 2-APIs [see also 1]:

1) Using a Principal
   mozilla::net::NewChannel(
     nsIURI* uri,                    // required
     nsIPrincipal* loadingPrincipal, // required
     nsILoadGroup* loadgroup,        // optional for now (should be required for non-system-principal)
     uint32_t loadFlags,             // required
     eLoadType loadType,             // required (image/stylesheet/xhr/beacon/etc)
     uint32_t securityFlags,         // required (same-origin-only, cors-with-credentials,
                                                  cors-without-credentials, cross-origin,
                                                  allow javascript:, allow chrome:,
                                                  allow-inherit-principal, suppress prompts)
     nsIChannel** result);

2) Using a loadingNode (Useful for DOM code. Principal, loadgroup and referrer is grabbed from loadingNode0
   mozilla::net::NewChannel(
    nsIURI* uri,                   // required
    nsINode* loadingNode,          // required (for API-based loads this is the document)
    uint32_t loadFlags,            // required
    eLoadType loadType,            // required
    uint32_t securityFlags,        // required
    nsIChannel** result);

From what we have experienced in changing callsites (by following guidelines from the proposed API), I think it makes the most sense if we end up having the following two functions available to create channels:

1) Using the *SYSTEMPRINCIPAL*:
  inline nsresult
  NS_NewChannel(nsIURI*              aURI,
                nsIPrincipal*        aSystemPrincipal,
                uint32_t             aSecurityFlags,
                nsContentPolicyType  aContentPolicyType,
                uint32_t             aLoadFlags,
                nsIChannel**         outChannel);

2) Using the requestingNode
  inline nsresult
  NS_NewChannel(nsIURI*              aURI,
                nsINode*             aRequestingNode,
                uint32_t             aSecurityFlags,
                nsContentPolicyType  aContentPolicyType,
                uint32_t             aLoadFlags,
                nsIChannel**         outChannel);

The next steps going forward are:
  * add      uint32_t               aSecurityFlags
  * delete   nsIIOService*          aIoService
  * delete   nsILoadGroup*          aLoadGroup (should be null if we use systemPrincipal, otherwise
                                                we can get it from the aRequestingNode)
  * delete   nsIInterfaceRequestor* aCallbacks
  * delete   nsIChannelPolicy*      aChannelPolicy (all CSP needs is aContentPolicyType and aPrincipal
                                                    which is stored in the channel now anyway)

Important from a security perspective is, that the following things are frozen past construction,
so we can reason about security at any point throughout the lifetime of a channel:
    * aURI
    * aSystemPrincipal (aRequestingNode)
    * aSecurityFlags
    * aContentPolicyType

[1] https://etherpad.mozilla.org/BetterNeckoSecurityHooks
We should probably keep the "nsIIOService*" argument. It's there as a performance optimization.
Reporter

Comment 16

5 years ago
Pushed changes to github that replace NS_NewInputStreamChannel() with NS_NewInputStreamChannel2().  For 11 callers, we are using systemPrincipal and probably the wrong content policy type.  We will have to followup with reviewers to see if we can get the right loading info.

netwerk/streamconv/test/TestStreamConv.cpp
netwerk/test/TestStreamChannel.cpp
parser/xml/src/nsSAXXMLReader.cpp
rdf/base/src/nsRDFXMLParser.cpp
dom/src/json/nsJSON.cpp
docshell/base/docshell.cpp::LoadStream
content/base/src/DOMParser.cpp
gfx/thebes/gfxSVGGlyphs.cpp
image/decoders/icon/android/nsIconChannel.cpp
image/decoders/icon/gtk/nsIconChannel.cpp
image/decoders/icon/qt/nsIconChannel.cpp
Reporter

Updated

5 years ago
Keywords: addon-compat
Reporter

Updated

5 years ago
Depends on: 1066843
Reporter

Updated

5 years ago
Depends on: 1067517
Reporter

Updated

5 years ago
Depends on: 1076978
Reporter

Updated

5 years ago
Depends on: 1083422
Reporter

Updated

5 years ago
No longer blocks: 418354
Depends on: 418354
Reporter

Updated

5 years ago
Depends on: 1084504
All js callers should have a loadInfo attached before we move security checks. Bug 1087720 is going to assert that.
Depends on: 1087720
Reporter

Updated

4 years ago
Depends on: 1119386
Reporter

Updated

4 years ago
Depends on: 1129707
Reporter

Comment 18

4 years ago
Copying over a followup from bug 1087726 comment 39:

(In reply to Tanvi Vyas [:tanvi] from comment #27)
> > ::: browser/base/content/nsContextMenu.js
> Okay, I see what you are saying.  Currently, no security checks are done
> when trying to "save link as".  So you can save http://www.evil.com even if
> it's from http://www.nice.com.  When we move security checks to asyncOpen(),
> that will change here
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> nsContextMenu.js#1293.  If we put in systemPrincipal, we essentially bypass
> these checks.  That seems like the right thing to do here, because there are
> less malicious situations where we do want to be able to save link as
> (http://a.com has a link to http://b.com that the user "saves link as"). 
> However, the url we are saving is coming from a webpage, so per Jonas, we
> shouldn't use system here.  needinfo'ing him to see what he thinks.

We've decided to use the doc here instead of systemPrincipal.  When we move security checks, we should be careful to test this scenario (save link as on a link who's origin differs from the top level document's origin and make sure that the html is actually saved to disk).
Reporter

Updated

4 years ago
No longer depends on: 1084504
Reporter

Updated

4 years ago
Keywords: meta
Reporter

Updated

4 years ago
Depends on: 1135243
Reporter

Updated

4 years ago
Depends on: 1132784
Reporter

Updated

4 years ago
Depends on: 1143922
Reporter

Updated

4 years ago
Blocks: 838395
Summary: Call Content Policies after a channel is created → [meta] Call Content Policies after a channel is created
You need to log in before you can comment on or make changes to this bug.