994318 - CSP in C++: add caching to CSP policy checks

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: policycache

Implements a cache much like in bug 897745 so repeated hits to ShouldLoad with the same URI don't require policy checks.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8405639 [details] [diff] [review]
policycache

Bitrotted due do changes in parser and utils. Can you please export again once we are done reviewing parser and utils? Thanks

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Attachment: policycache

Ok, here's an updated patch.  No tests directly, but all the CSP tests should still pass.  Fabrice implemented the first cache, so if we need an additional reviewer we could flag him (but this is pretty straightforward).

Comment 4

[Garrett Robinson [:grobinson]]

Comment on attachment 8418681 [details] [diff] [review]
policycache

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

Is setRequestContext only meant to be called once? If not, it seems like calling it should also invalidate the cache. If so, maybe we should enforce that somehow?

::: content/base/src/nsCSPContext.cpp
@@ +188,5 @@
>        // * Console error reporting, bug 994322
>      }
>    }
> +  // Done looping, cache any relevant result
> +  if (cacheKey.Length() > 0 && !isPreload) {

"isPreload" is "problematicPreload" in the earlier patch that introduced it. I don't care which name you prefer to use, just FYI you might have a merge conflict here.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8418681 [details] [diff] [review]
policycache

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

This looks pretty good, only some nits which should be easy to fix!

::: content/base/src/nsCSPContext.cpp
@@ +45,5 @@
>  
>  #define CSPCONTEXTLOG(args) PR_LOG(GetCspContextLog(), 4, args)
>  
> +#define CSP_CACHE_URI_CUTOFF_SIZE 512
> +

With respect to scope and type-safety we should use static const uint32_t.

@@ +54,5 @@
> +nsresult
> +CreateCacheKey_Internal(nsIURI* aContentLocation,
> +                        nsContentPolicyType aContentType,
> +                        nsACString& oCacheKey)
> +{

Nit: Since we are using 'out' as Prefix for outgoing arguments everywhere else in nsCSPContext we should be consistent and also rename oCacheKey to outCacheKey.

@@ +63,5 @@
> +  bool isDataScheme = false;
> +  nsresult rv = aContentLocation->SchemeIs("data", &isDataScheme);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  oCacheKey.Truncate(0);

Nit: no need for an argument, just use Truncate()

@@ +119,5 @@
> +  rv = CreateCacheKey_Internal(aContentLocation, aContentType, cacheKey);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  bool isCached = mShouldLoadCache.Get(cacheKey, outDecision);
> +  if (cacheKey.Length() > 0 && isCached) {

Flip this condition around so that the boolean condition (isChached) is checked first.

@@ +188,5 @@
>        // * Console error reporting, bug 994322
>      }
>    }
> +  // Done looping, cache any relevant result
> +  if (cacheKey.Length() > 0 && !isPreload) {

That's not an issue actuall becuase I changed problematicPreload to isPreload in an earlier patch and propagated that change all the way up the patch-queue.

@@ +290,5 @@
>    if (policy) {
>      mPolicies.AppendElement(policy);
>    }
> +  // reset cache since effective policy changes
> +  mShouldLoadCache.Clear();

Shouldn't it be sufficient to reset the cache only if mPolicies changes? In other words, can't we move mShouldLoadCache() inside the the {} of |if (policy)| ?

::: content/base/src/nsCSPContext.h
@@ +43,1 @@
>  };

Nit: Should we align mPolicies, mSelfURI with mShouldLoadCache?

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Attachment: policycache

Updated with review comments.  Carrying over r=grobinson,ckerschb.  This should be ready to land after the dependencies do.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Attachment: policycache

Had a little bitrot.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4655f5da435

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage in the push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7df548210d7

https://tbpl.mozilla.org/php/getParsedLog.php?id=40027557&tree=Mozilla-Inbound

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

the patch for bug 994320 was the culprit.  Fixed that and relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd21ae215a2

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7bd21ae215a2