Crash in nssTrustDomain_GetActiveSlots
Categories
(NSS :: Libraries, defect, P2)
Tracking
(firefox-esr9199+ fixed, firefox97 wontfix, firefox98- wontfix, firefox99+ fixed)
People
(Reporter: jschanck, Assigned: jschanck)
References
Details
(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [nss-fx][sec-survey][post-critsmash-triage][adv-main99-][adv-esr91.8-])
Crash Data
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
dveditz
:
sec-approval+
|
Details | Review |
Sample crash report: https://crash-stats.mozilla.org/report/index/3d08324b-d26d-4bf2-b74c-de3310220211
The crash occurs in nssTrustDomain_GetActiveSlots
in the following block of code:
NSSRWLock_LockRead(td->tokensLock);
[...]
nssList_GetArray(td->tokenList, (void **)tokens, count);
NSSRWLock_UnlockRead(td->tokensLock);
count = 0;
for (tp = tokens; *tp; tp++) {
NSSSlot *slot = nssToken_GetSlot(*tp);
if (!PK11_IsDisabled(slot->pk11slot)) {
slots[count++] = slot;
} else {
nssSlot_Destroy(slot);
}
}
The nssList_GetArray
function does not addref the tokens that it copies to tokens
. Consequently these tokens can be destroyed by another thread after we release td->tokensLock
and before we call nssToken_GetSlot
. We should hold td->tokensLock
until after the loop.
Assignee | ||
Comment 1•3 years ago
|
||
Comment 2•3 years ago
|
||
Similarly to bug 1745667 comment 13, this race falls into the "sec-moderate for Firefox" territory for the same reasons. There are about half scary-looking _WRITE access violations, but those are atomic increments and not a very powerful gadget (never say never, of course).
The bulk of the reported crashes are on ESR though. I wouldn't mind seeing both of these bugs backported
Updated•3 years ago
|
Comment 3•3 years ago
|
||
Changing the priority to p2 as the bug is tracked by a release manager for the current nightly.
See What Do You Triage for more information
Assignee | ||
Comment 4•3 years ago
|
||
Comment on attachment 9264049 [details]
Bug 1755555 - Hold tokensLock through nssToken_GetSlot calls in nssTrustDomain_GetActiveSlots. r=rrelyea
Security Approval Request
- How easily could an exploit be constructed based on the patch?: The patch clearly identifies a few lines of code where a lock is not held and where a race can lead to a uaf. However, the race only occurs at shutdown, and the low crash volume suggests that the timing is delicate.
- Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
- 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?: The patch applies cleanly to esr.
- How likely is this patch to cause regressions; how much testing does it need?: The risk of regressions is very low.
Comment 5•3 years ago
|
||
According to John this is going to ride the trains to make sure it's really fixed the timing-dependent crash. We'll want it on ESR when we ship it on Release.
Comment 6•3 years ago
|
||
Comment on attachment 9264049 [details]
Bug 1755555 - Hold tokensLock through nssToken_GetSlot calls in nssTrustDomain_GetActiveSlots. r=rrelyea
sec-approval+ = dveditz
Updated•3 years ago
|
Assignee | ||
Comment 7•3 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/a36477f0ee5060bdfcedb69ce51b64ff32300747
https://hg.mozilla.org/mozilla-central/rev/1ecdb4c33adf
Updated•3 years ago
|
Comment 8•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Description
•