<a class="header-button" href="https://bugzilla-dev.allizom.org/home" title="Go to home page"> Bugzilla

Comment 1

•

9 years ago

I'm debugging a couple of other security bugs in this area at the moment so I should probably look at this too.

Assignee: nobody → bbirtles

Status: NEW → ASSIGNED

Comment 2

•

9 years ago

It's going to be next week before I get to look at this properly.

Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)

Updated

•

9 years ago

Group: core-security → dom-core-security

Comment 3

•

9 years ago

Thanks Brian, when you get a chance can you also let us know what the regression may be from (and which versions are affected) Tracking for 49 since it's sec-critical.

tracking-firefox49: --- → +

Flags: needinfo?(bbirtles)

Comment 4

•

9 years ago

Mantaroh ran this test case on a debug build and found out we were failing this assertion: Assertion failure: !mIsObservingRefreshDriver (Timeline should have disassociated from the refresh driver before being destroyed), at /builds/slave/m-in-l64-asan-d-00000000000000/build/src/obj-firefox/dist/include/mozilla/dom/DocumentTimeline.h:42 Looking at the stack from comment 0, it seems most likely we're failing to disassociate the timeline from the refresh driver when it is destroyed. As a result, on the next tick, when we iterate through the refresh driver observers we access freed memory here: https://dxr.mozilla.org/mozilla-central/rev/f8e3b81a79f45ef8647c98281a9a00d1ddb28b73/layout/base/nsRefreshDriver.cpp#1458 If that's the case, this could be a long-standing regression. We did some work to fix timeline lifecycles at some point, but I'm not sure if that caused this or not. Mantaroh is going to try and create a mochitest for this so we can find a regression range for the assertion failure.

Assignee

Comment 5

•

9 years ago

Attached file testcase.html (deleted) — Details

We can reproduce all platform using this crashtest. The regression result is as follow. Regression: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=eaf90fc6c08264b8fd26125f62178dee4cf2ce43&tochange=f2abcc29e65aa7a8fc3e0f48069a3491cd46a1a6 I think that cause of this problem is patch of part 7. As discussed with Brian, we should have refcounts to DocumentTimeline in Animation object. And, perhaps we should remove observer when releasing the relationship of animation and timeline.

Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)

Comment 6

•

9 years ago

Based on the regression range in comment 5, this regression appeared in Firefox 49.

status-firefox47: --- → unaffected

status-firefox48: --- → unaffected

status-firefox50: --- → affected

Flags: needinfo?(bbirtles)

Updated

•

9 years ago

tracking-firefox50: --- → +

Comment 7

•

8 years ago

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #5) > Created attachment 8761927 [details] > testcase.html > > We can reproduce all platform using this crashtest. > The regression result is as follow. > > Regression: > https://hg.mozilla.org/integration/mozilla-inbound/ > pushloghtml?fromchange=eaf90fc6c08264b8fd26125f62178dee4cf2ce43&tochange=f2ab > cc29e65aa7a8fc3e0f48069a3491cd46a1a6 > > I think that cause of this problem is patch of part 7. > > As discussed with Brian, we should have refcounts to DocumentTimeline in > Animation object. And, perhaps we should remove observer when releasing the > relationship of animation and timeline. Looking at this more closely, it turns out we already do keep an owning reference from Animation to its AnimationTimeline. So perhaps the problem is simply that the last thing holding a reference to the timeline is the Animation, and when we call SetTimeline and release that reference, the timeline gets destroyed without unregistering from the refresh driver. I also notice that we're using the timeline as the Animation's parent object which doesn't seem right. We probably should update that to store an nsIDocument* (and check that makes sense in light of bug 1277456).

Assignee

Comment 8

•

8 years ago

Attached patch Bug 1277272 - Remove RefreshObserver when removing animation from timeline. (obsolete) (deleted) — Details — Splinter Review

Thanks brian. I fixed the DocumentTimeline. And I added crashtest into dom/animation/test/crashtest. Could you please reiview this patch?

Assignee: bbirtles → mantaroh

Attachment #8763050 - Flags: review?(bbirtles)

Comment 9

•

8 years ago

Comment on attachment 8763050 [details] [diff] [review] Bug 1277272 - Remove RefreshObserver when removing animation from timeline. Review of attachment 8763050 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/animation/DocumentTimeline.cpp @@ +185,5 @@ > +DocumentTimeline::RemoveAnimation(Animation* aAnimation) > +{ > + AnimationTimeline::RemoveAnimation(aAnimation); > + > + if (GetRefreshDriver() && mAnimations.IsEmpty()) { We should check if mIsObservingRefreshDriver is true first. e.g. if (mIsObservingRefreshDriver && mAnimations.IsEmpty()) { nsRefreshDriver* refreshDriver = GetRefreshDriver(); if (refreshDriver) { ... } } ::: dom/animation/DocumentTimeline.h @@ +70,5 @@ > > void NotifyRefreshDriverCreated(nsRefreshDriver* aDriver); > void NotifyRefreshDriverDestroying(nsRefreshDriver* aDriver); > > + void RemoveAnimation(Animation* aAnimation) override; This should go after NotifyAnimationUpdated so that it is together with the other AnimationTimeline overrides. ::: dom/animation/test/crashtests/1277272.html @@ +1,1 @@ > +<html> Let's split this test into a separate patch we can land after the fix (so that no one can exploit this in the time between landing this patch and it reaching the branches / Nightly build).

Attachment #8763050 - Flags: review?(bbirtles)

Assignee

Comment 10

•

8 years ago

Attached patch Bug 1277272 - Remove RefreshObserver when removing animation from timeline. (obsolete) (deleted) — Details — Splinter Review

Attachment #8763050 - Attachment is obsolete: true

Attachment #8763078 - Flags: review?(bbirtles)

Assignee

Comment 11

•

8 years ago

Attached patch Bug 1277272 - Add crashtest into dom/animation/test. (obsolete) (deleted) — Details — Splinter Review

Thanks Brian. I fixed previous patch, and separated test. Could you please review these patches again?

Attachment #8763080 - Flags: review?(bbirtles)

Comment 12

•

8 years ago

Comment on attachment 8763078 [details] [diff] [review] Bug 1277272 - Remove RefreshObserver when removing animation from timeline. Review of attachment 8763078 [details] [diff] [review]: ----------------------------------------------------------------- r=me with changes made. ::: dom/animation/DocumentTimeline.cpp @@ +188,5 @@ > + > + if (mIsObservingRefreshDriver && mAnimations.IsEmpty()) { > + nsRefreshDriver* refreshDriver = GetRefreshDriver(); > + if (refreshDriver) { > + GetRefreshDriver()->RemoveRefreshObserver(this, Flush_Style); This should just use |refreshDriver|. @@ +189,5 @@ > + if (mIsObservingRefreshDriver && mAnimations.IsEmpty()) { > + nsRefreshDriver* refreshDriver = GetRefreshDriver(); > + if (refreshDriver) { > + GetRefreshDriver()->RemoveRefreshObserver(this, Flush_Style); > + mIsObservingRefreshDriver = false; On further thought, we should probably never have a situation where mIsObservingRefreshDriver and true and GetRefreshDriver() == nullptr. So let's just make this: if (mIsObservingRefreshDriver && mAnimations.IsEmpty()) { MOZ_ASSERT(GetRefreshDriver(), "GetRefreshDriver() should not be null when" " mIsObservingRefreshDriver is true"); GetRefreshDriver()->RemoveRefreshObserver ... ... }

Attachment #8763078 - Flags: review?(bbirtles) → review+

Comment 13

•

8 years ago

Comment on attachment 8763080 [details] [diff] [review] Bug 1277272 - Add crashtest into dom/animation/test. Review of attachment 8763080 [details] [diff] [review]: ----------------------------------------------------------------- Can't we simplify this test further? ::: dom/animation/test/crashtests/crashtests.list @@ +5,5 @@ > pref(dom.animations-api.core.enabled,true) load 1216842-3.html > pref(dom.animations-api.core.enabled,true) load 1216842-4.html > pref(dom.animations-api.core.enabled,true) load 1216842-5.html > pref(dom.animations-api.core.enabled,true) load 1216842-6.html > +pref(dom.animations-api.core.enabled,true) load 1277272.html This should be 1277272-1.html. We name all tests like this so if we need to add additional tests for the same bug we don't have to rename existing tests.

Attachment #8763080 - Flags: review?(bbirtles)

Assignee

Comment 14

•

8 years ago

Attached patch Bug 1277272 - Remove RefreshObserver when removing animation from timeline. (deleted) — Details — Splinter Review

fixed comment #12.

Attachment #8763078 - Attachment is obsolete: true

Attachment #8763162 - Flags: review+

Assignee

Comment 15

•

8 years ago

Attached patch Bug 1277272 - Add crashtest into dom/animation/test. (obsolete) (deleted) — Details — Splinter Review

Thanks Brian. I misled the specification of crashtest. We should reload testcase page in order to reproduce crash. So I implemented crashtest as iframe. Could you please confirm this patch?

Attachment #8763080 - Attachment is obsolete: true

Attachment #8763164 - Flags: review?(bbirtles)

Comment 16

•

8 years ago

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15) > I misled the specification of crashtest. > We should reload testcase page in order to reproduce crash. So I implemented > crashtest as iframe. So, do you mean that otherwise it doesn't reproduce reliably?

Comment 17

•

8 years ago

Comment on attachment 8763164 [details] [diff] [review] Bug 1277272 - Add crashtest into dom/animation/test. Review of attachment 8763164 [details] [diff] [review]: ----------------------------------------------------------------- r=me with changes made (but please don't land this or push it to try until we've landed the fix in part 1) ::: dom/animation/test/crashtests/1277272-1-inner.html @@ +6,5 @@ > + t.animation = document.body.animate([{marks: 'crop'},{marks: 'crop'}], 12); > + > + document.write('<html><body></body></html>'); > + setTimeout(play.bind(t), 4); > + setTimeout(setTimelineWithGC.bind(t), 5); Just curious, is there any reason we need to write the test like this? Could we just write: var animation = document.body.animate(...); document.write(...); setTimeout(function() { animation.play() }, 4); setTimeout(function() { animation.timeline = undefined; SpecialPowers.Cu.forceGC(); window.top.continueTest(); }, 5); ? ::: dom/animation/test/crashtests/1277272-1.html @@ +9,5 @@ > + document.documentElement.className = ""; > + return; > + } > + > + let frame = document.getElementById("frame"); (Nit: let just means you can re-assign frame. If you want to use ES6 syntax here, this should probably be const) @@ +15,5 @@ > +} > + > +function continueTest() { > + setTimeout(start.bind(window), 40); > +} Is the 40, here, special? Or is 1 enough? Or would requestAnimationFrame, even, work?

Attachment #8763164 - Flags: review?(bbirtles) → review+

Comment 18

•

8 years ago

Comment on attachment 8763162 [details] [diff] [review] Bug 1277272 - Remove RefreshObserver when removing animation from timeline. [Security approval request comment] > How easily could an exploit be constructed based on the patch? With some difficulty since this relies on the timing of GC. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, although it is a simple patch so it wouldn't be hard to reason about the issue. > Which older supported branches are affected by this flaw? Aurora (49) > If not all supported branches, which bug introduced the flaw? Bug 1178662 > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Not yet, but this patch probably applies cleanly to Aurora (currently confirming). If it doesn't it will be easy to create. > How likely is this patch to cause regressions; how much testing does it need? Average likelihood. I'm not aware of any particular risks. There is an automated test available we can land later once this has been applied to all affected branches.

Attachment #8763162 - Flags: sec-approval?

Comment 19

•

8 years ago

(In reply to Brian Birtles (:birtles) from comment #18) > > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? > Not yet, but this patch probably applies cleanly to Aurora (currently > confirming). If it doesn't it will be easy to create. Confirmed that this patch applies cleanly to aurora.

Al Billings [:abillings - ex-MoCo]

Assignee

Comment 20

•

8 years ago

Attached patch Bug 1277272 - Add crashtest into dom/animation/test. (deleted) — Details — Splinter Review

Thanks Brian. (In reply to Brian Birtles (:birtles) from comment #17) > r=me with changes made (but please don't land this or push it to try until > we've landed the fix in part 1) OK. I will not land until sec-approval+ and landing part 1 patch. > ::: dom/animation/test/crashtests/1277272-1.html > @@ +9,5 @@ > > + document.documentElement.className = ""; > > + return; > > + } > > + > > + let frame = document.getElementById("frame"); > > (Nit: let just means you can re-assign frame. If you want to use ES6 syntax > here, this should probably be const) I've fixed using var syntax for consistent with other crashtests. > @@ +15,5 @@ > > +} > > + > > +function continueTest() { > > + setTimeout(start.bind(window), 40); > > +} > > Is the 40, here, special? Or is 1 enough? Or would requestAnimationFrame, > even, work? No, We can use 1 ms. So I fixed using 1ms.

Attachment #8763164 - Attachment is obsolete: true

Attachment #8763257 - Flags: review+

Comment 21

•

8 years ago

Comment on attachment 8763162 [details] [diff] [review] Bug 1277272 - Remove RefreshObserver when removing animation from timeline. sec-approval+. Let's get this nominated for Aurora as well to get it fixed there.

Attachment #8763162 - Flags: sec-approval? → sec-approval+

Updated

•

8 years ago

Blocks: 1178662

Has Regression Range: --- → yes

Flags: sec-bounty?

Keywords: regression

Comment 22

•

8 years ago

Comment on attachment 8763162 [details] [diff] [review] Bug 1277272 - Remove RefreshObserver when removing animation from timeline. Approval Request Comment [Feature/regressing bug #]:　Bug 1178662 [User impact if declined]: Potentially exploitable heap-use-after-free vulnerability will remain [Describe test coverage new/current, TreeHerder]: Will land on trunk soon. Automated test case will be landed after part 1 has landed on both trunk and aurora. [Risks and why]: Only the usual risk that this introduces some other unforeseen regression. There is no particular reason to suspect regressions in this case except for the fact that, due to the sensitive nature of the bug, it has not been thoroughly exercised on try server. [String/UUID change made/needed]: None.

Attachment #8763162 - Flags: approval-mozilla-aurora?

https://hg.mozilla.org/integration/mozilla-inbound/rev/c62fb7ec8761

Assignee

Comment 23

•

8 years ago

Carsten Book [:Tomcat]

Comment 24

•

8 years ago

https://hg.mozilla.org/mozilla-central/rev/c62fb7ec8761

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox50: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla50

Updated

•

8 years ago

Flags: sec-bounty? → sec-bounty+

Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)

Updated

•

8 years ago

Group: dom-core-security → core-security-release

Comment 25

•

8 years ago

Comment on attachment 8763162 [details] [diff] [review] Bug 1277272 - Remove RefreshObserver when removing animation from timeline. Regression from 49, let's uplift so that we don't end up shiping this to release.

Attachment #8763162 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Carsten Book [:Tomcat]

Comment 26

•

8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/64c1a1a17a6db5e81b8be2605acbfc3dd2f1e101

status-firefox49: affected → fixed

Assignee

Comment 27

•

8 years ago

Sorry, I forgot adding leave-open flag. I landed the test code. https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f29b3fa5ab

Status: RESOLVED → REOPENED

Resolution: FIXED → ---

Carsten Book [:Tomcat]

Comment 28

•

8 years ago

https://hg.mozilla.org/mozilla-central/rev/b3f29b3fa5ab adding leave-open per comment #27 :)

Keywords: leave-open

Kyle Huey (Exited; not receiving bugmail, old account, do not use)

Updated

•

8 years ago

Group: core-security-release

status-firefox-esr45: --- → unaffected

Comment 29

•

8 years ago

So what's actually going on here? Why is this open before it's RESOLVED FIXED?

Group: core-security-release

Flags: needinfo?(dveditz)

David Baron :dbaron:

Comment 30

•

8 years ago

Was the intent here that this should have been resolved in comment 28, since the fix reached central in comment 24 and the test reached central in comment 28?

Flags: needinfo?(mantaroh)