1384741 - Stylo: incorrect CSP reports when layout.css.servo.enabled is true

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Peter Gerber (:pege)]

Nightly reports incorrect CSP violations when layout.css.servo.enabled is enabled.

Steps to reproduce:

1. enable layout.css.servo.enabled
2. open JS console
3. go to https://duckduckgo.com
4. now go to https://tocco.ch (by typing it in the address bar)
5. observe the CSP errors in the JS console (they do not appear when servo.enabled = false)

JS console output:

Content Security Policy: The page’s settings observed the loading of a resource at https://duckduckgo.com/font/ProximaNova-Sbold-webfont.woff (“font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com”). A CSP report is being sent.

Content Security Policy: The page’s settings observed the loading of a resource at https://duckduckgo.com/font/ProximaNova-Reg-webfont.woff (“font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com”). A CSP report is being sent.

CSP reports:

{
 "csp-report": {
  "violated-directive": "font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com",
  "referrer": "",
  "original-policy": "default-src https://www.tocco.ch; style-src https://www.tocco.ch 'unsafe-inline' data: https://fonts.googleapis.com https://maxcdn.bootstrapcdn.com; font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com; script-src https://www.tocco.ch 'unsafe-inline' 'unsafe-eval' https://ssl.google-analytics.com https://s7.addthis.com https://m.addthis.com https://m.addthisedge.com https://graph.facebook.com https://api-public.addthis.com https://maps.googleapis.com https://www.linkedin.com https://analytics.google.com https://www.google.com https://ssl.gstatic.com; connect-src https://www.tocco.ch https://s7.addthis.com https://m.addthis.com; img-src * data:; child-src https://www.tocco.ch https://manual.tocco.ch https://s7.addthis.com https://docs.google.com https://www.youtube.com https://calendar.google.com https://www.google.com https://www.facebook.com; report-uri https://csp-reports.tocco.ch/r",
  "document-uri": "https://www.tocco.ch/",
  "blocked-uri": "https://duckduckgo.com"
 },
 "x-source-info": {
  "timestamp": "2017-07-26T22:27:53.614739Z",
  "user-agent": "Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0",
  "path": "/r",
  "client-address": "155.4.230.97",
  "host": "csp-reports.tocco.ch"
 }
}

{
 "csp-report": {
  "violated-directive": "font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com",
  "referrer": "",
  "original-policy": "default-src https://www.tocco.ch; style-src https://www.tocco.ch 'unsafe-inline' data: https://fonts.googleapis.com https://maxcdn.bootstrapcdn.com; font-src https://www.tocco.ch data: https://fonts.google.com https://fonts.gstatic.com https://maxcdn.bootstrapcdn.com; script-src https://www.tocco.ch 'unsafe-inline' 'unsafe-eval' https://ssl.google-analytics.com https://s7.addthis.com https://m.addthis.com https://m.addthisedge.com https://graph.facebook.com https://api-public.addthis.com https://maps.googleapis.com https://www.linkedin.com https://analytics.google.com https://www.google.com https://ssl.gstatic.com; connect-src https://www.tocco.ch https://s7.addthis.com https://m.addthis.com; img-src * data:; child-src https://www.tocco.ch https://manual.tocco.ch https://s7.addthis.com https://docs.google.com https://www.youtube.com https://calendar.google.com https://www.google.com https://www.facebook.com; report-uri https://csp-reports.tocco.ch/r",
  "document-uri": "https://www.tocco.ch/",
  "blocked-uri": "https://duckduckgo.com"
 },
 "x-source-info": {
  "timestamp": "2017-07-26T22:27:53.655327Z",
  "user-agent": "Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0",
  "path": "/r",
  "client-address": "155.4.230.97",
  "host": "csp-reports.tocco.ch"
 }
}

Comment 1

[Darkspirit]

Thank you :)
I can confirm the different console outputs (related to CSP) in Nightly 56 x64 20170726100241 @ Debian Testing.

Comment 2

[Darkspirit]

Attachment: Screenshot_20170727_120549.png

Comment 3

[Cameron McCormack (:heycam)]

Thanks for the bug report, Peter.

It looks like this is due to the pre-emptive calling of FontFaceSet::IsFontLoadAllowed we added in part 4 of bug 1376964.  I guess it wasn't obvious to me that the NS_CheckContentLoadPolicy call not only does a check, but could result in CSP reports being sent.

I guess what I want is a way to call NS_CheckContentLoadPolicy, and to store away the CSP report that would be made due to the CSP failure, so that later on, from the style worker threads, if we look up the cached IsFontLoadAllowed result and find that it failed due to a CSP check, that we can make the report at that time.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports.

Review commit: https://reviewboard.mozilla.org/r/162730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162730/

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8891630 [details]
Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports.

Chrisoph, can you tell me if the changes I'm making to the CSP code seems reasonable to you?  I want to capture all of the reports that the nsCSPContext might make while under the FontFaceSet::IsFontLoadAllowed call, so that I can dispatch their runnables later if a page actually tries to make such a request.

Comment 6

[Cameron McCormack (:heycam)]

I initially thought I'd just pass an argument all the way down from NS_CheckContentLoadPolicy, but I wasn't sure that would work if redirects are involved, since it seemed tricky to ensure that argument would get into CSPService::AsyncOnChannelRedirect.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8891630 [details]
Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports.

(In reply to Cameron McCormack (:heycam) from comment #5)
> Chrisoph, can you tell me if the changes I'm making to the CSP code seems
> reasonable to you?

Yes, that all seems reasonable to me. Please also make sure to check with a dom/ peer, but as I said, looks good to me.

(In reply to Cameron McCormack (:heycam) from comment #6)
> I initially thought I'd just pass an argument all the way down from
> NS_CheckContentLoadPolicy, but I wasn't sure that would work if redirects
> are involved, since it seemed tricky to ensure that argument would get into
> CSPService::AsyncOnChannelRedirect.

Unfortunately too many addons are still relying NS_CheckContentLoadPolicy(), hence updating that macro is not going to work. In other words, you are using the right approach.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Obviously a testcase would be nice to have for that particular usecase.

Also JFYI: there are numerous CSP tests, you can run them with:
./mach test dom/security/test/csp

Comment 9

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77008ae961bc63e3b711c4644670700ebc7b6a6c&group_state=expanded

Comment 10

[Cameron McCormack (:heycam)]

Attachment: Bug 1384741 - Part 2: Allow file_report_chromescript.js to listen for more than one CSP violation report.

Review commit: https://reviewboard.mozilla.org/r/165570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165570/

Comment 11

[Cameron McCormack (:heycam)]

Attachment: Bug 1384741 - Part 3: Buffer up CSP violation reports when pre-emptively checking cached font loads, and dispatch them when trying to use cached fonts.

Review commit: https://reviewboard.mozilla.org/r/165572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165572/

Comment 12

[Cameron McCormack (:heycam)]

Attachment: Bug 1384741 - Part 4: Test that we don't send CSP violation reports for cached fonts we don't actually use.

Review commit: https://reviewboard.mozilla.org/r/165574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165574/

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8891630 [details]
Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162730/diff/1-2/

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8894387 [details]
Bug 1384741 - Part 3: Buffer up CSP violation reports when pre-emptively checking cached font loads, and dispatch them when trying to use cached fonts.

https://reviewboard.mozilla.org/r/165572/#review170842

::: gfx/thebes/gfxUserFontSet.cpp:1408
(Diff revision 1)
> +    gfxUserFontSet* aUserFontSet) const
> +{
> +    LoadResultEntry* entry = mAllowedFontSets.GetEntry(aUserFontSet);
> +    MOZ_ASSERT(entry, "UpdateAllowedFontSets should have been called and "
> +                      "added an entry to mAllowedFontSets");
> +    if (!entry->mViolations.IsEmpty()) {

Can we assert that `entry->mAllowed == entry->mViolations.IsEmpty()` here? i.e. if allowed, there are no violations; if not, at least one should have been recorded.

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8894388 [details]
Bug 1384741 - Part 4: Test that we don't send CSP violation reports for cached fonts we don't actually use.

https://reviewboard.mozilla.org/r/165574/#review170942

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8891630 [details]
Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports.

https://reviewboard.mozilla.org/r/162730/#review172460

::: dom/base/nsIDocument.h:878
(Diff revision 2)
> +
> +  /**
> +   * Called when a CSP violation is encountered that would generate a report
> +   * while buffering is enabled.
> +   */
> +  void ReportCSPViolation(nsIRunnable* aReportingRunnable)

Maybe this should be called BufferCSPViolation?

::: dom/base/nsIDocument.h:881
(Diff revision 2)
> +   * while buffering is enabled.
> +   */
> +  void ReportCSPViolation(nsIRunnable* aReportingRunnable)
> +  {
> +    MOZ_ASSERT(mBufferingCSPViolations);
> +    mBufferedCSPViolations.AppendElement(aReportingRunnable);

Could there be enough of these in flight that we could OOM here?  If so, it might be better to drop reports on the floor on OOM than crash...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8894386 [details]
Bug 1384741 - Part 2: Allow file_report_chromescript.js to listen for more than one CSP violation report.

https://reviewboard.mozilla.org/r/165570/#review172462

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8894387 [details]
Bug 1384741 - Part 3: Buffer up CSP violation reports when pre-emptively checking cached font loads, and dispatch them when trying to use cached fonts.

https://reviewboard.mozilla.org/r/165572/#review170842

> Can we assert that `entry->mAllowed == entry->mViolations.IsEmpty()` here? i.e. if allowed, there are no violations; if not, at least one should have been recorded.

We might have recorded mAllowed = false for a non-CSP violation related reason.  Or the CSP directives might not have indicated that reports are sent.  But I can assert that if the array is non-empty, then mAllowed must be false.

Comment 20

[Cameron McCormack (:heycam)]

Comment on attachment 8894387 [details]
Bug 1384741 - Part 3: Buffer up CSP violation reports when pre-emptively checking cached font loads, and dispatch them when trying to use cached fonts.

https://reviewboard.mozilla.org/r/165572/#review170842

> We might have recorded mAllowed = false for a non-CSP violation related reason.  Or the CSP directives might not have indicated that reports are sent.  But I can assert that if the array is non-empty, then mAllowed must be false.

Actually, I think that's not even possible to assert, since we might only have CSP violation reports enabled, but the load is still allowed.

Comment 21

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eaba522ad86c57e967d26e97dad40985f58ca75

Comment 22

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s c81087bbf0d3 -d cbacd472fc22: rebasing 413121:c81087bbf0d3 "Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports. r=bz"
merging dom/base/nsDocument.cpp
merging dom/base/nsIDocument.h
merging dom/security/nsCSPContext.cpp
warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/base/nsIDocument.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8891630 [details]
Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports.

We should uplift this to beta for the pref experiment to avoid annoying web site operators with spurious CSP report violations.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1376964
[User impact if declined]: spurious CSP violation reports can be sent if stylo is enabled
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no, just landed on autoland
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: all four patches on this bug
[Is the change risky?]: not very
[Why is the change risky/not risky?]: it only affects stylo, and we have tests
[String changes made/needed]: none

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41c3fa41a268
Part 1: Add facility to buffer up CSP violation reports. r=bz
https://hg.mozilla.org/integration/autoland/rev/de7dba78e6fe
Part 2: Allow file_report_chromescript.js to listen for more than one CSP violation report. r=bz
https://hg.mozilla.org/integration/autoland/rev/549366daed98
Part 3: Buffer up CSP violation reports when pre-emptively checking cached font loads, and dispatch them when trying to use cached fonts. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/4d5fc5ec7769
Part 4: Test that we don't send CSP violation reports for cached fonts we don't actually use. r=jfkthame

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a3ab2692bc9
Followup bustage fix. r=me

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/41c3fa41a268
https://hg.mozilla.org/mozilla-central/rev/de7dba78e6fe
https://hg.mozilla.org/mozilla-central/rev/549366daed98
https://hg.mozilla.org/mozilla-central/rev/4d5fc5ec7769
https://hg.mozilla.org/mozilla-central/rev/1a3ab2692bc9

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8891630 [details]
Bug 1384741 - Part 1: Add facility to buffer up CSP violation reports.

Avoid unnecessary CSP warnings. Please uplift for beta 3 to support the upcoming stylo experiment on beta.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 29

[Cameron McCormack (:heycam)]

try push for rebased patches on beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=728218fad66f5c09385588b979507788eca44b39

Comment 30

[Cameron McCormack (:heycam)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/2ca70aa40eae0268d6abf65175aa93a7c14fa7a1
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/ce4bac7cc01ee52323fbd9b118a338622cfc068c
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/083d55339a1dff04531da5aca017191bac5f42a3
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/578f340363a37e0887038e39a970e6b83cd00563
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/87ae6494d5b1fff0b2cd388c6d639e29f438fe77