1494356 - Styling SVG element via DOM API before adding it to DOM produce CSP errors (due to SVG elements reparsing their inline style attribute on document insertion, ever since bug 387466)

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Andrey Roenko]

Attachment: firefox.svg.bug.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180913170256

Steps to reproduce:

1. Create SVG element in SVG namespace.
2. Style it via element.style.whatever.
3. Attach to DOM.


Actual results:

CSP Error is reported to the console:
> Content Security Policy: The page’s settings blocked the loading of a resource at self (“style-src”).


Expected results:

Styles should apply and no CSP errors should be generated.
If you swap steps 2 and 3 everything works as expected.

Comment 1

[Andrey Roenko]

I've found that this behavior appeared in version 45.

Comment 2

[Robert Longson [:longsonr]]

Could you use https://mozilla.github.io/mozregression/ to get a more exact regression range?

Comment 3

[Andrey Roenko]

This is the last bad build I've got after bisecting by date.

app_name: firefox
build_date: 2015-11-18
build_file: /home/flapenguin/.mozilla/mozregression/persist/2015-11-18--mozilla-central--firefox-45.0a1.en-US.linux-x86_64.tar.bz2
build_type: nightly
build_url: https://archive.mozilla.org/pub/firefox/nightly/2015/11/2015-11-18-03-02-32-mozilla-central/firefox-45.0a1.en-US.linux-x86_64.tar.bz2
changeset: eb3016abd37db2e6a6d923265047e84b12c0af61
pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cc48981c026c50fdf80d47b040ae1fb8fe99ad07&tochange=eb3016abd37db2e6a6d923265047e84b12c0af61
repo_name: mozilla-central
repo_url: https://hg.mozilla.org/mozilla-central

mozregression tried to bisect further, but logged a lot of "WARNING : Skipping build" and apologized.

Comment 4

[Daniel Veditz [:dveditz]]

Is it possible SVG internally is synthesizing the individual attributes into a "style=''" string attribute when it's appended to the document? And then at that point CSP might interpret it as an inline string rather than an allowed script manipulation of individual attributes?

dholbert or jwatt: any idea why the testcase SVG and svgFixed behave differently depending on whether the attributes are changed before the element is appended to the document or after?

Comment 5

[Daniel Holbert [:dholbert]]

Thanks for tracking down a mozregression range.  Presumably the relevant changes in that pushlog are the ones from bug 663570 ("Implement Content Security Policy via <meta> tag"), which is where we first started parsing the CSP in your testcase.

I suspect we probably had the same bug before, though it was a little harder to trigger because you had to use HTTP headers rather than an inline <meta> tag to set the CSP.

> dholbert or jwatt: any idea why the testcase SVG and svgFixed behave differently
> depending on whether the attributes are changed before the element is appended to the document or after?

Not offhand, (no. Nor am I sure why it works for HTML but doesn't for SVG.)

Comment 6

[Daniel Holbert [:dholbert]]

I poked around in gdb for a bit, and found:


- The inline style attribute gets rejected (and treated as a CSP violation) *when the svg element is appended*, with this backtrace:

#0  0x00007fcd002ad88c in nsCSPContext::AsyncReportViolation(...)
    at /scratch/work/builds/mozilla-inbound/mozilla/dom/security/nsCSPContext.cpp:1358
#1  0x00007fcd002af768 in nsCSPContext::reportInlineViolation(...)
    at /scratch/work/builds/mozilla-inbound/mozilla/dom/security/nsCSPContext.cpp:436
#2  0x00007fcd002afc63 in nsCSPContext::GetAllowsInline(...)
    at /scratch/work/builds/mozilla-inbound/mozilla/dom/security/nsCSPContext.cpp:512
#3  0x00007fcd00de3f34 in nsStyleUtil::CSPAllowsInlineStyle(...)
    at /scratch/work/builds/mozilla-inbound/mozilla/layout/style/nsStyleUtil.cpp:486
#4  0x00007fccfe2b12d8 in nsStyledElement::ParseStyleAttribute(...)
    at /scratch/work/builds/mozilla-inbound/mozilla/dom/base/nsStyledElement.cpp:211
#5  0x00007fcd003b2cba in nsSVGElement::BindToTree(...)
    at /scratch/work/builds/mozilla-inbound/mozilla/dom/svg/nsSVGElement.cpp:260

...whereas for the div, we instead call nsGenericHTMLElement::BindToTree -- which does not call ParseStyleAttribute() etc.

So it seems like we're taking a different codepath to parse/handle the style attribute when appending a SVG node vs. an HTML node -- and for the SVG case, we honor the style-src CSP rule (and block the inline style attribute) whereas we don't for the HTML case.

We probably want to make those codepaths/not-honoring consistent.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (simplified a bit)

Here's a slightly clearer testcase.

Comment 8

[Daniel Holbert [:dholbert]]

It looks like for the <div> case, we call "ReparseStyleAttribute" rather than "ParseStyleAttribute":


#0  0x00007fccfe2b1a43 in nsStyledElement::ReparseStyleAttribute(bool, bool)
    (this=0x7fccf3b680d0, aForceInDataDoc=false, aForceIfAlreadyParsed=false)
    at /scratch/work/builds/mozilla-inbound/mozilla/dom/base/nsStyledElement.cpp:164
#1  0x00007fccfe0911a9 in mozilla::dom::Element::BindToTree



And we call that on each property setting operation, too.  This actually happens for SVG elements, as well, but I think it gets stomped on by the ParseStyleAttribute call noted above.

It looks like we're calling ParseStyleAttribute for some special XBL handling stuff -- it looks like this whole chunk of nsSVGElement::BindToTree is intended to be removed ("...this can all go away"):

>  if (oldVal && oldVal->Type() == nsAttrValue::eCSSDeclaration) {
>    // we need to force a reparse because the baseURI of the document
>    // may have changed, and in particular because we may be clones of
>    // XBL anonymous content now being bound to the document we should
>    // render in and due to the hacky way in which we implement the
>    // interaction of XBL and SVG resources.  Once we have a sane
>    // ownerDocument on XBL anonymous content, this can all go away.
>    nsAttrValue attrValue;
>    nsAutoString stringValue;
>    oldVal->ToString(stringValue);
>    // Force in data doc, since we already have a style rule
>    ParseStyleAttribute(stringValue, nullptr, attrValue, true);
>    // Don't bother going through SetInlineStyleDeclaration; we don't
>    // want to fire off mutation events or document notifications anyway
>    bool oldValueSet;
>    rv = mAttrs.SetAndSwapAttr(nsGkAtoms::style, attrValue, &oldValueSet);
>    NS_ENSURE_SUCCESS(rv, rv);
>  }

It looks like this reparsing was added in bug 387466 (back in 2010), and the testcase involves XBL. So I suspect the removal depends on us deprecating XBL.

Comment 9

[Jonathan Watt [:jwatt]]

One question to answer before anything else is whether attributes that are mapped into style should be blocked when the 'style' attribute is blocked. In HTML it seems to make sense that they should be (but neither Chrome nor Firefox do), but in SVG that would likely break most SVGs. Then again, if we're not going to block SVG mapped attributes, is there much to gain from blocking the 'style' attribute?

I'd like to understand the threat model that blocking the 'style' attribute is aimed at a bit better. In bug 763879 comment 67 from 2013 I found: "Selectors, the 'phone home' attack which should be mitigated by the other *-src CSP directives) and 'defacement' are the ones that have been discussed in the WebAppSec WG."

At any rate, I've sent an email to the WG: https://www.w3.org/mid/[e-mail]

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> It looks like this reparsing was added in bug 387466 (back in 2010), and the
> testcase involves XBL. So I suspect the removal depends on us deprecating
> XBL.

We could also check whether any of our browser code is actually using XBL with SVG. If it is not, then we could just break XBL-in-SVG now without waiting for its full removal.

Comment 11

[Daniel Holbert [:dholbert]]

Just to clarify -- this bug isn't using mapped attributes at all [not in HTML nor in SVG].  Instead, it's using the elem.style.[propertyName] JavaScript API, and that *does work* in all browsers even with style-src:none.  It's only broken in one special-case shown in the testcase here.

It seems that "style-src:none" (or anything other than 'unsafe-inline', really) prevents the inline "style" attribute from being read & honored.  But I guess elem.style.[propertyName] mutations -- from CSP-allowed JS -- are assumed to be trusted and are allowed to bypass this layer of blocking, I guess.

This inconsistency definitely seems odd. I don't immediately see justification for it in the spec, but I imagine it must be there somewhere since it's (basically) interoperable.  The explanation at e.g. https://stackoverflow.com/a/46105241 makes some sense, but it still seems like a strange special case.

Comment 12

[Daniel Holbert [:dholbert]]

hmm, I guess the spec-justification for this behavior is:

> The style-src directive governs several things:
> ...
> 4. The following CSS algorithms are gated on the unsafe-eval source expression:
>   1.    insert a CSS rule
>   2.    parse a CSS rule,
>   3.    parse a CSS declaration block
>   4.    parse a group of selectors
https://w3c.github.io/webappsec-csp/#directive-style-src

To honor a style="..." attribute, or to call setAttribute("style", "..."), we need to parse a CSS Declaration Block (which is what the "..." represents here).  And per this spec text, that is under the purview of CSP.

Whereas |elem.style.[propertyName] = "someValue"| technically doesn't have any declaration-block-parsing -- it's just parsing a CSS Value.

This subtlety seems a bit strange, but it's somewhat concrete, at least. I imagine this explains why SVG mapped attributes are expected to work -- they don't require parsing a declaration block. Just an attribute name and the value (which happens to be a CSS Value).  (Again, mapped attributes aren't actually involved in this bug -- just mentioning them to [maybe] close the loop on comment 9.)

Comment 13

[Daniel Veditz [:dveditz]]

We still have XBL reftests that include SVG but as we're removing XBL in Firefox I no longer see any actual XBL widgets with SVG. Do we still need this special case in the code, or do we need to wait until XBL is 100% gone?

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Daniel Veditz [:dveditz] from comment #13)
> as we're removing XBL in Firefox I no longer see any actual XBL widgets with SVG.

Great!

> Do we still need
> this special case in the code, or do we need to wait until XBL is 100% gone?

I think XBL being gone from SVG content is sufficient, per comment 10.  Though we'd probably want to be sure we assert that we're not getting XBL-in-SVG, to be sure [and to detect any sneaky cases that we might've missed through inspection].

Comment 15

[Daniel Veditz [:dveditz]]

jkt is going to throw in an assert and see if a try run comes up clean, and then try to remove this code if it does.

Comment 16

[Jonathan Kingston [:jkt] he/him]

I added in debug asserts for when we use XBL to bind anything of an SVG namespace and removed the restyling code also. This should hopefully point us at what is broken by removing this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f405229552555f76d2eedd30761bb2892eb4d3d4


This is somewhat related to Bug 903372 given that xml:base is probably a big reason why we needed to calculate these. So it might be that becomes a blocker as a few tests are breaking there.

Comment 17

[Jonathan Kingston [:jkt] he/him]

The comparison of layout/reftests/svg/moz-only/xbl-basic-01.svg is the reftest that we will need to remove to get this to work. I'm not sure if we have others too. I saw some crash tests too that likely will crash again with this assert. I suspect we can rip those out too.

Comment 18

[Jonathan Kingston [:jkt] he/him]

I'm pretty sure the only issue is the datetimebox.xml using svg: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/datetimebox.xml

This is why there was some wpt tests failing on try, I commented out the binding and this was fixed.

I'm adding Bug 1496242 as a dependency for that reason, it looks like it's almost done :).

Comment 19

[Jonathan Kingston [:jkt] he/him]

:dholbert will there be an issue if "xbl is loading svg" rather than the previously discussed "xbl in svg"?

Removing the code itself only breaks a reftest that is for this code: https://treeherder.mozilla.org/testview.html?repo=try&revision=899832a94291254593a1d45e5030eac7a6d27194

The only thing I see this might break is datetimebox.xml as per Comment 18. I tested this manually and it appears to be fine given that it's not really related to what this code is doing.

Also it seems that background-image svg's are fine where as this svg tag isn't we could easily move this to be a background image also I think.

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kingston [:jkt] from comment #19)
> :dholbert will there be an issue if "xbl is loading svg" rather than the
> previously discussed "xbl in svg"?
[...]
> 
> The only thing I see this might break is datetimebox.xml as per Comment 18.
> I tested this manually and it appears to be fine given that it's not really
> related to what this code is doing.

I don't know for sure, but I don't think there will be any issue.

I agree with your suspicion that XBL-causing-SVG-to-be-generated is entirely different from SVG-content-with-an-XBL-binding-applied-to-it. And breaking/unsupporting the latter doesn't seem like it should have any effect on the former.

Comment 21

[Jonathan Kingston [:jkt] he/him]

So even though Bug 903372 has closed we still need to fix the failing test (tests/layout/reftests/svg/use-localRef-marker-01.svg ).

Chatting with emilio on irc highlighted the problem here, we are cloning the the <use> element href svg into a new shadow root before the embedding svg is created.
This causes an issue because the idtracker code isn't at the correct location until the tree is binded: https://searchfox.org/mozilla-central/rev/47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/dom/base/IDTracker.cpp#23 The current code reparses the style on rebind which we are trying to remove. The base location is calculated here: https://searchfox.org/mozilla-central/rev/47edbd91c43db6229cf32d1fc4bae9b325b9e2d0/dom/base/FragmentOrElement.cpp#336 It's only correct on re-parse.
Chrome doesn't support the remote document defs here and so renders something quite different anyway.

---

GetMarkerURI -> ResolveURLUsingLocalRef -> GetBaseURLForLocalRef is the call path and it's being called with the document svg for aDocURI.

Simpler test case is this:

doc.svg
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <defs>
    <marker id="circleMarker" markerWidth="8" markerHeight="8" refX="5" refY="5">
      <circle cx="5" cy="5" r="2" style="stroke: none; fill: red;"/>
    </marker>
  </defs>
  <use xlink:href="embed.svg#path"/>
</svg>

embed.svg
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <defs>
    <marker id="circleMarker" markerWidth="8" markerHeight="8" refX="5" refY="5">
      <circle cx="5" cy="5" r="2" style="stroke: none; fill: lime;"/>
    </marker>
  </defs>
  <path id="path" d="M20,20 L70,20 L120,20" style="stroke: blue; stroke-width: 2px; marker-start: url(#circleMarker);"/>
</svg>

My view is that #circleMarker should be lime and never should be trying to reference the red marker in doc.svg

In SVGObserverUtils (https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/layout/svg/SVGObserverUtils.cpp#1548,1567) 1548 resolves to embed.svg and aDocURI is doc.svg#circleMarker

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1494356 - Remove SVG style attribute reparsing code. r=longsonr,dholbert

Now that XBL and xml:base are gone, and that <svg:use> is sane (using
shadow trees instead of NAC), we can just remove this, and fix this bug.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdff19ec13e2
Remove SVG style attribute reparsing code. r=longsonr

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/24387 for changes under testing/web-platform/tests

Comment 26

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 27

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fdff19ec13e2

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot