Bug 131033

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.

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?

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

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

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

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 on attachment 228731 [details] Patch V2 Clearing review flag for now.

Created attachment 275396 [details] Patch V3 Just updating the patch so that it applies on top of bug 119492 and bug 85733.