Closed Bug 1325193 Opened 3 years ago Closed 3 years ago

AddressSanitizer: heap-use-after-free in [@ Length] with READ of size 8

Categories

(Core :: DOM: Animation, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed

People

(Reporter: truber, Assigned: hiro)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [post-critsmash-triage] )

Attachments

(8 files, 2 obsolete files)

Attached file testcase.html
The attached testcase causes a heap uaf in mozilla-central rev 10a5e2fc24e4.

==21851==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060006603b0 at pc 0x7fae8a3b554f bp 0x7ffc258e3230 sp 0x7ffc258e3228
READ of size 8 at 0x6060006603b0 thread T0
    #0 0x7fae8a3b554e in Length /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:515:37
    #1 0x7fae8a3b554e in LastElement /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:1207
    #2 0x7fae8a3b554e in mozilla::dom::KeyframeEffectReadOnly::ComposeStyle(RefPtr<mozilla::AnimValuesStyleRule>&, nsCSSPropertyIDSet const&) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:522
    #3 0x7fae8a3b3757 in mozilla::dom::Animation::ComposeStyle(RefPtr<mozilla::AnimValuesStyleRule>&, nsCSSPropertyIDSet const&) /home/worker/workspace/build/src/dom/animation/Animation.cpp:981:7
    #4 0x7fae8a3cc77d in mozilla::EffectCompositor::ComposeAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, mozilla::TimeStamp) /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:700:5
    #5 0x7fae8a3cbd45 in mozilla::EffectCompositor::MaybeUpdateAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, nsStyleContext*) /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:370:3
    #6 0x7fae8a3ccd2e in mozilla::EffectCompositor::GetAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, nsStyleContext*) /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:406:3
    #7 0x7fae8e65c538 in nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, mozilla::CSSPseudoElementType, mozilla::dom::Element*, unsigned int) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:979:18
    #8 0x7fae8e660a53 in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1392:10
    #9 0x7fae8e801617 in ResolveStyleFor /home/worker/workspace/build/src/layout/style/nsStyleSet.h:137:12
    #10 0x7fae8e801617 in ResolveStyleFor /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/StyleSetHandleInlines.h:96
    #11 0x7fae8e801617 in nsCSSFrameConstructor::ResolveStyleContext(nsStyleContext*, nsIContent*, nsFrameConstructorState*) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5048
    #12 0x7fae8e7fe980 in ResolveStyleContext /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5017:10
Attached file log.txt
Attached file log_SEGV.txt
Alternate stack
Group: core-security → dom-core-security
Attached file log_uaf_read4.txt
Alternate stack
birtles, could you look into this?
Flags: needinfo?(bbirtles)
Hiro is more likely to know what is going on here.
Flags: needinfo?(bbirtles) → needinfo?(hiikezoe)
In this failure case, lastSegment.mToValue [1] is a null StyleAnimationValue because the to value of the last segment is a missing keyframe.  So, we need to use the underlying style value there for such cases.

https://hg.mozilla.org/mozilla-central/file/10a5e2fc24e4/dom/animation/KeyframeEffectReadOnly.cpp#l527
Flags: needinfo?(hiikezoe)
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Keywords: sec-high
Attached file log_uap_read8.txt
Another variation. Let me know if testcases for these other similar crashes would help.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> In this failure case, lastSegment.mToValue [1] is a null StyleAnimationValue
> because the to value of the last segment is a missing keyframe.  So, we need
> to use the underlying style value there for such cases.

Apart from this, the reason which causes this heap-use-after-free is that mProperties is destroyed in KeyframeEffectReadOnly::UpdateProperties() because GetTargetStyleContext()[1] ends up calling EffectCompositor::UpdateEffectProperties() and unfortunately at the time mProperties is not equal to the old one since there are an animation of font-size and an animation of perspective specified in *'ex'*.

So, we need to avoid this type of nested calls of UpdateEffectProperties() somehow.

https://hg.mozilla.org/mozilla-central/file/5ea0c495d3b2/dom/animation/KeyframeEffectReadOnly.cpp#l379
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > In this failure case, lastSegment.mToValue [1] is a null StyleAnimationValue
> > because the to value of the last segment is a missing keyframe.  So, we need
> > to use the underlying style value there for such cases.
> 
> Apart from this, the reason which causes this heap-use-after-free is that
> mProperties is destroyed in KeyframeEffectReadOnly::UpdateProperties()
> because GetTargetStyleContext()[1] ends up calling
> EffectCompositor::UpdateEffectProperties() and unfortunately at the time
> mProperties is not equal to the old one since there are an animation of
> font-size and an animation of perspective specified in *'ex'*.
> 
> So, we need to avoid this type of nested calls of UpdateEffectProperties()
> somehow.

To avoid this, we need a new method that resolves style context without solving animating properties (bug 1324966 especially bug 1324966 comment 2), but it will be a tough job.  So I would like to add a check of mIsComposing in KeyframeEffectReadOnly::UpdateProperties() in this bug just like we do in KeyframeEffectReadOnly::ComposeStyle() [1].

[1] https://hg.mozilla.org/mozilla-central/file/a2741dd43eea/dom/animation/KeyframeEffectReadOnly.cpp#l442

Brian, what do you think?
Flags: needinfo?(bbirtles)
Seems ok as a temporary workaround provided we add a comment to that effect.
Flags: needinfo?(bbirtles)
Attached patch A crashtest (obsolete) — Splinter Review
I couldn't find a way to replace setTimeout with other stuff.

Also, on ASAN build, this heap-use-after-free does not happen if the timeout value is smaller one, e.g 100ms. I can see the heap-use-after-free reliably if it's 800ms on my local machine.

On debug build, an assertion [1] in StyleAnimationValue::GetUnit() is hit regardless of timeout.

[1] https://hg.mozilla.org/mozilla-central/file/0f9d69253ac6/layout/style/StyleAnimationValue.h#l368
Attachment #8825191 - Flags: review?(bbirtles)
Attachment #8825191 - Attachment is patch: true
Attachment #8825182 - Flags: review?(bbirtles) → review+
For the crashtest, 1s is pretty long. I know you can reproduce reliably locally with a timeout of 800ms, but how about on automation. Do we reproduce reasonably reliably on automation with a timeout of 500ms?
Comment on attachment 8825191 [details] [diff] [review]
A crashtest

(In reply to Brian Birtles (:birtles) from comment #13)
> For the crashtest, 1s is pretty long. I know you can reproduce reliably
> locally with a timeout of 800ms, but how about on automation. Do we
> reproduce reasonably reliably on automation with a timeout of 500ms?

I am not sure.  I will try it later once the patch landed.
Attachment #8825191 - Flags: review?(bbirtles)
https://hg.mozilla.org/mozilla-central/rev/77638ee6f84d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8825182 [details] [diff] [review]
Stop processing KeyframeEffectReadOnly::UpdateProperties while composing the same effect's style

I did forget to request sec-approval, and I chatted with abillings on IRC, he suggested to request sec-approval now.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easy.  It needs a font-size animation, other animation properties specified in font-size(e.g. em) and a missing keyframe.  

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, I don't think so.

Which older supported branches are affected by this flaw?
53.

If not all supported branches, which bug introduced the flaw?
Bug 1305325.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't think this needs to be backported.

How likely is this patch to cause regressions; how much testing does it need?
Very low. This patch just avoids a nested calls of a function.
Attachment #8825182 - Flags: sec-approval?
sec-approval generally isn't needed if only trunk is affected. Also, you should go ahead and land the test in that case.
Comment on attachment 8825182 [details] [diff] [review]
Stop processing KeyframeEffectReadOnly::UpdateProperties while composing the same effect's style

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> sec-approval generally isn't needed if only trunk is affected. Also, you
> should go ahead and land the test in that case.

Wow, thanks.  That's good to know.
Attachment #8825182 - Flags: sec-approval?
Ah, Hiro contacted me and I didn't know it was Trunk only. Yeah, if it only affects Trunk, you're good to land whenever.
Attached patch A crashtestSplinter Review
I did two tries with 500ms timeout:

debug build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3566fb5b0fb1101a4d4bcee4c4d737551faece00
opt build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42a46226db9be905258d3c4e9d635f9d3dcea4b3

The test cases on debug build crashed on all platform because of an assertion at StyleAnimationValue::GetUnit().

The test cases on opt build also crashed on most of platforms (an exception is Windows).  On linux32/linux64 it passes on non task cluster but failed on task cluster.  So I think 500ms timeout is enough value for this test case.

Also, this patch moves setTimeout call after creation of animation. I think it's safer way rather than before the creation.

Brian, what do you think?
Attachment #8825191 - Attachment is obsolete: true
Attachment #8825593 - Flags: review?(bbirtles)
BTW, I retriggered the opt crashtest runs that had initially succeeded in your "opt build" Try run there.  With any luck, this test may still fail some fraction of the time on those platforms. :) (in unpatched builds)  The retriggers will hopefully tell.

Anyway, for timing-dependent tests like this: if the crashtest fails reasonably frequently in unpatched builds (frequently enough that we'd notice if this regressed), on multiple platforms, then IMO it's still good.
(In reply to Daniel Holbert [:dholbert] from comment #22)
> BTW, I retriggered the opt crashtest runs that had initially succeeded in
> your "opt build" Try run there.  With any luck, this test may still fail
> some fraction of the time on those platforms. :) (in unpatched builds)  The
> retriggers will hopefully tell.

Oh! Daniel, you always appear suddenly out of nowhere with helpful advises! Thanks for the tip!
Attachment #8825593 - Attachment is patch: true
Attachment #8825593 - Flags: review?(bbirtles) → review+
Thanks philor.  Leaving ni? to me.
Flags: needinfo?(hikezoe)
I did forget to fix the case of comment 6.   I had a patch for that, but I've lost it somewhere..
Flags: needinfo?(hikezoe)
Comment on attachment 8825676 [details] [diff] [review]
Get underlying style value in the case where the last segment is missing keyframe for accumulation of iteration composite

Clearing review request since a test fails on opt build.
Attachment #8825676 - Flags: review?(bbirtles)
I did forget to enter test refresh mode in the test.
Attachment #8825676 - Attachment is obsolete: true
Attachment #8825698 - Flags: review?(bbirtles)
Attachment #8825698 - Attachment is patch: true
Comment on attachment 8825698 [details] [diff] [review]
Get underlying style value in the case where the last segment is missing keyframe for accumulation of iteration composite

Review of attachment 8825698 [details] [diff] [review]:
-----------------------------------------------------------------

Great work, thanks for doing this.
Attachment #8825698 - Flags: review?(bbirtles) → review+
Group: dom-core-security → core-security-release
Blocks: 1334591
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.