1361100 - Racy accesses to nsTimerImpl::mEventTarget in TimerThread::PostTimerEvent

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Byron Campen [:bwc]]

TimerThread::PostTimerEvent reads nsTimerImpl::mEventTarget without any locking. If another thread calls SetTarget, we can get undefined behavior.

Comment 1

[Byron Campen [:bwc]]

Bunches of crashes here: https://crash-stats.mozilla.com/signature/?signature=nsCOMPtr%3CT%3E%3A%3AnsCOMPtr%3CT%3E%20%7C%20TimerThread%3A%3APostTimerEvent&date=%3E%3D2017-04-24T16%3A26%3A00.000Z&date=%3C2017-05-01T16%3A26%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

See a bunch that look like UAF (which would be the result of a race where SetTarget resulted in the destruction of the event target that PostTimerEvent is using).

Comment 2

[Byron Campen [:bwc]]

I think this should close this race; right now we are clearing nsTimerImpl::mCallback before removing it from the TimerThread. This puts the timer in a state where SetTarget() will succeed, which means it can race against PostTimerEvent. If we remove the nsTimerImpl before clearing mCallback, that should prevent this race.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=318e345ccba91ba2cfe2f24965c5264132e4182e

I'm seeing some valgrind leaks though. Need to determine what that's about.

Comment 3

[Byron Campen [:bwc]]

Attachment: Perform all timer init after removal from TimerThread

.

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8864906 [details] [diff] [review]
Perform all timer init after removal from TimerThread

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

   It would be tricky, because the change is subtle. It might solve other problems than this particular bug, however, so maybe someone could come up with something we have not noticed.

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

   It hints at a race, so it might interest someone.

Which older supported branches are affected by this flaw?

   Probably all, in one form or another. Just squeezing another race out of the code.

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

   Backporting to 54 should be trivial, if that ends up being useful.

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

   Pretty unlikely; I'm not adding anything to an existing critical section, so there should be no additional risk of deadlock.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

This race causes a UAF, so bumping to sec-high

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8864906 [details] [diff] [review]
Perform all timer init after removal from TimerThread

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

One of those "oh, duh" bugs. =/

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 8864906 [details] [diff] [review]
Perform all timer init after removal from TimerThread

sec-approval=dveditz

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8864906 [details] [diff] [review]
Perform all timer init after removal from TimerThread

Approval Request Comment
[Feature/Bug causing the regression]:

   Well, this has never really been safe.

[User impact if declined]:

   Potential exploitable UAFs, perhaps could be triggered deliberately from content.

[Is this code covered by automated tests?]:

   Perhaps, but it is such a rare crash it is hard to catch.

[Has the fix been verified in Nightly?]:

   Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 

   No.

[List of other uplifts needed for the feature/fix]:

   None, I think.

[Is the change risky?]:

   No, it is just moving a little more trivial code inside a critical section.

[String changes made/needed]:

   None.

Comment 9

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75431a550bc6b949c3ae992792f561ea0d6d5a2e
Bug 1361100: Perform all timer init after removal from TimerThread. r=froydnj

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/75431a550bc6

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8864906 [details] [diff] [review]
Perform all timer init after removal from TimerThread

Fix a sec-high. Beta54+. Should be in 54 beta 8.
Hi :bwc,
Is this worth uplifting to ESR52?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d6b02b22e29c

Comment 13

[Byron Campen [:bwc]]

(In reply to Gerry Chang [:gchang] from comment #11)
> Comment on attachment 8864906 [details] [diff] [review]
> Perform all timer init after removal from TimerThread
> 
> Fix a sec-high. Beta54+. Should be in 54 beta 8.
> Hi :bwc,
> Is this worth uplifting to ESR52?

No, this won't apply to 52 since it doesn't have any locking at all.