Allow SVG images to not taint the canvas with drawImage/drawPattern

Allow SVG images to not taint the canvas with drawImage/drawPattern

This is a merge of http://trac.webkit.org/changeset/153876 by Timothy
Hatcher with a large modification to prevent leaks through embedded
images. In SVGImage::hasSingleSecurityOrigin, this patch checks
that the SVG image does not contain other images. I've reported
this to the WebKit team in wkbug.com/119639

The main idea in this patch is to allow single origin images to be
drawn into a canvas by checking SVGImage::hasSingleSecurityOrigin().
At the moment we are blacklisting <foreignObject>, <image>, and
<feImage>.

A leak of data is possible through SVG's <a> element, and this patch
disables <a> in both HTML and SVG if the content is embedded through
an SVG image (one day, we may white-list <foreignObject>).

BUG=249037
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156422

[abarth-chromium, 2013-08-12 19:47:43]

https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLImageEle...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLImageEle...
Source/core/html/HTMLImageElement.cpp:118: setIsLink(!value.isNull() &&
!shouldProhibitLinks(this));
Can we have an ASSERT in setIsLink?

https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
Source/core/svg/graphics/SVGImage.cpp:92: for (Element* element =
ElementTraversal::firstWithin(rootElement); element; element =
ElementTraversal::next(element, rootElement)) {
Does this traverse shadow DOM?

[abarth-chromium, 2013-08-12 20:02:56]

I feel like we need a thumbs up from the security team for this CL.  This kind
of thing makes me nervous, but I don't have a concrete attack.

[pdr., 2013-08-14 22:03:49]

On 2013/08/12 20:02:56, abarth wrote:
> I feel like we need a thumbs up from the security team for this CL.  This kind
> of thing makes me nervous, but I don't have a concrete attack.

cc'ing Abhishek and Chris.
@security folks, do you see a path towards this landing? Firefox allows
everything we're allowing here and more (they allow <foreignObject> too).

[Tom Sepez, 2013-08-15 17:53:59]

https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLAnchorEl...
File Source/core/html/HTMLAnchorElement.h (right):

https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLAnchorEl...
Source/core/html/HTMLAnchorElement.h:160: bool shouldProhibitLinks(Element*);
nit: mustProhibitLinks()

https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
Source/core/svg/graphics/SVGImage.cpp:100: if
(element->hasTagName(SVGNames::feImageTag))
I worry about folks introducing a new element type and not updating this
blacklist. Is there a way to make this determination via virtual method of an
element with only the safe SVG ones overriding it with "true" (whitelist
approach)?

[pdr., 2013-08-15 20:55:08]

On 2013/08/15 17:53:59, Tom Sepez wrote:
>
https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLAnchorEl...
> File Source/core/html/HTMLAnchorElement.h (right):
> 
>
https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLAnchorEl...
> Source/core/html/HTMLAnchorElement.h:160: bool shouldProhibitLinks(Element*);
> nit: mustProhibitLinks()
> 
>
https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
> File Source/core/svg/graphics/SVGImage.cpp (right):
> 
>
https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
> Source/core/svg/graphics/SVGImage.cpp:100: if
> (element->hasTagName(SVGNames::feImageTag))
> I worry about folks introducing a new element type and not updating this
> blacklist. Is there a way to make this determination via virtual method of an
> element with only the safe SVG ones overriding it with "true" (whitelist
> approach)?

I can absolutely do that.

@tsepez, what's your feeling on the high-level security issue here? If I have
the go-ahead, I'll fix this up and land it ASAP.

[Tom Sepez, 2013-08-15 21:02:00]

On 2013/08/15 20:55:08, pdr wrote:
> On 2013/08/15 17:53:59, Tom Sepez wrote:
> >
>
https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLAnchorEl...
> > File Source/core/html/HTMLAnchorElement.h (right):
> > 
> >
>
https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLAnchorEl...
> > Source/core/html/HTMLAnchorElement.h:160: bool
shouldProhibitLinks(Element*);
> > nit: mustProhibitLinks()
> > 
> >
>
https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
> > File Source/core/svg/graphics/SVGImage.cpp (right):
> > 
> >
>
https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
> > Source/core/svg/graphics/SVGImage.cpp:100: if
> > (element->hasTagName(SVGNames::feImageTag))
> > I worry about folks introducing a new element type and not updating this
> > blacklist. Is there a way to make this determination via virtual method of
an
> > element with only the safe SVG ones overriding it with "true" (whitelist
> > approach)?
> 
> I can absolutely do that.
> 
> @tsepez, what's your feeling on the high-level security issue here? If I have
> the go-ahead, I'll fix this up and land it ASAP.

Higher-level, I'm not the expert. But I see no reason to block this.

[pdr., 2013-08-16 21:19:17]

On 2013/08/12 19:47:43, abarth wrote:
>
https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLImageEle...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLImageEle...
> Source/core/html/HTMLImageElement.cpp:118: setIsLink(!value.isNull() &&
> !shouldProhibitLinks(this));
> Can we have an ASSERT in setIsLink?

This is tricky because Node::setIsLink can't access the toElement cast required
to assert shouldProhibitLinks. Assuming I'm not overlooking something silly,
would a comment suffice here?

[abarth-chromium, 2013-08-19 18:13:32]

On 2013/08/16 21:19:17, pdr wrote:
> This is tricky because Node::setIsLink can't access the toElement cast
required
> to assert shouldProhibitLinks. Assuming I'm not overlooking something silly,
> would a comment suffice here?

Maybe in debug mode we should move setIsLink out-of-line so we can see the
toElement cast?

[pdr., 2013-08-19 23:16:16]

https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLAnchorEl...
File Source/core/html/HTMLAnchorElement.h (right):

https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLAnchorEl...
Source/core/html/HTMLAnchorElement.h:160: bool shouldProhibitLinks(Element*);
On 2013/08/15 17:53:59, Tom Sepez wrote:
> nit: mustProhibitLinks()

Done.

https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLImageEle...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/22604008/diff/1/Source/core/html/HTMLImageEle...
Source/core/html/HTMLImageElement.cpp:118: setIsLink(!value.isNull() &&
!shouldProhibitLinks(this));
On 2013/08/12 19:47:44, abarth wrote:
> Can we have an ASSERT in setIsLink?

Done.

https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
Source/core/svg/graphics/SVGImage.cpp:92: for (Element* element =
ElementTraversal::firstWithin(rootElement); element; element =
ElementTraversal::next(element, rootElement)) {
On 2013/08/12 19:47:44, abarth wrote:
> Does this traverse shadow DOM?

Fortunately, custom elements cannot be created declaratively so I think we're
good here.

https://codereview.chromium.org/22604008/diff/1/Source/core/svg/graphics/SVGI...
Source/core/svg/graphics/SVGImage.cpp:100: if
(element->hasTagName(SVGNames::feImageTag))
On 2013/08/15 17:53:59, Tom Sepez wrote:
> I worry about folks introducing a new element type and not updating this
> blacklist. Is there a way to make this determination via virtual method of an
> element with only the safe SVG ones overriding it with "true" (whitelist
> approach)?

I think this will actually end up being prohibitively expensive to do :/

[abarth-chromium, 2013-08-19 23:54:36]

LGTM

I suspect we'll have some security bugs in this feature over time, but the
benefit seems worth fighting through them.

https://codereview.chromium.org/22604008/diff/15001/Source/core/svg/graphics/...
File Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/22604008/diff/15001/Source/core/svg/graphics/...
Source/core/svg/graphics/SVGImage.cpp:101: return false;
This blacklist is scary to me.  I'm sure we're missing some things...

https://codereview.chromium.org/22604008/diff/15001/Source/core/svg/graphics/...
Source/core/svg/graphics/SVGImage.cpp:106: return true;
Can we RELEASE_ASSERT or something that the document's readystate has reached
interactive?  We should be sure that we're done loading the svg image.

[pdr., 2013-08-20 06:59:13]

On 2013/08/19 23:54:36, abarth wrote:
> LGTM
> 
> I suspect we'll have some security bugs in this feature over time, but the
> benefit seems worth fighting through them.
> 
>
https://codereview.chromium.org/22604008/diff/15001/Source/core/svg/graphics/...
> File Source/core/svg/graphics/SVGImage.cpp (right):
> 
>
https://codereview.chromium.org/22604008/diff/15001/Source/core/svg/graphics/...
> Source/core/svg/graphics/SVGImage.cpp:101: return false;
> This blacklist is scary to me.  I'm sure we're missing some things...

Long-term, I think we should remove this check and allow <foreignObject>. Baby
steps :)

> 
>
https://codereview.chromium.org/22604008/diff/15001/Source/core/svg/graphics/...
> Source/core/svg/graphics/SVGImage.cpp:106: return true;
> Can we RELEASE_ASSERT or something that the document's readystate has reached
> interactive?  We should be sure that we're done loading the svg image.
I think this is handled with the m_page check, which is used elsewhere for this
purpose.

[commit-bot: I haz the power, 2013-08-20 07:07:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/22604008/26001

[abarth-chromium, 2013-08-20 07:17:28]

On 2013/08/20 06:59:13, pdr wrote:
> I think this is handled with the m_page check, which is used elsewhere for
this
> purpose.

That seems unrelated to the m_page check.

[commit-bot: I haz the power, 2013-08-20 08:55:46]

Failed to apply patch for Source/core/rendering/svg/RenderSVGRoot.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/svg/RenderSVGRoot.cpp
  Hunk #2 FAILED at 111.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/rendering/svg/RenderSVGRoot.cpp.rej

Patch:       Source/core/rendering/svg/RenderSVGRoot.cpp
Index: Source/core/rendering/svg/RenderSVGRoot.cpp
diff --git a/Source/core/rendering/svg/RenderSVGRoot.cpp
b/Source/core/rendering/svg/RenderSVGRoot.cpp
index
3e63935b13d1cc7bfd47d01161016b5e70a647fc..c6cb3c6447d0d1b50d7fe5f95319acfa05030ec2
100644
--- a/Source/core/rendering/svg/RenderSVGRoot.cpp
+++ b/Source/core/rendering/svg/RenderSVGRoot.cpp
@@ -40,6 +40,7 @@
 #include "core/rendering/svg/SVGResourcesCache.h"
 #include "core/svg/SVGElement.h"
 #include "core/svg/SVGSVGElement.h"
+#include "core/svg/graphics/SVGImage.h"
 
 using namespace std;
 
@@ -110,22 +111,7 @@ void
RenderSVGRoot::computeIntrinsicRatioInformation(FloatSize& intrinsicSize, d
 
 bool RenderSVGRoot::isEmbeddedThroughSVGImage() const
 {
-    if (!node())
-        return false;
-
-    Frame* frame = node()->document()->frame();
-    if (!frame)
-        return false;
-
-    // Test whether we're embedded through an img.
-    if (!frame->page())
-        return false;
-
-    ChromeClient* chromeClient = frame->page()->chrome().client();
-    if (!chromeClient || !chromeClient->isSVGImageChromeClient())
-        return false;
-
-    return true;
+    return SVGImage::isInSVGImage(toSVGSVGElement(node()));
 }
 
 bool RenderSVGRoot::isEmbeddedThroughFrameContainingSVGDocument() const

[pdr., 2013-08-20 17:29:18]

On 2013/08/20 07:17:28, abarth wrote:
> On 2013/08/20 06:59:13, pdr wrote:
> > I think this is handled with the m_page check, which is used elsewhere for
> this
> > purpose.
> 
> That seems unrelated to the m_page check.

m_page is set in SVGImage::dataChanged if allDataReceived=true. Am I
misunderstanding what you mean? Since this failed the CQ we have a chance to fix
this.

[commit-bot: I haz the power, 2013-08-20 18:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/22604008/40001

[abarth-chromium, 2013-08-20 19:31:54]

We talked in person.  We agreed that the is correct as written today but that
adding an ASSERT might help us in the future if the code changes.

[commit-bot: I haz the power, 2013-08-21 02:54:22]

Change committed as 156422