TImeout code for calling SetupMacCommandLine on main thread can lead to a UAF
Categories
(Toolkit :: Application Update, defect)
Tracking
()
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 | ||
Comment 1•1 year ago
|
||
Updated•1 year ago
|
Comment 2•1 year ago
|
||
Set release status flags based on info from the regressing bug 1342887
| Assignee | ||
Comment 3•1 year ago
|
||
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
Comment 4•1 year ago
|
||
Nice find. This sounds like it would be difficult for a web page to exploit, so I'll mark it sec-moderate.
Updated•1 year ago
|
Comment 6•1 year ago
|
||
Comment 7•1 year ago
|
||
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-firefox121towontfix.
For more information, please visit BugBot documentation.
| Assignee | ||
Comment 8•1 year ago
|
||
Original Revision: https://phabricator.services.mozilla.com/D194792
Updated•1 year ago
|
Comment 9•1 year ago
|
||
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.orgwhile 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.
| Assignee | ||
Updated•1 year ago
|
Comment 10•1 year ago
|
||
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.
Updated•1 year ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Comment 11•1 year ago
|
||
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.
Updated•1 year ago
|
Comment 12•1 year ago
|
||
Updated•1 year ago
|
Comment 13•1 year ago
|
||
Comment 14•1 year ago
|
||
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.
Comment 15•11 months ago
|
||
Making Firefox 122 security bugs public. [bugspam filter string: Pilgarlic-Towers]
Description
•