Disable <meta referrer> on 37 due to outstanding middle click bug and risk to uplifting its fix

RESOLVED FIXED in Firefox 37

Status

()

--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: geekboy, Assigned: geekboy)

Tracking

37 Branch
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 unaffected, firefox37+ fixed, firefox38 unaffected, firefox39 unaffected)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
The fix for bug 1113431 (flaw in <meta referrer>) hasn't landed yet, and it will be too risky to uplift to 37 this late in the cycle if it does land soon.  We need to disable <meta referrer> handling and ALSO the CSP referrer directive (since it uses the same logic for enforcing referrer policies).
(Assignee)

Comment 1

4 years ago
bz: would you prefer we take the same "disable it" patch you wrote for bug 1134606 to disable this again, or should I put it behind a pref (or some other way) to disable the feature?
Flags: needinfo?(bzbarsky)
Whatever is simpler, though if we think we'll need it again for 38 a pref might be nice...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 3

4 years ago
Created attachment 8574311 [details] [diff] [review]
disable_meta

The patch from bug 1134606 applied to beta 37, but required a small (trivial) merge.  This is it after the merge.
(Assignee)

Comment 4

4 years ago
Created attachment 8574312 [details] [diff] [review]
disable csp referrer

Chris, we need to disable the referrer directive in beta 37 (as we discussed).  Does this patch look appropriate?
Attachment #8574312 - Flags: review?(mozilla)
Comment on attachment 8574312 [details] [diff] [review]
disable csp referrer

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

Hey Sid, while your patch definitely serves the purpose of disabling the CSP referrer directive, I would also comment out:
a) referrerDirectiveValue() in the *.h and *.cpp file of the Parser, as well as
b) the only callsite of that method here:
  http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#931

It's up to you, but in case someone browses the code it's not immediately obvious why our CSP implementation does not accept the referrer directive in our CSP implementation.
Attachment #8574312 - Flags: review?(mozilla) → review+
Tracking 37 as we need to disable this feature.

Sid - With the r+ I think we're ready to land for Beta 4 (gtb today). If that's correct, can you please add a Beta approval request to this bug?
status-firefox36: --- → unaffected
status-firefox37: --- → affected
status-firefox38: --- → unaffected
status-firefox39: --- → unaffected
tracking-firefox37: --- → +
Flags: needinfo?(sstamm)
(Assignee)

Comment 7

4 years ago
Created attachment 8574760 [details] [diff] [review]
disable csp referrer

Added early return for the csp referrer parsing.

Approval Request Comment
[Feature/regressing bug #]: See comment 0, this is disabling an incomplete feature before it merges to release (removal from beta 37 only).
[User impact if declined]: users will have an incomplete (and possibly problematic) implementation of a feature that should be held back.
[Describe test coverage new/current, TreeHerder]:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b342bd33a7b
[Risks and why]: riskier to let the feature merge to release than to disable it like this.
[String/UUID change made/needed]: None.
Attachment #8574312 - Attachment is obsolete: true
Flags: needinfo?(sstamm)
Attachment #8574760 - Flags: review+
Attachment #8574760 - Flags: approval-mozilla-beta?
(Assignee)

Comment 8

4 years ago
Comment on attachment 8574311 [details] [diff] [review]
disable_meta

Approval Request Comment
[Feature/regressing bug #]: see bug 1113431 and comment 0 (incomplete feature, fix didn't land quick enough)
[User impact if declined]: improper and problematic feature implementation will ship
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b342bd33a7b
[Risks and why]: none significant (this was held back from 36 already)
[String/UUID change made/needed]: none

Note: this patch is a carry over from bug 1134606 and only has minor bitrot, so carrying over r+.
Attachment #8574311 - Flags: review+
Attachment #8574311 - Flags: approval-mozilla-beta?
(Assignee)

Comment 9

4 years ago
Lawrence: please flag checkin-needed or find ckerschb if you want to check this in today; I'm not likely available to do the checkin for the rest of the day.
Flags: needinfo?(lmandel)
Comment on attachment 8574311 [details] [diff] [review]
disable_meta

As previously discussed with the sec team, we are disabling this feature in 37. Beta+
Flags: needinfo?(lmandel)
Attachment #8574311 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8574760 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/173e1bbcd0b6
https://hg.mozilla.org/releases/mozilla-beta/rev/a982b8eabc42
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox37: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.