1420001 - (CVE-2017-7844) History leakage through anchors in SVG in canvas tags

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Jackson]

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.

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Daniel Holbert [:dholbert]]

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?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Disable :visited if the document is being used as an image.

Thanks for half-writing the patch Daniel :P

Comment 5

[Daniel Holbert [: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.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Do we need some sort of follow-up bug for investigating why reftests didn't catch this issue?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Ok, talked with Daniel, he thinks differently, I'm requesting approval + uplift to branches.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

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 11

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 12

[Daniel Holbert [:dholbert]]

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.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

This seems sec-high to me.

sec-approval+ for trunk. I'll approve for beta as well.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/19398

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d1599028c579

Comment 16

[Ryan VanderMeulen [:RyanVM]]

This was merged to m-c as well.
https://hg.mozilla.org/mozilla-central/rev/ed1802f246cb

Comment 17

[Al Billings [:abillings - ex-MoCo]]

This is going into 57.0.1 today, right?

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8931776 [details] [diff] [review]
Disable :visited if the document is being used as an image.

Approved, thanks

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/83885c5b00ed4947579565a404e252bc7e077cb3

Comment 20

[Daniel Holbert [:dholbert]]

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.

Comment 21

[Daniel Veditz [:dveditz]]

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.

Comment 22

[Daniel Holbert [:dholbert]]

Thanks! Just landed on inbound.

Comment 23

[Diane Hosfelt]

Does this still need to be restricted?

Comment 24

[Eddy Petrișor]

(In reply to Daniel Holbert [:dholbert] (AFK Aug 23-25) from comment #1)

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.

Is the test link above valid for this bug? Because I can see it (the red link to example) both on 68.0.2 (64-bit) and on nightly 70.0a1 (2019-08-22) (64 de biți) on Windows 10, while the first (http://homepages.cs.ncl.ac.uk/dan.jackson/avisited/) looks correct - no visited rendering for either of the links.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Can you clarify? You can see it if you go directly to http://homepages.cs.ncl.ac.uk/dan.jackson/avisited/example.com.svg, right? But not if you go to data:text/html,<img src="http://homepages.cs.ncl.ac.uk/dan.jackson/avisited/example.com.svg">.

If so that's working as expected.