Closed Bug 1866840 (CVE-2024-0752) Opened 1 year ago Closed 1 year ago

TImeout code for calling SetupMacCommandLine on main thread can lead to a UAF

Categories

(Toolkit :: Application Update, defect)

defect

Tracking

()

VERIFIED FIXED
122 Branch
Tracking Status
firefox-esr115 --- wontfix
firefox120 --- wontfix
firefox121 --- wontfix
firefox122 --- verified

People

(Reporter: nika, Assigned: nika)

References

(Regression)

Details

(Keywords: csectype-uaf, regression, sec-moderate, Whiteboard: [adv-main122+])

Attachments

(2 files, 2 obsolete files)

In bug 1342887, a timeout was added to the synchronous wait in UpdateDriverSetupMacCommandLine for a dispatch to the main thread. Unfortunately, this timeout will likely lead to a UAF if triggered, as the closure for the runnable holds references to stack local values: https://searchfox.org/mozilla-central/rev/f030995a79461379153293c0e07f4982afe9ac28/toolkit/xre/nsUpdateDriver.cpp#91.

This code should either be changed to remove the timeout (likely changing to to instead use a helper like SyncRunnable which makes this sort of thing safer, or it should be modified to work safely, by copying all relevant state onto the heap. Given the types of some of the captured data (including mutating raw pointer in/out parameters), however, this seems like it would be a bit tricky.

That being said, looking at the callers of UpdateDriverSetupMacCommandLine, I actually think it is possible to avoid the call and dispatch all-together.

This function appears to be called indirectly through ProcessUpdates, which only appears to be called off-main-thread in nsUpdateProcessor::StartStagedUpdate. In this case the restart parameter is false, and we should be post-XPCOM-startup meaning that most of the SetupMacCommandLine function is a no-op. In addition, the only non-noop section only removes command line flags, and if restart is false, those flags will never be in the passed in argc array.

I believe the UpdateDriverSetupMacCommandLine function can be removed, and the relevant code inserted into the call-site: https://searchfox.org/mozilla-central/rev/f030995a79461379153293c0e07f4982afe9ac28/toolkit/xre/nsUpdateDriver.cpp#619

if (restart) {
  MOZ_RELEASE_ASSERT(NS_IsMainThread(),
                     "restart may only be set for calls on the main thread");
  CommandLineServiceMac::SetupMacCommandLine(argc, argv, restart);
}
Assignee: nobody → nika
Status: NEW → ASSIGNED

Set release status flags based on info from the regressing bug 1342887

I realize I never wrote it in this bug, but i found this issue through :gsvelto's comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1864241#c1

Nice find. This sounds like it would be difficult for a web page to exploit, so I'll mark it sec-moderate.

Pushed by nlayzell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dca6957049cc Only call SetupMacCommandLine if we're restarting, r=application-update-reviewers,nalexander
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 122 Branch

The patch landed in nightly and beta is affected.
:nika, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox121 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(nika)
Attachment #9366926 - Flags: approval-mozilla-beta?

Uplift Approval Request

  • Risk associated with taking this patch: Medium-High
  • Is Android affected?: no
  • Code covered by automated testing: no
  • Steps to reproduce for manual QE testing: Two flows should be checked: 1. Start Firefox with open -a path/to/Firefox.app https://mozilla.org while an update is staged, but has not yet been applied. 2. Restart firefox while it is running for an update, and ensure that the pages restore as expected, no crashes occur, and the update is applied.
  • Fix verified in Nightly: yes
  • Needs manual QE test: yes
  • String changes made/needed: None
  • User impact if declined: Very rare racy UAF if the main thread is very busy when applying an update. Likely very difficult to exploit due to only occurring in specific cases when applying a pending update.
  • Explanation of risk level: Changes to largely untested macOS-specific update applicaiton logic.
Flags: qe-verify+
Flags: needinfo?(nika)

Comment on attachment 9366926 [details]
Bug 1866840 - Only call SetupMacCommandLine if we're restarting, r=#application-update-reviewers

Given the medium-high risk and moderate severity of this, I'm inclined to let this ride the Fx122 train since we're a week out from Fx121 going to RC. Feel free to ping me if you feel that this should be reconsidered.

Attachment #9366926 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9366926 - Attachment is obsolete: true
QA Whiteboard: [qa-triaged]

Verified as fixed on the latest Nightly 122.0a1 on macOS 13 - no crashes occur while updating using the two flows mentioned in Comment 9.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Whiteboard: [adv-main122+]
Alias: CVE-2024-0752

Comment on attachment 9377949 [details]
Bug 1866840 - Add fuzzing Nyx coverage build. r?#taskgraph-reviewers!

Revision D200454 was moved to bug 1875285. Setting attachment 9377949 [details] to obsolete.

Attachment #9377949 - Attachment is obsolete: true

Making Firefox 122 security bugs public. [bugspam filter string: Pilgarlic-Towers]

Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: