Bug 131033

Summary: Security Policy error when using MathML in canvas
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: ASSIGNED ---    
Severity: Normal CC: achristensen, ap, bfulgham, cfleizach, commit-queue, dbarton, dino, d-r, dtrebbien, fmalita, gyuyoung.kim, krit, mrobinson, pdr, sam, schenney, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://fred-wang.github.io/TeXZilla/examples/toImageWebGL.html
Bug Depends on: 156176, 85733, 119492    
Bug Blocks:    
Attachments:
Description Flags
Screenshot of the testcase (Gecko)
none
Screenshot of the testcase (WebKitGTK+)
none
Patch
none
Patch V2
none
Patch V3 none

Description Frédéric Wang (:fredw) 2014-04-01 06:26:20 PDT
Created attachment 228278 [details]
Screenshot of the testcase (Gecko)

The page

http://www.maths-informatique-jeux.com/international/mathml-in-webgl/threejs/

uses THREE.js to draw three squares with the following textures:

- A red PNG square: http://www.maths-informatique-jeux.com/international/mathml-in-webgl/threejs/32x32.png
- A green SVG square: http://www.maths-informatique-jeux.com/international/mathml-in-webgl/threejs/32x32.svg
- A blue SVG square with MathML foreign object: http://www.maths-informatique-jeux.com/international/mathml-in-webgl/threejs/32x32-mathml.svg

In attachment, I give a screenshot of the rendering in Gecko.

In WebKit, the two first squares render correctly, but the last one fails with "SecurityError: DOM Exception 18: An attempt was made to break through the security policy of the user agent.".
Comment 1 Frédéric Wang (:fredw) 2014-04-01 06:30:57 PDT
Created attachment 228279 [details]
Screenshot of the testcase (WebKitGTK+)

Here is a screenshot of the testcase in WebKit: the blue square is missing. I opened the MathML-in-SVG texture in a separate window to show that it renders correctly.
Comment 2 Frédéric Wang (:fredw) 2014-04-01 07:01:56 PDT
Created attachment 228283 [details]
Patch

So I just read the code and the problem is that SVGImage::hasSingleSecurityOrigin() returns false whenever the SVG image contains a foreign object:

// Don't allow foreignObject elements since they can leak information with arbitrary HTML (like spellcheck or control theme).
if (descendantsOfType<SVGForeignObjectElement>(*rootElement).first())
  return false;

If I remove these lines, the following example renders correctly (modulo bug 126516... on which I'm working on in bug 119038):
http://www.maths-informatique-jeux.com/international/mathml-in-webgl/

What about relaxing the condition and allowing foreignObject with MathML content?
Comment 3 Frédéric Wang (:fredw) 2014-04-07 06:45:48 PDT
Created attachment 228731 [details]
Patch V2

This one keeps forbidding XHTML but allow some MathML.

Example: https://www.youtube.com/watch?v=sN4E090yKOw

Below is an overview of the MathML supported in WebKit.

Most MathML elements are just allowing some specific math layout:

- math: the MathML root
- msubsup, mover, munder, munderover, msub, msup, mmultiscripts, mprescripts, none: draw scripted expressions
- mtable, mtr, mtd: same as HTML tables.
- mfrac: draw a fraction.
- msqrt, mroot: draw radicals
- merror: draw an error message (specific style)
- mphantom: make the invisible content
- mrow: container to group expressions
- menclose: strikeout some content, or add a frame etc
- mfenced: draw fenced expressions ; equivalent to mrow+mo
- mspace: adjust formula spacing

Some elements are a bit more special:

- maction: add some basic interactivity (useless for SVGImage, though).
- mstyle: allow to apply style attribute to content e.g. mathcolors, mathbackground etc (this is not the same as HTML <style>)
- mi, mn, mo, mtext: containers for text. they can also contain "phrasing content" (http://www.w3.org/TR/html5/dom.html#phrasing-content-1)
- semantics, annotation, annotation-xml: attach some annotations to a MathML formula (e.g. LaTeX source). Note that annotation-xml can contain flow content (http://www.w3.org/TR/html5/dom.html#flow-content-1).

Most of the MathML attributes are specific to the elements above. Here are the generic presentation attributes:

- background, color, mathbackground, mathcolor: change the colors
- dir: change the directionality
- fontfamily, fontsize, fontstyle, fontweight, mathsize: change font properties
- mathvariant: change the style of math characters
- href: not supported (https://bugs.webkit.org/show_bug.cgi?id=85733), but we can just forbid it.
Comment 4 Alexey Proskuryakov 2014-04-07 09:42:34 PDT
Comment on attachment 228731 [details]
Patch V2

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

> Source/WebCore/svg/SVGForeignObjectElement.cpp:174
> +    // We don't allow arbitrary HTML content since this could leak information (e.g. spellcheck or control theme).

Can't MathML elements that are allowed in this patch leak information like installed fonts or font preferences? How did you come up with the list of what to allow?

> Source/WebCore/svg/SVGForeignObjectElement.cpp:178
> +    // Most MathML elements are safe since they just do some math layout without advanced user interfaces or external resources. However, we are precautious and forbid some special elements.

This blacklist based security check concerns me. Generally, it's much better to have whitelisting than blacklisting.

Additional full traversal seems like it would be bad for performance.
Comment 5 Frédéric Wang (:fredw) 2014-04-07 10:02:50 PDT
(In reply to comment #4)
> > Source/WebCore/svg/SVGForeignObjectElement.cpp:174
> > +    // We don't allow arbitrary HTML content since this could leak information (e.g. spellcheck or control theme).
> 
> Can't MathML elements that are allowed in this patch leak information like installed fonts or font preferences? How did you come up with the list of what to allow?

Yes, but I don't know whether it is a concern or not. Fonts were not mentioned on bug 119492. I think this applies to SVG <text> too and actually one can already use Javascript to measure box and get some information on user fonts...

> 
> > Source/WebCore/svg/SVGForeignObjectElement.cpp:178
> > +    // Most MathML elements are safe since they just do some math layout without advanced user interfaces or external resources. However, we are precautious and forbid some special elements.
> 
> This blacklist based security check concerns me. Generally, it's much better to have whitelisting than blacklisting.
> 
> Additional full traversal seems like it would be bad for performance.

So what would you suggest instead?

I'm not a security expert, but per bug 119492 only @href is a potential problem to me (but it is not supported yet anyway).
Comment 6 Alex Christensen 2014-06-06 16:45:08 PDT
(In reply to comment #5)
> So what would you suggest instead?
Does Gecko do a full traversal?  How do they let it through?  Returning false if there is any element that is not a MathMLElement with only a list of allowed attributes and tags would be safer, but it would hurt performance if there were many "safe" MathMLElements.  The result of a traversal could be stored somewhere in case it needed to be done again, but I think doing at least one full traversal is the only way to make sure this foreign object is safe.
Comment 7 Frédéric Wang (:fredw) 2016-04-01 05:25:03 PDT
SVGImage has changed in bug 119639. There is a new comment suggesting that foreignObject could be allowed in SVG image in the future, but I can't find the corresponding bug (and bug 119639 is not public).

> // FIXME: Once foreignObject elements within SVG images are updated to not leak cross-origin data
> // (e.g., visited links, spellcheck) we can remove the SVGForeignObjectElement check here and
> // research if we can remove the Image::hasSingleSecurityOrigin mechanism entirely.

I'll probably go back to this once the patch for MathML href lands.
Comment 8 Frédéric Wang (:fredw) 2016-04-01 05:25:49 PDT
Comment on attachment 228731 [details]
Patch V2

Clearing review flag for now.
Comment 9 Frédéric Wang (:fredw) 2016-04-01 06:00:19 PDT
Created attachment 275396 [details]
Patch V3

Just updating the patch so that it applies on top of bug 119492 and bug 85733.