Bug 104111 – REGRESSION (r129585): Cannot load DATA URI resources within the context of an SVG image

• Component: SVG
• Status: RESOLVED WORKSFORME

[George May]

Created attachment 177721 [details]
Test case

The patch applied by bug 97498 causes incorrect behavior regarding the usage of DATA URI resources within the context of an SVG image.
The patch blocks all types of requests, and not just requests that do indeed cause network activity. 
It broke a W3C test case for loading PNG images inside SVG images (http://dev.w3.org/SVG/profiles/1.1F2/test/svg/struct-image-04-t.svg) when loaded as an image inside an HTML page (attached).
The attached test case works perfectly in FF.

[George May]

Created attachment 177722 [details]
Test case SVG (from W3c)

[George May]

Created attachment 177723 [details]
Patch

[WebKit Review Bot]

Comment on attachment 177723 [details]
Patch 

Rejecting attachment 177723 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 177723 [details]
Patch 

Rejecting attachment 177723 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[George May]

Created attachment 177728 [details]
Patch

[WebKit Review Bot]

Attachment 177728 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Florin Malita]

Thanks for the patch George. I think it's a good idea to support data URI resources in image-embedded SVG (see https://bugs.webkit.org/show_bug.cgi?id=99677 also), but there's more work to be done:

1) This only covers Chromium. IIRC, other platforms don't have a NetworkingContext at all for SVG-as-image, so it's not going to be quite as easy.

2) Needs to include a test exercising this functionality. You could maybe use one from 99677, or re-enable the failing W3C test.

3) Needs to include ChangeLog entries (probably why the style bot is barking) - see http://www.webkit.org/coding/contributing.html

[George May]

Created attachment 178823 [details]
A better fix

The new patch should handle all cases.
I've enabled resources referenced via DATA URI to be loaded correctly (presumably under all platforms).

[George May]

Created attachment 178824 [details]
An even better fix

OOPS. mistake. Now it's a better fix :-)

[George May]

(In reply to comment #9)
> Created an attachment (id=178824) [details]
> An even better fix
> 
> OOPS. mistake. Now it's a better fix :-)

Ping...:-)

[Florin Malita]

Comment on attachment 178824 [details]
An even better fix

View in context: https://bugs.webkit.org/attachment.cgi?id=178824&action=review

> LayoutTests/svg/as-image/resources/svg-with-embedded-image-via-data-uri.svg:2
> +<image x="0" y="0" width="240" height="150" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAPAAAACWCAMAAADXJvXnAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAgY0hSTQAAeiYAAICEAAD6AAAAgOgAAHUwAADqYAAAOpgAABdwnLpRPAAAAwBQTFRF///////3///v9//+//bv/Pjm9Pfv7/fm9u/e+O/U7+7m5u/e5uzm8+XT4uXe4t7R7dq+1NjQ5smn0s+9z87IxcrH0cOtx8W9vcbFvcO9tcW9vsC12riPwr6rtb29w7SjtbWwya6LrbWlrbKwta+graylpK2pxqR7wKSIraicpaSfpKWUsZ+Ku51znKWcmqOltZt7pZyUnJycn52MnJyUkpyclJyUsZNznJSUjJyUnZSMsJBplJSUlJSMjZScnJB/jJSUi5SMhJSUo4xpjIyNm4lzjIyEgoyUpYRfhIyMiIx7goyEe4yMmINrhISMhISEiYRzmH9ehIN7e4SMe4SEmnxSe4R7c4SMc4SEc4R7h3xme3uEfntze3t7c3uEc3t7cntza3uEc3tra3t7fnNmc3N7c3NzgnBac3Nrim1La3N7a3Nza3NrY3N7Y3Nzc2trY3Nra2tzcmtaa2trbWtje2dMY2tzY2trYWtiWmtzWmtrb2RSbGNecWNJY2NrY2NjWmNrY2FaWmNjWmNaUmNrUmNhbllCZlpMWlpjWlpacVQzWlpQUlpjUlpaUlpSSlpjUlpKSlpaXlNCSlpSWlJNUlJaUlJSSlJjUlJKSlJaUVJCSlJSSlJKQlJaXUsyQlJSUEpSUkpKQlJKUkpCSkpKSkpCQkpSQkpKTUc6QkpCOkpSQko6OkpKSkJCOkpCQkJKQkJCQkI6OkJKTT4oOkJCOkI6Qj8xMUJKOkIxMUJCQjo6MUI6OjpCOjo6PTopOjoxMTpCMTo6MToxKTpCKTo6KToxOjExMTE6MTExMjEpKTE6KTExKTEpITExLiwhITEpKSkpMicUISkxISkpISkhISkZGSkpISkQGSkhISEhISEZISEQGSEhGSEZGSEQECEhGSEIECEZIRkQGRkhIRkIGRkZGRkQGRkIEBkZEBkQEBkICBkZGRAZEBkAGRAQGRAIEBAZEBAQEBAICBAQDRAACBAIEAgJEAgACAgQCAgICAgAAAgIAAgACAAAAAAIAAAAoYb3nAAAAAlwSFlzAAALEQAACxEBf2RfkQAAPyZJREFUeF61nQ1clWWe949OWVmKr5mpoyjKi2FICJNASiDhGhEETGsyOpuBjO7shJJORIZMkBLMDjJ8SM7SjkDA1DiwgjuzDhJ8djYYyDCpHfFln3wGlD77mR5obQZrH5/f739d133uc8CX3e35K+c+b9j53v/3/3XdJ8fjjz/11FOPU3B0k6e/853vPCuy+TvJTyXjTevXr8fP448nJ+O1zc9+b/v2733vr78HUW/ju/noBvLXlO9TXvCQH9pEvWR/5pVXzKMXtbieUa+of9T+O+r+K1p+pOR1hxtocnKyjVmINxP4aT5NXpHHnwLx0yQGqov3FoBdvJ7AN+bFx74Z8AsvjMvrDvz66woYnJY8/fTThpnAm7+zeTMUnPy4UrACxu8AebOcC61caFfZwy2p18L94Q9dujR6HaNfoyS3449eedFDne42oV588UX1S6JcEQc/vyGG2kQALdTyUAFrsxfbF/unWeOlzeo3RMbhFZXaBXpwmbOnAQq8NnU3e7ShatPEwVL6WDvWz8D6Da3g/hjiePTRx8RGITRTC0CAIfLU02Lq2gqUAcj79QmyeD3US2+1aMXJbkArH9IAu/uf9aldtLx3Y2JNK8q1cAH88FoSw2JJALpntLqIQ+Fj8MKmbXYPXnOOHtfUz2hrNjaNI3hfeGG75pQjtGes2RgkjwRyPVbx6kXjfRpSfXBQuo7yO/Z/R4xY2/IPtTHz/WLLVO+

This is a nice image but pretty large - we could save some space by using the canonical 100x100 green rectangle instead:

"data:image/png;base64,/9j/4AAQSkZJRgABAQEAXQBcAAD/2wBDAAUDBAQEAwUEBAQFBQUGBwwIBwcHBw8LCwkMEQ8SEhEPERETFhwXExQaFRERGCEYGh0dHx8fExciJCIeJBweHx7/2wBDAQUFBQcGBw4ICA4eFBEUHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh4eHh7/wAARCABkAGQDASIAAhEBAxEB/8QAFQABAQAAAAAAAAAAAAAAAAAAAAf/xAAUEAEAAAAAAAAAAAAAAAAAAAAA/8QAFQEBAQAAAAAAAAAAAAAAAAAAAAf/xAAUEQEAAAAAAAAAAAAAAAAAAAAA/9oADAMBAAIRAxEAPwCVAJ4lIAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD//Z"

> LayoutTests/svg/as-image/svg-with-embedded-image.html:12
> +<body onload="if(window.testRunner) setTimeout('window.testRunner.notifyDone();', 10);" style="overflow:hidden">

Possibly unrelated to the patch: I'm a bit surprised that we need a delay here. The other SVG-as-image tests seem to work fine without it...

> Source/WebCore/ChangeLog:8
> +        Whenever we encounter a DATA URI resource, we can load it right away, regardless of the resourceLoader settings.

Clever fix :) I *think* this is OK, but I'm not a resource loading expert so I'll let a real reviewer weigh in.

[Alexey Proskuryakov]

Who is the right person to review this?

[Eric Seidel (no email)]

CC'd loader peeps.

[Adam Barth]

Comment on attachment 178824 [details]
An even better fix

I don't think that this change is safe.  The CachedResourceLoader assumes that it has a real Frame, which is uses in an essential way as part of the loading process.

[Adam Barth]

SVGImages cannot load subresources.  Changing that correctly would require a major change to the loader, and it's not clear we even want to change that.

Instead of using <img> to load your SVG, consider using <object>.  If you use object, you'll be able to load subresources and you'll get much better performance.

[Adam Barth]

I recommend closing this bug as WONTFIX.

[Philip Rogers]

(In reply to comment #15)
> SVGImages cannot load subresources.  Changing that correctly would require a major change to the loader, and it's not clear we even want to change that.
> 

FWIW, Mozilla seems to have made the same choice: https://bugzilla.mozilla.org/show_bug.cgi?id=628747#c34

> Instead of using <img> to load your SVG, consider using <object>.  If you use object, you'll be able to load subresources and you'll get much better performance.

Lets not perpetuate the performance aspect of <object> and turn it into some cargo cult web knowledge. I'm working on fixing the performance issues of svg-as-image in wkbug.com/104693.

[Adam Barth]

> Lets not perpetuate the performance aspect of <object> and turn it into some cargo cult web knowledge.

Ok.  :)

> I'm working on fixing the performance issues of svg-as-image in wkbug.com/104693.

That's great!

[George May]

(In reply to comment #16)
> I recommend closing this bug as WONTFIX.

Hi,
Sorry, been away for a while.
As Philip mentioned, FF does allow using data URIs inside SVG, as it doesn't effect security.
Is there anything currently unsafe with my patch? What needs to be fixed? 

Thx!

[Philip Rogers]

(In reply to comment #19)
> (In reply to comment #16)
> > I recommend closing this bug as WONTFIX.
> 
> Hi,
> Sorry, been away for a while.
> As Philip mentioned, FF does allow using data URIs inside SVG, as it doesn't effect security.
> Is there anything currently unsafe with my patch? What needs to be fixed? 
> 
> Thx!

I should have been clearer: Mozilla chose to disallow data:uri images from themselves loading other data:uri images for reasons similar to Adam's objection (security complexity). This case is slightly different. I have a followup question though: did you intend to remove the chromium/ResourceHandle.cpp change from the patch?

Abarth gave an r- and listed some concerns, I think you'll need to address those to move this forward.

[George May]

Hi, as a start i would like DATA URI resources from "http://"-loaded images to be enabled. Does such a change posses any security risk? (I see none, but I'm not an expert...)
BTW, why is data uri images loading another data uri images bad?

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #16)
> > > I recommend closing this bug as WONTFIX.
> > 
> > Hi,
> > Sorry, been away for a while.
> > As Philip mentioned, FF does allow using data URIs inside SVG, as it doesn't effect security.
> > Is there anything currently unsafe with my patch? What needs to be fixed? 
> > 
> > Thx!
> 
> I should have been clearer: Mozilla chose to disallow data:uri images from themselves loading other data:uri images for reasons similar to Adam's objection (security complexity). This case is slightly different. I have a followup question though: did you intend to remove the chromium/ResourceHandle.cpp change from the patch?
> 
> Abarth gave an r- and listed some concerns, I think you'll need to address those to move this forward.

[Robert Longson]

(In reply to comment #20)
> 
> I should have been clearer: Mozilla chose to disallow data:uri images from themselves loading other data:uri images for reasons similar to Adam's objection (security complexity). This case is slightly different. I have a followup question though: did you intend to remove the chromium/ResourceHandle.cpp change from the patch?
> 

No we didn't, we allow unlimited nesting of data URIs. The only restriction is that image files must be complete in the one file so you can't refer to anything in another file than the image file.

[George May]

(In reply to comment #22)
> (In reply to comment #20)
> > 
> > I should have been clearer: Mozilla chose to disallow data:uri images from themselves loading other data:uri images for reasons similar to Adam's objection (security complexity). This case is slightly different. I have a followup question though: did you intend to remove the chromium/ResourceHandle.cpp change from the patch?
> > 
> 
> No we didn't, we allow unlimited nesting of data URIs. The only restriction is that image files must be complete in the one file so you can't refer to anything in another file than the image file.

This is what my current patch is doing,
I assume any DATA URI resource is safe as it cannot leak data...

[David Kilzer (:ddkilzer)]

Google fixed this with this bug/commit:

SVG with embedded image doesn't display properly when in <img>
https://code.google.com/p/chromium/issues/detail?id=224317
http://src.chromium.org/viewvc/blink?revision=152093&view=revision

Which then caused this security issue:

Heap-use-after-free in WebCore::StyleResolver::loadPendingImages
https://code.google.com/p/chromium/issues/detail?id=248843
https://src.chromium.org/viewvc/blink?revision=153029&view=revision

Which then caused this security issue:

Heap-use-after-free in WebCore::StyleResolver::loadPendingImages
https://code.google.com/p/chromium/issues/detail?id=256013
http://src.chromium.org/viewvc/blink?revision=153969&view=revision

Which then caused this security issue:

Heap-use-after-free in WebCore::XMLDocumentParser::append
https://code.google.com/p/chromium/issues/detail?id=278908
http://src.chromium.org/viewvc/blink?view=revision&revision=157914

Please be careful if merging this fix from Blink.

[David Kilzer (:ddkilzer)]

<rdar://problem/16469583>

[Steven Vachon]

http://www.svachon.com/webframes/examples.html

The non-vector examples do not work in WebKit/Safari, but they work everywhere else. Please fix this in time for Safari 8's release.

[Said Abou-Hallawa]

(In reply to comment #26)
> http://www.svachon.com/webframes/examples.html
> 
> The non-vector examples do not work in WebKit/Safari, but they work
> everywhere else. Please fix this in time for Safari 8's release.

The current build of WebKit/Safari displays the page http://www.svachon.com/webframes/examples.html exactly the same as Chrome and Firefox.

[Said Abou-Hallawa]

 According to the w3c specs, the SVG as an image is allowed to load any external sub-resource like an image or css expect for the data uri.  https://www.w3.org/wiki/SVG_Security.

The current build of WebKit/Safari is following these rules so this bug is not repro anymore.