1333846 - Stack-buffer-overflow in mozilla::StyleAnimationValue::GetScaleValue

Status: VERIFIED FIXED

(Core :: DOM: Animation, defect)

Description

[Atte Kettunen]

Attachment: repro-file.html

Tested on:

OS: Ubuntu 16.04

Firefox: ASAN build from https://index.taskcluster.net/v1/task/gecko.v2.mozilla-central.latest.firefox.linux64-asan-opt/artifacts/public/build/target.tar.bz2 
sourcestamp: c989c7b352279925edf138373e4ca3f1540dbd5f

ASAN-trace:

==28958==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffc72fa1918 at pc 0x7fef22f240af bp 0x7ffc72fa14b0 sp 0x7ffc72fa14a8
READ of size 8 at 0x7ffc72fa1918 thread T0
    #0 0x7fef22f240ae in mozilla::StyleAnimationValue::GetScaleValue(nsIFrame const*) const /home/worker/workspace/build/src/layout/style/StyleAnimationValue.cpp:4828:33
    #1 0x7fef23b0c37a in ContainsAnimatedScale /home/worker/workspace/build/src/layout/painting/ActiveLayerTracker.cpp:491:24
    #2 0x7fef23b0c37a in mozilla::ActiveLayerTracker::IsScaleSubjectToAnimation(nsIFrame*) /home/worker/workspace/build/src/layout/painting/ActiveLayerTracker.cpp:518
    #3 0x7fef2336fd75 in GetTransformToAncestorExcludingAnimated /home/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:2676:7
    #4 0x7fef2336fd75 in nsLayoutUtils::GetTransformToAncestorScaleExcludingAnimated(nsIFrame*) /home/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:2695
    #5 0x7fef235ef332 in nsImageFrame::MaybeDecodeForPredictedSize() /home/worker/workspace/build/src/layout/generic/nsImageFrame.cpp:729:7
    #6 0x7fef235ee283 in nsImageFrame::OnSizeAvailable(imgIRequest*, imgIContainer*) /home/worker/workspace/build/src/layout/generic/nsImageFrame.cpp:587:7
    #7 0x7fef235ed952 in nsImageFrame::Notify(imgIRequest*, int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/worker/workspace/build/src/layout/generic/nsImageFrame.cpp:503:12
    #8 0x7fef1f0462b0 in nsImageLoadingContent::Notify(imgIRequest*, int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/worker/workspace/build/src/dom/base/nsImageLoadingContent.cpp:157:9
    #9 0x7fef1ed586be in imgRequestProxy::Notify(int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/worker/workspace/build/src/image/imgRequestProxy.cpp:808:3
    #10 0x7fef1ed103db in operator() /home/worker/workspace/build/src/image/ProgressTracker.cpp:320:42
    #11 0x7fef1ed103db in void mozilla::image::ImageObserverNotifier<mozilla::image::ObserverTable const*>::operator()<void mozilla::image::SyncNotifyInternal<mozilla::image::ObserverTable const*>(mozilla::image::ObserverTable const* const&, bool, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)::{lambda(mozilla::image::IProgressObserver*)#1}>(void mozilla::image::SyncNotifyInternal<mozilla::image::ObserverTable const*>(mozilla::image::ObserverTable const* const&, bool, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)::{lambda(mozilla::image::IProgressObserver*)#1}) /home/worker/workspace/build/src/image/ProgressTracker.cpp:279
    #12 0x7fef1ed10020 in void mozilla::image::SyncNotifyInternal<mozilla::image::ObserverTable const*>(mozilla::image::ObserverTable const* const&, bool, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/image/ProgressTracker.cpp:320:5
    #13 0x7fef1ed0ba93 in operator() /home/worker/workspace/build/src/image/ProgressTracker.cpp:399:5
    #14 0x7fef1ed0ba93 in Read<(lambda at /home/worker/workspace/build/src/image/ProgressTracker.cpp:398:19)> /home/worker/workspace/build/src/image/CopyOnWrite.h:154
    #15 0x7fef1ed0ba93 in mozilla::image::ProgressTracker::SyncNotifyProgress(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/image/ProgressTracker.cpp:398
    #16 0x7fef1ed14fc7 in mozilla::image::RasterImage::NotifyProgress(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<unsigned int> const&, mozilla::image::DecoderFlags, mozilla::image::SurfaceFlags) /home/worker/workspace/build/src/image/RasterImage.cpp:1597:3
    #17 0x7fef1ed1f888 in mozilla::image::RasterImage::NotifyDecodeComplete(mozilla::image::DecoderFinalStatus const&, mozilla::image::ImageMetadata const&, mozilla::image::DecoderTelemetry const&, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<unsigned int> const&, mozilla::image::DecoderFlags, mozilla::image::SurfaceFlags) /home/worker/workspace/build/src/image/RasterImage.cpp:1637:3
    #18 0x7fef1ed942d9 in operator() /home/worker/workspace/build/src/image/IDecodingTask.cpp:87:5
    #19 0x7fef1ed942d9 in mozilla::detail::RunnableFunction<mozilla::image::IDecodingTask::NotifyDecodeComplete(mozilla::NotNull<mozilla::image::RasterImage*>, mozilla::NotNull<mozilla::image::Decoder*>)::$_2>::Run() /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.h:314
    #20 0x7fef1c937e8b in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1261:7
    #21 0x7fef1c9347e0 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:394:10
    #22 0x7fef1d73eb0f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #23 0x7fef1d6b0d98 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3
    #24 0x7fef1d6b0d98 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231
    #25 0x7fef1d6b0d98 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211
    #26 0x7fef22b0707f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #27 0x7fef24b3e991 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #28 0x7fef24cf91ac in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4461:10
    #29 0x7fef24cfacf1 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4638:8
    #30 0x7fef24cfbfcc in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4729:16
    #31 0x4df705 in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:234:10
    #32 0x4df705 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:305
    #33 0x7fef37d1c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #34 0x41ba58 in _start (/home/attekett/Downloads/firefox/firefox+0x41ba58)

Address 0x7ffc72fa1918 is located in stack of thread T0 at offset 600 in frame
    #0 0x7fef23b0bb1f in mozilla::ActiveLayerTracker::IsScaleSubjectToAnimation(nsIFrame*) /home/worker/workspace/build/src/layout/painting/ActiveLayerTracker.cpp:508

  This frame has 5 object(s):
    [32, 80) ''
    [112, 160) ''
    [192, 256) '__begin.i'
    [288, 352) '__end.i'
    [384, 592) 'segment.i' <== Memory access at offset 600 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /home/worker/workspace/build/src/layout/style/StyleAnimationValue.cpp:4828:33 in mozilla::StyleAnimationValue::GetScaleValue(nsIFrame const*) const
Shadow bytes around the buggy address:
.
.
.

On debug-build:

[28901] ###!!! ASSERTION: uninitialized: 'mUnit != eUnit_Null', file /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/StyleAnimationValue.h, line 369
Assertion failure: GetUnit() == StyleAnimationValue::eUnit_Transform, at /home/worker/workspace/build/src/layout/style/StyleAnimationValue.cpp:4819

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Gosh! I missed we have code[1] that uses AnimationPropertySegment in ActiveLayerTracker.cpp.

[1] https://hg.mozilla.org/mozilla-central/file/d92fd6b6d6bf/layout/painting/ActiveLayerTracker.cpp#l479

We should move this kind of code into dom/animation/.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Note that current aurora (firefox 53) is affected by this issue, but will not be affected by default when it's released since the feature is disabled by a pref (dom.animations-api.core.enabled) on beta and release channels.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1333846 - Part 1: Move ContainsAnimatedScale() codes into dom/animation/

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1333846 - Part 2: Evaluate scale values for additive or accumulative animations

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1333846 - Part 3: Crashtest

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Brian, here is another patch set.  I did forget to set ni to you. ;-)

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8830962 [details] [diff] [review]
Bug 1333846 - Part 1: Move ContainsAnimatedScale() codes into dom/animation/

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

r=me with the following comments addressed.

::: dom/animation/AnimationUtils.cpp
@@ +88,5 @@
> +/* static */ bool
> +AnimationUtils::EffectSetContainsAnimatedScale(EffectSet& aEffects,
> +                                               nsIFrame* aFrame)
> +{
> +  for (dom::KeyframeEffectReadOnly* effect : aEffects) {

If ContainsAnimatedScale is a const method (see below) this could be a const pointer (and presumably |aEffects| could be const).

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +1771,5 @@
> +    if (prop.mProperty != eCSSProperty_transform) {
> +      continue;
> +    }
> +
> +    for (AnimationPropertySegment segment : prop.mSegments) {

Can we make this a const ref while we're at it? i.e. const AnimationPropertySegment&

::: dom/animation/KeyframeEffectReadOnly.h
@@ +354,5 @@
>    bool NeedsBaseStyle(nsCSSPropertyID aProperty) const;
>  
> +  // Returns true if the effect is current state and has scale animation.
> +  // |aFrame| is used for calculation of scale values.
> +  bool ContainsAnimatedScale(nsIFrame* aFrame);

Can't this be const?

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 8830963 [details] [diff] [review]
Bug 1333846 - Part 2: Evaluate scale values for additive or accumulative animations

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

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +1775,5 @@
> +    if (NeedsBaseStyle(prop.mProperty)) {
> +      StyleAnimationValue baseStyle =
> +        EffectCompositor::GetBaseStyle(prop.mProperty, aFrame);
> +      MOZ_DIAGNOSTIC_ASSERT(!baseStyle.IsNull(),
> +          "The base value should be set");

I'm having a hard time keeping up with all the changes around GetBaseStyle recently so my comment might not make much sense, but in my tree I see EffectCompositor::GetBaseStyle calling ResolveStyleByRemovingAnimation and then asserting that the result is not null.

If GetBaseStyle should never return null then it probably should just be named BaseStyle and we can drop the assertion here.

At the same time, I suspect we *don't* want to trigger style resolution in  KeyframeEffectReadOnly::ContainsAnimated and so baseStyle should be allowed to be null and we should fail gracefully when that is the case.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #8)
> Comment on attachment 8830962 [details] [diff] [review]
> Bug 1333846 - Part 1: Move ContainsAnimatedScale() codes into dom/animation/
> 
> Review of attachment 8830962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the following comments addressed.
> 
> ::: dom/animation/AnimationUtils.cpp
> @@ +88,5 @@
> > +/* static */ bool
> > +AnimationUtils::EffectSetContainsAnimatedScale(EffectSet& aEffects,
> > +                                               nsIFrame* aFrame)
> > +{
> > +  for (dom::KeyframeEffectReadOnly* effect : aEffects) {
> 
> If ContainsAnimatedScale is a const method (see below) this could be a const
> pointer (and presumably |aEffects| could be const).
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp
> @@ +1771,5 @@
> > +    if (prop.mProperty != eCSSProperty_transform) {
> > +      continue;
> > +    }
> > +
> > +    for (AnimationPropertySegment segment : prop.mSegments) {
> 
> Can we make this a const ref while we're at it? i.e. const
> AnimationPropertySegment&

Oho! Good catch! Thanks! I did not notice that the segment value is neither reference nor pointer!

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)
> Comment on attachment 8830963 [details] [diff] [review]
> Bug 1333846 - Part 2: Evaluate scale values for additive or accumulative
> animations
> 
> Review of attachment 8830963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp
> @@ +1775,5 @@
> > +    if (NeedsBaseStyle(prop.mProperty)) {
> > +      StyleAnimationValue baseStyle =
> > +        EffectCompositor::GetBaseStyle(prop.mProperty, aFrame);
> > +      MOZ_DIAGNOSTIC_ASSERT(!baseStyle.IsNull(),
> > +          "The base value should be set");
> 
> I'm having a hard time keeping up with all the changes around GetBaseStyle
> recently so my comment might not make much sense, but in my tree I see
> EffectCompositor::GetBaseStyle calling ResolveStyleByRemovingAnimation and
> then asserting that the result is not null.
> 
> If GetBaseStyle should never return null then it probably should just be
> named BaseStyle and we can drop the assertion here.

That's right. GetBaseStyle() for nsIFrame should never fail because nsIFrame should have a valid nsStyleContext, BaseStyle() sounds nice idea to me. I will do it in a later bug and will drop the assertion in that bug.

> At the same time, I suspect we *don't* want to trigger style resolution in 
> KeyframeEffectReadOnly::ContainsAnimated and so baseStyle should be allowed
> to be null and we should fail gracefully when that is the case.

Right. Once we have fixed bug 1331704 properly, we don't need to resolve style here, but I think we still need it.
After bug 1331704, we can change GetBaseStyle() for nsIFrame to just return the cached style value I think.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Move ContainsAnimatedScale() codes into dom/animation/

Comment 13

[Frederik Braun [:freddy]]

Hiro, are you still actively working on this? Seems like part 2 is missing a review request.
As a stack-buffer-overflow, this seems pretty dangerous and we want it fixed ASAP.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Evaluate scale values for additive or accumulative animations

Yes, I am still working on this.

Brian, would you mind looking this again?

I've left renaming/handling GetBaseStyle problems because it might change what we do in bug 1331704.

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8834613 [details] [diff] [review]
Part 2: Evaluate scale values for additive or accumulative animations

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

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +1775,5 @@
> +    if (NeedsBaseStyle(prop.mProperty)) {
> +      StyleAnimationValue baseStyle =
> +        EffectCompositor::GetBaseStyle(prop.mProperty, aFrame);
> +      MOZ_DIAGNOSTIC_ASSERT(!baseStyle.IsNull(),
> +        "The base value should be set");

Given that we have an open bug where the base style is not set when we expect it to, do we really want to crash Nightly and DevEdition for this? Would MOZ_ASSERT be enough here combined with an early return if baseStyle is not set?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #15)
> Comment on attachment 8834613 [details] [diff] [review]
> Part 2: Evaluate scale values for additive or accumulative animations
> 
> Review of attachment 8834613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp
> @@ +1775,5 @@
> > +    if (NeedsBaseStyle(prop.mProperty)) {
> > +      StyleAnimationValue baseStyle =
> > +        EffectCompositor::GetBaseStyle(prop.mProperty, aFrame);
> > +      MOZ_DIAGNOSTIC_ASSERT(!baseStyle.IsNull(),
> > +        "The base value should be set");
> 
> Given that we have an open bug where the base style is not set when we
> expect it to, do we really want to crash Nightly and DevEdition for this?
> Would MOZ_ASSERT be enough here combined with an early return if baseStyle
> is not set?

Ah indeed.  I was misunderstanding this value will be passed to the compositor. Thanks!

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Evaluate scale values for additive or accumulative animations

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8834743 [details] [diff] [review]
Part 2: Evaluate scale values for additive or accumulative animations

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
As far as I can tell it's really hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
Aurora (Firefox 53) but the feature which causes this stack-over-flow is behind the pref that is off by default on beta and release channels.

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?
These patch can be applied to aurora cleanly.

How likely is this patch to cause regressions; how much testing does it need?
Not likely.  This patch essentially does the same thing of null check.  I run locally all of tests in dom/animation/tests, layout/style/crashtests, layout/style/test_animationsXX, layoute/style/test_transitionXX and the test in part 3 patch.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Note that part 1 patch is a prerequisite patch for part 2.  I will land part 3 after this bug is in public.
Thank you!

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8834743 [details] [diff] [review]
Part 2: Evaluate scale values for additive or accumulative animations

sec-approval+ for trunk. We'll want an Aurora patch nominated for this. You could make an argument for Beta as well, even though it is pref'd off by default.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e52024becabfb29cb2893d88fa54a3fce743f50a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3525deba71e9461fddeb887038c2df4d9ed409d4

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8834743 [details] [diff] [review]
Part 2: Evaluate scale values for additive or accumulative animations

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1305325.
[User impact if declined]: stack buffer over flow.
[Is this code covered by automated tests?]: Several tests I wrote in comment 18 cover this, and part 3 patch also does.
[Has the fix been verified in Nightly?]: Not yet, just landed on inbound.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Part 1 patch in this bug (attachment 8832813 [details] [diff] [review])
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]: It just does null check and if it's the case of null, the function returns a safer value to avoid mistaking that the transform has no scale value at all. 
[String changes made/needed]: None

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment 18 was somewhat misleading.  I meant the feature (bug 1305325) will be disabled when the current aurora (firefox 53) becomes beta and release.  Neither current beta (firefox 52) nor release (firefox 51) is affected by this.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e52024becabf
https://hg.mozilla.org/mozilla-central/rev/3525deba71e9

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8834743 [details] [diff] [review]
Part 2: Evaluate scale values for additive or accumulative animations

Fix a sec-high. Aurora53+.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e77ccea90900
https://hg.mozilla.org/releases/mozilla-aurora/rev/a6a8ca19a011

Comment 27

[Paul Silaghi, QA [:pauly]]

Reproduced the assertion on nightly 2017-01-25 asan-debug build, Ubuntu 14.04 x46.
Verified fixed Fx 53b11, 54.0a2(2017-04-11) asan-debug builds.