Closed Bug 1476592 Opened Last year Closed Last year

Remove the cache from nsCSPContext

Categories

(Core :: DOM: Security, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Whiteboard: [domsecurity-backlog1] [domsecurity-active])

Attachments

(2 files, 4 obsolete files)

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.
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog1] [domsecurity-active]
Attached patch csp_blockedURI5.patch (obsolete) — Splinter Review
Attachment #8992943 - Flags: review?(ckerschb)
Depends on: 1476280
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
Attachment #8992943 - Flags: review?(ckerschb) → review+
Summary: Remove the cache from nsCSPContext and store the CSP decision in nsILoadInfo → Remove the cache from nsCSPContext
Attachment #8992943 - Attachment is obsolete: true
Attachment #8994562 - Flags: review?(ckerschb)
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.
Attachment #8994563 - Flags: review?(ckerschb)
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.
Attachment #8994752 - Flags: review?(ckerschb)
Attachment #8994752 - Flags: review?(aosmond)
Attachment #8994563 - Attachment is obsolete: true
Attachment #8994563 - Flags: review?(ckerschb)
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.
Attachment #8994752 - Flags: review?(aosmond)
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.
Flags: needinfo?(aosmond)
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.
Attachment #8994752 - Flags: review?(ckerschb)
Comment on attachment 8994562 [details] [diff] [review]
part 1 - no cache

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

Removal is fine. r=me
Attachment #8994562 - Flags: review?(ckerschb) → review+
Attachment #8994752 - Attachment is obsolete: true
Attachment #8995966 - Flags: review?(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
Attachment #8995966 - Flags: review?(ckerschb) → review+
(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.
Flags: needinfo?(aosmond)
Attachment #8995966 - Attachment is obsolete: true
Attachment #8996355 - Flags: review?(aosmond)
Attachment #8996355 - Flags: review?(aosmond) → review+
Pushed by amarchesini@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/049419453384
https://hg.mozilla.org/mozilla-central/rev/42983355094d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.