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.
Created attachment 177722 [details] Test case SVG (from W3c)
Created attachment 177723 [details] Patch
Comment on attachment 177723 [details] Patch Rejecting attachment 177723 [details] from review queue. georgemay137@yahoo.com 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.
Comment on attachment 177723 [details] Patch Rejecting attachment 177723 [details] from commit-queue. georgemay137@yahoo.com 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.
Created attachment 177728 [details] Patch
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.
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
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).
Created attachment 178824 [details] An even better fix OOPS. mistake. Now it's a better fix :-)
(In reply to comment #9) > Created an attachment (id=178824) [details] > An even better fix > > OOPS. mistake. Now it's a better fix :-) Ping...:-)
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.
Who is the right person to review this?
CC'd loader peeps.
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.
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.
I recommend closing this bug as WONTFIX.
(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.
> 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!
(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!
(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.
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.
(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.
(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...
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.
<rdar://problem/16469583>
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.
(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.
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.