* 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

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!

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

Comment on attachment 699009 [details] [diff] [review] patch >+ // see as previous case s/as // ? r=me

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.

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

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

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

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 on attachment 699009 [details] [diff] [review] patch ok, I'll pull these requests given that sec-approval? is for sec-high and sec-critical

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

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

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 on attachment 700809 [details] [diff] [review] patch 2: make these errors not be exploitable [Approval Request Comment] same as previous patch

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.

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

oh, thanks for landing.