Holger Fuhrmannek Reporter
	Description • 7 years ago

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

	Cameron McCormack (:heycam)
	Comment 2 • 7 years ago

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

	u459114 Assignee
	Comment 3 • 7 years ago

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

(svgStyle->*aPaint).Type() returns eStyleSVGPaintType_Color actually.

	u459114 Assignee
	Comment 4 • 7 years ago

(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

	Robert Longson [:longsonr]
	Comment 5 • 7 years ago

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.

	Robert Longson [:longsonr]
	Comment 9 • 7 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/19efc28f755b

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 7 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/0b2552d18368

	Robert Longson [:longsonr]
	Comment 13 • 7 years ago

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

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 14 • 7 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 15 • 7 years ago

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

	Daniel Holbert [:dholbert]
	Comment 16 • 7 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 17 • 7 years ago

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

	Daniel Veditz [:dveditz]
	Comment 18 • 7 years ago

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

	Daniel Holbert [:dholbert]
	Comment 19 • 7 years ago

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.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 7 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 21 • 7 years ago

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

	Daniel Veditz [:dveditz]
	Comment 22 • 7 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 23 • 7 years ago

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

	Daniel Holbert [:dholbert]
	Comment 24 • 7 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 25 • 7 years ago

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

	Daniel Holbert [:dholbert]
	Comment 26 • 7 years ago

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

	Cameron McCormack (:heycam)
	Comment 27 • 7 years ago

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

	u459114 Assignee
	Comment 28 • 7 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 29 • 7 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 30 • 7 years ago

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.

	u459114 Assignee
	Comment 32 • 7 years ago

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

	Sylvestre Ledru [:Sylvestre]
	Comment 33 • 7 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 7 years ago

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

	Andrei Vaida [:avaida]
	Comment 35 • 7 years ago

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