Closed
Bug 1303685
Opened 8 years ago
Closed 8 years ago
Gather telemetry before removing 'referrer' directive from CSP
Categories
(Core :: DOM: Security, defect, P3)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: ckerschb, Assigned: tuhinatwyla)
References
Details
(Whiteboard: [domsecurity-backlog1])
Attachments
(1 file, 10 obsolete files)
4.38 KB,
patch
|
francois
:
review+
|
Details | Diff | Splinter Review |
Reporter | ||
Updated•8 years ago
|
Assignee | ||
Comment 1•8 years ago
|
||
I am working on this bug as a part of Outreachy application. I have some queries :
1- Which build system to configure Firefox to? I have the build system for Android(C++) set up.
2- What does the boolean value mReferrerPolicySet in nsIDocument.h[1] represent ?
3- The bug asks to log a warning for deprecating CSP referrer directive. Is CSPPARSERLOG("message");[2] the right way to log warnings ?
4- Please guide me on the approach :
a- Adding a metric CSP_REFERRER_POLICY in Histogram.json[3]
b- Using boolean mReferrerPolicySet[4] to increment the Telemetry by 1 in nsDocument.cpp[5]
c- Logging a warning in nsCSPParser.cpp[6]
[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2861
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#862
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#3806
[4] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIDocument.h#2861
[5] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#1450
[6] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#867
Flags: needinfo?(ckerschb)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(FrederikBraun)
Comment 2•8 years ago
|
||
(Please needinfo fbraun@mozilla.com, not the other Frederik Braun :-))
Most of the questions are well-explained on articles in MDN or our wiki, so let me respond with a lot of links :-)
1) https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Source_Code and https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
2) Looking at dxr.mozilla.org (like you did!), it seems this is an internal state variable to figure out of the document has a referrer policy
3) No, CSPPARSERLOG is initialized here https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#25 and used for internal/debug logging, that is disabled during release. See also NSPR-Logging (now called MOZLOG I think). Logging to the developer console happens in other code, e.g., here https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#258
4) Sorry, I have not worked with telemetry yet. I trust you can figure this out for yourself. If not, I want to encourage you learning to ask the right people and the right questions on IRC chat (https://wiki.mozilla.org/IRC) ;-) - I would be even more excited if you managed to write a very short summary if there isn't any kind of documentation to add gather new telemetry. The folks in our documentation team would instantly like you (and possibly turn this into a great page on MDN) :-)
Flags: needinfo?(FrederikBraun)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8796918 -
Flags: review?(fbraun)
Assignee | ||
Updated•8 years ago
|
Attachment #8796918 -
Flags: review?(ckerschb)
Reporter | ||
Comment 5•8 years ago
|
||
Comment on attachment 8796918 [details] [diff] [review]
patch.diff
Review of attachment 8796918 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsDocument.cpp
@@ +1455,5 @@
> if (mHasUnsafeEvalCSP) {
> Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1);
> }
> + if (mReferrerPolicySet) {
> + Accumulate(Telemetry::CSP_REFERRER_POLICY, 1);
mReferrerPoliySet is also used if the referrer is set by the actual header, not only by CSP. I think you have to add an additional flag, similar to mHasUnafeEvalCSP.
::: dom/security/nsCSPParser.cpp
@@ +867,5 @@
>
> // the referrer policy is valid, so go ahead and use it.
> + const char16_t* params[] = { mCurToken.get() };
> + logWarningErrorToConsole(nsIScriptError::warningFlag, "ValidReferrerPolicy",
> + params, ArrayLength(params));
Bug 1303682 already added a deprecation warning - please remove!
Attachment #8796918 -
Flags: review?(fbraun)
Attachment #8796918 -
Flags: review?(ckerschb)
Attachment #8796918 -
Flags: review-
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8796918 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8797741 -
Flags: review?(fbraun)
Assignee | ||
Updated•8 years ago
|
Attachment #8797741 -
Flags: review?(ckerschb)
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8797741 [details] [diff] [review]
fix.diff
Review of attachment 8797741 [details] [diff] [review]:
-----------------------------------------------------------------
Almost, changes within nsDocument.cpp, nsIDocument.h and Histograms.json look good. Please update the code according to my suggestions underneath and flag me again for review.
::: dom/security/nsCSPParser.cpp
@@ +566,5 @@
> + if (doc) {
> + doc->SetHasReferrerPolicyCSP(true);
> + }
> + return new nsCSPKeywordSrc(CSP_KeywordToEnum(mCurToken));
> + }
Referrer is not a keyword. Please just add the code where you query the doc from the context here [1] and we are good to go - thanks!
[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#873
::: dom/security/nsCSPUtils.h
@@ +124,5 @@
> CSP_UNSAFE_INLINE,
> CSP_UNSAFE_EVAL,
> CSP_NONE,
> CSP_NONCE,
> + CSP_REFERRER_POLICY,
Please discard all changes within nsCSPUtils.h
Attachment #8797741 -
Flags: review?(fbraun)
Attachment #8797741 -
Flags: review?(ckerschb)
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → tuhinatwyla
Status: NEW → ASSIGNED
Reporter | ||
Comment 8•8 years ago
|
||
Adding francois because we need a data-review for gathering telemetry data in the end.
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8797741 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8799249 -
Flags: review?(ckerschb)
Assignee | ||
Updated•8 years ago
|
Attachment #8799249 -
Flags: review?(fbraun)
Comment 10•8 years ago
|
||
Comment on attachment 8799249 [details] [diff] [review]
new.diff
Review of attachment 8799249 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me, but we need a module peer to give an r+ before you can land.
I found some tiny nits that I'd like you to fix below, then flag ckerschb for review:
::: dom/security/nsCSPParser.cpp
@@ +558,5 @@
> doc->SetHasUnsafeEvalCSP(true);
> }
> return new nsCSPKeywordSrc(CSP_KeywordToEnum(mCurToken));
> }
> +
You introduced a newline here. Please remove.
::: toolkit/components/telemetry/Histograms.json
@@ +3854,5 @@
> + "alert_emails": ["seceng@mozilla.com"],
> + "bug_numbers": [1303685],
> + "expires_in_version": "55",
> + "kind": "count",
> + "description": "Number of unique pages that contain an referrer CSP directive"
Nit: "a CSP referrer directive"
Attachment #8799249 -
Flags: review?(fbraun) → review+
Reporter | ||
Comment 11•8 years ago
|
||
Comment on attachment 8799249 [details] [diff] [review]
new.diff
Review of attachment 8799249 [details] [diff] [review]:
-----------------------------------------------------------------
r=me, but please fix freddy's nits.
@Francois: Can you do a quick data-review for the telemetry data gathered? Thanks!
Attachment #8799249 -
Flags: review?(francois)
Attachment #8799249 -
Flags: review?(ckerschb)
Attachment #8799249 -
Flags: review+
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8799249 -
Attachment is obsolete: true
Attachment #8799249 -
Flags: review?(francois)
Attachment #8799471 -
Flags: review?(fbraun)
Attachment #8799471 -
Flags: review?(ckerschb)
Assignee | ||
Updated•8 years ago
|
Attachment #8799471 -
Flags: review?(francois)
Assignee | ||
Comment 13•8 years ago
|
||
Adding Francois to review the new diff.
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8799471 [details] [diff] [review]
final.diff
Review of attachment 8799471 [details] [diff] [review]:
-----------------------------------------------------------------
Carrying over r+ from Freddy and Me.
Francois, can you please do the data-review?
Attachment #8799471 -
Flags: review?(fbraun)
Attachment #8799471 -
Flags: review?(ckerschb)
Attachment #8799471 -
Flags: review+
Comment 15•8 years ago
|
||
Comment on attachment 8799471 [details] [diff] [review]
final.diff
Review of attachment 8799471 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not sure that the telemetry data collected here is the right one.
What question are you trying to answer? Is it: what perecentage of sites with a site policy include the "referrer" directive?
If so, I would suggest using a "boolean" type and reporting either "false" when we encounter a CSP policy without "referrer" and "true" when we encounter a CSP policy with a "referrer" directive.
Attachment #8799471 -
Flags: review?(francois) → review-
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8799471 -
Attachment is obsolete: true
Attachment #8800351 -
Flags: review?(francois)
Attachment #8800351 -
Flags: review?(fbraun)
Attachment #8800351 -
Flags: review?(ckerschb)
Comment 17•8 years ago
|
||
Comment on attachment 8800351 [details] [diff] [review]
new.diff
Review of attachment 8800351 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/telemetry/Histograms.json
@@ +3854,5 @@
> + "alert_emails": ["seceng@mozilla.com"],
> + "bug_numbers": [1303685],
> + "expires_in_version": "55",
> + "kind": "boolean",
> + "description": "whether CSP referrer directive is present"
Assuming that the probe is in the right place (I'll let Christoph/Freddy confirm), the description needs to be more explicit.
Maybe something like:
Whether a document with a CSP policy (report-only or enforcing) contains a referrer directive ("true") or not ("false").
Attachment #8800351 -
Flags: review?(francois) → review-
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8800351 -
Attachment is obsolete: true
Attachment #8800351 -
Flags: review?(fbraun)
Attachment #8800351 -
Flags: review?(ckerschb)
Attachment #8800537 -
Flags: review?(francois)
Attachment #8800537 -
Flags: review?(fbraun)
Attachment #8800537 -
Flags: review?(ckerschb)
Reporter | ||
Comment 19•8 years ago
|
||
Comment on attachment 8800537 [details] [diff] [review]
new.diff
Review of attachment 8800537 [details] [diff] [review]:
-----------------------------------------------------------------
Clearing review, please follow my nit and flag us again - thanks!
::: dom/base/nsDocument.cpp
@@ +1454,5 @@
> }
> if (mHasUnsafeEvalCSP) {
> Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1);
> }
> + Accumulate(Telemetry::CSP_REFERRER_POLICY, mHasReferrerPolicyCSP);
This was correct before, now we are reporting for *all* documents which pollutes the result. Please revert.
Attachment #8800537 -
Flags: review?(francois)
Attachment #8800537 -
Flags: review?(fbraun)
Attachment #8800537 -
Flags: review?(ckerschb)
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8800537 -
Attachment is obsolete: true
Attachment #8800793 -
Flags: review?(francois)
Attachment #8800793 -
Flags: review?(fbraun)
Attachment #8800793 -
Flags: review?(ckerschb)
Comment 21•8 years ago
|
||
Comment on attachment 8800793 [details] [diff] [review]
final.diff
Review of attachment 8800793 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsDocument.cpp
@@ +1455,5 @@
> if (mHasUnsafeEvalCSP) {
> Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1);
> }
> + if (mHasReferrerPolicyCSP) {
> + Accumulate(Telemetry::CSP_REFERRER_POLICY, true);
This won't keep track of CSP policies that don't use the referrer directive.
You should replace this if statement with:
Accumulate(Telemetry::CSP_REFERRER_POLICY, mHasReferrerPolicyCSP);
so that a telemetry ping is generated regardless of whether or not a referrer directive is present.
::: toolkit/components/telemetry/Histograms.json
@@ +3849,5 @@
> "expires_in_version": "55",
> "kind": "count",
> "description": "Number of unique pages that contain an unsafe-eval CSP directive"
> },
> + "CSP_REFERRER_POLICY": {
nit: A better name for this would be "CSP_REFERRER_DIRECTIVE".
Attachment #8800793 -
Flags: review?(francois) → review-
Assignee | ||
Comment 22•8 years ago
|
||
Attachment #8800980 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8800793 -
Flags: review?(fbraun)
Attachment #8800793 -
Flags: review?(ckerschb)
Assignee | ||
Updated•8 years ago
|
Attachment #8800980 -
Flags: review?(fbraun)
Attachment #8800980 -
Flags: review?(ckerschb)
Assignee | ||
Comment 23•8 years ago
|
||
Attachment #8800793 -
Attachment is obsolete: true
Attachment #8800980 -
Attachment is obsolete: true
Attachment #8800980 -
Flags: review?(francois)
Attachment #8800980 -
Flags: review?(fbraun)
Attachment #8800980 -
Flags: review?(ckerschb)
Attachment #8800982 -
Flags: review?(francois)
Attachment #8800982 -
Flags: review?(fbraun)
Attachment #8800982 -
Flags: review?(ckerschb)
Comment 24•8 years ago
|
||
Comment on attachment 8800982 [details] [diff] [review]
final.diff
Review of attachment 8800982 [details] [diff] [review]:
-----------------------------------------------------------------
Please address the following, then flag Francois for review.
::: dom/base/nsDocument.cpp
@@ +1454,5 @@
> }
> if (mHasUnsafeEvalCSP) {
> Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1);
> }
> + Accumulate(Telemetry::CSP_REFERRER_DIRECTIVE, mHasReferrerPolicyCSP);
As Christoph mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c19: This reports for *every* document, whether it has the directive.
What we want is to report only for the documents that have the directive.
Please wrap this in an "if" statement and set the value to 1, like the other cases above.
Attachment #8800982 -
Flags: review?(francois)
Attachment #8800982 -
Flags: review?(fbraun)
Attachment #8800982 -
Flags: review?(ckerschb)
Attachment #8800982 -
Flags: feedback+
Assignee | ||
Comment 25•8 years ago
|
||
The reason for not enclosing it in if statement is that CSP_REFERRER_DIRECTIVE is a boolean metric that is true when mHasReferrerPolicyCSP is true and false otherwise. The telemetry page would generate a pie chart of two segments(true and false), where 'true' is the % of documents that have mHasReferrerPolicyCSP true and 'false' is the % of documents that have mHasReferrerPolicyCSP false. This would report for all documents, mark them either true or false.
Refer Francois' comment https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c21
Should I continue with CSP_REFERRER_DIRECTIVE as boolean or count ?
Flags: needinfo?(francois)
Flags: needinfo?(fbraun)
Flags: needinfo?(ckerschb)
Comment 26•8 years ago
|
||
Comment on attachment 8800982 [details] [diff] [review]
final.diff
Right. Sorry about the misunderstanding :(
Francois, please review.
Flags: needinfo?(francois)
Flags: needinfo?(fbraun)
Flags: needinfo?(ckerschb)
Attachment #8800982 -
Flags: review?(francois)
Updated•8 years ago
|
Attachment #8800982 -
Flags: review?(francois) → review+
Comment 27•8 years ago
|
||
Also, datareview+.
Updated•8 years ago
|
Keywords: checkin-needed
Comment 28•8 years ago
|
||
does not apply cleanly, could you take a look, thanks
Flags: needinfo?(tuhinatwyla)
Keywords: checkin-needed
Assignee | ||
Comment 29•8 years ago
|
||
This is the same patch on the latest firefox code. Do I need to get it reviewed again ?
Flags: needinfo?(tuhinatwyla) → needinfo?(cbook)
Attachment #8802177 -
Flags: review?(fbraun)
Attachment #8802177 -
Flags: review?(ckerschb)
Reporter | ||
Comment 30•8 years ago
|
||
Comment on attachment 8802177 [details] [diff] [review]
patch.diff
Review of attachment 8802177 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsDocument.cpp
@@ +1438,5 @@
> }
> if (mHasUnsafeEvalCSP) {
> Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1);
> }
> + Accumulate(Telemetry::CSP_REFERRER_DIRECTIVE, mHasReferrerPolicyCSP);
I think I already pointed out that this is incorrect. This will report for *all* documents whether we set a referrer Policy or not. The information we want to gather is how many pages that have a CSP policy use the referrer directive, which is quite different, no?
Attachment #8802177 -
Flags: review?(fbraun)
Attachment #8802177 -
Flags: review?(ckerschb)
Attachment #8802177 -
Flags: review-
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(cbook)
Reporter | ||
Comment 31•8 years ago
|
||
Comment on attachment 8802177 [details] [diff] [review]
patch.diff
Review of attachment 8802177 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsDocument.cpp
@@ +1443,1 @@
> }
Probably best if you just move the Accumulate part within
if (mHasCSP) {
...
Accumulate(Telemetry::CSP_REFERRER_DIRECTIVE, mHasReferrerPolicyCSP);
}
Assignee | ||
Comment 32•8 years ago
|
||
refer https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c25 and
https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c26
Flags: needinfo?(ckerschb)
Reporter | ||
Comment 33•8 years ago
|
||
(In reply to tuhina chatterjee from comment #32)
> refer https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c25 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=1303685#c26
Please see comment 31 - thanks!
Flags: needinfo?(ckerschb)
Comment 34•8 years ago
|
||
Sorry, I should have responded to this earlier. I missed Christoph's comment the first time and then assumed from Freddy's reply that it was now clear.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> ::: dom/base/nsDocument.cpp
> @@ +1438,5 @@
> > }
> > if (mHasUnsafeEvalCSP) {
> > Accumulate(Telemetry::CSP_UNSAFE_EVAL_DOCUMENTS_COUNT, 1);
> > }
> > + Accumulate(Telemetry::CSP_REFERRER_DIRECTIVE, mHasReferrerPolicyCSP);
>
> I think I already pointed out that this is incorrect. This will report for
> *all* documents whether we set a referrer Policy or not. The information we
> want to gather is how many pages that have a CSP policy use the referrer
> directive, which is quite different, no?
The original implementation was to use a count that was only counting the documents that have the directive. I'm not entirely sure whether or not that kind of usage is supported in telemetry because I haven't really seen it, but more importantly, I don't think it's going to tell you anything useful.
For example, if the number you get is "50". What does that tell you? It could just be the same person reloading a single page 50 times. In that case, usage of that directive is actually almost zero. However, if those 50 are actually out of about 500 CSP policies, then it's a big deal.
That's why I suggested using a boolean and looking at the ratio of "CSP policies with referrer" / "all documents with a CSP policy"
I think that will give you the number you need: "The referrer directive is used in 0.00001% of CSP policies for documents that have one."
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(ckerschb)
Reporter | ||
Comment 35•8 years ago
|
||
(In reply to François Marier [:francois] from comment #34)
> That's why I suggested using a boolean and looking at the ratio of "CSP
> policies with referrer" / "all documents with a CSP policy"
Sorry for the lag of response. I was out at a conference. Anyway, I agree - I suppose that's what I suggested within comment 31. If the page has a CSP we collect whether the referrer directive is in use.
Flags: needinfo?(ckerschb)
Assignee | ||
Comment 36•8 years ago
|
||
Final diff
Attachment #8800982 -
Attachment is obsolete: true
Attachment #8802177 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8807605 -
Flags: review?(francois)
Attachment #8807605 -
Flags: review?(fbraun)
Comment 37•8 years ago
|
||
Comment on attachment 8807605 [details] [diff] [review]
final_review_patch.diff
Review of attachment 8807605 [details] [diff] [review]:
-----------------------------------------------------------------
datareview+ provided you change the email address
::: toolkit/components/telemetry/Histograms.json
@@ +3874,5 @@
> "kind": "count",
> "description": "Number of unique pages that contain an unsafe-eval CSP directive"
> },
> + "CSP_REFERRER_DIRECTIVE": {
> + "alert_emails": ["seceng@mozilla.com"],
Use "seceng-telemetry@mozilla.com" instead.
Attachment #8807605 -
Flags: review?(francois) → review+
Comment 38•8 years ago
|
||
Comment on attachment 8807605 [details] [diff] [review]
final_review_patch.diff
Review of attachment 8807605 [details] [diff] [review]:
-----------------------------------------------------------------
You don't need my review if you have Francois'.
Attachment #8807605 -
Flags: review?(fbraun)
Reporter | ||
Comment 39•8 years ago
|
||
Tuhina, it seems this bug is almost ready to land. Do you wanna push it over the finishing line and get it landed?
Flags: needinfo?(tuhinatwyla)
Reporter | ||
Comment 40•8 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #39)
> Tuhina, it seems this bug is almost ready to land. Do you wanna push it over
> the finishing line and get it landed?
OK, this bug is basically done, let me finish that up.
Flags: needinfo?(tuhinatwyla)
Comment 41•8 years ago
|
||
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb7a1208ee7
Add telemetry for CSP referrer directive. r=ckerschb,francois
Comment 42•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•