New issue
Advanced search Search tips

Issue 918460 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Regression

Blocked on:
issue 907047

Blocking:
issue 294129



Sign in to add a comment

Canvas is tainted after drawing SVG including <foreignObject>, loaded with data: URL

Reported by xilef...@gmail.com, Jan 2

Issue description

UserAgent: 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:
 
dat.gui.zip
26.8 KB Download
Labels: Needs-Triage-M71 Needs-Bisect
Blockedon: 907047
Cc: davidqu@chromium.org aaronhk@chromium.org fs...@chromium.org
Components: Blink>Canvas Blink>ServiceWorker
Owner: falken@chromium.org
Status: Assigned (was: Unconfirmed)
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?

Cc: vamshi.kommuri@chromium.org
Labels: -Pri-2 -Needs-Bisect RegressedIn-71 ReleaseBlock-Stable Triaged-ET M-72 M-71 hasbisect Pri-1
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!
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'
Labels: -M-71 -RegressedIn-71 Target-72 Target-73 M-73
Removing M-71 per comment#4.

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
Status: Started (was: Assigned)
looking
Cc: hirosh...@chromium.org pdr@chromium.org yhirano@chromium.org
Components: Blink>SVG
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac
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
Cc: kouhei@chromium.org
+kouhei@ for SVG
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.)
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.
Labels: RegressedIn-72
Blocking: 294129
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.

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

I have a patch under review at https://chromium-review.googlesource.com/c/chromium/src/+/1400433 but the bots aren't happy. Investigating.
Project Member

Comment 18 by bugdroid1@chromium.org, 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

Summary: Canvas is tainted after drawing SVG including <foreignObject>, loaded with data: URL (was: TexImage2D with SVGImage <ForeignObject> source taints canvas)
Fix landed. Will request merge after it cycles through canary.
Will drop ServiceWorker and WebGL components as this is reproducible with just canvas + SVG.
Components: -Blink>ServiceWorker -Blink>WebGL
NextAction: 2019-01-10
Pls update bug with canary result tomorrow and comment on merge safety. Thank you.
Labels: Merge-Request-72
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
Labels: -Merge-Request-72 Merge-Approved-72
Approving merge to M72 branch 3626 based on comments #15 and #23. Please merge ASAP and mark bug as fixed. Thank you.
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.
Labels: -Merge-Approved-72 Merge-Merged-72-3626
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}
Project Member

Comment 27 by bugdroid1@chromium.org, Jan 10

Labels: merge-merged-3626
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

Status: Fixed (was: Started)
The NextAction date has arrived: 2019-01-10
Verified fix in chrome Dev 73.0.3667.2
NextAction: ----

Sign in to add a comment