Closed Bug 1384741 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: pege, Assigned: heycam)

References

(Blocks 1 open bug, )

Details

(Keywords: nightly-community)

Attachments

(5 files)

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"
 }
}
Thank you :)
I can confirm the different console outputs (related to CSP) in Nightly 56 x64 20170726100241 @ Debian Testing.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
Priority: -- → P2
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.
Flags: needinfo?(cam)
Blocks: 1376964
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.
Attachment #8891630 - Flags: feedback?(ckerschb)
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 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.
Attachment #8891630 - Flags: feedback?(ckerschb) → feedback+
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
Priority: P2 → --
Priority: -- → P2
Flags: needinfo?(cam)
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.
Attachment #8894387 - Flags: review?(jfkthame) → review+
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
Attachment #8894388 - Flags: review?(jfkthame) → review+
Attachment #8891630 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Attachment #8894386 - Flags: review?(bobbyholley) → review?(xidorn+moz)
Attachment #8891630 - Flags: review?(xidorn+moz) → review?(bzbarsky)
Attachment #8894386 - Flags: review?(xidorn+moz) → review?(bzbarsky)
Flags: needinfo?(cam)
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...
Attachment #8891630 - Flags: review?(bzbarsky) → review+
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
Attachment #8894386 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(cam)
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 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.
Flags: needinfo?(cam)
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 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
Attachment #8891630 - Flags: approval-mozilla-beta?
Pushed by cmccormack@mozilla.com:
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 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.
Attachment #8891630 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing.
Flags: needinfo?(cam)
Flags: in-testsuite+
Flags: needinfo?(cam)
Depends on: 1406474
You need to log in before you can comment on or make changes to this bug.