1476592 - Remove the cache from nsCSPContext

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement)

Description

[Andrea Marchesini [:baku]]

This has been discussed with ckerschb on IRC.
We need the cache because we want to dispatch CSP violation events only once per channel/nsILoadInfo, but the cache cannot be based on the URL of the request: otherwise 2 independent requests, with the same URL, will trigger just 1 event, instead of 2.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: csp_blockedURI5.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8992943 [details] [diff] [review]
csp_blockedURI5.patch

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

yes, that looks sane to me. thanks,r=me

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - no cache

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - sendViolationReports parameter

This fixes the imgLoader. If there are other issues, we can fix them with a similar approach.

The refactoring of CSPContext's cache was becoming a too big project.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - sendViolationReports parameter

aosmond, can you review the imgLoader part?

The issue is that we want to dispatch CSP violation events only once, also when imgLoader uses cache. In order to do this I use aSendCSPViolationEvents=false in ShouldLoadCacheImage(). But I need to call that again if a new channel has not been created. There are 3 scenarios (as far I can say):

1. no cache - we create a nsIChannel and this triggers CSP checks.
2. we have cache but we need to create a nsIChannel - similar to 1.
3. we have cache and we use it directly.

In scenario 3, I need to trigger CSP checks with aSendCSPViolationEvents=true again.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8994752 [details] [diff] [review]
part 2 - sendViolationReports parameter

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

::: dom/base/nsContentPolicyUtils.h
@@ +220,5 @@
>                            nsILoadInfo      *loadInfo,
>                            const nsACString &mimeType,
>                            int16_t          *decision,
> +                          nsIContentPolicy *policyService = nullptr,
> +                          bool aSendViolationReports = true)

Aren't we missing doing something with this parameter here? It is hard to evaluate the context in imgLoader without knowing what happens.

::: image/imgLoader.cpp
@@ +748,5 @@
>    }
>  
>    // Content Policy Check on Cached Images
> +  return ShouldLoadCachedImage(request, aCX, triggeringPrincipal, aPolicyType,
> +                               false /* aSendCSPViolationReports */);

nit: we usually put the boolean after the comment in image/

@@ +2042,5 @@
> +                                    aLoadFlags, aLoadPolicyType,
> +                                    aProxyRequest, aTriggeringPrincipal,
> +                                    aCORSMode);
> +    if (aNewChannelCreated && validateRequest) {
> +      *aNewChannelCreated = true;

ValidateRequestWithNewChannel doesn't always create a new channel so the name is pretty misleading. It can share an existing channel if it is already in the process of validating the cache. You probably want to assert again in that case.

I assume the check happens synchronously via nsIChannel::AsyncOpen2 and will fail if the check fails? It looks to me there is an (existing) bug where if AsyncOpen2 fails, we don't clean up the imgCacheValidator properly, so we get stuck with a validator which will never get OnStartRequest called (I assume no events are called if the open fails) and any future requests for the image will block forever on it.

@@ +2379,5 @@
> +        // This is ugly but it's needed to report CSP violations. We have 3
> +        // scenarios:
> +        // - we don't have cache. We are not in this if() stmt. A new channel is
> +        //   created and that triggers the CSP checks.
> +        // - We have a cache entry and this is blocked by CSP directives.

I find the comment sort of confusing. I would say something like "This is ugly but it is needed to report CSP violations. A cached entry may be otherwise blocked by CSP directives, so we need to explicitly check. Otherwise if we don't have a cache entry, or we need to validate the cache entry first, we will trigger the CSP checks as part of creating a new channel."

@@ +2383,5 @@
> +        // - We have a cache entry and this is blocked by CSP directives.
> +        DebugOnly<bool> shouldLoad =
> +          ShouldLoadCachedImage(request, aLoadingDocument, aTriggeringPrincipal,
> +                                aContentPolicyType, true /* aSendCSPViolationReports */);
> +        MOZ_ASSERT(shouldLoad);

Maybe I am misunderstanding the intent here. If it fails a CSP directive, it may return false. We are checking here to make sure we issue events if we do fail any CSP directives, but we assert that none occur.

The ValidateEntry call above (and ShouldLoadCachedImage indirectly) would have prevented us entering this path if there were CSP violations, no? My reading is that it would toss the cached entry and enter the else below instead.

Comment 7

[Andrea Marchesini [:baku]]

Before apply your comments I want to be sure we are on the same path here.

> I assume the check happens synchronously via nsIChannel::AsyncOpen2 and will
> fail if the check fails? It looks to me there is an (existing) bug where if
> AsyncOpen2 fails, we don't clean up the imgCacheValidator properly, so we
> get stuck with a validator which will never get OnStartRequest called (I
> assume no events are called if the open fails) and any future requests for
> the image will block forever on it.

I can fix this, but it's a separate bug. is it?

> @@ +2383,5 @@
> > +        // - We have a cache entry and this is blocked by CSP directives.
> > +        DebugOnly<bool> shouldLoad =
> > +          ShouldLoadCachedImage(request, aLoadingDocument, aTriggeringPrincipal,
> > +                                aContentPolicyType, true /* aSendCSPViolationReports */);
> > +        MOZ_ASSERT(shouldLoad);
> 
> Maybe I am misunderstanding the intent here. If it fails a CSP directive, it
> may return false. We are checking here to make sure we issue events if we do
> fail any CSP directives, but we assert that none occur.

CSP is a static set of directives. If ShouldLoadCacheImage() fails, we should not be here because ValidateEntry() would return false. So, if we are here, it means that ValidateEntry() has succeeded, and ShouldLoadCacheImage() has returned true.
I want to call it again, passing aSendCSPViolationReports=true in order to dispatch some CSP violation events if needed.

The issue is this: we have 2 kinds of CSP violations: blocking violations and 'report-only' violations. The second types do not block the loading but they trigger the dispatching of a CSP violation event. If we are here, definitely CSP doesn't block this loading, but maybe we still need to dispatch 'report-only' CSP violation events. Here why I call it again passing aSendCSPViolationReport = true.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8994752 [details] [diff] [review]
part 2 - sendViolationReports parameter

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

Instead of changing the interface I would rather add a boolean to the loadInfo, something like mSendCSPReports, which is set to true by default and then in the only cases where we don't want the violation report to be send, we change it to true. That way we don't have to change the signature of ShouldLoad within nsIContentPolicy.idl, only the signature of the internal shouldLoad() within nsIContentSecurityPolicy.idl.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8994562 [details] [diff] [review]
part 1 - no cache

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

Removal is fine. r=me

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - sendViolationReports parameter

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8995966 [details] [diff] [review]
part 2 - sendViolationReports parameter

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

That looks way better. Thanks for updating. r=me

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #7)
> Before apply your comments I want to be sure we are on the same path here.
> 
> > I assume the check happens synchronously via nsIChannel::AsyncOpen2 and will
> > fail if the check fails? It looks to me there is an (existing) bug where if
> > AsyncOpen2 fails, we don't clean up the imgCacheValidator properly, so we
> > get stuck with a validator which will never get OnStartRequest called (I
> > assume no events are called if the open fails) and any future requests for
> > the image will block forever on it.
> 
> I can fix this, but it's a separate bug. is it?
> 

Yes, you can file a separate bug, no need to fix it here.

> > @@ +2383,5 @@
> > > +        // - We have a cache entry and this is blocked by CSP directives.
> > > +        DebugOnly<bool> shouldLoad =
> > > +          ShouldLoadCachedImage(request, aLoadingDocument, aTriggeringPrincipal,
> > > +                                aContentPolicyType, true /* aSendCSPViolationReports */);
> > > +        MOZ_ASSERT(shouldLoad);
> > 
> > Maybe I am misunderstanding the intent here. If it fails a CSP directive, it
> > may return false. We are checking here to make sure we issue events if we do
> > fail any CSP directives, but we assert that none occur.
> 
> CSP is a static set of directives. If ShouldLoadCacheImage() fails, we
> should not be here because ValidateEntry() would return false. So, if we are
> here, it means that ValidateEntry() has succeeded, and
> ShouldLoadCacheImage() has returned true.
> I want to call it again, passing aSendCSPViolationReports=true in order to
> dispatch some CSP violation events if needed.
> 
> The issue is this: we have 2 kinds of CSP violations: blocking violations
> and 'report-only' violations. The second types do not block the loading but
> they trigger the dispatching of a CSP violation event. If we are here,
> definitely CSP doesn't block this loading, but maybe we still need to
> dispatch 'report-only' CSP violation events. Here why I call it again
> passing aSendCSPViolationReport = true.

Okay, so report only violations will cause NS_CP_ACCEPTED to still return true. Got it, sounds good now.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - sendViolationReports parameter

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/049419453384
Remove the cache from nsCSPContext - part 1, r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/42983355094d
Remove the cache from nsCSPContext - part 2 - sendViolationReports parameter, r=ckerschb, r=aosmond

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/049419453384
https://hg.mozilla.org/mozilla-central/rev/42983355094d