layout.css.visited_links_enabled doesn't seem to completely disable :visited

RESOLVED FIXED in Firefox -esr60

Status

()

defect
RESOLVED FIXED
Last year
9 months ago

People

(Reporter: mismith, Assigned: emilio)

Tracking

60 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr6064+ fixed, firefox63 fixed)

Details

(Whiteboard: [pixel-stealing])

Attachments

(3 attachments)

In theory, turning off layout.css.visited_links_enabled should mitigate bug 1455898 - but it doesn't. Some sort of :visited-related processing still seems to be happening even with this pref off. Maybe something was broken in the switch to Stylo?
To clarify: turning off layout.css.visited_links_enabled does make all links *appear* unvisited, visually.
Component: Graphics → CSS Parsing and Computation
Flags: needinfo?(emilio)
Assignee: nobody → emilio
Flags: needinfo?(emilio)
We force a repaint from ContentStateChangedInternal if visited links are
disabled, and that's observable. Let's cut it off as early as we can to avoid
timing attacks even when :visited is disabled.
We match with AllLinksVisitedAndUnvisited for style invalidation, so all this
stuff is just not needed. The comment points to the attribute tests in
bug 1328509, but those still trivially pass with this change.

I think this was unneeded since I introduced AllLinksVisitedAndUnvisited, or
maybe since https://github.com/servo/servo/pull/19520. In any case it doesn't
really matter, and I already had done this cleanup in my WIP patches for
bug 1406622, but I guess this is a slightly more suitable place to land them :)
I don't think I have access to the test-case, but could you try a build from:

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

And confirm it does what you expect? Thanks!
Flags: needinfo?(lists)
Nvm, I do have access, and I can confirm that a build with these patches makes the test-case fail.
Flags: needinfo?(lists)
Comment on attachment 8999863 [details]
Bug 1477773: no-op visited changes earlier if visited links are disabled.

Xidorn Quan [:xidorn] UTC+10 has approved the revision.
Attachment #8999863 - Flags: review+
Comment on attachment 8999864 [details]
Bug 1477773: Simplify visited-related code in invalidation.

Xidorn Quan [:xidorn] UTC+10 has approved the revision.
Attachment #8999864 - Flags: review+
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9a17cc8a62
no-op visited changes earlier if visited links are disabled. r=xidorn
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c42d90e56a6
Simplify visited-related code in invalidation. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/dc9a17cc8a62
https://hg.mozilla.org/mozilla-central/rev/4c42d90e56a6
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1488155
Whiteboard: [pixel-stealing]
Emilio, I presume this also affects esr60?  Would you be willing to rebase these there to bring them down for Tor?
Flags: needinfo?(emilio)
Yeah, it does. Here's a patch with the first commit (second was just cleanup so didn't bother rebasing it), and the fix for bug 1488155 as well.
Flags: needinfo?(emilio)
Comment on attachment 9022267 [details] [diff] [review]
Rebase for ESR.

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Tor Browser would like to take these patches to mitigate history stealing attacks in the configuration where Tor Browser saves browsing history.

User impact if declined: Tor would have to apply it manually.

Fix Landed on Version: 63.0a1 / 20180816100035

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): I put it at Medium because while it's been tested for months, it's a medium-sized patch.

String or UUID changes made by this patch: None
Attachment #9022267 - Flags: approval-mozilla-esr60?
Comment on attachment 9022267 [details] [diff] [review]
Rebase for ESR.

Makes sense to keep ESR60.0.4 in line with 64 release to prevent this type of attack and protect user privacy.
Attachment #9022267 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Depends on: 1485655
You need to log in before you can comment on or make changes to this bug.