641731 - SVG images should not honor :visited selectors

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we add the ability to paint SVG images to a canvas (currently doesn't seem to work), then we would create a CSS history sniffing attack where a page can paint an SVG image to a canvas, where the SVG image contains an <a> element whose :visited styling affects the display of the SVG image. Then the page could get inspect the image data in the canvas.

A variant would use a foreignobject inside the SVG image, containing an HTML <a>.

I think the best way to block such issues is to never honor :visited selectors inside an SVG image document.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> If we add the ability to paint SVG images to a canvas (currently doesn't seem
> to work)

It works if you have a non-percent-valued height & width on the image.

> then we would create a CSS history sniffing attack where a page can
> paint an SVG image to a canvas, Then the page
> could get inspect the image data in the canvas.

Nope -- painting an SVG image automatically taints the canvas, so the page shouldn't be able to read the image data.

So I think this is INVALID, unless I'm misunderstanding?

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> (In reply to comment #0)
> > If we add the ability to paint SVG images to a canvas (currently doesn't seem
> > to work)
> 
> It works if you have a non-percent-valued height & width on the image.

(see e.g. the penguin on http://blog.dholbert.org/2010/10/svg-as-image.html )

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #1)
> > then we would create a CSS history sniffing attack where a page can
> > paint an SVG image to a canvas, Then the page
> > could get inspect the image data in the canvas.
> 
> Nope -- painting an SVG image automatically taints the canvas, so the page
> shouldn't be able to read the image data.

Oh, we taint. Hmm. Don't we want to stop doing that?

Comment 4

[Daniel Holbert [:dholbert]]

> Oh, we taint. Hmm. Don't we want to stop doing that?

Possibly -- tainting was more important when we allowed external resources (per bug 276431 comment 130), but now that we block external resource-loads (bug 628747), it might be safer to stop tainting, if we make sure not to leak :visited information.

It's also possible we already don't style :visited content at all, in SVG-as-an-image -- I'm pretty sure I checked at one point, and no :link/:visited style
was applied (though I'm not sure why).

(For the record, support for drawing SVG-as-an-image to a canvas was added in bug 589558.)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Even with tainting we still have a problem here since in WebGL at least it's easy to use timing attacks to recover pixel data, especially if you only need one pixel as you do here. So we really do need to fix this.

Comment 6

[Mats Palmgren (inactive)]

Is it intentional that :visited doesn't match <a xlink:href> in (standalone) SVG?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know.

Comment 8

[Robert Longson [:longsonr]]

:visited should match <a> in a standalone SVG, yes there's a w3c test for it... http://dev.w3.org/SVG/profiles/1.1F2/test/svg/styling-css-06-b.svg

There should be no default colour change though i.e. without an explicit :visited selection colour in a page's stylesheet but that's bug 571099

Comment 9

[Jonathan Watt [:jwatt]]

If you grep for :visited under layout/reftests/svg you should also find some :visited tests there.

Comment 10

[Mats Palmgren (inactive)]

My bad, I was using a non-default profile with history disabled, sorry for the noise.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: testcase

OK, here's a testcase for this bug. It contains links to this bug page.  If you've got this bug page in your history, the text renders as red (visited), but it should render as lime (un-visited) so as not to leak history.

Side note:  The SVG embedded in the testcase has this block of style:
  <![CDATA[
      a:link   {
        // It's weird, but this block is required for us to fail in image
        // contexts.  Without *some* decl (containing anything) for a:link,
        // we end up (correctly) ignoring :visited status in images for some
        // reason.  But with an "a:link" decl, we honor :visited status. (Bad!)
      }
      a:link    > text, a:link    > tspan   { fill: lime; }
      a:visited > text, a:visited > tspan   { fill: red;  }
  ]]>

As noted by that "it's weird" comment, it appears that we already do the right thing (ignore visited status) *IF* there's no "a:link {}" declaration in the SVG's style.

If a:link {} is there, though, we fail.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> As noted by that "it's weird" comment, it appears that we already do the
> right thing (ignore visited status) *IF* there's no "a:link {}" declaration
> in the SVG's style.
> 
> If a:link {} is there, though, we fail.

(This is why I thought this already worked as we'd like it to, up in comment 4 -- I think I was testing without a magic "a:link {}" declaration.  So, I saw visited style when viewing the SVG directly, and unvisited style when viewing it as an image, which is the desired result.)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (without a:link{}) -- already working correctly

To demonstrate the side note about "a:link {}" from the previous two comments, here's another testcase that works correctly in my nightly build.

This testcase is identical to the previous one, except that the (empty) "a:link { // It's weird [etc] }" declaration has been removed from the SVG.  This is sufficient to get correct behavior. (lime text)  (Note that it still renders as red if you view the SVG directly, so the visited status is still being applied in non-image contexts)

Again, I don't know what's making us do the right thing here.

Comment 14

[Daniel Holbert [:dholbert]]

2 new things:
 - testcase 1 is WFM (lime text) in a debug build, so there may be a race condition involved.
 - testcase 2 triggers an ABORT_IF_FALSE failure. I filed bug 690096 on that.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
>  - testcase 1 is WFM (lime text) in a debug build, so there may be a race
> condition involved.
(sorry, disregard that part -- I think that was just because I hadn't visited the link target yet in that profile)

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This seems to fix it.  Writing some reftests; will repost patch with tests shortly.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: reftests as patch

Here's a patch with reftests for this.

On my machine, without the fix applied:
 - svg-image-visited-1.html fails
 - svg-image-visited-2.html passes in opt builds, but aborts in debug builds (bug 690096)

With the fix applied, they both pass (and don't abort).

Running the tests through try server with & without the patch right now.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 563250 [details] [diff] [review]
reftests as patch

With fix, tests passed on TryServer.
 https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=6a07defba402

Without fix, the included reftests failed on TryServer, matching my local experience from comment 17 exactly, with the exception of one sporadic pass on Linux Opt & one sporadic pass on WinXP Opt (presumably because the 100ms timeout expired before we loaded the :visited style).
  https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=dd6a5f894d46

I'm not worried about the sporadic passes.  Of course, we could increase the 100ms delay to reduce the likelihood of sporadic-test-passes-in-buggy-browser-versions, but I don't think it's worth the hit to reftest run-time.  If we regress this bug, the included tests will fail often enough to let us know something's wrong.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: reftests as patch

Added one thing to the reftests patch -- this version compares the SVG files themselves (svg-image-visited-*-helper.svg) against the reference case, using test_visited_reftests.html.  (I assert that they won't match, because visitedness *should* be honored in non-image contexts.)

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 563202 [details] [diff] [review]
fix v1

r=dbaron if you switch the order of the && so the state.HasState() check comes first, since that's faster than all these other things in the ||.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 563555 [details] [diff] [review]
reftests as patch

r=dbaron

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #20)
> r=dbaron if you switch the order of the && so the state.HasState() check comes first

Thanks, will do.  Also applying s/GetOwnerDoc()/OwnerDoc()/ since bug 682420 renamed that method in the last few weeks.

Comment 23

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2052ffffcb6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f606afe21762

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2052ffffcb6b
https://hg.mozilla.org/mozilla-central/rev/f606afe21762

Comment 25

[Eric Shepherd [:sheppy]]

This is already documented here:

https://developer.mozilla.org/en/SVG/SVG_as_an_Image