1407056 - Override page CSP for content injected by expanded principals

Status: RESOLVED FIXED

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

Description

[Kris Maglione [:kmag]]

Content injected by extensions is not expected to be subject to page CSP. In order achieve that immunity, we need to carefully track the principals that injected a piece of content, and take that into account in CSP checks.

For the moment, I intend to restrict the exemptions to content injected by expanded principals that subsume the loading principal for the request. That should cover any content injected by extension content scripts.

As a follow-up, we'll probably need to extend this to loads triggered by stylesheets with extension principals, since those have ordinary codebase principals and can be injected into ordinary web content in several ways.


Arguably we may want to do the same for content injected by system principals at some point, but I'm leaving that as a separate issue until we have a specific use case.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1407056: Part 1 - Provide more consistent principal/origin URL to content policies.

We're currently fairly vague and inconsistent about the values we provide to
content policy implementations for requestOrigin and requestPrincipal. In some
cases they're the triggering principal, sometimes the loading principal,
sometimes the channel principal.

Our existing content policy implementations which require or expect a loading
principal currently retrieve it from the context node. Since no current
callers require the principal to be the loading principal, and some already
expect it to be the triggering principal (which there's currently no other way
to retrieve), I chose to pass the triggering principal whenever possible, but
use the loading principal to determine the origin URL.

As a follow-up, I'd like to change the nsIContentPolicy interface to
explicitly receive loading and triggering principals, or possibly just
LoadInfo instances, rather than poorly-defined request
origin/principal/context args. But since that may cause trouble for
comm-central, I'd rather not do it as part of this bug.

Review commit: https://reviewboard.mozilla.org/r/187882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187882/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1407056: Part 2 - Override page CSP for loads by expanded principals.

Per the CSP specification, content injected by extensions is meant to be
exempt from page CSP. This patch takes care of the most common case of content
injected by extension content scripts, which always have expanded principals
which inherit from the page principal.

In a follow-up, we'll probably need to extend the exemption to stylesheet
content loaded by extension codebase principals.

Review commit: https://reviewboard.mozilla.org/r/187884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187884/

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1407056: Part 3 - Test that CSP overrides apply correctly based on triggering principals.

Review commit: https://reviewboard.mozilla.org/r/187886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187886/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916816 [details]
Bug 1407056: Part 1 - Provide more consistent principal/origin URL to content policies.

https://reviewboard.mozilla.org/r/187882/#review192952

I'd like answers to some of the questions below.

::: commit-message-50821:18
(Diff revision 1)
> +use the loading principal to determine the origin URL.
> +
> +As a follow-up, I'd like to change the nsIContentPolicy interface to
> +explicitly receive loading and triggering principals, or possibly just
> +LoadInfo instances, rather than poorly-defined request
> +origin/principal/context args. But since that may cause trouble for

For what it's worth, it doesn't seem like that would be a hard update for comm-central to make, so if it makes things easier here please feel free to do it.

::: docshell/base/nsDocShell.cpp:10014
(Diff revision 1)
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      int16_t shouldLoad = nsIContentPolicy::ACCEPT;
>      rv = NS_CheckContentLoadPolicy(contentType,
>                                     aURI,
> +                                   aTriggeringPrincipal, // loading principal

OK, so this is a new-window toplevel load.  Is aTriggeringPrincipal the right thing for "loading principal" here?

::: dom/base/nsContentPolicyUtils.h:153
(Diff revision 1)
>           do_GetService(NS_CONTENTPOLICY_CONTRACTID);                          \
>      if (!policy)                                                              \
>          return NS_ERROR_FAILURE;                                              \
>                                                                                \
>      return policy-> action (contentType, contentLocation, requestOrigin,      \
> -                            context, mimeType, extra, originPrincipal,        \
> +                            context, mimeType, extra, triggeringPrincipal,    \

OK, so this is passing triggeringPrincipal to "action"...

::: dom/base/nsContentPolicyUtils.h:161
(Diff revision 1)
>  
>  /* Passes on parameters from its "caller"'s context. */
>  #define CHECK_CONTENT_POLICY_WITH_SERVICE(action, _policy)                    \
>    PR_BEGIN_MACRO                                                              \
>      return _policy-> action (contentType, contentLocation, requestOrigin,     \
> -                             context, mimeType, extra, originPrincipal,       \
> +                             context, mimeType, extra, triggeringPrincipal,   \

And so is this....

::: dom/base/nsContentPolicyUtils.h:175
(Diff revision 1)
>   * purpose */
>  #define CHECK_PRINCIPAL_AND_DATA(action)                                      \
>    nsCOMPtr<nsIURI> requestOrigin;                                             \
>    PR_BEGIN_MACRO                                                              \
> -  if (originPrincipal) {                                                      \
> -      bool isSystem = originPrincipal->GetIsSystemPrincipal();                \
> +  if (loadingPrincipal) {                                                     \
> +      bool isSystem = loadingPrincipal->GetIsSystemPrincipal();               \

Probably worth documenting why the system-ness of loading principal, not triggering principal, is what matters here.

::: dom/base/nsContentPolicyUtils.h:193
(Diff revision 1)
>                        do_GetService(                                          \
>                                "@mozilla.org/data-document-content-policy;1"); \
>                    if (dataPolicy) {                                           \
>                        nsContentPolicyType externalType =                      \
>                            nsContentUtils::InternalContentPolicyTypeToExternal(contentType); \
>                        dataPolicy-> action (externalType, contentLocation,     \

But this is passing loadingPrincipal?  Why?

::: dom/base/nsContentPolicyUtils.h:210
(Diff revision 1)
>    PR_END_MACRO
>  
>  /**
>   * Alias for calling ShouldLoad on the content policy service.  Parameters are
> - * the same as nsIContentPolicy::shouldLoad, except for the originPrincipal
> - * parameter, which should be non-null if possible, and the last parameter,
> + * the same as nsIContentPolicy::shouldLoad, except for the loadingPrincipal
> + * and triggeringPrincipal parameters, which should be non-null if possible,

With what semantics?  Like LoadInfo's?  If so, please document accordingly.

::: dom/base/nsContentPolicyUtils.h:237
(Diff revision 1)
>  }
>  
>  /**
>   * Alias for calling ShouldProcess on the content policy service.  Parameters
> - * are the same as nsIContentPolicy::shouldLoad, except for the originPrincipal
> - * parameter, which should be non-null if possible, and the last parameter,
> + * are the same as nsIContentPolicy::shouldLoad, except for the
> + * loadingPrincipal and triggeringPrincipal parameters, which should be

Again, document.

::: dom/base/nsIContentPolicy.idl:403
(Diff revision 1)
>     * @param aContentLocation  the location of the content being checked; must
>     *                          not be null
>     *
>     * @param aRequestOrigin    OPTIONAL. the location of the resource that
> -   *                          initiated this load request; can be null if
> -   *                          inapplicable
> +   *                          that is loading the request. This will generally
> +   *                          be the URI of the loading principal for the

Probably worth making it clear that "loading principal" is meant in the same sense as loadinfo here?

::: dom/base/nsIContentPolicy.idl:426
(Diff revision 1)
>     *
>     * @param aRequestPrincipal an OPTIONAL argument, defines the principal that
>     *                          caused the load. This is optional only for
>     *                          non-gecko code: all gecko code should set this
> -   *                          argument.  For navigation events, this is
> -   *                          the principal of the page that caused this load.
> +   *                          argument. This should generally be the same as
> +   *                          the triggering principal for the resulting

Again, worth pointing to the loadinfo docs to make it clear what "triggering principal" means.

::: dom/security/nsContentSecurityManager.cpp:462
(Diff revision 1)
> -    : aLoadInfo->LoadingPrincipal();
> -
>    int16_t shouldLoad = nsIContentPolicy::ACCEPT;
>    rv = NS_CheckContentLoadPolicy(internalContentPolicyType,
>                                   uri,
> -                                 principal,
> +                                 aLoadInfo->LoadingPrincipal(),

So this is not consistent with the docshell code (which passed the same thing for loading principal and triggering principal for the "should we open a new window?" check).

Also, this will pass null for toplevel documents for the loading principal, but that's what we use to get the "origin uri".  Do we not have any more consumers that care about the "origin uri" in those case?

::: layout/style/Loader.cpp:2111
(Diff revision 1)
>      context = mDocument;
> +    loadingPrincipal = mDocument->NodePrincipal();
>    }
>  
>    nsIPrincipal* principal = aParentSheet->Principal();
> -  nsresult rv = CheckContentPolicy(principal, aURL, context, false);
> +  nsresult rv = CheckContentPolicy(loadingPrincipal, principal, aURL, context, false);

So this changes the "origin uri" for @import loads from the parent sheet to the document.  Again, do we have any consumers who care about the origin URI, and if so are they OK with this change?

::: layout/style/Loader.cpp:2298
(Diff revision 1)
>    if (!mEnabled) {
>      LOG_WARN(("  Not enabled"));
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> -  nsresult rv = CheckContentPolicy(aOriginPrincipal, aURL, mDocument, aIsPreload);
> +  nsresult rv = CheckContentPolicy(nullptr, aOriginPrincipal, aURL, mDocument, aIsPreload);

When aOriginPrincipal is not null, I think it would make more sense to use mDocument->NodePrincipal() (if mDocument is not null, which I think is actually always true when aOriginPrincipal is non-null) as the loading principal here...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916817 [details]
Bug 1407056: Part 2 - Override page CSP for loads by expanded principals.

https://reviewboard.mozilla.org/r/187884/#review192954

::: dom/security/nsCSPService.cpp:142
(Diff revision 1)
> -  // pass a document).
> +  // requesting principal's CSP overrides our document CSP, use the request
> +  // principal. Otherwise, use the document principal.
>    nsCOMPtr<nsINode> node(do_QueryInterface(aRequestContext));
> -  nsCOMPtr<nsIPrincipal> principal = node ? node->NodePrincipal()
> -                                          : aRequestPrincipal;
> +  nsCOMPtr<nsIPrincipal> principal;
> +  if (!node || (aRequestPrincipal &&
> +                BasePrincipal::Cast(aRequestPrincipal)->OverridesCSP(node->NodePrincipal()))) {

Is this under 80 lines?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916818 [details]
Bug 1407056: Part 3 - Test that CSP overrides apply correctly based on triggering principals.

https://reviewboard.mozilla.org/r/187886/#review192958

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:649
(Diff revision 1)
> -  const pageURL = `${BASE_URL}/page.html`;
> +const pageURL = `${BASE_URL}/page.html`;
> -  const pageURI = Services.io.newURI(pageURL);
> +const pageURI = Services.io.newURI(pageURL);
>  
> +/**
> + * Tests that various types of inline content elements initiate requests
> + * with the triggering pringipal of the caller that requested the load.

principal

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:679
(Diff revision 1)
> + * depending on whether the load was initiated by an extension or the
> + * content page.
> + */
> +add_task(async function test_contentscript_csp() {
> +  // We currently don't get the full set of CSP reports when running in
> +  // network scheduling chaos mode. It's not entirely clear why.

Is there a bug tracking this?  Please cite the number here.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916818 [details]
Bug 1407056: Part 3 - Test that CSP overrides apply correctly based on triggering principals.

https://reviewboard.mozilla.org/r/187886/#review192960

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8916816 [details]
Bug 1407056: Part 1 - Provide more consistent principal/origin URL to content policies.

https://reviewboard.mozilla.org/r/187882/#review192952

> OK, so this is a new-window toplevel load.  Is aTriggeringPrincipal the right thing for "loading principal" here?

No, it should probably be null, but I was generally trying to stray on the side of keeping things closer to the previous behavior when I wasn't entirely sure.

> But this is passing loadingPrincipal?  Why?

I didn't think about it clearly enough. I think this should be triggeringPrincipal.

> So this is not consistent with the docshell code (which passed the same thing for loading principal and triggering principal for the "should we open a new window?" check).
> 
> Also, this will pass null for toplevel documents for the loading principal, but that's what we use to get the "origin uri".  Do we not have any more consumers that care about the "origin uri" in those case?

I think the triggering principal should probably be null in the other docshell case, which would make it consistent with this case.

As for requestOrigin, our remaining consumers are kind of a mess as far as what they expect. The two remaining non-test consumers are both in WebExtension code. One of them expects it to be the URI of the document the content is being loaded into, so it can forbid versioned JS on extension pages. The other expects it to be the URI of the triggering principal, and probably needs to be updated now.

The rest of the users are in tests, and seem to mostly expect it to be the URI of the loading principal.

In all of those cases, though, they only sometimes got what they expected :( But I think they should get more consistently correct results after this change than before it.

> So this changes the "origin uri" for @import loads from the parent sheet to the document.  Again, do we have any consumers who care about the origin URI, and if so are they OK with this change?

Per above, they mostly expected this to be the URI of the loading principal before, so I believe this is an improvement on the previous behavior.

> When aOriginPrincipal is not null, I think it would make more sense to use mDocument->NodePrincipal() (if mDocument is not null, which I think is actually always true when aOriginPrincipal is non-null) as the loading principal here...

I'd have to dig through some more code to be sure, but I don't think the document principal will ever be useful here. I think this is only ever called from the XUL prototype cache and from nsIDOMWindowUtils in a system principal document to preload sheets which will generally be used in another document. In either case, the origin of mDocument isn't especially interesting to content policy consumers.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8916818 [details]
Bug 1407056: Part 3 - Test that CSP overrides apply correctly based on triggering principals.

https://reviewboard.mozilla.org/r/187886/#review192958

> Is there a bug tracking this?  Please cite the number here.

I was planning to file a bug. I'm still trying figure out more about what's going on, first, though.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916816 [details]
Bug 1407056: Part 1 - Provide more consistent principal/origin URL to content policies.

https://reviewboard.mozilla.org/r/187882/#review192952

> No, it should probably be null, but I was generally trying to stray on the side of keeping things closer to the previous behavior when I wasn't entirely sure.

OK.  So the previous behavior was non-null in the script security manager too; I think making it null both places is probably OK.

> I think the triggering principal should probably be null in the other docshell case, which would make it consistent with this case.
> 
> As for requestOrigin, our remaining consumers are kind of a mess as far as what they expect. The two remaining non-test consumers are both in WebExtension code. One of them expects it to be the URI of the document the content is being loaded into, so it can forbid versioned JS on extension pages. The other expects it to be the URI of the triggering principal, and probably needs to be updated now.
> 
> The rest of the users are in tests, and seem to mostly expect it to be the URI of the loading principal.
> 
> In all of those cases, though, they only sometimes got what they expected :( But I think they should get more consistently correct results after this change than before it.

The thing that cares about versioned JS isn't a problem for the "document load" cases.  The other case we should sort out.

The test cases we can just adjust to do whatever, of course.

> Per above, they mostly expected this to be the URI of the loading principal before, so I believe this is an improvement on the previous behavior.

OK, great.

> I'd have to dig through some more code to be sure, but I don't think the document principal will ever be useful here. I think this is only ever called from the XUL prototype cache and from nsIDOMWindowUtils in a system principal document to preload sheets which will generally be used in another document. In either case, the origin of mDocument isn't especially interesting to content policy consumers.

There are four callsites of InternalLoadNonDocumentSheet.  Two of them pass null for aOriginPrincipal.  The other two are the LoadSheet() call from XULDocument::AddPrototypeSheets and the LoadSheet() calls rrom nsXBLResourceLoader::LoadResources, HTMLEditor::ReplaceStyleSheet, and nsDocument::PreloadStyle.

The XULDocument case is not very interesting, if we ignore remote XUL; it just has system principals all around.

The XBL resource loader case is also not terribly interesting, ignoring remote XUL/XBL.

The HTML editor case actually ends up passing null too.

The nsDocument::PreloadStyle call, though is what the parser does to preload styles while blocked on a loading script.  That one really should use the document principal as the loading principal, assuming anyone at all cares about the loading principal.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #10)
> The nsDocument::PreloadStyle call, though is what the parser does to preload
> styles while blocked on a loading script.  That one really should use the
> document principal as the loading principal, assuming anyone at all cares
> about the loading principal.

Ah. Makes sense.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8916816 [details]
Bug 1407056: Part 1 - Provide more consistent principal/origin URL to content policies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187882/diff/1-2/

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8916817 [details]
Bug 1407056: Part 2 - Override page CSP for loads by expanded principals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187884/diff/1-2/

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8916818 [details]
Bug 1407056: Part 3 - Test that CSP overrides apply correctly based on triggering principals.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187886/diff/1-2/

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916816 [details]
Bug 1407056: Part 1 - Provide more consistent principal/origin URL to content policies.

https://reviewboard.mozilla.org/r/187882/#review193306

::: docshell/base/nsDocShell.cpp:10014
(Diff revisions 1 - 2)
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      int16_t shouldLoad = nsIContentPolicy::ACCEPT;
>      rv = NS_CheckContentLoadPolicy(contentType,
>                                     aURI,
> -                                   aTriggeringPrincipal, // loading principal
> +                                   nullptr, // loading principal

Document that this is null because this is a toplevel window?

::: layout/style/Loader.cpp:2298
(Diff revisions 1 - 2)
>    if (!mEnabled) {
>      LOG_WARN(("  Not enabled"));
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> -  nsresult rv = CheckContentPolicy(nullptr, aOriginPrincipal, aURL, mDocument, aIsPreload);
> +  nsCOMPtr<nsIPrincipal> loadingPrincipal = mDocument ? mDocument->NodePrincipal() : nullptr;

We really do want to use null for loadingPrincipal if aOriginPrincipal (triggering principal) is null, I'd think...

Comment 16

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8916817 [details]
Bug 1407056: Part 2 - Override page CSP for loads by expanded principals.

https://reviewboard.mozilla.org/r/187884/#review193940

Sorry for the lag, I was on PTO. Bz's review always subsumes mine anyway :)

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8916816 [details]
Bug 1407056: Part 1 - Provide more consistent principal/origin URL to content policies.

https://reviewboard.mozilla.org/r/187882/#review194092

Sorry for the lag of repsonse - that looks good. Have you also checked for all appearances for .shouldLoad [1]? If not, please update those as well, thanks.


[1] https://dxr.mozilla.org/mozilla-central/search?q=.shouldLoad(&redirect=false

::: dom/html/ImageDocument.cpp:106
(Diff revision 2)
>    if (secMan) {
>      secMan->GetChannelResultPrincipal(channel, getter_AddRefs(channelPrincipal));
>    }
>  
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  channel->GetLoadInfo(getter_AddRefs(loadInfo));

nsCOMPtr<nsILoadInfo> loadInfo = channel->GetLoadInfo();

Comment 18

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> Have you also checked for all appearances for .shouldLoad [1]? If not, please
> update those as well, thanks.

The only direct ShouldLoad calls I could find in C++ were in existing ShouldLoad
or ShouldProcess callbacks (and passed along their original args), and [1] which
already follows the new behavior.

The only JS calls were in RemoteAddonParent, which just proxies calls from the
child process, and shouldn't be used anymore anyway, and CSP tests, which aren't
nsIContentPolicy calls, and don't have triggering principal arguments.

[1]: http://searchfox.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#389

Comment 19

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/197ce71943518bfb260f2b2cb3f91b55e58f9341
Bug 1407056: Part 1 - Provide more consistent principal/origin URL to content policies. r=bz,ckerschb

https://hg.mozilla.org/integration/mozilla-inbound/rev/41e5a4b54c93e40e92e1e697efa936e90a4c5a41
Bug 1407056: Part 2 - Override page CSP for loads by expanded principals. r=bz,krizsa

https://hg.mozilla.org/integration/mozilla-inbound/rev/d480c295d4eb093b1890056cc724e12bda7ac3ca
Bug 1407056: Part 3 - Test that CSP overrides apply correctly based on triggering principals. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7ff96cd4d0fd465a7b94925358558764867c03
Bug 1407056: Follow-up: Don't try to truncate data URI strings to a longer length. r=me

Comment 20

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c0e650042cde2a01e58b3bb6adf6a8eeacca38b
Bug 1407056: Follow-up: Skip test on Android debug for flakiness. r=bustage

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/197ce7194351
https://hg.mozilla.org/mozilla-central/rev/41e5a4b54c93
https://hg.mozilla.org/mozilla-central/rev/d480c295d4eb
https://hg.mozilla.org/mozilla-central/rev/ef7ff96cd4d0
https://hg.mozilla.org/mozilla-central/rev/6c0e650042cd