Closed Bug 827687 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 --- fixed
firefox20 --- fixed
firefox21 + fixed
firefox-esr10 --- wontfix
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: jruderman, Assigned: dbaron)

References

Details

(4 keywords, Whiteboard: [asan][adv-main19+])

Attachments

(4 files, 1 obsolete file)

Attached file 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
Group: core-security
Attached file 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.
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!
Attached patch patchSplinter Review
Attachment #699009 - Flags: review?(bzbarsky)
(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.
(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.)
What rating would you give this?
It's reading past the end of an array and doing floating-point comparisons on the memory it finds there.  What rating is that?
OS: Mac OS X → All
Hardware: x86_64 → All
Comment on attachment 699009 [details] [diff] [review]
patch

>+      // see as previous case

s/as // ?

r=me
Attachment #699009 - Flags: review?(bzbarsky) → review+
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"?
I hate C.
Attachment #700125 - Attachment is obsolete: true
Attachment #700125 - Flags: review?(bzbarsky)
Attachment #700809 - Flags: review?(bzbarsky)
Comment on attachment 700809 [details] [diff] [review]
patch 2: make these errors not be exploitable

r=me
Attachment #700809 - Flags: review?(bzbarsky) → review+
(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 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)
Attachment #699009 - Flags: sec-approval?
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
Attachment #700809 - Flags: sec-approval?
I'd say call it sec-moderate. "doesn't look sec-critical, but tricky enough we can't say for sure it isn't"
Summary: Heap buffer overflow [@ ElementAnimations::EnsureStyleRuleFor] with CSS animation → Out of bounds read [@ ElementAnimations::EnsureStyleRuleFor] with CSS animation
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
Attachment #699009 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/52e73ebc07e4
https://hg.mozilla.org/mozilla-central/rev/d53d71d1c92e
Assignee: nobody → dbaron
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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
Attachment #699009 - Flags: approval-mozilla-beta?
Attachment #699009 - Flags: approval-mozilla-aurora?
Comment on attachment 700809 [details] [diff] [review]
patch 2: make these errors not be exploitable

[Approval Request Comment]
same as previous patch
Attachment #700809 - Flags: approval-mozilla-beta?
Attachment #700809 - Flags: approval-mozilla-aurora?
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.
Attachment #699009 - Flags: approval-mozilla-beta?
Attachment #699009 - Flags: approval-mozilla-beta+
Attachment #699009 - Flags: approval-mozilla-aurora?
Attachment #699009 - Flags: approval-mozilla-aurora+
Attachment #700809 - Flags: approval-mozilla-beta?
Attachment #700809 - Flags: approval-mozilla-beta+
Attachment #700809 - Flags: approval-mozilla-aurora?
Attachment #700809 - Flags: approval-mozilla-aurora+
oh, thanks for landing.
Whiteboard: [asan] → [asan][adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.