Canvas is tainted after drawing SVG including <foreignObject>, loaded with data: URL
Reported by
xilef...@gmail.com,
Jan 2
|
||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36 Steps to reproduce the problem: 1. Open zipped file 2. Drag index.html into browser 3. A spinning cube should appear What is the expected behavior? A spinning textured cube should show on the left side What went wrong? Error: Failed to execute 'texImage2D' on 'WebGL2RenderingContext': Tainted canvases may not be loaded. Did this work before? Yes 71.0.3578.98 Windows 10 64-bit Does this work in other browsers? Yes Chrome version: 71.0.3578.98 Channel: stable OS Version: 10.0 Flash Version:
,
Jan 2
This used to work. Ran a per-revision bisect: python bisect_builds.py -o -a win64 -g 550398 -b 619493 --use-local-cache -- --no-first-run http://localhost:8080 You are probably looking for a change made after 610497 (known good), but no later than 610498 (first known bad). CHANGELOG URL: The script might not always return single CL as suspect as some perf builds might get missing due to failure. https://chromium.googlesource.com/chromium/src/+log/e373f9b913736c34dcee1994120e08d0641da586..889ddafc7932e318d1f213bb4e0926e71cb94657 falken@ can you look into this regression when you get back from vacation?
,
Jan 3
Tried checking the issue on reported chrome version 71.0.3578.98 using Windows 10 as per the steps mentioned in comment#0, but somehow we couldn't reproduce it from our end - even after running it on local server. Removing Needs-Bisect as bisect info is already provided in C#2. As we couldn't access the Bug associated with the CL provided in comment#2 and as per the revision numbers mentioned, we assume that CL which was first introduced in M-72(72.0.3619.0) may Probably have been merged to M-71 branch as well hence adding RegressedIn-71 and RB-Stable(M71 & M72) for tracking. Thanks!
,
Jan 3
Hi, Sorry I somehow posted the wrong Chrome version - the problem appears on Canary and Chromium only. I just reinstalled Canary to '73.0.3659.0' and the taint error appears, same problem with Chromium '73.0.3651.0'. Everything runs fine with Chrome '71.0.3578.98'
,
Jan 3
Removing M-71 per comment#4.
,
Jan 3
Running `bisect-builds.py` on linux64 isolates the same CL: You are probably looking for a change made after 610497 (known good), but no later than 610498 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/e373f9b913736c34dcee1994120e08d0641da586..889ddafc7932e318d1f213bb4e0926e71cb94657
,
Jan 8
looking
,
Jan 8
cc some people who may understand this better.
The demo works on Firefox and Safari so indeed this looks like a real regression. However I'm not confident I understand the security considerations in the code.
Now the demo fails because BaseRenderingContext2D::drawImage() calls SetOriginTaintedByContent() because the image source (an SVG image)'s CanvasImageSource::WouldTaintOrigin() returns true. That returns true via ImageElementBase::WouldTaintOrigin() -> ImageResourceContent::IsAccessAllowed() -> ImageResource::IsAccessAllowed() returns false. In ImageResource::IsAccessAllowed(), the ImageResource is a data URL and the response type is kDefault as expected. But it returns false because does_current_frame_has_single_security_origin is false:[1]
bool ImageResource::IsAccessAllowed(
ImageResourceInfo::DoesCurrentFrameHaveSingleSecurityOrigin
does_current_frame_has_single_security_origin) const {
if (does_current_frame_has_single_security_origin !=
ImageResourceInfo::kHasSingleSecurityOrigin) {
// <--- We early return here.
return false;
}
// <-- -We would return true if we got here.
return GetResponse().IsCorsSameOrigin();
}
|does_current_frame_has_single_security_origin| comes from SVGImage::CurrentFrameHasSingleSecurityOrigin() which is returning false because it traverses the DOM and finds a node s.t. IsSVGForeignObjectElement(*node) is true.
The explanation is:[2]
// Don't allow foreignObject elements or images that are not known to be
// single-origin since these can leak cross-origin information.
Before the breaking CL[3], the canvas wasn't tainted because there was a special case for data: URL image sources at CanvasRenderingContext::WouldTaintOrigin() that avoiding calling CanvasImageSource::WouldTaintOrigin() altogether:
if (source_url.ProtocolIsData() ||
clean_urls_.Contains(source_url.GetString())) {
return false;
}
I don't really understand why we need |does_current_frame_has_single_security_origin| instead of relying on IsCorsSameOrigin() and why data: needs to be a special case that overrides that. A "safe" way to address this regression is probably readding the data URL special case in CanvasRenderingContext::WouldTaintOrigin(). I do find it weird that we need the special case there even though ImageResource::IsAccessAllowed() would return false if called properly. I would prefer adding the data: URL exception to IsAccessAllowed() but I don't know if that will cause another security bug.
[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/loader/resource/image_resource.h?rcl=c4b3411cc95c819e508b7d58b17abff4f8f410c6&l=128
[2] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/svg/graphics/svg_image.cc?l=152&rcl=c4b3411cc95c819e508b7d58b17abff4f8f410c6
[3] https://chromium-review.googlesource.com/c/chromium/src/+/1347953
,
Jan 8
+kouhei@ for SVG
,
Jan 8
This sounds like a dupe of issue 294129, and I think that issues does a good enough job of enumerating the "fears" involved here so I will not reiterate why this code is here. Suffice to say though that it was decided in that bug to remove this restriction. (This could in turn make Image::CurrentFrameHasSingleSecurityOrigin unused, yielding an IsAccessAllowed merely checking for CORS-SO.)
,
Jan 8
Thanks fs! Agree this is a dupe of issue 294129. I therefore propose the following plan: 1. Partially revert the breaking CL, i.e., re-introduce the data: URL special case in canvas_rendering_context.cc. 2. Merge that partial revert to M72. 3. Close this regression bug and continue tracking this topic in issue 294129.
,
Jan 8
,
Jan 8
falken@ your plan in #11 sounds good, and fs@ thanks for digging up that old bug. Let me block Issue 294129 on this one so there's a chain of explanation for anyone who needs it later.
,
Jan 8
Reminder M72 Stable is coming VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Jan 8
The fix for this regression should be pretty small and I would support back-merging it to M72. I don't know how many WebGL applications were broken by this (probably not that many - we should add a WebGL conformance test for this) but we should fix it ASAP.
,
Jan 8
I have a patch under review at https://chromium-review.googlesource.com/c/chromium/src/+/1400433 but the bots aren't happy. Investigating.
,
Jan 8
Updated patch at https://chromium-review.googlesource.com/c/chromium/src/+/1400433.
,
Jan 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/91ae23a52889d7d2e09b328de03918168a01f798 commit 91ae23a52889d7d2e09b328de03918168a01f798 Author: Matt Falkenhagen <falken@chromium.org> Date: Wed Jan 09 01:33:06 2019 canvas: Restore the data: URL special case for tainting. CanvasRenderingContext::WouldTaintOrigin() had a special case for data URLs that was removed in r610498.[1] The assumption was that just calling CanvasImageSource::WouldTaintOrigin() would return false on data URLs. It turns out that function can return true due to a historical restriction on SVG foreign object nodes, as discussed in bug 294129. This CL reverses that behavior change, so data URLs again don't taint the canvas. It partially reverts r610498 and dependent change r613433. A WPT test is added. Chrome now passes the test despite bug 294129 being open because it has this special case for data URLs on canvas. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1347953 Bug: 294129, 918460 Change-Id: I7c8cb4d37d950693956785c291dfd7660c42e662 Reviewed-on: https://chromium-review.googlesource.com/c/1400433 Reviewed-by: Kenneth Russell <kbr@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/heads/master@{#620985} [modify] https://crrev.com/91ae23a52889d7d2e09b328de03918168a01f798/third_party/blink/renderer/core/html/canvas/canvas_image_source.h [modify] https://crrev.com/91ae23a52889d7d2e09b328de03918168a01f798/third_party/blink/renderer/core/html/canvas/canvas_rendering_context.cc [modify] https://crrev.com/91ae23a52889d7d2e09b328de03918168a01f798/third_party/blink/renderer/core/html/canvas/image_element_base.cc [modify] https://crrev.com/91ae23a52889d7d2e09b328de03918168a01f798/third_party/blink/renderer/core/html/canvas/image_element_base.h [modify] https://crrev.com/91ae23a52889d7d2e09b328de03918168a01f798/third_party/blink/renderer/core/html/media/html_video_element.h [add] https://crrev.com/91ae23a52889d7d2e09b328de03918168a01f798/third_party/blink/web_tests/external/wpt/2dcontext/drawing-images-to-the-canvas/drawimage_svg_image_with_foreign_object_does_not_taint.html
,
Jan 9
Fix landed. Will request merge after it cycles through canary.
,
Jan 9
Will drop ServiceWorker and WebGL components as this is reproducible with just canvas + SVG.
,
Jan 9
,
Jan 9
Pls update bug with canary result tomorrow and comment on merge safety. Thank you.
,
Jan 10
Requesting merge. I confirmed the regression is fixed on Canary 73.0.3666.0. The fix is expected to be safe: it's small and just a partial revert to original code. No relevant new crashes seen.[1] [1] https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27+AND+expanded_custom_data.ChromeCrashProto.ptype%3D%27renderer%27+AND+expanded_custom_data.ChromeCrashProto.channel%3D%27canary%27&compProp=product.Version&v1=73.0.3666.0&v2=73.0.3665.0#magicsignature:50,-magicsignature2:30,-stablesignature:30
,
Jan 10
Approving merge to M72 branch 3626 based on comments #15 and #23. Please merge ASAP and mark bug as fixed. Thank you.
,
Jan 10
Working on the manual merge since refactoring happened in the meantime. Still safe, just mechanical fixing of merge conflict and compiling to ensure it builds.
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/22de51b5fecebdc776936494ef4b6c8edd94acd0 Commit: 22de51b5fecebdc776936494ef4b6c8edd94acd0 Author: falken@chromium.org Commiter: falken@chromium.org Date: 2019-01-10 02:05:16 +0000 UTC M72: canvas: Restore the data: URL special case for tainting. CanvasRenderingContext::WouldTaintOrigin() had a special case for data URLs that was removed in r610498.[1] The assumption was that just calling CanvasImageSource::WouldTaintOrigin() would return false on data URLs. It turns out that function can return true due to a historical restriction on SVG foreign object nodes, as discussed in bug 294129. This CL reverses that behavior change, so data URLs again don't taint the canvas. It partially reverts r610498 and dependent change r613433. A WPT test is added. Chrome now passes the test despite bug 294129 being open because it has this special case for data URLs on canvas. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1347953 (cherry picked from commit 91ae23a52889d7d2e09b328de03918168a01f798) Bug: 294129, 918460 Change-Id: I7c8cb4d37d950693956785c291dfd7660c42e662 Reviewed-on: https://chromium-review.googlesource.com/c/1400433 Reviewed-by: Kenneth Russell <kbr@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#620985} Reviewed-on: https://chromium-review.googlesource.com/c/1404537 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#630} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/22de51b5fecebdc776936494ef4b6c8edd94acd0 commit 22de51b5fecebdc776936494ef4b6c8edd94acd0 Author: Matt Falkenhagen <falken@chromium.org> Date: Thu Jan 10 02:05:16 2019 M72: canvas: Restore the data: URL special case for tainting. CanvasRenderingContext::WouldTaintOrigin() had a special case for data URLs that was removed in r610498.[1] The assumption was that just calling CanvasImageSource::WouldTaintOrigin() would return false on data URLs. It turns out that function can return true due to a historical restriction on SVG foreign object nodes, as discussed in bug 294129. This CL reverses that behavior change, so data URLs again don't taint the canvas. It partially reverts r610498 and dependent change r613433. A WPT test is added. Chrome now passes the test despite bug 294129 being open because it has this special case for data URLs on canvas. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1347953 (cherry picked from commit 91ae23a52889d7d2e09b328de03918168a01f798) Bug: 294129, 918460 Change-Id: I7c8cb4d37d950693956785c291dfd7660c42e662 Reviewed-on: https://chromium-review.googlesource.com/c/1400433 Reviewed-by: Kenneth Russell <kbr@chromium.org> Commit-Queue: Matt Falkenhagen <falken@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#620985} Reviewed-on: https://chromium-review.googlesource.com/c/1404537 Reviewed-by: Matt Falkenhagen <falken@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#630} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/22de51b5fecebdc776936494ef4b6c8edd94acd0/third_party/blink/renderer/core/html/canvas/canvas_rendering_context.cc [add] https://crrev.com/22de51b5fecebdc776936494ef4b6c8edd94acd0/third_party/blink/web_tests/external/wpt/2dcontext/drawing-images-to-the-canvas/drawimage_svg_image_with_foreign_object_does_not_taint.html
,
Jan 10
,
Jan 10
The NextAction date has arrived: 2019-01-10
,
Jan 11
Verified fix in chrome Dev 73.0.3667.2
,
Jan 11
|
||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||
Comment 1 by vamshi.kommuri@chromium.org
, Jan 2