Bug 119492

Summary: Implement SVGImage::hasSingleSecurityOrigin()
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: SVGAssignee: Timothy Hatcher <timothy>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cevans, cmarcelo, commit-queue, d-r, dtrebbien, esprehn+autocc, fmalita, fred.wang, glenn, graouts, inferno, joepeck, jonathan, kangil.han, kondapallykalyan, krit, pdr, rniwa, rtakacs, rwlbuis, schenney, seokju, thorton, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=156176
Bug Depends on:    
Bug Blocks: 131033, 118677, 119639    
Attachments:
Description Flags
Proposed Change (WIP)
none
Proposed Change (WIP 2)
none
Patch
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Timothy Hatcher 2013-08-05 12:00:39 PDT
By not implementing hasSingleSecurityOrigin(), any SVG rendered into a canvas will taint the canvas and prevent getting the pixels or image data. We should implement a basic version of hasSingleSecurityOrigin() that allows simple SVGs to not taint a canvas.
Comment 1 Philip Rogers 2013-08-05 12:44:07 PDT
Curious: do you have a plan for handling cross-origin visited link colors (<a>) and foreignObject?
Comment 2 Timothy Hatcher 2013-08-05 21:59:52 PDT
My interest (Web Inspector) only cares about simple SVGs, nothing interactive. No linked content, just paths really.
Comment 3 Timothy Hatcher 2013-08-06 20:33:36 PDT
Created attachment 208235 [details]
Proposed Change (WIP)

Here is an attempt at getting this working for simple SVGs. This works for the images the Inspector uses.

Any thoughts? Am I completely off base here?
Comment 4 Philip Rogers 2013-08-06 21:47:18 PDT
Comment on attachment 208235 [details]
Proposed Change (WIP)

SVG images (<img src="svg.svg">) shouldn't be able to load external resources to begin with (even same origin!), so these checks do not seem appropriate. Furthermore, a one-time static check can be circumvented using SMIL; the following svg image will pass the selector check but will still contain an xlink:href:
<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<!DOCTYPE svg  PUBLIC '-//W3C//DTD SVG 1.1//EN'  'http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd'>
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="200" height="200">
  <rect id="rect" x="100" y="100" width="100" height="100" fill="green"/>
  <use x="-100" y="-100" width="50" height="50" transform="rotate(10)">
    <set attributeName="xlink:href" to="#rect" begin="0s"/>
  </use>
</svg>


My understanding of the security threat here is that svg images can leak cross origin data through <a> (due to the link highlight color), foreignObject, and possibly tref. Disabling these explicitly may be a better approach.
Comment 5 Timothy Hatcher 2013-08-07 06:07:35 PDT
(In reply to comment #4)
> (From update of attachment 208235 [details])
> SVG images (<img src="svg.svg">) shouldn't be able to load external resources to begin with (even same origin!), so these checks do not seem appropriate.

Okay, that simplifies things. (FWIW, external resources use to load in SVG images in Safari 6, but no longer do in TOT.)

> Furthermore, a one-time static check can be circumvented using SMIL; the following svg image will pass the selector check but will still contain an xlink:href.

This is not a static check. It happens each time the SVG is drawn into the canvas, so the SMIL approach changes nothing. It still gets caught.

> My understanding of the security threat here is that svg images can leak cross origin data through <a> (due to the link highlight color), foreignObject, and possibly tref. Disabling these explicitly may be a better approach.

Link visited color I get. <foreignObject> I get if it contains XHTML which can contain links too. I don't see how <tref> can be a problem.

My gut now says checking for "*|a[*|href]" elements is enough, even for XHTML inside <foreignObject>. But I am an SVG noob, please, correct me if I am wrong.
Comment 6 Timothy Hatcher 2013-08-07 06:13:41 PDT
Created attachment 208263 [details]
Proposed Change (WIP 2)
Comment 7 Timothy Hatcher 2013-08-07 08:35:03 PDT
The other option would be to have a setting to force links to be unvisited and use that for SVGImage's Page.
Comment 8 Timothy Hatcher 2013-08-07 12:41:10 PDT
Created attachment 208292 [details]
Patch
Comment 9 Darin Adler 2013-08-07 12:51:45 PDT
Comment on attachment 208292 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:286
> +    DEFINE_STATIC_LOCAL(AtomicString, questionableElementsSelector, ("*|a[*|href]", AtomicString::ConstructFromLiteral));
> +    RefPtr<Element> questionableElement = rootElement->querySelector(questionableElementsSelector, ASSERT_NO_EXCEPTION);

I’d rather see a hand-written loop than use of querySelector here. The strange need for an atomic string and such. Any good reason not to write the loop? It's a simple traversal with a hasTagName and hasAttribute check on each element.
Comment 10 Timothy Hatcher 2013-08-07 12:56:34 PDT
(In reply to comment #9)
> (From update of attachment 208292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208292&action=review
> 
> > Source/WebCore/svg/graphics/SVGImage.cpp:286
> > +    DEFINE_STATIC_LOCAL(AtomicString, questionableElementsSelector, ("*|a[*|href]", AtomicString::ConstructFromLiteral));
> > +    RefPtr<Element> questionableElement = rootElement->querySelector(questionableElementsSelector, ASSERT_NO_EXCEPTION);
> 
> I’d rather see a hand-written loop than use of querySelector here. The strange need for an atomic string and such. Any good reason not to write the loop? It's a simple traversal with a hasTagName and hasAttribute check on each element.

querySelector is smarter and cached, so repeated calls will have better performance than a loop of all the elements.
Comment 11 Build Bot 2013-08-07 13:15:15 PDT
Comment on attachment 208292 [details]
Patch

Attachment 208292 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1370885

New failing tests:
http/tests/security/canvas-remote-read-svg-image.html
fast/canvas/svg-taint.html
http/tests/security/canvas-remote-read-data-url-svg-image.html
Comment 12 Build Bot 2013-08-07 13:15:18 PDT
Created attachment 208294 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.4
Comment 13 Joseph Pecoraro 2013-08-07 13:27:16 PDT
Comment on attachment 208292 [details]
Patch

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

Looks good to me. Tim also answered Darin's question.

>>> Source/WebCore/svg/graphics/SVGImage.cpp:286
>>> +    RefPtr<Element> questionableElement = rootElement->querySelector(questionableElementsSelector, ASSERT_NO_EXCEPTION);
>> 
>> I’d rather see a hand-written loop than use of querySelector here. The strange need for an atomic string and such. Any good reason not to write the loop? It's a simple traversal with a hasTagName and hasAttribute check on each element.
> 
> querySelector is smarter and cached, so repeated calls will have better performance than a loop of all the elements.

It looks like querySelector only caches the "SelectorQuery" object, but doesn't cache seem to cache anything else. selectorQuery->queryFirst(…) looks like it would still always happen and walk the tree. So I'm not sure repeated queries would actually perform better. So Darin's comment still seems reasonable.
Comment 14 Timothy Hatcher 2013-08-07 14:57:30 PDT
Created attachment 208301 [details]
Patch
Comment 15 Philip Rogers 2013-08-07 15:17:39 PDT
I think this is heading in the right direction, but a few issues remain.


> This is not a static check. It happens each time the SVG is drawn into the canvas, so the SMIL approach changes nothing. It still gets caught.
Did you try the example I provided?


> My gut now says checking for "*|a[*|href]" elements is enough, even for XHTML inside <foreignObject>. But I am an SVG noob, please, correct me if I am wrong.

Because href is useful in even simple svg examples, I would recommend blacklisting <foreignObject> instead of the href attribute.

Firefox may have something to teach us here about <a>. Firefox does not highlight link colors in svg images at all; this is doable by checking if we're an image in SVGAElement.cpp before setting setIsLink. I think this may be a better approach than walking the dom looking for links.

Lastly, subimages are another issue. You'll want to recursively walk all <image>s to ensure they all have a single origin as well. Otherwise, someone could embed a data uri svg image that itself contains <a> elements.
Comment 16 Timothy Hatcher 2013-08-07 15:31:44 PDT
(In reply to comment #15)
> I think this is heading in the right direction, but a few issues remain.
> 
> 
> > This is not a static check. It happens each time the SVG is drawn into the canvas, so the SMIL approach changes nothing. It still gets caught.
> Did you try the example I provided?
> 
> 
> > My gut now says checking for "*|a[*|href]" elements is enough, even for XHTML inside <foreignObject>. But I am an SVG noob, please, correct me if I am wrong.
> 
> Because href is useful in even simple svg examples, I would recommend blacklisting <foreignObject> instead of the href attribute.

You can have links in SVG without <foreignObject>. I was blocking <a href> not other useful uses of href. My first patch did block all href uses, and that was wrong. My latest patch blocks any link element.

> Firefox may have something to teach us here about <a>. Firefox does not highlight link colors in svg images at all; this is doable by checking if we're an image in SVGAElement.cpp before setting setIsLink. I think this may be a better approach than walking the dom looking for links.

That could work. But we would also need to block <foreignObject> or do similar in HTMLAnchorElement.

> Lastly, subimages are another issue. You'll want to recursively walk all <image>s to ensure they all have a single origin as well. Otherwise, someone could embed a data uri svg image that itself contains <a> elements.

Good point.
Comment 17 Philip Rogers 2013-08-07 15:36:29 PDT
(In reply to comment #16)
> (In reply to comment #15)
> > I think this is heading in the right direction, but a few issues remain.
> > 
> > 
> > > This is not a static check. It happens each time the SVG is drawn into the canvas, so the SMIL approach changes nothing. It still gets caught.
> > Did you try the example I provided?
> > 
> > 
> > > My gut now says checking for "*|a[*|href]" elements is enough, even for XHTML inside <foreignObject>. But I am an SVG noob, please, correct me if I am wrong.
> > 
> > Because href is useful in even simple svg examples, I would recommend blacklisting <foreignObject> instead of the href attribute.
> 
> You can have links in SVG without <foreignObject>. I was blocking <a href> not other useful uses of href. My first patch did block all href uses, and that was wrong. My latest patch blocks any link element.
> 
> > Firefox may have something to teach us here about <a>. Firefox does not highlight link colors in svg images at all; this is doable by checking if we're an image in SVGAElement.cpp before setting setIsLink. I think this may be a better approach than walking the dom looking for links.
> 
> That could work. But we would also need to block <foreignObject> or do similar in HTMLAnchorElement.
> 
> > Lastly, subimages are another issue. You'll want to recursively walk all <image>s to ensure they all have a single origin as well. Otherwise, someone could embed a data uri svg image that itself contains <a> elements.
> 
> Good point.

Awesome, I think we're on the same page. Just to make sure, is this the current plan?
1) Block <foreignObject>.
2) Recursively check that subimages are from the same origin.
3) Block <a> either by not coloring it like a link, or by walking the tree.
Comment 18 Timothy Hatcher 2013-08-07 15:45:57 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > I think this is heading in the right direction, but a few issues remain.
> > > 
> > > 
> > > > This is not a static check. It happens each time the SVG is drawn into the canvas, so the SMIL approach changes nothing. It still gets caught.
> > > Did you try the example I provided?
> > > 
> > > 
> > > > My gut now says checking for "*|a[*|href]" elements is enough, even for XHTML inside <foreignObject>. But I am an SVG noob, please, correct me if I am wrong.
> > > 
> > > Because href is useful in even simple svg examples, I would recommend blacklisting <foreignObject> instead of the href attribute.
> > 
> > You can have links in SVG without <foreignObject>. I was blocking <a href> not other useful uses of href. My first patch did block all href uses, and that was wrong. My latest patch blocks any link element.
> > 
> > > Firefox may have something to teach us here about <a>. Firefox does not highlight link colors in svg images at all; this is doable by checking if we're an image in SVGAElement.cpp before setting setIsLink. I think this may be a better approach than walking the dom looking for links.
> > 
> > That could work. But we would also need to block <foreignObject> or do similar in HTMLAnchorElement.
> > 
> > > Lastly, subimages are another issue. You'll want to recursively walk all <image>s to ensure they all have a single origin as well. Otherwise, someone could embed a data uri svg image that itself contains <a> elements.
> > 
> > Good point.
> 
> Awesome, I think we're on the same page. Just to make sure, is this the current plan?
> 1) Block <foreignObject>.
> 2) Recursively check that subimages are from the same origin.
> 3) Block <a> either by not coloring it like a link, or by walking the tree.

If you do 3, you shouldn't need to do 2, right? You also don't need to do 1 if HTMLAnchorElement in SVGImage does not color.
Comment 19 Timothy Hatcher 2013-08-07 19:02:02 PDT
Comment on attachment 208301 [details]
Patch

I have this working differently now. It is very simple to prevent SVG and HTML links from rendering as links when in an SVGImage. I just need to redo the tests now that even links are allowed and there is no tainting at all for SVG.
Comment 20 Timothy Hatcher 2013-08-08 06:57:20 PDT
Created attachment 208340 [details]
Patch
Comment 21 Timothy Hatcher 2013-08-08 08:18:32 PDT
Data URL SVG images are not allowed. If they were, this latest patch would still work.
Comment 22 Philip Rogers 2013-08-08 10:51:41 PDT
I like where this patch is going! One issue that remains is <foreignObject>: I think we should still disable this completely because it opens a whole can of security worms since fO can contain almost arbitrary html.

(In reply to comment #21)
> Data URL SVG images are not allowed. If they were, this latest patch would still work.

Unfortunately this is a bit of a security red herring. There's a bug where you can pre-seed the memorycache and get data uri images to work in SVG by loading the image twice. The Swiffy Flash->SVG converter uses the preseed technique to workaround wkbug.com/118068.
Comment 23 Darin Adler 2013-08-08 10:59:11 PDT
Comment on attachment 208340 [details]
Patch

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

Looks good. review- because of change log, but I also suggest doing this without touching Element.h/cpp

> Source/WebCore/ChangeLog:15
> +        (WebCore::SVGImage::hasSingleSecurityOrigin): Added. Look for links and return
> +        false if any link is found.

This no longer matches the patch. Need a revised change log.

> Source/WebCore/dom/Element.h:592
> +    bool isEmbeddedThroughSVGImage() const;

It’s great to have this be a function, but I don’t think it’s good as a member function. Can we make it a free function in HTMLAnchorElement.h/cpp and have the other files include HTMLAnchorElement.h? Or something similar in some SVG source file? Also, maybe we could name it for its effect rather than for what it checks, shouldDisableLinks or shouldProhibitLinks?

> Source/WebCore/svg/graphics/SVGImage.h:57
> +    // SVG images disallow external resources (including same origin).
> +    virtual bool hasSingleSecurityOrigin() const OVERRIDE { return true; }

The comment here is too oblique, I think. Maybe something more like this?

    // Because SVG image rendering disallows external resources and links, these images effectively are restricted to a single security origin.
Comment 24 Timothy Hatcher 2013-08-08 11:12:43 PDT
(In reply to comment #22)
> I like where this patch is going! One issue that remains is <foreignObject>: I think we should still disable this completely because it opens a whole can of security worms since fO can contain almost arbitrary html.

I disagree that foreignObject needs blocked, unless you have a concrete example of an issue.
Comment 25 Timothy Hatcher 2013-08-08 11:15:04 PDT
Comment on attachment 208340 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        false if any link is found.
> 
> This no longer matches the patch. Need a revised change log.

Opps.

>> Source/WebCore/dom/Element.h:592
>> +    bool isEmbeddedThroughSVGImage() const;
> 
> It’s great to have this be a function, but I don’t think it’s good as a member function. Can we make it a free function in HTMLAnchorElement.h/cpp and have the other files include HTMLAnchorElement.h? Or something similar in some SVG source file? Also, maybe we could name it for its effect rather than for what it checks, shouldDisableLinks or shouldProhibitLinks?

OK, I started down that road but ended up with this. I'm fine making it standalone. I kept the isEmbeddedThroughSVGImage name so it could be shared by RenderSVGRoot.cpp, which is unrelated to links.

>> Source/WebCore/svg/graphics/SVGImage.h:57
>> +    virtual bool hasSingleSecurityOrigin() const OVERRIDE { return true; }
> 
> The comment here is too oblique, I think. Maybe something more like this?
> 
>     // Because SVG image rendering disallows external resources and links, these images effectively are restricted to a single security origin.

OK.
Comment 26 Timothy Hatcher 2013-08-08 11:21:33 PDT
(In reply to comment #22)
> I like where this patch is going! One issue that remains is <foreignObject>: I think we should still disable this completely because it opens a whole can of security worms since fO can contain almost arbitrary html.

Scripts and sub-resource loading are all disabled in SVGImage. That applies to <foreignObject> too.
Comment 27 Timothy Hatcher 2013-08-08 11:53:32 PDT
Created attachment 208358 [details]
Patch
Comment 28 Darin Adler 2013-08-08 12:13:18 PDT
Comment on attachment 208358 [details]
Patch

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

review- because I still would prefer we not touch Element.h

> Source/WebCore/dom/Element.h:802
> +bool isElementContainedInSVGImage(const Element*);

This is the wrong header file. I suggest putting it into SVGImage.h or HTMLAnchorElement.h since it’s about SVG images and links.

One of the points of having something be a free function instead of a member function is that we can encapsulate ideas better and not pollute the core classes with all the more complex concepts.

It would be better to have this function name talk about link policy, rather than the SVG Image check. That would make the reason for the check at all the call sites clearer. This function would then be the one place to add comments explaining the policy. Otherwise, at each call site we’d raise the question, “Why is an SVG image special here?” And then have to add comments everywhere.

If you don’t agree, and want to keep something more like the current name, I would use isInSVGImage; I don’t think the words “element contained” are needed.

> Source/WebCore/svg/graphics/SVGImage.cpp:38
> +#include "NodeTraversal.h"

Why?
Comment 29 Darin Adler 2013-08-08 12:14:40 PDT
Comment on attachment 208358 [details]
Patch

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

>> Source/WebCore/dom/Element.h:802
>> +bool isElementContainedInSVGImage(const Element*);
> 
> This is the wrong header file. I suggest putting it into SVGImage.h or HTMLAnchorElement.h since it’s about SVG images and links.
> 
> One of the points of having something be a free function instead of a member function is that we can encapsulate ideas better and not pollute the core classes with all the more complex concepts.
> 
> It would be better to have this function name talk about link policy, rather than the SVG Image check. That would make the reason for the check at all the call sites clearer. This function would then be the one place to add comments explaining the policy. Otherwise, at each call site we’d raise the question, “Why is an SVG image special here?” And then have to add comments everywhere.
> 
> If you don’t agree, and want to keep something more like the current name, I would use isInSVGImage; I don’t think the words “element contained” are needed.

Oops, I missed your comment about RenderSVGRoot. I think that SVGImage.h should have the isInSVGImage, and HTMLAnchorElement.h should have shouldProhibitLinks or shouldAllowLinks, and the second function should call the first.
Comment 30 Darin Adler 2013-08-08 12:15:07 PDT
Comment on attachment 208358 [details]
Patch

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

>>> Source/WebCore/dom/Element.h:802
>>> +bool isElementContainedInSVGImage(const Element*);
>> 
>> This is the wrong header file. I suggest putting it into SVGImage.h or HTMLAnchorElement.h since it’s about SVG images and links.
>> 
>> One of the points of having something be a free function instead of a member function is that we can encapsulate ideas better and not pollute the core classes with all the more complex concepts.
>> 
>> It would be better to have this function name talk about link policy, rather than the SVG Image check. That would make the reason for the check at all the call sites clearer. This function would then be the one place to add comments explaining the policy. Otherwise, at each call site we’d raise the question, “Why is an SVG image special here?” And then have to add comments everywhere.
>> 
>> If you don’t agree, and want to keep something more like the current name, I would use isInSVGImage; I don’t think the words “element contained” are needed.
> 
> Oops, I missed your comment about RenderSVGRoot. I think that SVGImage.h should have the isInSVGImage, and HTMLAnchorElement.h should have shouldProhibitLinks or shouldAllowLinks, and the second function should call the first.

Maybe not literally "SVGImage.h", but whatever file we have that is closest to that.
Comment 31 Timothy Hatcher 2013-08-08 12:54:38 PDT
Created attachment 208363 [details]
Patch
Comment 32 Timothy Hatcher 2013-08-08 12:59:40 PDT
Comment on attachment 208358 [details]
Patch

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

>>>> Source/WebCore/dom/Element.h:802
>>>> +bool isElementContainedInSVGImage(const Element*);
>>> 
>>> This is the wrong header file. I suggest putting it into SVGImage.h or HTMLAnchorElement.h since it’s about SVG images and links.
>>> 
>>> One of the points of having something be a free function instead of a member function is that we can encapsulate ideas better and not pollute the core classes with all the more complex concepts.
>>> 
>>> It would be better to have this function name talk about link policy, rather than the SVG Image check. That would make the reason for the check at all the call sites clearer. This function would then be the one place to add comments explaining the policy. Otherwise, at each call site we’d raise the question, “Why is an SVG image special here?” And then have to add comments everywhere.
>>> 
>>> If you don’t agree, and want to keep something more like the current name, I would use isInSVGImage; I don’t think the words “element contained” are needed.
>> 
>> Oops, I missed your comment about RenderSVGRoot. I think that SVGImage.h should have the isInSVGImage, and HTMLAnchorElement.h should have shouldProhibitLinks or shouldAllowLinks, and the second function should call the first.
> 
> Maybe not literally "SVGImage.h", but whatever file we have that is closest to that.

I put shouldProhibitLinks on Element so the three subclasses can all use it, not just HTMLAnchorElement.

>> Source/WebCore/svg/graphics/SVGImage.cpp:38
>> +#include "NodeTraversal.h"
> 
> Why?

Forgot to remove this.
Comment 33 Philip Rogers 2013-08-08 13:44:07 PDT
(In reply to comment #24)
> (In reply to comment #22)
> > I like where this patch is going! One issue that remains is <foreignObject>: I think we should still disable this completely because it opens a whole can of security worms since fO can contain almost arbitrary html.
> 
> I disagree that foreignObject needs blocked, unless you have a concrete example of an issue.

You can leak the user's theme by drawing a button in a foreign object and then reading it out of the canvas. If you care about the user's spellcheck dictionary, you may be able to coax an input box to underline a misspelled word. I'm not sure if these are serious issues, but these haven't been exposed before.

I'm cc'ing inferno and cevans in case they can think of other attack vectors.
Comment 34 Chris Evans 2013-08-08 14:23:05 PDT
"I disagree that foreignObject needs blocked, unless you have a concrete example of an issue."

This seems like a grenade that isn't worth playing with in this initial patch. I recommend we land with <foreignObject> blocked and then file a follow-up bug to iterate and improve, with input from webkit-security and other security experts.

Certainly, the Blink variant of this patch will be blocking <foreignObject>, so it'd be nice to be compatible. By also blocking <foreignObject>, you'll mitigate the risk that Safari ends up with a security bug that Chrome and Opera do not have.
Comment 35 Darin Adler 2013-08-08 19:09:21 PDT
Comment on attachment 208363 [details]
Patch

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

I’m sorry, not trying to be difficult, but I still say review-

> Source/WebCore/dom/Element.h:679
> +    bool shouldProhibitLinks() const;

I’m sorry to be a broken record, but I still do not think this should be a member function. I think a free function in HTMLAnchorElement.h would be better. The other link-hosting classes can include HTMLAnchorElement.h to get this.
Comment 36 Timothy Hatcher 2013-08-08 19:31:44 PDT
Comment on attachment 208363 [details]
Patch

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

>> Source/WebCore/dom/Element.h:679
>> +    bool shouldProhibitLinks() const;
> 
> I’m sorry to be a broken record, but I still do not think this should be a member function. I think a free function in HTMLAnchorElement.h would be better. The other link-hosting classes can include HTMLAnchorElement.h to get this.

No problem. I'll make it standalone.
Comment 37 Timothy Hatcher 2013-08-08 19:33:02 PDT
(In reply to comment #34)
> "I disagree that foreignObject needs blocked, unless you have a concrete example of an issue."
> 
> This seems like a grenade that isn't worth playing with in this initial patch. I recommend we land with <foreignObject> blocked and then file a follow-up bug to iterate and improve, with input from webkit-security and other security experts.
> 
> Certainly, the Blink variant of this patch will be blocking <foreignObject>, so it'd be nice to be compatible. By also blocking <foreignObject>, you'll mitigate the risk that Safari ends up with a security bug that Chrome and Opera do not have.

I'll block it.
Comment 38 Timothy Hatcher 2013-08-08 20:34:36 PDT
Created attachment 208393 [details]
Patch
Comment 39 Darin Adler 2013-08-08 23:17:18 PDT
Comment on attachment 208393 [details]
Patch

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

> Source/WebCore/svg/graphics/SVGImage.cpp:72
> +    Frame* frame = m_page->mainFrame();
> +    SVGSVGElement* rootElement = toSVGDocument(frame->document())->rootElement();

For the record, there was no need to put frame into a local variable.
Comment 40 WebKit Commit Bot 2013-08-08 23:38:43 PDT
Comment on attachment 208393 [details]
Patch

Clearing flags on attachment: 208393

Committed r153876: <http://trac.webkit.org/changeset/153876>
Comment 41 WebKit Commit Bot 2013-08-08 23:38:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Philip Rogers 2013-08-16 19:53:16 PDT
*** Bug 108755 has been marked as a duplicate of this bug. ***
Comment 43 Frédéric Wang (:fredw) 2014-04-01 07:28:24 PDT
(In reply to comment #34)
> This seems like a grenade that isn't worth playing with in this initial patch. I recommend we land with <foreignObject> blocked and then file a follow-up bug to iterate and improve, with input from webkit-security and other security experts.
> 
> Certainly, the Blink variant of this patch will be blocking <foreignObject>, so it'd be nice to be compatible. By also blocking <foreignObject>, you'll mitigate the risk that Safari ends up with a security bug that Chrome and Opera do not have.

FYI, I've opened bug 131033 for MathML in <foreignObject>. The use case that I have in mind is to integrate mathematical formulas in 3D schemas. At the moment, with WebKit, I can not integrate them directly in the WebGL canvas (and have to use https://github.com/unconed/MathBox.js to position formulas above the canvas).

MathML can have links but this is not implemented yet (bug 85733). Gecko seems to block the color for MathML links when embedded in a WebGL canvas. I don't know see other security issues off the top of my head (except of course that HTML can itself be embedded in MathML, and so this should be handled too).