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

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

(4 keywords)

54 Branch
mozilla55
csectype-race, csectype-uaf, sec-high, testcase-wanted
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main54+])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
TimerThread::PostTimerEvent reads nsTimerImpl::mEventTarget without any locking. If another thread calls SetTarget, we can get undefined behavior.
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → docfaraday
Group: core-security → dom-core-security
Keywords: csectype-race, sec-moderate
(Assignee)

Comment 4

2 years ago
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.
Attachment #8864906 - Attachment filename: . → move_timer_init.patch
Attachment #8864906 - Flags: sec-approval?
Attachment #8864906 - Flags: review?(nfroyd)
This race causes a UAF, so bumping to sec-high
Keywords: sec-moderate → csectype-uaf, sec-high
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. =/
Attachment #8864906 - Flags: review?(nfroyd) → review+
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → wontfix
status-firefox-esr52: --- → affected
tracking-firefox54: --- → +
tracking-firefox55: --- → +
tracking-firefox-esr52: --- → 54+
Keywords: testcase-wanted
Comment on attachment 8864906 [details] [diff] [review]
Perform all timer init after removal from TimerThread

sec-approval=dveditz
Attachment #8864906 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 8

2 years ago
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.
Attachment #8864906 - Flags: approval-mozilla-beta?
(Assignee)

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/75431a550bc6b949c3ae992792f561ea0d6d5a2e
Bug 1361100: Perform all timer init after removal from TimerThread. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/75431a550bc6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
Flags: needinfo?(docfaraday)
Attachment #8864906 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 13

2 years ago
(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.
status-firefox-esr52: affected → wontfix
Flags: needinfo?(docfaraday)
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
tracking-firefox-esr52: 54+ → ---
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.