Closed Bug 1418243 Opened 7 years ago Closed 6 years ago

CSP violation: violatedDirective/effectiveDirective

Categories

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

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: cfu, Assigned: cfu)

References

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(3 files, 6 obsolete files)

Fix SecurityPolicyViolationEvent.violatedDirective.
13.24 KB, patch
cfu
: review+
Details | Diff | Splinter Review
Enable web-platform tests which expected FAIL because of violatedDirective.
46.57 KB, patch
cfu
: review+
Details | Diff | Splinter Review
Fix mochitest failures due to violationDirective change.
9.98 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review 
The violatedDirective/effectiveDirective now contains the whole rule (i.e., directive + values), while web-platform tests expect it to have only the directive part.
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
This string comes from nsCSPDirective::toString
https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/security/nsCSPUtils.cpp#1074
and there are some overrides
https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/security/nsCSPUtils.cpp#1325
https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/security/nsCSPUtils.cpp#1343
https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/security/nsCSPUtils.cpp#1361
which is called in nsCSPPolicy::permits
https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/security/nsCSPUtils.cpp#1428
and nsCSPPolicy::getDirectiveStringForContentType
https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/security/nsCSPUtils.cpp#1589

However, it seems not appropriate to me to modify the nsCSPDirective::toString method because nsCSPPolicy::toString calls it to collect the complete policy string
https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/security/nsCSPUtils.cpp#1536

I can think of 2 approaches:
1. perform string process in nsCSPContext::GatherSecurityPolicyViolationEventData that splits aViolatedDirective by the first space
   https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/security/nsCSPContext.cpp#894
2. add a method to nsCSPDirective, maybe named getName, that returns only the directive part, and make nsCSPPolicy::permits and nsCSPPolicy::getDirectiveStringForContentType call it instead of toString.
Assignee: nobody → cfu
Attachment #8938276 - Flags: review?(ckerschb)
Working with the patch from bug 1418241 makes many web platform tests pass.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c467ace08b5f108f13a036fdfbb930bbf7499504
Comment on attachment 8938276 [details]
Bug 1418243 - Fix SecurityPolicyViolationEvent.violatedDirective.

https://reviewboard.mozilla.org/r/209050/#review214820

I imagine that enables a lot of web-platform tests, right? Can I see a TRY run and also review the test updates?

::: dom/security/nsCSPContext.cpp:896
(Diff revision 1)
>    }
>  
> +  // Extract directive name.
> +  nsAutoString violatedDirective(aViolatedDirective);
> +  aViolatedDirective = Substring(violatedDirective, 0, violatedDirective.Find(" "));
> +

huh, that sounds risky. Can we at least add an assertion that it's any of the valid directives?

Please add a comment explaining the difference between effective and violated directive to the code
Attachment #8938276 - Flags: review?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Comment on attachment 8938276 [details]
> Bug 1418243 - Fix SecurityPolicyViolationEvent.violatedDirective.
> 
> https://reviewboard.mozilla.org/r/209050/#review214820
> 
> I imagine that enables a lot of web-platform tests, right? Can I see a TRY
> run and also review the test updates?

Sure I will prepare a try run for this bug only.

Besides, as far as I know most tests check both violatedDirective and blockedURI, so fixing only this bug or bug 1418241 may not enable that many tests. But once we have both bugs fixed, tons of wpts will be enabled as this try run shows (failures because they expected FAIL or ERROR but got PASS or OK)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c467ace08b5f108f13a036fdfbb930bbf7499504
As the first step I would like to fix this bug, so I will update the patch and trigger a try build for it.
Attachment #8938276 - Flags: review?(ckerschb)
Attachment #8939496 - Flags: review?(ckerschb)
Attachment #8938276 - Flags: review?(ckerschb)
Comment on attachment 8938276 [details]
Bug 1418243 - Fix SecurityPolicyViolationEvent.violatedDirective.

https://reviewboard.mozilla.org/r/209050/#review216622

I like the direction where this is going, but I would like to see one more iteration. thanks

::: dom/security/nsCSPContext.cpp:72
(Diff revision 2)
>  
>  static const uint32_t CSP_CACHE_URI_CUTOFF_SIZE = 512;
>  
> +static void
> +ExtractDirectiveName(nsAString& aDirective)
> +{

We usually don't resue incoming args as outgoing args. I guess it's fine in this case, but please add a comment here and on the callsite.

::: dom/security/nsCSPContext.cpp:76
(Diff revision 2)
> +ExtractDirectiveName(nsAString& aDirective)
> +{
> +  nsAutoString directive(aDirective);
> +
> +  // Remove value part.
> +  directive = Substring(directive, 0, directive.Find(" "));

please add a small example. e.g.
default-src foo -> default-src

::: dom/security/nsCSPContext.cpp:107
(Diff revision 2)
> +    "frame-ancestors", // 6.3.2
> +    "navigation-to", // 6.3.3
> +    // 6.4 Reporting Directives
> +    "report-uri", // 6.4.1
> +    "report-to" // 6.4.2
> +  };

I like the additional verification, but it's yet another list of directives. Any chance we could reuse the one defined in nsCSPUtils.h [1]? Otherwise we have to maintain several lists of directives in the future.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPUtils.h#75
Attachment #8938276 - Flags: review?(ckerschb)
Comment on attachment 8939496 [details]
Bug 1418243 - Enable web-platform tests which expected FAIL because of violatedDirective.

https://reviewboard.mozilla.org/r/209830/#review216624

This looks great, r=me
Attachment #8939496 - Flags: review?(ckerschb) → review+
I re-implemented by adding a new method nsCSPDirective::getDirName. The previous ExtractDirectiveName became ValidateDirectiveName, using the list defined in nsCSPUtils.h.
This change doesn't cause new wpt failures
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01c52ac9ef29bdc70291d45cffc74df00dc82f4b
Comment on attachment 8938276 [details]
Bug 1418243 - Fix SecurityPolicyViolationEvent.violatedDirective.

https://reviewboard.mozilla.org/r/209050/#review217748

Thanks, I like the solution with getDirName much more, thanks for incorporating that. This is basically already an r+ but I would like to see it one more time, must to get my question underneath resolved.

::: dom/security/nsCSPContext.cpp:75
(Diff revision 3)
> +static bool
> +ValidateDirectiveName(const nsAString& aDirective)
> +{
> +  static const auto directives = [] () {
> +    std::unordered_set<std::string> directives;
> +    constexpr size_t N = sizeof(CSPStrDirectives) / sizeof(CSPStrDirectives[0]);

nit: don't use one letter variable names, e.g. make this dirLen or dirLength

::: dom/security/nsCSPContext.cpp:913
(Diff revision 3)
>        NS_WARNING("No blocked URI (null aBlockedContentSource) for CSP violation report.");
>      }
>      aViolationEventInit.mBlockedURI = NS_ConvertUTF8toUTF16(reportBlockedURI);
>    }
>  
> -  // violated-directive
> +  if (ValidateDirectiveName(aViolatedDirective)) {

I am not sure if we want an if() here, do we? I guess we just want:
MOZ_ASSERT(ValidateDirectiveName(aViolatedDirective), "bla");

In theory that should never return false, right?
Attachment #8938276 - Flags: review?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> ::: dom/security/nsCSPContext.cpp:913
> (Diff revision 3)
> >        NS_WARNING("No blocked URI (null aBlockedContentSource) for CSP violation report.");
> >      }
> >      aViolationEventInit.mBlockedURI = NS_ConvertUTF8toUTF16(reportBlockedURI);
> >    }
> >  
> > -  // violated-directive
> > +  if (ValidateDirectiveName(aViolatedDirective)) {
> 
> I am not sure if we want an if() here, do we? I guess we just want:
> MOZ_ASSERT(ValidateDirectiveName(aViolatedDirective), "bla");
> 
> In theory that should never return false, right?

MOZ_ASSERT looks better, but since it is effective only in debug builds, there might be some problems in release builds:

1. the ValidateDirectiveName function will be unused and cause build error

2. the violation will still be reported even the directive name is invalid

So I tried to use NS_WARN_IF instead of MOZ_ASSERT. Besides I'm wondering if it is good to trigger an assertion failure in the condition so that we can stop the program in debug builds, like

    if (NS_WARN_IF(!ValidateDirectiveName(aViolatedDirective))) {
      MOZ_ASSERT(false, "!ValidateDirectiveName(aViolatedDirective)");
      return NS_ERROR_ILLEGAL_VALUE;
    }
Comment on attachment 8938276 [details]
Bug 1418243 - Fix SecurityPolicyViolationEvent.violatedDirective.

https://reviewboard.mozilla.org/r/209050/#review218490

(In reply to Chung-Sheng Fu [:cfu] from comment #19)
> So I tried to use NS_WARN_IF instead of MOZ_ASSERT. Besides I'm wondering if
> it is good to trigger an assertion failure in the condition so that we can
> stop the program in debug builds, like
> 
>     if (NS_WARN_IF(!ValidateDirectiveName(aViolatedDirective))) {
>       MOZ_ASSERT(false, "!ValidateDirectiveName(aViolatedDirective)");
>       return NS_ERROR_ILLEGAL_VALUE;
>     }

I don't think we should stop the violation from being sent in that case. Again, in theory getting an invalid directive name should never happen. I think it makes sense to just use MOZ_ASSERT(ValidateDirectiveName) and also use add ifedef DEBUG around ValidateDirectiveName. Worst case we send a violation with in invalid directive name. Adding MOZ_ASSERT() however helps to find problems in debug builds and also allows our Fuzzing team to do a better job. Agreed?
Attachment #8938276 - Flags: review?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> I don't think we should stop the violation from being sent in that case.
> Again, in theory getting an invalid directive name should never happen. I
> think it makes sense to just use MOZ_ASSERT(ValidateDirectiveName) and also
> use add ifedef DEBUG around ValidateDirectiveName. Worst case we send a
> violation with in invalid directive name. Adding MOZ_ASSERT() however helps
> to find problems in debug builds and also allows our Fuzzing team to do a
> better job. Agreed?

Thank you very much. In fact I was also considering using ifdef but I couldn't tell if it is an acceptable approach, so this works very well for me :)
Comment on attachment 8938276 [details]
Bug 1418243 - Fix SecurityPolicyViolationEvent.violatedDirective.

https://reviewboard.mozilla.org/r/209050/#review218730

Thanks for incorporating those changes, that looks great now. r=me

::: dom/security/nsCSPContext.cpp:76
(Diff revision 6)
> +/**
> + * This function is only used for MOZ_ASSERT in
> + * nsCSPContext::GatherSecurityPolicyViolationEventData, so only compile it
> + * in debug builds in order to avoid build error caused by -Wunused-function
> + * in release builds.
> + */

nit: you can shorten that comment:
This function is only used for verification purposes within GatherSecurityPolicyViolationEventData
Attachment #8938276 - Flags: review?(ckerschb) → review+
MozReview-Commit-ID: 8DQ7CI5exUL
Attachment #8938276 - Attachment is obsolete: true
Attachment #8942826 - Attachment is obsolete: true
Attachment #8939496 - Attachment is obsolete: true
Attachment #8942827 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/778a37000696
Fix SecurityPolicyViolationEvent.violatedDirective. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/5357dbb6df2b
Enable web-platform tests which expected FAIL because of violatedDirective. r=ckerschb
Keywords: checkin-needed
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcef08d7bfb
Backed out 2 changesets for failing mochitest at dom/security/test/csp/test_frame_ancestors_ro.html and mochitest devtools at  devtools/client/webconsole/test/browser_webconsole_bug_1010953_cspro.js a=merge
Backed out for failing mochitest at dom/security/test/csp/test_frame_ancestors_ro.html and mochitest devtools at  devtools/client/webconsole/test/browser_webconsole_bug_1010953_cspro.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5357dbb6df2b38e24d52c803ca0909adf6bc6f9a

Failure logs: test_frame_ancestors_ro.html : https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5357dbb6df2b38e24d52c803ca0909adf6bc6f9a

and

browser_webconsole_bug_1010953_cspro.js : https://treeherder.mozilla.org/logviewer.html#?job_id=156553627&repo=mozilla-inbound&lineNumber=15638

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcef08d7bfb
Flags: needinfo?(cfu)
Attachment #8942828 - Attachment is obsolete: true
Attachment #8942829 - Attachment is obsolete: true
Sorry I didn't notice there are some mochitests expect the old violation-directive format. Could you please take a look a the new patch? Sorry for inconvenience.

Here is the try link with all tests
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdcc569dbc6037fdf80850df8ba1cd391a4555e0
Flags: needinfo?(cfu)
Comment on attachment 8943123 [details] [diff] [review]
Fix mochitest failures due to violationDirective change.

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

Thanks. r=me
Please run such changes through TRY next time before those changes get checked in.
Attachment #8943123 - Flags: review?(ckerschb) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #36)
> Comment on attachment 8943123 [details] [diff] [review]
> Fix mochitest failures due to violationDirective change.
> 
> Review of attachment 8943123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. r=me
> Please run such changes through TRY next time before those changes get
> checked in.

Yes, definitely. Sorry again and thanks for your patience.
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25fd1d8d42b4
Fix SecurityPolicyViolationEvent.violatedDirective. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b40a9ee5e9b
Enable web-platform tests which expected FAIL because of violatedDirective. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1752a8ab7a0
Fix mochitest failures due to violationDirective change. r=ckerschb
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: