1084652 - CSP: violated directive empty when sending reports

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

The violated directive should never be empty when sending reports. We have to fall back to the default directive in case we can not find a restricting directive [1].

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.cpp#937

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1084652.patch

Please note, that falling back to the default directive must be correct in such cases, otherwise csp woulnd't have blocked the load in the first place.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8507256 [details] [diff] [review]
bug_1084652.patch

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

It would be nice to have tests for this.

::: content/base/src/nsCSPUtils.cpp
@@ +926,5 @@
>  }
>  
>  void
>  nsCSPPolicy::getDirectiveStringForContentType(nsContentPolicyType aContentType,
>                                                nsAString& outDirective) const

If there are limits on the uses of this function (preconditions or which other functions/classes/modules can use it), please add a doc style comment above the function explaining when it should and should not be used.

@@ +941,5 @@
> +  }
> +  // if we haven't found a matching directive yet,
> +  // the contentType must be restricted by the default directive
> +  if (defaultDir) {
> +    defaultDir->toString(outDirective);

Will the frame-ancestors directive (or others that are not supposed to) fall through to the default-src directive here?

@@ +946,2 @@
>    }
>  }

What is the default return value for getDirectiveStringForContentType()?  What if something goes horribly wrong and we can't find a specific directive or default directive?  Please add an assertion at the end of this and return some type of obvious erroneous value in the string if there's no relevant directive found.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #2)
> Comment on attachment 8507256 [details] [diff] [review]
> bug_1084652.patch
> 
> Review of attachment 8507256 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks Sid, here is a list of things that got updated:

* Updated test_csp_report.html to fall back to the default-src, instead of using script-src. I think that's a valid change since it doesn't limit testcoverage but also covers the case where the violated-directive was empty.

* Added a block comment on top of getDirectiveStringForContentType explaining usage and restrictions.

* Other directives, like the mentioned frame-ancestors want fall back (or fall into) a wrong category (default-src) because we only call getDirectiveStringForContentType after ::allows() returned false. frame-ancestors for example calls ::permits() which returns the violatedDirective.

* In case something goes horribly wrong with getDirectiveStringForContentType I added an assertion and also assign a string explaining the error to outDirective.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1084652_v1.patch

Carrying over r+ from Sid.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1084652_tests.patch

I think there is no need for a new test - updating test_csp_report adds additional test-coverage but does not remove any in my opinion.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8513845 [details] [diff] [review]
bug_1084652_v1.patch

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

::: dom/base/nsCSPContext.cpp
@@ +826,5 @@
>          NS_ASSERTION(supportscstr, "Couldn't allocate nsISupportsCString");
>          supportscstr->SetData(NS_ConvertUTF16toUTF8(aObserverSubject));
>          mObserverSubject = do_QueryInterface(supportscstr);
>        }
> +      NS_ASSERTION(!mViolatedDirective.IsEmpty(), "Can not send reports without a violated directive");

nit: Please do this at the top of the constructor and operate on aViolatedDirective instead of mViolatedDirective.  The error is triggered by the caller passing an invalid argument (not invalid state in the constructed CSPReportSenderRunnable).  The goal of this assertion is to make it obvious when we try to construct an invalid CSPReportSenderRunnable.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8513846 [details] [diff] [review]
bug_1084652_tests.patch

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

Yeah, this should be fine.  It tests the "use default directive as violated directive in reports" behavior we want to test.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #6)
> Comment on attachment 8513845 [details] [diff] [review]
> bug_1084652_v1.patch
> 
> Review of attachment 8513845 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsCSPContext.cpp
> @@ +826,5 @@
> >          NS_ASSERTION(supportscstr, "Couldn't allocate nsISupportsCString");
> >          supportscstr->SetData(NS_ConvertUTF16toUTF8(aObserverSubject));
> >          mObserverSubject = do_QueryInterface(supportscstr);
> >        }
> > +      NS_ASSERTION(!mViolatedDirective.IsEmpty(), "Can not send reports without a violated directive");
> 
> nit: Please do this at the top of the constructor and operate on
> aViolatedDirective instead of mViolatedDirective.  The error is triggered by
> the caller passing an invalid argument (not invalid state in the constructed
> CSPReportSenderRunnable).  The goal of this assertion is to make it obvious
> when we try to construct an invalid CSPReportSenderRunnable.

Indeed, that makes things more explicit. It's not an invalid object state - it's invalid argument to the constructor.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97a2c1e265cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/228f249d2931

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97a2c1e265cf
https://hg.mozilla.org/mozilla-central/rev/228f249d2931