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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

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?

Comment 1

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

To clarify: turning off layout.css.visited_links_enabled does make all links *appear* unvisited, visually.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1477773: no-op visited changes earlier if visited links are disabled.

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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1477773: Simplify visited-related code in invalidation.

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 :)

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Nvm, I do have access, and I can confirm that a build with these patches makes the test-case fail.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8999864 [details]
Bug 1477773: Simplify visited-related code in invalidation.

Xidorn Quan [:xidorn] UTC+10 has approved the revision.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9a17cc8a62
no-op visited changes earlier if visited links are disabled. r=xidorn

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c42d90e56a6
Simplify visited-related code in invalidation. r=xidorn

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/dc9a17cc8a62
https://hg.mozilla.org/mozilla-central/rev/4c42d90e56a6

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Emilio, I presume this also affects esr60?  Would you be willing to rebase these there to bring them down for Tor?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Rebase for ESR.

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.

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/ec13fda7c9b0