1006881 - [meta] Call Content Policies after a channel is created

Status: NEW

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

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.

Comment 1

[Tanvi Vyas[:tanvi]]

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.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 4

[Tanvi Vyas[:tanvi]]

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.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If we pass in a node/document, we can still grab the context window from that and pass the context window to nsIContentPolicy.

Comment 6

[Tanvi Vyas[:tanvi]]

Attachment: Bug1006881-wip-05-07-14.patch

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.

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: change-script-callsite-05-09-14.patch

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.

Comment 8

[Tanvi Vyas[:tanvi]]

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.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 11

[Tanvi Vyas[:tanvi]]

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

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

In case someone is tracking progress, we push our patches to:
https://github.com/ckerschb/gecko-dev/tree/revampGeckoSecHook

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We should probably keep the "nsIIOService*" argument. It's there as a performance optimization.

Comment 16

[Tanvi Vyas[:tanvi]]

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

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

All js callers should have a loadInfo attached before we move security checks. Bug 1087720 is going to assert that.

Comment 18

[Tanvi Vyas[:tanvi]]

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