Closed Bug 1573160 Opened 1 year ago Closed 1 year ago

use-after-free possibility in nsThreadPool

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 69+ fixed
firefox-esr68 69+ fixed
firefox68 --- wontfix
firefox69 + fixed
firefox70 + fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

(Keywords: csectype-race, sec-high, Whiteboard: [adv-main69+][adv-esr68.1+][adv-esr60.9+][post-critsmash-triage])

Attachments

(1 file)

While reviewing some other code, I think we have a latent use-after-free possibility in nsThreadPool. The sequence of events is something like:

T1. Dispatch an event into the thread pool.
T1. Discover that we need to spawn a new thread.
T1. Spawn a new thread.

Now, we get interrupted here:

https://searchfox.org/mozilla-central/source/xpcom/threads/nsThreadPool.cpp#117

T2. Call Shutdown() on the thread pool.
T2. Executes bits of shutdown, specifically:

https://searchfox.org/mozilla-central/source/xpcom/threads/nsThreadPool.cpp#333-347

So the thread pool is marked as shutting down, and we've completely cleared out the list of threads...except that we have one pending thread that might be added to our thread list.

T2. Start shutting down all the threads in the thread pool.
T1. Resumes, discovers that we have 0 threads in the pool, so adds itself to the list of threads.
T1. Dispatches an event into the thread pool.
T2. Releases the last reference to the thread pool.
T2. Thread pool is freed.
...new thread is scheduled to run...
NT. Tries to touch things in the now-freed thread pool object.
NT. Boom.

Can somebody check that logic?

Group: core-security → dom-core-security

Comment on attachment 9084692 [details]
Bug 1573160 - shutdown newly-spawned threads if we're shutting down; r=mccr8

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: You'd have to force thread pool construction and then time thread pool shutdown with spawning another thread. The timing issues don't feel very easy, but exploit authors gonna exploit.
  • 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?: everything
  • 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?: The patch should cleanly backport; it is straightforward to create branch-specific patches if need be.
  • How likely is this patch to cause regressions; how much testing does it need?: Not likely.
Attachment #9084692 - Flags: sec-approval?

Asking for sec-approval since I suppose this bug is probably sec-high given the UAF possibilities.

Assignee: nobody → nfroyd

I'll just mark it sec-high, then.

Sec-approval+ for mozilla-central and I've added tracking flags.

I'd like to get Beta, ESR68, and ESR60 patches nominated if possible so we can land them after this is on trunk.

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

Comment on attachment 9084692 [details]
Bug 1573160 - shutdown newly-spawned threads if we're shutting down; r=mccr8

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: Users subject to more exploits than normal.
  • Fix Landed on Version: 70
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Low risk: this is well understood code, and the change in behavior is small.
  • String or UUID changes made by this patch: None

Beta/Release Uplift Approval Request

  • User impact if declined: Users subject to more exploits than normal.
  • 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): Low risk: this is well understood code, and the change in behavior is small.
  • String changes made/needed: None
Attachment #9084692 - Flags: approval-mozilla-esr68?
Attachment #9084692 - Flags: approval-mozilla-esr60?
Attachment #9084692 - Flags: approval-mozilla-beta?
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Comment on attachment 9084692 [details]
Bug 1573160 - shutdown newly-spawned threads if we're shutting down; r=mccr8

Fixes a possible UAF. Approved for 69.0b15, 68.1esr, and 60.9esr.

Attachment #9084692 - Flags: approval-mozilla-esr68?
Attachment #9084692 - Flags: approval-mozilla-esr68+
Attachment #9084692 - Flags: approval-mozilla-esr60?
Attachment #9084692 - Flags: approval-mozilla-esr60+
Attachment #9084692 - Flags: approval-mozilla-beta?
Attachment #9084692 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main69+][adv-esr68.1+][adv-esr60.9+]
Flags: qe-verify-
Whiteboard: [adv-main69+][adv-esr68.1+][adv-esr60.9+] → [adv-main69+][adv-esr68.1+][adv-esr60.9+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.