1739421 - Ensure access to mITimer is done under lock in nsTimerImpl

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

In nsTimerImpl::Fire(), we call callbackDuringFire.match() and pass a number of lambdas with references to mITimer... but we're not under the mutex.

See near the top of the function where we grab stuff under mutex, including kungFuDeathGrip on mITimer, which tells me that something else can null out mITimer once the lock is dropped.

Just using the kungFuDeathGrip should resolve this safely

Comment 1

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

Attachment: Bug 1739421: Use kungFuDeathGrip when firing a timer r=bwc

Comment 2

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

Comment on attachment 9249256 [details]
Bug 1739421: Use kungFuDeathGrip when firing a timer r=bwc

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Pretty darn hard, perhaps not impossible
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Trivial, no risk
• How likely is this patch to cause regressions; how much testing does it need?: No regressions likely

Comment 3

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9249256 [details]
Bug 1739421: Use kungFuDeathGrip when firing a timer r=bwc

Approved to land and request uplift

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Use kungFuDeathGrip when firing a timer r=bwc
https://hg.mozilla.org/integration/autoland/rev/786dcb2d210d3662928497544f3e2b1e51a0e771
https://hg.mozilla.org/mozilla-central/rev/786dcb2d210d

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Can you suggest a security rating for this bug?

Comment 6

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

sec-high because of potential for UAF; no indication that this actually has occurred

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jesup, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 9

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

I don't think this needs an uplift, it's a proactive fix for something seen with static analysis

Comment 10

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

On second thought, while it should only be nullable (via Cancel), that unlocked write would be a potential TSAN violation, and this is a very, very safe patch, so let's uplift

Comment 11

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

Comment on attachment 9249256 [details]
Bug 1739421: Use kungFuDeathGrip when firing a timer r=bwc

Beta/Release Uplift Approval Request

• User impact if declined: Possible TSAN violation
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We already held the timer in a kungfudeathgrip, so using that instead of mITimer is super-low-risk
• String changes made/needed: none

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9249256 [details]
Bug 1739421: Use kungFuDeathGrip when firing a timer r=bwc

Approved for 95 beta 9, thanks.

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a6081863b59

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9249256 [details]
Bug 1739421: Use kungFuDeathGrip when firing a timer r=bwc

Also approved for 91.4esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/b2ce3f908336