Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: fkiefer, Assigned: fkiefer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

To get a better idea of CSP usage we should add some telemetry.

This bug adds telemetry for:
a) how many sites use CSP
b) how many use unsafe-inline
c) how many use unsafe-eval
Created attachment 8725794 [details]
MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg

Review commit: https://reviewboard.mozilla.org/r/37641/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37641/
Attachment #8725794 - Flags: review?(mozilla)
Comment on attachment 8725794 [details]
MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg

https://reviewboard.mozilla.org/r/37641/#review34477

Looks good to me - thanks, but please answer my question underneath before landing.

::: toolkit/components/telemetry/Histograms.json:3298
(Diff revision 1)
> +  },

I don't know what low, high, and n_buckets is, also I don't know what 'opt-out' means in that context. Do you know or did you copy those from MixedContent, SafeBrowsing or other related Telementry code?
Attachment #8725794 - Flags: review?(mozilla) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Comment on attachment 8725794 [details]
> MozReview Request: Bug 1252829 - CSP Telemetry, r?ckerschb
> 
> I don't know what low, high, and n_buckets is, also I don't know what
> 'opt-out' means in that context. Do you know or did you copy those from
> MixedContent, SafeBrowsing or other related Telementry code?

so if the rest is ok, we should get better numbers for the histogram. I'd suggest something like [1]. That should give us a good idea of the distribution of pages. Higher than 1000 is probably not interesting looking at [2]. But this also depends on what exactly we're interested in. Thoughts?

[1] https://telemetry.mozilla.org/histogram-simulator/#low=0&high=1000&n_buckets=20&kind=exponential&generate=log-normal
[2] https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-03-03&keys=__none__!__none__!__none__&max_channel_version=nightly%252F47&measure=PLACES_PAGES_COUNT&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-01-25&table=0&trim=1&use_submission_date=0
Flags: needinfo?(mozilla)
Actually, I took a closer look and I think we have to update a few things:
> "alert_emails": ["seceng@mozilla.com"],
Shouldn't that be: seceng-telemetry@mozilla.com

> "expires_in_version": "55",
Probably we don't want that to expire ever, can't we use |"expires_in_version": "never",|?

> "kind": "exponential",
> "low": 1000,
> "high": 150000,
> "n_buckets": 20,
> "releaseChannelCollection": "opt-out",

Again, I don't fully know what those parameters mean. We know that only around 1% of sites use CSP. If you take the Alexa Top 1million sites.


The other thing I just realized is
>  if (mHasCSP) {
>        Accumulate(Telemetry::CSP_PAGES_COUNT, 1);
>  }
shouldn't we have an |else| branch here that does something like |Accumulate(Telemetry::CSP_PAGES_COUNT, 0);|?

Probably someone who has done Telemetry more often than me has a better understanding of how the accumulation happens.

Please don't land before we have sorted those issues - thanks!
Flags: needinfo?(franziskuskiefer)
Clearing my needinfo for now.
Flags: needinfo?(mozilla)
Comment on attachment 8725794 [details]
MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37641/diff/1-2/
Attachment #8725794 - Attachment description: MozReview Request: Bug 1252829 - CSP Telemetry, r?ckerschb → MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb
https://reviewboard.mozilla.org/r/37641/#review35859

::: dom/base/nsDocument.cpp:1591
(Diff revision 2)
> +        Accumulate(Telemetry::CSP_PAGES_COUNT, 0);

When using `"count"` probes here, you only need to `Accumulate(..., 1)` and don't need this.

::: toolkit/components/telemetry/Histograms.json:3286
(Diff revision 2)
> +    "kind": "exponential",

From the `Accumulate(..., 1)` code, you actually want to use `"kind": "count"` here.

::: toolkit/components/telemetry/Histograms.json:3290
(Diff revision 2)
> +    "releaseChannelCollection": "opt-out",

Is collecting this data from the beta channel sufficient?
Unless there is a specific need to collect this from (nearly) all release users, let's leave out this line (it defaults to "opt-in").
Comment on attachment 8725794 [details]
MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37641/diff/2-3/
Thanks Georg for the good feedback. I updated the patch as discussed.
Flags: needinfo?(franziskuskiefer) → needinfo?(gfritzsche)
So this should be fine.

Ally, can you do the data collection review?
Flags: needinfo?(gfritzsche) → needinfo?(a.m.naaktgeboren)
https://reviewboard.mozilla.org/r/37641/#review35919

::: dom/base/nsDocument.cpp:1591
(Diff revision 3)
> +        Accumulate(Telemetry::CSP_PAGES_COUNT, 0);

You should remove the 0 count here.
not my day, forgot to update that. Will wait for data collection review from ally before uploading a new version.
maybe Benjamin has a minute for this data collection review.
Flags: needinfo?(benjamin)

Comment 15

2 years ago
https://reviewboard.mozilla.org/r/37641/#review36393

::: toolkit/components/telemetry/Histograms.json:3287
(Diff revision 3)
> +  "CSP_PAGES_COUNT": {
> +    "alert_emails": ["seceng@mozilla.com"],
> +    "bug_numbers": [1252829],
> +    "expires_in_version": "55",
> +    "kind": "count",
> +    "description": "Number of unique pages that contain a CSP"

"pages" here is a little ambiguous. Does this count all loaded document (including frames/iframes), or just top-level documents? I suggest using "documents" instead of "pages" and clarifying that. That way it's clear that in-page navigation such as .pushState or .replace aren't counted in the totals.

Is it important for you to know the ratio of "documents with CSP" to "total documents" or is this count useful on its own?

Comment 16

2 years ago
Comment on attachment 8725794 [details]
MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg

data-review=me in any case
Flags: needinfo?(benjamin)
Attachment #8725794 - Flags: feedback+

Updated

2 years ago
Flags: needinfo?(a.m.naaktgeboren)
So, I am fine with that. Again, what we really want to know is how many (toplevel) sites use CSP, how many of those use unsafe-inline and unsafe-eval. It seems we can query the total number of sites from the mixed content telemetry reports as well. Please incorporate Benjamin's suggestions and we are good to go. Finally getting some stats on CSP.
Comment on attachment 8725794 [details]
MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37641/diff/3-4/
Attachment #8725794 - Attachment description: MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb → MozReview Request: Bug 1252829 - CSP Telemetry, r=ckerschb, p=bsmedberg
Attachment #8725794 - Flags: feedback+
Keywords: checkin-needed
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> Actually, I took a closer look and I think we have to update a few things:
> > "alert_emails": ["seceng@mozilla.com"],
> Shouldn't that be: seceng-telemetry@mozilla.com

I think seceng will not be happy to receive those emails. I suppose you wanted to update that before landing, no?
 
> > "expires_in_version": "55",
> Probably we don't want that to expire ever, can't we use
> |"expires_in_version": "never",|?

Why do we only care about CSP telemetry only up to FF55? Didn't we agree on using 'never'?
Flags: needinfo?(franziskuskiefer)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> > Actually, I took a closer look and I think we have to update a few things:
> > > "alert_emails": ["seceng@mozilla.com"],
> > Shouldn't that be: seceng-telemetry@mozilla.com
> 
> I think seceng will not be happy to receive those emails. I suppose you
> wanted to update that before landing, no?

Ah, missed that. seceng@ is what most other probes have as well. I don't know who gets those e-mails anyway.

> > > "expires_in_version": "55",
> > Probably we don't want that to expire ever, can't we use
> > |"expires_in_version": "never",|?
> 
> Why do we only care about CSP telemetry only up to FF55? Didn't we agree on
> using 'never'?

'never' is not accepted by telemetry folks anymore. 55 should be fine for a while. We have to update then once we hit 54 latest.
Flags: needinfo?(franziskuskiefer)

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/08b8dd56a2b8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.