The current nightly build of firefox crashs with the attached testcase. ASAN: ==7339==ERROR: AddressSanitizer: SEGV on unknown address 0x00009fdf8004 (pc 0x7f64e9cc1700 bp 0x7ffcd1419ab0 sp 0x7ffcd14199a0 T0) ==7339==The signal is caused by a READ memory access. #0 0x7f64e9cc16ff in isNothing /obj-firefox/dist/include/mozilla/Maybe.h:225:36 #1 0x7f64e9cc16ff in mozilla::css::URLValueData::IsLocalRef() const /layout/style/nsCSSValue.cpp:2961 #2 0x7f64ea54332d in ResolveURLUsingLocalRef(nsIFrame*, mozilla::css::URLValueData const*) /layout/svg/SVGObserverUtils.cpp:1011:14 #3 0x7f64ea53fc12 in GetPaintURI /layout/svg/SVGObserverUtils.cpp:1067:10 #4 0x7f64ea53fc12 in SVGObserverUtils::GetPaintServer(...) /layout/svg/SVGObserverUtils.cpp:652 ...

Marking security-sensitive. CJ, is this something you can look at?

Assertion failure: MOZ_ASSERT((svgStyle->*aPaint).Type() == nsStyleSVGPaintType::eStyleSVGPaintType_Server); (svgStyle->*aPaint).Type() returns eStyleSVGPaintType_Color actually.

(In reply to C.J. Ku[:cjku](UTC+8) from comment #3) > Assertion failure: > MOZ_ASSERT((svgStyle->*aPaint).Type() == > nsStyleSVGPaintType::eStyleSVGPaintType_Server); > > (svgStyle->*aPaint).Type() returns eStyleSVGPaintType_Color actually. And crash in the next line in non-debug build

This test https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGObserverUtils.cpp#634 needs to come after https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGObserverUtils.cpp#642 and be on frame rather than targetFrame.

Comment on attachment 8912116 [details] [diff] [review] 1402798 - Part 2. Add a crash test for applying fill:url() property to an SVG text element. should land this later given that this is a security bug.

This grafts cleanly to Beta and ESR52. Please request approval when you get a chance. Probably should have had sec-approval before landing too since it affects more than trunk and doesn't currently have a rating.

Let's hope this isn't sec-high because we've now 0 dayed ourselves.

Hi Ken, Astley, could you please promptly get this patch landed on Beta and ESR52? Dan, Al, fyi

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11) > This grafts cleanly to Beta and ESR52. Please request approval when you get > a chance. Probably should have had sec-approval before landing too since it > affects more than trunk and doesn't currently have a rating. Yes, this shouldn't have landed yet and without sec-approval and a security rating. Is this something we need to point release over? I need someone to set sec-approval? on the patch and answer the template questions that are asked.

This seems bad, and I'd be willing to believe it's exploitable. Basically, in affected builds, we're abusing a union{nscolor, URLValue*} and picking the wrong thing out of it: https://dxr.mozilla.org/mozilla-central/rev/76a26ef7c493311c170ae83eb0c1d6592a21396d/layout/style/nsStyleStruct.h#3435-3438 In reality, it's storing a nscolor (per comment 3), but we're pulling out the URLValue* and using it. So, we're calling methods on a pointer-value, with the pointer value being entirely controllable by the author (who can presumably set whatever nscolor they like and influence the pointer value thusly). Specifically, we call these methods on the bogus pointer-value: - aURL->IsLocalRef() - aURL->GetURI() - aURL->ResolveLocalRef(baseURI) (here, aURL is the bogus URLValue* pointer) https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGObserverUtils.cpp#1002-1020 None of those calls are virtual functions, so none of those will directly lead to running arbitrary code from a bogus vtable pointer. But I wouldn't be surprised if there are second-order side effects from whatever bogus URI that we might end up returning (which could itself be a bogus pointer and which could have virtual functions called on it). So I think we should consider this sec-critical.

(In reply to Al Billings [:abillings] from comment #15) > Is this something we need to point release over? I would lean towards probably-yes. > I need someone to set sec-approval? on the patch and answer the template > questions that are asked. I'll do that.

(In reply to Ritu Kothari (:ritu) from comment #14) > Hi Ken, Astley, could you please promptly get this patch landed on Beta and > ESR52? Dan, Al, fyi Can we not? mozilla-central is a fairly large haystack. When we land on Beta and ESR 52 we instantly raise the profile to "not only is this bug hidden, they think it's important". Let's gather the sec-approval information first, and then figure out the best timing for uplift. Which might end up being "promptly" anyway, but it'll be a better informed decision.

Comment on attachment 8912115 [details] [diff] [review] 1402798 - Part 1. Check the type of an SVGPaint object from the correct frame. [Security approval request comment] How easily could an exploit be constructed based on the patch? * Probably medium difficulty, as far as exploits go. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? * Sort of. The commit message gives a hint that we're looking up the value on the wrong frame, and that the frame we're using now has the wrong type. With a bit of poking around (to figure out what thing has the wrong type), I'll bet an attacker could figure out what the problem is. Which older supported branches are affected by this flaw? All supported branches. If not all supported branches, which bug introduced the flaw? I think this started being a problem as of Bug 652991's patches -- specifically, this is the commit that switched us from using a nsStyleSVGPaint whose type we had checked, to one whose type we hadn't necessarily checked: https://hg.mozilla.org/mozilla-central/rev/685de09e5481#l6.35 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Per comment 11, it sounds like backports are easy, and I don't think they're risky. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions. It looks to me like it only changes behavior in cases where we'd otherwise trigger undefined behavior, so it should make us strictly safer.

OK, let me know if we need to act for 56 and how fast. I will be offline this afternoon and will check back tonight.

After today, I'll be on PTO for a week so I expect Dan is stuck on point for this one.

Given Daniel's information this does likely need to land promptly on beta. Does it need to land on previous releases? The patch applies cleanly which would imply we had the same problem, but the crashtest doesn't crash in Fx55 or Fx56. I only see the crash in Fx57 bp-dc4aa72f-7eb4-4893-930b-a041e0170928 Of course running an older ASAN version might show the problem is still there, but something else made this more obvious.

Happily, I don't see any crash or ASAN errors when loading the testcase in the latest ASAN build from https://treeherder.mozilla.org/#/jobs?repo=mozilla-release (I tried both debug & opt ASAN builds from there) I'm using mozregression to see when this started crashing nightly builds, to see if that blames a more recent commit whose contents clue us into how & why we're safe on 56 (hopefully).

OK -- the testcase *only* seems to crash/assert in stylo-enabled configurations. That's the most proximal reason why 56 is OK. And I think the *fact* that it crashes for stylo is a bug. Technically, we shouldn't have needed this patch, I think (though it is good as "belt and suspenders"). In particular: if I turn off stylo, then I'm finding it to be impossible to generate a testcase where |aTargetFrame| and |frame| differ at all on their "fill" value. (In this case, aTargetFrame would be a nsTextFrame or a nsFirstLetterFrame, and |frame| is the closest <svg:text> ancestor.) They always agree on "fill" (it seems) because... (a) they inherit it from their ancestor. (b) even if I try to override it (e.g. by adding "fill:url(#foo)" or "fill:green" in a text::first-letter selector), it has no effect -- our style context still contains whatever property was inherited from the <svg:text> ancestor. This is because ::first-letter is specced as only accepting a certain whitelist of properties, and "fill" is not on the whitelist: https://www.w3.org/TR/CSS22/selector.html#first-letter So: bottom-line, the old code *should* have been fine, and the fact that it's *not* fine with stylo enabled means there's a stylo bug here where we're doing inheritance wrong into a first-letter-frame, or something like that. (heycam and/or CJ, please sanity-check my thinking here; you've each looked at this SVG text stuff more than I have, I think.) So, I think this means we don't need to treat this as severe enough to uplift beyond any stylo-enabled configurations. We can, but it shouldn't be necessary from a sec perspective.

(And in particular, 57 is the first release where we're [tentatively] enabling stylo, so we should uplift this there but don't necessarily need to worry beyond that.)

I filed bug 1404167 for the stylo behavior-difference that I noted in comment 24. (I included a debugging patch on that new bug -- the patch adds an assertion that should not fail, but does currently fail in stylo-enabled configurations on trunk.) For now, I think we can call 56 and 52 unaffected (though again, heycam, please check my logic on this). And we might still want to take an uplift to 52 as part of its regular release cycle, if it's easy, as a belt-and-suspenders thing (since the patch applies, even though it shouldn't technically change behavior).

(In reply to Daniel Holbert [:dholbert] from comment #24) > So: bottom-line, the old code *should* have been fine Yes, I think you are right. We would need the <text> element to have a color-valued fill (or stroke) property, and its child ::-moz-svg-text or grandchild ::first-letter (or ::first-line) to have a url(). There is no way to way to set a fill or stroke on the ::first-letter or ::first-line, and there is no way to address the ::-moz-svg-text anonymous box either.

Comment on attachment 8912115 [details] [diff] [review] 1402798 - Part 1. Check the type of an SVGPaint object from the correct frame. [Security approval request comment] How easily could an exploit be constructed based on the patch? * Probably medium difficulty, as far as exploits go. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? * Sort of. The commit message gives a hint that we're looking up the value on the wrong frame, and that the frame we're using now has the wrong type. With a bit of poking around (to figure out what thing has the wrong type), I'll bet an attacker could figure out what the problem is. Which older supported branches are affected by this flaw? All the branches that enable stylo(FF 57 and FF 58). If not all supported branches, which bug introduced the flaw? There is a bug on ::first-letter restyle in stylo(bug 1404167), so we hit this crash only after enabling stylo as the backend. As a result, I think we can see bug 1330412 as start point of this bug. The patch in bug 1404167 fix this crash from the upstream, while this patch tries to prevent the crash even if we get a wrong style value. At least one of these two fixes need to be uplifted to 57 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Per comment 11, it sounds like backports are easy, and I don't think they're risky. How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions. It looks to me like it only changes behavior in cases where we'd otherwise trigger undefined behavior, so it should make us strictly safer.

(Sorry, I could've sworn I requested sec-approval up above in comment 19. I or bugzilla must've stomped on the flag before the comment got committed I guess.)

Comment on attachment 8912115 [details] [diff] [review] 1402798 - Part 1. Check the type of an SVGPaint object from the correct frame. sec-approval+ for trunk. Please nominate a patch for beta (57) so this goes out fixed in that release as well.

Comment on attachment 8912115 [details] [diff] [review] 1402798 - Part 1. Check the type of an SVGPaint object from the correct frame. Approval Request Comment [Feature/Bug causing the regression]: Bug 1330412 (enable stylo) [User impact if declined]: AS brwoser may crash with a comnination of an svg text element and a ::first-letter in it. [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: this patch (part 1) and part 2 (a crash test case) [Is the change risky?]: no [Why is the change risky/not risky?]: minor code change, has a test case. [String changes made/needed]: N/A

Comment on attachment 8912115 [details] [diff] [review] 1402798 - Part 1. Check the type of an SVGPaint object from the correct frame. Fix a sec critical and there is a chance that we ship with stylo, taking it. Should be in 57b5

https://hg.mozilla.org/releases/mozilla-beta/rev/eef9559e379b https://hg.mozilla.org/releases/mozilla-beta/rev/1ce3b4548c14

(In reply to C.J. Ku[:cjku](UTC+8) from comment #32) > [Is this code covered by automated tests?]: yes > [Has the fix been verified in Nightly?]: yes > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on C.J. Ku's assessment on manual testing needs and the fact that this fix has automated coverage.