Last Comment Bug 641731 - SVG images should not honor :visited selectors
: SVG images should not honor :visited selectors
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla10
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on:
Blocks: historyleak
  Show dependency treegraph
 
Reported: 2011-03-14 20:07 PDT by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2012-02-14 16:12 PST (History)
9 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.33 KB, text/html)
2011-09-26 17:37 PDT, Daniel Holbert [:dholbert]
no flags Details
testcase 2 (without a:link{}) -- already working correctly (917 bytes, text/html)
2011-09-26 17:49 PDT, Daniel Holbert [:dholbert]
no flags Details
fix v1 (1.14 KB, patch)
2011-09-28 15:29 PDT, Daniel Holbert [:dholbert]
dbaron: review+
Details | Diff | Splinter Review
reftests as patch (5.04 KB, patch)
2011-09-28 17:32 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
reftests as patch (7.55 KB, patch)
2011-09-29 14:23 PDT, Daniel Holbert [:dholbert]
dbaron: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-14 20:07:48 PDT
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] 2011-03-14 20:28:16 PDT
(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] 2011-03-14 20:28:48 PDT
(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) 2011-03-14 20:33:01 PDT
(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] 2011-03-14 20:40:12 PDT
> 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) 2011-09-22 16:24:20 PDT
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 (:mats) 2011-09-24 01:34:46 PDT
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) 2011-09-24 04:32:03 PDT
I don't know.
Comment 8 Robert Longson 2011-09-24 04:43:51 PDT
: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] (back in October - email directly if necessary) 2011-09-24 04:47:48 PDT
If you grep for :visited under layout/reftests/svg you should also find some :visited tests there.
Comment 10 Mats Palmgren (:mats) 2011-09-26 08:32:37 PDT
My bad, I was using a non-default profile with history disabled, sorry for the noise.
Comment 11 Daniel Holbert [:dholbert] 2011-09-26 17:37:02 PDT
Created attachment 562603 [details]
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] 2011-09-26 17:42:47 PDT
(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] 2011-09-26 17:49:15 PDT
Created attachment 562611 [details]
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] 2011-09-28 14:43:32 PDT
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] 2011-09-28 14:48:25 PDT
(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] 2011-09-28 15:29:45 PDT
Created attachment 563202 [details] [diff] [review]
fix v1

This seems to fix it.  Writing some reftests; will repost patch with tests shortly.
Comment 17 Daniel Holbert [:dholbert] 2011-09-28 17:32:17 PDT
Created attachment 563250 [details] [diff] [review]
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] 2011-09-28 22:00:15 PDT
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] 2011-09-29 14:23:34 PDT
Created attachment 563555 [details] [diff] [review]
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-7 (review requests must explain patch) 2011-10-28 15:59:36 PDT
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-7 (review requests must explain patch) 2011-10-28 16:01:31 PDT
Comment on attachment 563555 [details] [diff] [review]
reftests as patch

r=dbaron
Comment 22 Daniel Holbert [:dholbert] 2011-10-28 17:53:38 PDT
(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 25 Eric Shepherd [:sheppy] 2011-12-14 09:47:00 PST
This is already documented here:

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

Note You need to log in before you can comment on or make changes to this bug.