1496418 - Update Content Policy check to not return early for system principal loads so we can check CSP on system privileged pages

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

After we have moved the CSP into the Client (see Bug 965637) we have to remove the early bail [1] for system principal loads within content policies so that loads of system privileged pages actually consult the CSP code and we can start to apply CSPs to system privileged about pages (see Bug 1492063).

[1] https://searchfox.org/mozilla-central/source/dom/base/nsContentPolicyUtils.h#169-203

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1496418_allow_system_principal_calls_to_csp.patch

Hey Smaug,

after we have moved the CSP from the Principal into the client we also want to enforce CSP for system privileged pages. Currently, Content Policies return early (mostly as an optimization) within CHECK_PRINCIPAL_AND_DATA.

Within this patch I am moving that fast path into each call site and return early if the loadingPrincipal is the SystemPrincipal. I am doing this within each content policy except within CSP where we actually want to evaluate the call.

Does that seem reasonable and would we accept such a patch? Alternatively we could check the type of contentpolicy and *only* consult the actual policy if the policy is of type nsiContentSecurityPolicy. I think now that legacy addons are gone we could leave it up to the individual polices and have contentPolicyUtils.h just to consult each policy but not performing additional checks.

What do you think?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dcb99bd0ded65033d2b5ea954c70eb041ae96e5

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Alternative approach:

I thought about this more and maybe we should not remove CHECK_PRINCIPAL_AND_DATA but rename it CHECK_PRINCIPAL_CSP_AND_DATA. Within that macro we could addtionally perform a CSP check and still return early in case the loadingPrincipal is the SystemPrincipal. That approach would definitely be a smaller change and would definitely not cause any unforseen problems.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9018533 [details] [diff] [review]
bug_1496418_allow_system_principal_calls_to_csp.patch

I don't like this approach, propagating system principal checks everywhere, which means it is guaranteed that we'll forget it at some
place later.

I guess the plan for the future is to allow by default for system principal, but let csp to override?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1496418_allow_system_principal_calls_to_csp.patch

(In reply to Olli Pettay [:smaug] (@TPAC, slower reviews) from comment #4)
> I guess the plan for the future is to allow by default for system principal,
> but let csp to override?

Right, so I guess you prefer the approach I explained in comment 3, which makes sense to me as well. Especially now that I have looked at the new patch this approach seems way cleaner.

What do you think?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=232231820f0389c0531dfe6b8e6cc86aabbbb87c

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9019977 [details] [diff] [review]
bug_1496418_allow_system_principal_calls_to_csp.patch

>+      if (isSystem) {                                                         \
>+        /* Check CSP for System Privileged pages */                           \
>+        nsCOMPtr<nsIContentPolicy> cspPolicy =                                \
>+          do_GetService("@mozilla.org/cspcontext;1");                         \
This is the only place where @mozilla.org/cspcontext;1 is used to create a service.
Do we really want that? Using a component both as an instance and service hints usually about a bug.

And in general, I think it would be good to avoid using component service
in this tiny bit hot code. Could we cache the relevant nsIContentPolicy somewhere and access that
here?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1496418_allow_system_principal_calls_to_csp.patch

(In reply to Olli Pettay [:smaug] (high review load) from comment #7)
> And in general, I think it would be good to avoid using component service
> in this tiny bit hot code. Could we cache the relevant nsIContentPolicy
> somewhere and access that here?

I tried to base my code on what was already used in that macro, e.g. the dataPolicy [1]. Ultimately I agree to your suggestion though, we shouldn't create a new instance in that hot piece of code. I added a helper function to nsCSPService, which in my opinion provides the cleanest solution.

Agreed?

[1] https://searchfox.org/mozilla-central/source/dom/base/nsContentPolicyUtils.h#190-191

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9021476 [details] [diff] [review]
bug_1496418_allow_system_principal_calls_to_csp.patch

>+#define CHECK_PRINCIPAL_CSP_AND_DATA(action)                                  \
>   nsCOMPtr<nsIURI> requestOrigin;                                             \
>   PR_BEGIN_MACRO                                                              \
>   if (loadingPrincipal) {                                                     \
>       /* We exempt most loads into any document with the system principal     \
>        * from content policy checks, mostly as an optimization. Which means   \
>        * that we need to apply this check to the loading principal, not the   \
>        * principal that triggered the load. */                                \
>       bool isSystem = loadingPrincipal->GetIsSystemPrincipal();               \
>+      if (isSystem) {                                                         \
>+        /* Check CSP for System Privileged pages */                           \
>+        CSPService::ConsultCSP(contentLocation, loadInfo, mimeType, decision);\
This part looks good, and I like the method name even :)

>+/* static */ void
>+CSPService::ConsultCSP(nsIURI* aContentLocation,
>+                       nsILoadInfo* aLoadInfo,
>+                       const nsACString &aMimeType,
>+                       int16_t *outDecision)
int16_t* aOutDecisision

>+{
>+  nsCOMPtr<nsIContentPolicy> cspPolicy;
>+  if (!cspPolicy) {
cspPolicy is always null here.


>+    nsresult rv;
>+    cspPolicy = do_CreateInstance("@mozilla.org/cspservice;1", &rv);
so we end up always creating a new instance, in a tad hot code path.

Could we refactor CSP checks to static methods and call such here?
> 
>+  // helper function to avoid creating a new instance of the
>+  // cspservice everytime we call content policies.
yeah, it is not doing that, but creates a new instance always ;)

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #9)
> >+  nsCOMPtr<nsIContentPolicy> cspPolicy;

Obviously that is missing a static keyword - good that we do code reviews - thanks!

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

But do we want to keep the component alive that way? Or should there be perhaps StaticRefPtr somewherE?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1496418_allow_system_principal_calls_to_csp.patch

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #11)
> But do we want to keep the component alive that way? Or should there be
> perhaps StaticRefPtr somewherE?

Oh I see what you mean. What do you prefer? StaticRefPtr?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I'd prefer StaticRefPtr, since I think that nsCOMPtr may create static constructor.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(But I still think CSPService should have static methods doing all the work. The class doesn't have any member variables storing a state [expect unused mAppStatusCache].)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9021490 [details] [diff] [review]
bug_1496418_allow_system_principal_calls_to_csp.patch

>   /**
>    * Returns true if this principal's CSP should override a document's CSP for
>    * loads that it triggers. Currently true for system principal, for expanded
>    * principals which subsume the document principal, and add-on codebase
>    * principals regardless of whether they subsume the document principal.
>    */
>   bool OverridesCSP(nsIPrincipal* aDocumentPrincipal)
>   {
>-    // SystemPrincipal can override the page's CSP by definition.
>-    if (mKind == eSystemPrincipal) {
>-      return true;
>-    }
>-
the comment for the method needs to be updated.

>+/* static */ void
>+CSPService::ConsultCSP(nsIURI* aContentLocation,
>+                       nsILoadInfo* aLoadInfo,
>+                       const nsACString &aMimeType,
>+                       int16_t *aOutDecision)
>+{
>+  static nsCOMPtr<nsIContentPolicy> cspPolicy;
>+  if (!cspPolicy) {
>+    nsresult rv;
>+    cspPolicy = do_CreateInstance("@mozilla.org/cspservice;1", &rv);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      return;
>+    }
>+  }
I'd prefer StaticRefPtr and use of ClearOnShutdown, since I assume otherwise you'll get leaks.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1496418_allow_system_principal_calls_to_csp.patch

Hey Smaug, thanks for bearing with me. FWIW, I filed Bug 1503575 to remove the unused member mAppStatusCache. Further, I created a new static function ConsultCSP so that the nsIContentPolicy implementation of shouldLoad() just forwards the call to that static method. That looks way cleaner now actually - thanks for the suggestion.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:ckerschb, could you have a look please?

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Before we can apply CSP to system privileged about pages we have to fix Bug 965637, in which we move the CSP from the Principal into the Client. Please note that the Meta Bug 1492063 for applying CSP to system privileged about: pages is blocked by 965637. At the moment we are fixing the last remaining blockers and as soon as we have landed Bug 965637 I'll try to land all the dependencies of Bug 1492063 so we end up having all about: pages secured by a CSP.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1496418: Update Content Policy checks and allow CSP checks for system principal triggered loads. r=mccr8, baku

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62b73610a2f7
Update Content Policy checks and allow CSP checks for system principal triggered loads. r=mccr8,baku

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/62b73610a2f7