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.

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

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

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

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

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.

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

I don't know.

: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

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

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

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.

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

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.

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.

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

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

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

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 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 on attachment 563555 [details] [diff] [review] reftests as patch r=dbaron

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

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

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

This is already documented here: https://developer.mozilla.org/en/SVG/SVG_as_an_Image