User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20171120142222 Steps to reproduce: Demo: http://homepages.cs.ncl.ac.uk/dan.jackson/avisited/ ...you may need to reload a couple of times to make it work. 1. Place an <a> tag for each history sniffing attempt in an external SVG document (within a 'foreignObject' tag) 2. Create an <img> that references the SVG file 3. Draws that image to a <canvas> element 4. Try one/both of: a. look through the pixels to find the a:visited colour; or b. save the canvas as a (PNG) Blob to see if there is a file size difference (the unvisited colour is the same as the background colour, resulting in a smaller file). Actual results: The page is (at least after reloading) able to determine URLs I have previously visited. Expected results: The page should not be able to sniff the user's browsing history. In particular, it looks like other browsers render "a:visited" the same as "a" when drawing SVG to an image.
2 years ago
Component: Untriaged → Document Navigation
Product: Firefox → Core
I can also reproduce by simply viewing this data URL: data:text/html,<img src="http://homepages.cs.ncl.ac.uk/dan.jackson/avisited/example.com.svg"> (The text paints as red, if you've visited example.com.) The bug stops reproducing (on the original testcase and on this ^^ data URL) if you disable layout.css.servo.enabled. So, this seems to be a regression from stylo, and hence affects 57 and later. I'm going to mark this as security-sensitive, since this is a relatively serious privacy leakage. We might want to consider shipping this in a 57.0.x dot release, if the fix ends up being upliftable. [Tracking Requested - why for this release]: Privacy-leakage, regression.
Status: UNCONFIRMED → NEW
Component: Document Navigation → CSS Parsing and Computation
Ever confirmed: true
Version: 58 Branch → 57 Branch
So this was bug 641731 regressing, basically. That bug *did* land with reftests -- not sure yet why they didn't catch this: https://dxr.mozilla.org/mozilla-central/rev/0bb0f14672fdda31c19aea1ed829e050d693b9af/layout/reftests/svg/as-image/reftest.list#192-201 They're are supposed to be testing that visited style *does not* apply in an <img>, using <a href=""> inside of an image that we've previously loaded, as noted here: https://dxr.mozilla.org/mozilla-central/rev/0bb0f14672fdda31c19aea1ed829e050d693b9af/layout/reftests/svg/as-image/svg-image-visited-1-helper.svg#17-20 Thoughts about why they might be failing to catch this: - maybe there's some delay between loading the image & the history DB being updated (so we haven't yet realized that the image has been visited, by the time we load the actual testcase) - maybe there's something special about file:// URIs (which reftests use by default) that makes this work a little differently. - maybe we disabled history-tracking / "places" during reftests? (I'm just having a faint memory of this change happening for reftests at some point, to speed up reftest runs... That would definitely prevent these reftests from being effective.)
The fix on bug 641731 was a very small tweak to layout/style/nsCSSRuleProcessor.cpp, to the function nsCSSRuleProcessor::GetContentState -- making it check aElement->OwnerDoc()->IsBeingUsedAsImage() alongside the existing "are we in private browsing mode" check: https://hg.mozilla.org/mozilla-central/rev/2052ffffcb6b (Because we also intentionally ignore :visited rules in private browsing mode, it seems.) It looks like the ignoring-:visited-during-private-browsing-mode feature *does work correctly* in Nightly, so that seems to have made it into stylo, AFAICT. Emilio/heycam, do you know where that check happens (where the nsCSSRuleProcessor::GetContentState equivalent might be in servo/stylo)? And can we add a nsIDocument::IsBeingUsedAsImage() check there?
Flags: needinfo?(dholbert) → needinfo?(emilio)
Assignee: nobody → emilio
Thanks for half-writing the patch Daniel :P
Attachment #8931776 - Flags: review?(dholbert)
Comment on attachment 8931776 [details] [diff] [review] Disable :visited if the document is being used as an image. Review of attachment 8931776 [details] [diff] [review]: ----------------------------------------------------------------- Lgtm, assuming you've tested it locally. Sadly the commit message kinda gives away the issue, but I guess the code-change itself does too, so there's no way around that. Please request sec-approval before landing.
Attachment #8931776 - Flags: review?(dholbert) → review+
According to https://wiki.mozilla.org/Security/Bug_Approval_Process, we only need to do that for sec-high / sec-crit. This bug isn't rated, but I suspect it's not a critical vulnerability of any sort. Do you really think this needs sec-approval to land?
Do we need some sort of follow-up bug for investigating why reftests didn't catch this issue?
(In reply to (PTO - Back 27-Nov) Ryan VanderMeulen [:RyanVM] from comment #7) > Do we need some sort of follow-up bug for investigating why reftests didn't > catch this issue? Yeah, definitely. Though I suspect the answer is "async link coloring", the same reason tons of :visited reftests are flakes.
Ok, talked with Daniel, he thinks differently, I'm requesting approval + uplift to branches.
Comment on attachment 8931776 [details] [diff] [review] Disable :visited if the document is being used as an image. Approval Request Comment [Feature/Bug causing the regression]: Stylo [User impact if declined]: Trivial user history query, so pretty high impact. Allows any website to trivially query your user history. [Is this code covered by automated tests?]: yes, but in practice they didn't catch the issue, so I guess the answer is "not really". [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: yes, see comment 0. [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: not risky [Why is the change risky/not risky?]: reuses an existing mechanism which was just missing a condition. [String changes made/needed]: none [Security approval request comment] How easily could an exploit be constructed based on the patch? Probably relatively easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not comments, but the patch itself is pretty self-explanatory. Which older supported branches are affected by this flaw? 57+ If not all supported branches, which bug introduced the flaw? Stylo Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No, but they should apply cleanly. How likely is this patch to cause regressions; how much testing does it need? Not much, it uses an already-existing mechanism, so the patch is quite simple.
Comment on attachment 8931776 [details] [diff] [review] Disable :visited if the document is being used as an image. See the comment above for the approval request.
Attachment #8931776 - Flags: approval-mozilla-release?
Thanks! Answering some questions/thoughts: (In reply to Emilio Cobos Álvarez [:emilio] from comment #6) > According to https://wiki.mozilla.org/Security/Bug_Approval_Process, we only > need to do that for sec-high / sec-crit.-approval to land? I think it's at least sec-high; CC'ing dveditz and abillings for their thoughts. I'll tag it as sec-high for now, since it enables trivial history-stealing which we've tried to make hard ever since bug 147777. Also noting that esr52 is unaffected (so no uplift needed there), since this is effectively a regression from stylo. (In reply to Emilio Cobos Álvarez [:emilio] from comment #8) > (In reply to (PTO - Back 27-Nov) Ryan VanderMeulen [:RyanVM] from comment #7) > > Do we need some sort of follow-up bug for investigating why reftests didn't > > catch this issue? > > Yeah, definitely. Though I suspect the answer is "async link coloring" (See also comment 2 - IIRC we turned off places for reftest runs at some point, and that sped up reftests but also inadvertantly kneecapped the reftests that were supposed to be testing this feature. We do also have this mochitest, though, and we probably need to morph the reftests such that they're run as part of that mochitest: https://dxr.mozilla.org/mozilla-central/source/layout/style/test/test_visited_reftests.html ) I filed bug 1420526 to get this effectively tested again, anyway.
This seems sec-high to me. sec-approval+ for trunk. I'll approve for beta as well.
This was merged to m-c as well. https://hg.mozilla.org/mozilla-central/rev/ed1802f246cb
Whiteboard: [regression from stylo] → [regression from stylo][adv-main57+][adv-esr52.5.1+]
This is going into 57.0.1 today, right?
Comment on attachment 8931776 [details] [diff] [review] Disable :visited if the document is being used as an image. Approved, thanks
Attachment #8931776 - Flags: approval-mozilla-release? → approval-mozilla-release+
Dan, am I OK to land some test-patches for this bug, to get this bug tested? (Test patches are over on bug 1420526.) I wanted to double-check before landing, since this bug is still closed, and IIRC typically we don't land automated tests until bugs are ready to open up. (But in this case, I think it should be fine because the test-patch doesn't reveal much to a hypothetical attacker beyond what they can already find out. The test patch is mostly shifting some already-existing tests from one test harness to another, to make them actually functional, per end of comment 12. I'm also adjusting the tests to make them more robust in our harness, but that's not super relevant to an attacker.
Yes, it's fine to land those tests now. For a crash testcase I'd want to wait for more uptake of the fix first but that doesn't apply in this case.
Thanks! Just landed on inbound.
Does this still need to be restricted?
You need to log in before you can comment on or make changes to this bug.