A canvas should not be tainted if it draws a data URL SVGImage with a <foreignObject>

[Said Abou-Hallawa]

After <http://trac.webkit.org/changeset/195614>, the canvas will tainted if it draws any SVGImage with a <foreignObject>. The reason for this is we do not want to SVGImages to leak cross-origin data (e.g., visited links, spellcheck).

But if the sourceURL of the SVGImage is a data URL, then there should not be a cross-origin data leak. So we should not taint the canvas in this case. We need to relax the tainting check for data URL SVGImage.

[Said Abou-Hallawa]

Created attachment 328199 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/35319543>

[EWS Watchlist]

Comment on attachment 328199 [details]
Patch

Attachment 328199 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5461862

New failing tests:
svg/as-image/svg-canvas-data-url-svg-with-foreign-object-not-tainted.html

[EWS Watchlist]

Created attachment 328204 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 328205 [details]
Patch

[Darin Adler]

Comment on attachment 328205 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:85
> +    const URL url = image->sourceURL();
> +    if (url.protocolIsData())
> +        return false;

It seems strange to modify this function. Why wouldn't we want hasSingleSecurityOrigin to return true for an SVG image from a data URL? Are there other clients of that function that need to get false for that case?

The const here is peculiar and not needed, nor is a local variable needed. If this check is correct, then I would write:

    if (image->sourceURL().protocolIsData())
        return false;

[Dean Jackson]

What if the foreign object references cross origin content?

[Said Abou-Hallawa]

Created attachment 328375 [details]
Patch

[Said Abou-Hallawa]

Created attachment 328378 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 328205 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:85
>> +        return false;
> 
> It seems strange to modify this function. Why wouldn't we want hasSingleSecurityOrigin to return true for an SVG image from a data URL? Are there other clients of that function that need to get false for that case?
> 
> The const here is peculiar and not needed, nor is a local variable needed. If this check is correct, then I would write:
> 
>     if (image->sourceURL().protocolIsData())
>         return false;

I think we can't modify SVGImage::hasSingleSecurityOrigin() to check for the data url source because SVGFEImageElement::hasSingleSecurityOrigin() calls Image::hasSingleSecurityOrigin().

Suppose we have the following HTML page:

    var svg = new Image();
    svg.src = "url-to-an-svg-with-an-feimage-which-has-forigen-object.svg";
    svg.onload = function() {
        ctx.drawImage(svg, 0, 0);
    }

And we have the following url-to-an-svg-with-an-feimage-which-has-forigen-object.svg:

<svg>
    <filter id="image">
        <feImage xlink:href="data:image/svg+xml;..."/>
    </filter>
    <rect width="100" height="100" filter="url(#image)"/>
</svg>

And suppose we changed SVGImage::hasSingleSecurityOrigin() to look like this:

bool SVGImage::hasSingleSecurityOrigin()
{
+    if (sourceURL().protocolIsData())
+        return true;
...
}

Then this svg will not taint the canvas regardless of it is content. In this case, of the following call stack will be executed:

CanvasRenderingContext::wouldTaintOrigin() calls SVGImage::hasSingleSecurityOrigin() for the SVGImage "url-to-an-svg-with-an-feimage-which-has-forigen-object.svg".
    SVGImage::hasSingleSecurityOrigin() calls SVGFEImageElement::hasSingleSecurityOrigin() for the child <feimage>.
        SVGFEImageElement::hasSingleSecurityOrigin() calls SVGImage::hasSingleSecurityOrigin() for its data url image.
            SVGImage::hasSingleSecurityOrigin() will return true because if (sourceURL().protocolIsData()) will be true.

And this wrong. But you are right about other places that need to do the same check. I searched the source code and I found out that taintsOrigin() in ImageBitmap.cpp needs a similar check and I fixed it the same way.

[Said Abou-Hallawa]

(In reply to Dean Jackson from comment #7)
> What if the foreign object references cross origin content?

I think that's fine. The data uri image are embedded in the page. The tags inside the SVG <foreignObject> should be treated the same way all other tags in the HTML page are treated.

For example the text inside the <a> tag and the text which is drawn on the canvas should be treated the same way.

<a href="http://www.example.com/">link</a>
<canvas></canvas>
<script>
    var data = "data:image/svg+xml;charset=utf-8,"
        + '<svg xmlns="http://www.w3.org/2000/svg">'
        + '    <foreignObject width="100%" height="100%">'
        + '        <xhtml:a href="http://www.example.com/">link</xhtml:a>'
        + '    </foreignObject>'
        + '</svg>';

    var svg = new Image();
    svg.onload = function() {
        var canvas = document.querySelector('canvas');
        var ctx = canvas.getContext('2d');
            ctx.drawImage(svg, 0, 0);
    }
    svg.src = data;
</script>

[Dean Jackson]

(In reply to Said Abou-Hallawa from comment #10)
> Comment on attachment 328205 [details]
> 
> I think we can't modify SVGImage::hasSingleSecurityOrigin() to check for the
> data url source because SVGFEImageElement::hasSingleSecurityOrigin() calls
> Image::hasSingleSecurityOrigin().
> 
> Suppose we have the following HTML page:
> 
>     var svg = new Image();
>     svg.src = "url-to-an-svg-with-an-feimage-which-has-forigen-object.svg";
>     svg.onload = function() {
>         ctx.drawImage(svg, 0, 0);
>     }
> 
> And we have the following
> url-to-an-svg-with-an-feimage-which-has-forigen-object.svg:
> 
> <svg>
>     <filter id="image">
>         <feImage xlink:href="data:image/svg+xml;..."/>
>     </filter>
>     <rect width="100" height="100" filter="url(#image)"/>
> </svg>
> 
> And suppose we changed SVGImage::hasSingleSecurityOrigin() to look like this:
> 
> bool SVGImage::hasSingleSecurityOrigin()
> {
> +    if (sourceURL().protocolIsData())
> +        return true;
> ...
> }
> 
> Then this svg will not taint the canvas regardless of it is content. In this
> case, of the following call stack will be executed:
> 
> CanvasRenderingContext::wouldTaintOrigin() calls
> SVGImage::hasSingleSecurityOrigin() for the SVGImage
> "url-to-an-svg-with-an-feimage-which-has-forigen-object.svg".
>     SVGImage::hasSingleSecurityOrigin() calls
> SVGFEImageElement::hasSingleSecurityOrigin() for the child <feimage>.
>         SVGFEImageElement::hasSingleSecurityOrigin() calls
> SVGImage::hasSingleSecurityOrigin() for its data url image.
>             SVGImage::hasSingleSecurityOrigin() will return true because if
> (sourceURL().protocolIsData()) will be true.
> 
> And this wrong. 

Sorry for being dumb, but what is wrong here? The image in the FEImage is an SVGImage, and it does have a single security origin right? (since it is a data URL) So the uppermost SVG, used to draw into the context, has a single security origin and should not taint the canvas?

[Said Abou-Hallawa]

(In reply to Dean Jackson from comment #12)
> (In reply to Said Abou-Hallawa from comment #10)
> > Comment on attachment 328205 [details]
> > 
> > I think we can't modify SVGImage::hasSingleSecurityOrigin() to check for the
> > data url source because SVGFEImageElement::hasSingleSecurityOrigin() calls
> > Image::hasSingleSecurityOrigin().
> > 
> > Suppose we have the following HTML page:
> > 
> >     var svg = new Image();
> >     svg.src = "url-to-an-svg-with-an-feimage-which-has-forigen-object.svg";
> >     svg.onload = function() {
> >         ctx.drawImage(svg, 0, 0);
> >     }
> > 
> > And we have the following
> > url-to-an-svg-with-an-feimage-which-has-forigen-object.svg:
> > 
> > <svg>
> >     <filter id="image">
> >         <feImage xlink:href="data:image/svg+xml;..."/>
> >     </filter>
> >     <rect width="100" height="100" filter="url(#image)"/>
> > </svg>
> > 
> > And suppose we changed SVGImage::hasSingleSecurityOrigin() to look like this:
> > 
> > bool SVGImage::hasSingleSecurityOrigin()
> > {
> > +    if (sourceURL().protocolIsData())
> > +        return true;
> > ...
> > }
> > 
> > Then this svg will not taint the canvas regardless of it is content. In this
> > case, of the following call stack will be executed:
> > 
> > CanvasRenderingContext::wouldTaintOrigin() calls
> > SVGImage::hasSingleSecurityOrigin() for the SVGImage
> > "url-to-an-svg-with-an-feimage-which-has-forigen-object.svg".
> >     SVGImage::hasSingleSecurityOrigin() calls
> > SVGFEImageElement::hasSingleSecurityOrigin() for the child <feimage>.
> >         SVGFEImageElement::hasSingleSecurityOrigin() calls
> > SVGImage::hasSingleSecurityOrigin() for its data url image.
> >             SVGImage::hasSingleSecurityOrigin() will return true because if
> > (sourceURL().protocolIsData()) will be true.
> > 
> > And this wrong. 
> 
> Sorry for being dumb, but what is wrong here? The image in the FEImage is an
> SVGImage, and it does have a single security origin right? (since it is a
> data URL) So the uppermost SVG, used to draw into the context, has a single
> security origin and should not taint the canvas?

You are not "dumb" Dean :). It took me awhile to remember why I wrote this comment and I realized this area is very confusing.

The html and the svg do not have the same security origin "if the svg includes foreign objects." See the for-loop in SVGImage::hasSingleSecurityOrigin() and <http://trac.webkit.org/changeset/195614>. See also the layout test LayoutTests/svg/as-image/svg-canvas-svg-with-feimage-with-link-tainted.html which looks very similar to what I described above. In fact this test case will fail if I implement Darin's suggestion.

I think the fix is to check whether the uppermost SVG is a data uri or not. If it is then, we should not taint the canvas. Putting the check of protocolIsData() at the beginning of SVGImage::hasSingleSecurityOrigin() will bypass the check of the <foreignObject> even if the uppermost SVG is not a data uri.

[Said Abou-Hallawa]

Created attachment 330750 [details]
Patch

[Said Abou-Hallawa]

(In reply to Said Abou-Hallawa from comment #14)
> Created attachment 330750 [details]
> Patch

I re-uploaded the same patch to make sure it can still be applied to the trunk.

[WebKit Commit Bot]

Comment on attachment 330750 [details]
Patch

Clearing flags on attachment: 330750

Committed r226599: <https://trac.webkit.org/changeset/226599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.