827687 - Out of bounds read [@ ElementAnimations::EnsureStyleRuleFor] with CSS animation

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase (may crash)

* In a non-debug ASan build, this appears as a heap buffer overflow 
[@ ElementAnimations::EnsureStyleRuleFor] (same signature as fixed bug 765218).

* In a non-ASan build, this crashes infrequently.
  * But adding onload-reload to the testcase makes it reliable enough:
    * bp-3a3f345f-bed8-46b1-a008-ee44f2130108
    * bp-cda868fb-6a82-4e77-86c9-3944a2130108

* In a debug build, the crash is masked by the abort in bug 662724:

###!!! ABORT: position should be in [0-1]: '0.0 <= positionInIteration && positionInIteration <= 1.0', file layout/style/nsAnimationManager.cpp, line 235

Comment 1

[Jesse Ruderman]

Attachment: stacks

It might be possible to get a better stack by disabling the assertion in a debug build.  Or compiling a non-debug build without optimizations.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

ElementAnimations::GetPositionInIteration has:

  uint32_t whichIteration = int(currentIterationCount);
  if (whichIteration == aIterationCount && whichIteration != 0) {
    // When the animation's iteration count is an integer (as it
    // normally is), we need to end at 100% of its last iteration
    // rather than 0% of the next one (unless it's zero).
    --whichIteration;
  }
  double positionInIteration =
    currentIterationCount - double(whichIteration);

which doesn't work when currentIterationCount is out of uint32_t or int range (never mind uint32_t vs. int, oops).




Also, while I'm here... we should really be using the length of the animation-name list in BuildAnimations, yikes!

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] from comment #2)
> Also, while I'm here... we should really be using the length of the
> animation-name list in BuildAnimations, yikes!

Filed this as bug 827698, which will coincidentally make this testcase no longer show the bug, even without the patch here.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(So, for the record (and for when we add the testcase after opening this bug), the assertion was happening at (0-based) index 4, though I don't see any reason it wouldn't have also happened at index 1.)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=e63a1700f149

Comment 7

[Andrew McCreight [:mccr8]]

What rating would you give this?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: make these errors not be exploitable

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's reading past the end of an array and doing floating-point comparisons on the memory it finds there.  What rating is that?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 699009 [details] [diff] [review]
patch

>+      // see as previous case

s/as // ?

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 700125 [details] [diff] [review]
patch 2: make these errors not be exploitable

I don't see how the "continue" helps.  Shouldn't it be "break"?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: make these errors not be exploitable

I hate C.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 700809 [details] [diff] [review]
patch 2: make these errors not be exploitable

r=me

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] from comment #9)
> It's reading past the end of an array and doing floating-point comparisons
> on the memory it finds there.  What rating is that?

It actually could do a little more than that, since it could get into the following pieces of code with memory (i.e., the segment variable) from off the end of the array:

  ComputedTimingFunction::GetValue, via this code:

        double valuePosition =
          segment->mTimingFunction.GetValue(positionInSegment);

  nsStyleAnimation::AddWeighted, via this code:

          nsStyleAnimation::Interpolate(prop.mProperty,
                                        segment->mFromValue, segment->mToValue,
                                        valuePosition, *val);

I don't see anything in either of these functions that's going to write to any of these bad pointers or going to call virtual functions through them, but the latter is a pretty big pile of code to audit to prove that it doesn't do anything bad.

So I haven't proven that this isn't sec-critical, although I'm also far from showing that it is.  (I'm not sure it's worth trying either.)

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 699009 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I'm not sure.  Given the pair of patches, it's relatively easy to tell what the problem is; probaly not that hard from either one alone.

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

No more than they have to.

Which older supported branches are affected by this flaw?

All supported ones.

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

N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Easy to create, and low risk.

How likely is this patch to cause regressions; how much testing does it need?

pretty low risk (patch 2 is even lower risk)

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 700809 [details] [diff] [review]
patch 2: make these errors not be exploitable

[Security approval request comment]
same as previous comment, with the note that this patch is probably even lower risk

Comment 17

[Daniel Veditz [:dveditz]]

I'd say call it sec-moderate. "doesn't look sec-critical, but tricky enough we can't say for sure it isn't"

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 699009 [details] [diff] [review]
patch

ok, I'll pull these requests given that sec-approval? is for sec-high and sec-critical

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/52e73ebc07e4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d53d71d1c92e

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/52e73ebc07e4
https://hg.mozilla.org/mozilla-central/rev/d53d71d1c92e

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 699009 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 435442 (implement css3-animations)
User impact if declined: crash, possibly with security implications
Testing completed (on m-c, etc.): on mozilla-central
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 700809 [details] [diff] [review]
patch 2: make these errors not be exploitable

[Approval Request Comment]
same as previous patch

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 699009 [details] [diff] [review]
patch

Given where we are in the cycle, let's land this low risk sg:moderate fix on pre-release branches.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9cc5d8098b
https://hg.mozilla.org/releases/mozilla-aurora/rev/ea3782d77355

https://hg.mozilla.org/releases/mozilla-beta/rev/328090eddb58
https://hg.mozilla.org/releases/mozilla-beta/rev/27549a1311a8

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

oh, thanks for landing.