Bugzilla

Assignee

Comment 2

•

8 years ago

It looks like we're destroying the timeline without unregistering from the refresh driver. Presumably this happens because we release the document created with Document.write (and we don't go through the usual presshell/refreshdriver destruction that we would for regular documents).

Assignee

Comment 3

•

8 years ago

I wonder if we fail this test here: http://searchfox.org/mozilla-central/rev/740bb4ed16d070cf5d466231b30e80b9204aec54/dom/base/nsDocument.cpp#2133

Or if we simply fail to call Reset at all.

Assignee

Comment 4

•

8 years ago

It's going to take a few days to triage and assign this properly (it always takes me a while to get an ASAN build going, especially with the fuzzing extension, but this time it appears I have to rebuild clang too). Anyway, it's at the top of my list.

Flags: needinfo?(bbirtles)

Andrew McCreight [:mccr8]

Assignee

Updated

•

8 years ago

Priority: -- → P1

Updated

•

8 years ago

Assignee: nobody → bbirtles

Comment 5

•

8 years ago

I have a handy ASAN build, so, I tried.

(In reply to Brian Birtles (:birtles) from comment #3)
> I wonder if we fail this test here:
> http://searchfox.org/mozilla-central/rev/
> 740bb4ed16d070cf5d466231b30e80b9204aec54/dom/base/nsDocument.cpp#2133
> 
> Or if we simply fail to call Reset at all.

Actually this works correctly, but after it,

        o856.currentTime = 183;

here, we re-observed the same refresh driver in DocumentTimeline::NotifyAnimationUpdated.

http://hg.mozilla.org/mozilla-central/file/0ba72e8027cf/dom/animation/DocumentTimeline.cpp#l130

Should we nullify all animations' mTimeline when nsDocument::Reset?

Comment 6

•

8 years ago

Attached patch Crash test based on attachment 8777297 — Details — Splinter Review

You can confirm the crash without fuzzing addon.

Assignee

Comment 7

•

8 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Created attachment 8777970 [details] [diff] [review]
> Crash test based on  attachment 8777297 [details]
> 
> You can confirm the crash without fuzzing addon.

Great! Thanks for that. Can you verify if this affects release channel too?

Flags: needinfo?(hiikezoe)

Updated

•

8 years ago

status-firefox48: --- → affected

status-firefox49: --- → affected

status-firefox50: --- → affected

status-firefox51: --- → affected

Flags: needinfo?(hiikezoe)

Assignee

Comment 8

•

8 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> I have a handy ASAN build, so, I tried.
> 
> (In reply to Brian Birtles (:birtles) from comment #3)
> > I wonder if we fail this test here:
> > http://searchfox.org/mozilla-central/rev/
> > 740bb4ed16d070cf5d466231b30e80b9204aec54/dom/base/nsDocument.cpp#2133
> > 
> > Or if we simply fail to call Reset at all.
> 
> Actually this works correctly, but after it,
> 
>         o856.currentTime = 183;
> 
> here, we re-observed the same refresh driver in
> DocumentTimeline::NotifyAnimationUpdated.

Oh, so the reset happens in f1 when we call document.write()? That makes sense.

> Should we nullify all animations' mTimeline when nsDocument::Reset?

I haven't looked into this properly, but I'm not sure if that works. I think we only store a reference to animations that are currently active (e.g. when NeedsTicks() returns true or some other changes has happened causing them to request a tick) so we couldn't get to them all by iterating over AnimationTimeline::mAnimations. If an animation was paused it might not be in the timeline's list of animations but if it was resumed it would end up calling NotifyAnimationUpdated(). Unless you mean to traverse the document and get all the animations that way?

I wonder if instead, after we get a call to NotifyRefreshDriverDestroying we could set a flag on the timeline. Then in NotifyAnimationUpdated() we could just skip re-adding to the refresh driver in that case. I'm not sure if we should clear that flag on a subsequent call to NotifyRefreshDriverCreated, but I suppose we should.

Would that work?

Comment 9

•

8 years ago

(In reply to Brian Birtles (:birtles) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > I have a handy ASAN build, so, I tried.
> > 
> > (In reply to Brian Birtles (:birtles) from comment #3)
> > > I wonder if we fail this test here:
> > > http://searchfox.org/mozilla-central/rev/
> > > 740bb4ed16d070cf5d466231b30e80b9204aec54/dom/base/nsDocument.cpp#2133
> > > 
> > > Or if we simply fail to call Reset at all.
> > 
> > Actually this works correctly, but after it,
> > 
> >         o856.currentTime = 183;
> > 
> > here, we re-observed the same refresh driver in
> > DocumentTimeline::NotifyAnimationUpdated.
> 
> Oh, so the reset happens in f1 when we call document.write()? That makes
> sense.
> 
> > Should we nullify all animations' mTimeline when nsDocument::Reset?
> 
> I haven't looked into this properly, but I'm not sure if that works. I think
> we only store a reference to animations that are currently active (e.g. when
> NeedsTicks() returns true or some other changes has happened causing them to
> request a tick) so we couldn't get to them all by iterating over
> AnimationTimeline::mAnimations. If an animation was paused it might not be
> in the timeline's list of animations but if it was resumed it would end up
> calling NotifyAnimationUpdated(). Unless you mean to traverse the document
> and get all the animations that way?

I meant to traverse all the animations.  I did't think it's a good idea though.

> I wonder if instead, after we get a call to NotifyRefreshDriverDestroying we
> could set a flag on the timeline. Then in NotifyAnimationUpdated() we could
> just skip re-adding to the refresh driver in that case. I'm not sure if we
> should clear that flag on a subsequent call to NotifyRefreshDriverCreated,
> but I suppose we should.

Yes, this should work.  A remaining concern is that when we can change the old timeline to a new one.  Is it unnecessary?  I don't remember we set timeline when the animation is associated with a document.

Updated

•

8 years ago

Group: core-security → dom-core-security

Assignee

Comment 10

•

8 years ago

Adding some debugging output to attachment 8777970 [details] [diff] [review] I see the following:

  REFTEST TEST-START | file:///home/brian/src/dom/animation/test/crashtests/1291665-1.html
  REFTEST TEST-LOAD | file:///home/brian/src/dom/animation/test/crashtests/1291665-1.html | 0 / 1 (0%)
  C++: >> DocumentTimeline(0x60e000064dc0): NotifyRefreshDriverCreated
          (refresh driver: 0x61800004b080)
  C++: << DocumentTimeline(0x60e000064dc0): NotifyRefreshDriverCreated

  JS: Calling animate

  C++: Animation constructed (0x6120000aa5c0)
  C++: >> DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
          (animation: 0x6120000aa5c0)
  C++:    Got refresh driver: 0x61800004b080
  C++:    Added ourselves as refresh driver observer
  C++: << DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
  C++: >> DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
          (animation: 0x6120000aa5c0)
  C++: << DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
  C++: >> DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
          (animation: 0x6120000aa5c0)
  C++: << DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated

  JS: Calling document.write

  C++: >> DocumentTimeline(0x60e000064dc0): NotifyRefreshDriverDestroying
          (refresh driver: 0x61800004b080)
  C++:    Unregistering as refresh observer
  C++: << DocumentTimeline(0x60e000064dc0): NotifyRefreshDriverDestroying

  JS: Calling setTimeout
  JS: Setting currentTime

  C++: >> DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated
          (animation: 0x6120000aa5c0)
  C++:    Got refresh driver: 0x61800004b080
  C++:    Added ourselves as refresh driver observer
  C++: << DocumentTimeline(0x60e000064dc0): NotifyAnimationUpdated

  JS: Nulling objects
  JS: Triggering GC
  JS: Calling location.reload

  C++: ~DocumentTimeline(0x60e000064dc0) [mIsObservingRefreshDriver: 1]
  C++: ~Animation(0x6120000aa5c0)
  =================================================================
  ==10913==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000064e38 at pc 0x7f965371dc02 bp 0x7ffc97611db0 sp 0x7ffc97611da8
  READ of size 8 at 0x60e000064e38 thread T0 (Web Content)
  REFTEST TEST-PASS | file:///home/brian/src/dom/animation/test/crashtests/1291665-1.html | (LOAD ONLY)
  REFTEST TEST-END | file:///home/brian/src/dom/animation/test/crashtests/1291665-1.html
      #0 0x7f965371dc01 in AddRef /home/brian/src/objdir-ff-asan/dist/include/mozilla/RefPtr.h:37:5
      #1 0x7f965371dc01 in AddRef /home/brian/src/objdir-ff-asan/dist/include/mozilla/RefPtr.h:384
      #2 0x7f965371dc01 in RefPtr /home/brian/src/objdir-ff-asan/dist/include/mozilla/RefPtr.h:111
      #3 0x7f965371dc01 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/brian/src/layout/base/nsRefreshDriver.cpp:1711
      #4 0x7f9653727aac in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/brian/src/layout/base/nsRefreshDriver.cpp:251:7
      #5 0x7f9653727779 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/brian/src/layout/base/nsRefreshDriver.cpp:270:5
      ...

So there appear to be a few problems here:

a. We can have a situation where we have a cycle between a DocumentTimeline and
   an Animation (i.e. where they are the only things keeping each other alive).
   When we break that cycle, there is nothing that causes the DocumentTimeline
   to be removed from its refresh driver. (The assertion in ~DocumentTimeline
   that checks mIsObservingRefreshDriver is false should fail in this case but
   this is an opt build so it doesn't show up above.)

b. Even after calling NotifyRefreshDriverDestroying() we can still end up
   calling GetRefreshDriver() from DocumentTimeline::NotifyAnimationUpdated and
   re-registering with the (supposedly) destroyed refresh driver.
   
   This is surprising, I would have thought that the pres context would not
   return a valid refresh driver after we call NotifyRefreshDriverDestroying()
   but I *think* what happens is that we don't actually destroy the refresh
   driver in nsDocument::Reset but we simply call that method as a means of
   getting the timeline to disassociate itself from the refresh driver before
   it is destroyed. The version history seems to show this too:

   https://hg.mozilla.org/mozilla-central/rev/84169b40100b4248f0cbdfbd0e42e27e358d5e80

So I think there are a few things we can do here:

1. Set a flag in DocumentTimeline::NotifyRefreshDriverDestroying to indicate
   "refresh driver has been destroyed" and, in
   DocumentTimeline::GetRefreshDriver(), return nullptr when that flag is set.
   In DocumentTimeline::NotifyRefreshDriverCreated we would clear the flag.

2. Make DocumentTimeline::Unlink unregister from the refresh driver.

3. Make Animation::Unlink call DocumentTimeline->RemoveAnimation.

4. In ~DocumentTimeline unregister from the refresh driver.

(1) alone would fix this bug, but I think we should probably do something
along the lines of (2) or (3) or both as well. We probably don't need (4) and
can just leave the assertion there---or should we convert it to a MOZ_CRASH?

For now I'll just do (1) and (2).

Assignee

Comment 11

•

8 years ago

(In reply to Brian Birtles (:birtles) from comment #10)
> (1) alone would fix this bug, but I think we should probably do something
> along the lines of (2) or (3) or both as well. We probably don't need (4) and
> can just leave the assertion there---or should we convert it to a MOZ_CRASH?
> 
> For now I'll just do (1) and (2).

On further thought, while (1) would fix *this* bug, we could have a very similar bug which would not be fixed by (1) if instead of using the default document timeline, we use a separately constructed timeline (i.e. using `new DocumentTimeline`) since we won't call NotifyRefreshDriverDestroying on that. So probably the minimum requirement for fixing this is (2). I'll make these up as separate patches and we can decide which to land where.

Status: NEW → ASSIGNED

Assignee

Comment 12

•

8 years ago

Attached patch Unregister from refresh driver in DocumentTimeline::Unlink — Details — Splinter Review

This is approach (2) from comment 10. I have verified that attachment 8777970 [details] [diff] [review] no longer crashes and ASAN build with this patch applied but I have not run it through try. This patch, I believe, also fixes the case when we have a timeline created with `new DocumentTimeline(...)`.

Attachment #8781392 - Flags: review?(hiikezoe)

Assignee

Updated

•

8 years ago

Attachment #8781392 - Attachment is patch: true

Assignee

Comment 13

•

8 years ago

Attached patch Ignore a refresh driver marked as destroyed in DocumentTimeline — Details — Splinter Review

This is approach (1) from comment 10. I think attachment 8781392 [details] [diff] [review] is enough and we don't need to land this patch to fix this bug.

However, I think it's a bug that we call NotifyRefreshDriverDestroyed and then sometimes re-register with it anyway.

We should either:

i.  Not call NotifyRefreshDriverDestroyed from nsDocument::Reset
ii. Land this patch sometime.

(i) seems simpler but I'm not sure if eagerly disassociating from the refresh driver might have other benefits such as releasing resources sooner and stopping the refresh driver from spinning. On the other hand, perhaps it could be argued that the default document should keep functioning even after the document is destroyed (like document timelines created using `new DocumentTimeline`).

So I'm not quite sure what to do here, but I'm posting this patch for now since we might use it later.

Attachment #8781395 - Flags: review?(hiikezoe)

Comment 14

•

8 years ago

Comment on attachment 8777970 [details] [diff] [review]
Crash test based on  attachment 8777297 [details]

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

It would be nice that we have a utility function which does;

  refreshDriver->RemoveRefreshObserver(this, Flush_Style);                            
  mIsObservingRefreshDriver = false;

and reuse the function in NotifyRefreshDriverDestroying() or rename NotifyRefreshDriverDestroying in a later bug?

Attachment #8777970 - Flags: review+

Comment 15

•

8 years ago

Comment on attachment 8781395 [details] [diff] [review]
Ignore a refresh driver marked as destroyed in DocumentTimeline

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

I am stamping this patch as r+ too.

(In reply to Brian Birtles (:birtles) from comment #13)
> However, I think it's a bug that we call NotifyRefreshDriverDestroyed and
> then sometimes re-register with it anyway.
> 
> We should either:
> 
> i.  Not call NotifyRefreshDriverDestroyed from nsDocument::Reset
> ii. Land this patch sometime.
> 
> (i) seems simpler but I'm not sure if eagerly disassociating from the
> refresh driver might have other benefits such as releasing resources sooner
> and stopping the refresh driver from spinning. On the other hand, perhaps it
> could be argued that the default document should keep functioning even after
> the document is destroyed (like document timelines created using `new
> DocumentTimeline`).
> 
> So I'm not quite sure what to do here, but I'm posting this patch for now
> since we might use it later.

+1 to land this later.

Attachment #8781395 - Flags: review?(hiikezoe) → review+

Comment 16

•

8 years ago

Comment on attachment 8781392 [details] [diff] [review]
Unregister from refresh driver in DocumentTimeline::Unlink

I am sorry, I accidentally set r+ against the test case.

Attachment #8781392 - Flags: review?(hiikezoe) → review+

Updated

•

8 years ago

Attachment #8777970 - Flags: review+

Assignee

Comment 17

•

8 years ago

Comment on attachment 8781392 [details] [diff] [review]
Unregister from refresh driver in DocumentTimeline::Unlink

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not too easily. It's clear the bug relates to failing to unregister from the refresh driver, but you have to be lucky with GC timing to trigger the bug (or find a way of forcing GC to happen at the right time--is there a way, other than using a chrome-privileges API to do that?).

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, however they make it clear that sometimes we fail to unregister from the refresh driver (which one could easily guess implies a UAF).

> Which older supported branches are affected by this flaw?
At least up to 48. I believe this was introduced in bug 1195180 which landed in 44, so I expect ESR45 is affected.

> If not all supported branches, which bug introduced the flaw?
(Bug 1195180)

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not yet but I can easily prepare them. They should not be very different, very difficult to create, or risky.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions. I have yet to run it through try for fear of exposing the vulnerability. Please advise how to proceed.

Attachment #8781392 - Flags: sec-approval?

Comment 18

•

8 years ago

sec-approval+ for trunk.
We'll want Aurora, Beta, and ESR45 patches made and nominated so they can land after this is good on trunk.

status-firefox48: affected → wontfix

status-firefox-esr45: --- → affected

tracking-firefox49: --- → +

tracking-firefox50: --- → +

tracking-firefox51: --- → +

tracking-firefox-esr45: --- → 49+

Updated

•

8 years ago

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

Assignee

Comment 19

•

8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f9686fc43a6e58d383ec1bdc10eae3072943c71

Assignee

Comment 20

•

8 years ago

Attached patch Backport of fix for ESR45 / Release (48); r=hiro — Details — Splinter Review

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
n/a (sec-high)
> User impact if declined: 
Potentially exploitable use-after-free
> Fix Landed on Version:
Has just landed on m-i (50) now.
> Risk to taking this patch (and alternatives if risky): 
Only the usual risk of unforeseen side effects although the changes are limited.
> String or UUID changes made by this patch: 
None

Approval Request Comment
[Feature/regressing bug #]: bug 1195180
[User impact if declined]: Potentially exploitable use-after-free
[Describe test coverage new/current, TreeHerder]: Just landed on m-i
[Risks and why]: Only the usual risk of unforeseen side effects (should be mitigated by watching results of landing on m-c).
[String/UUID change made/needed]: None

Attachment #8781814 - Flags: approval-mozilla-release?

Attachment #8781814 - Flags: approval-mozilla-esr45?

Assignee

Comment 21

•

8 years ago

Comment on attachment 8781392 [details] [diff] [review]
Unregister from refresh driver in DocumentTimeline::Unlink

Approval Request Comment
[Feature/regressing bug #]: bug 1195180
[User impact if declined]: Potentially exploitable use-after-free
[Describe test coverage new/current, TreeHerder]: Just landed on m-i
[Risks and why]: Only the usual risk of unforeseen side effects (should be mitigated by watching results of landing on m-c).
[String/UUID change made/needed]: None

Attachment #8781392 - Flags: approval-mozilla-beta?

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

Assignee

Comment 22

•

8 years ago

Comment on attachment 8781814 [details] [diff] [review]
Backport of fix for ESR45 / Release (48); r=hiro

Clearing release flag since I notice that in comment 18 this bug is marked as wontfix for 48.

Attachment #8781814 - Flags: approval-mozilla-release?

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Assignee

Updated

•

8 years ago

status-firefox-esr45: affected → checkin-pending

Comment 23

•

8 years ago

https://hg.mozilla.org/mozilla-central/rev/3f9686fc43a6

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox51: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla51

Updated

•

8 years ago

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

Updated

•

8 years ago

Attachment #8781392 - Flags: approval-mozilla-beta?

Attachment #8781392 - Flags: approval-mozilla-beta+

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

Attachment #8781392 - Flags: approval-mozilla-aurora+

Updated

•

8 years ago

Attachment #8781814 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+

Ryan VanderMeulen [:RyanVM]

Updated

•

8 years ago

Keywords: checkin-needed

Comment 24

•

8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/a55ede08a8ba
https://hg.mozilla.org/releases/mozilla-beta/rev/11354102d2d2

status-firefox49: affected → fixed

status-firefox50: affected → fixed

Flags: in-testsuite?

Keywords: checkin-needed

Ryan VanderMeulen [:RyanVM]

Comment 25

•

8 years ago

https://hg.mozilla.org/releases/mozilla-esr45/rev/7b668c5cec92

status-firefox-esr45: checkin-pending → fixed

Andrei Vaida [:avaida]

Updated

•

8 years ago

Flags: qe-verify+

Paul Silaghi, QA [:pauly]

Comment 26

•

8 years ago

Reproduced the crash on FX 48.0.2, Win 7.
Verified fixed FX 49.0, 50.0a2 (2016-09-06), 51.0a1 (2016-09-06), 45.4.0 ESR.

Status: RESOLVED → VERIFIED

status-firefox49: fixed → verified

status-firefox50: fixed → verified

status-firefox51: fixed → verified

status-firefox-esr45: fixed → verified

Updated

•

8 years ago

Whiteboard: [adv-main49+][adv-esr45.4+]

Updated

•

8 years ago

Alias: CVE-2016-5277

Updated

•

8 years ago

Flags: sec-bounty?

Updated

•

8 years ago

Flags: sec-bounty? → sec-bounty+