1755555 - Crash in nssTrustDomain_GetActiveSlots

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[John Schanck [:jschanck]]

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.

Comment 1

[John Schanck [:jschanck]]

Attachment: Bug 1755555 - Hold tokensLock through nssToken_GetSlot calls in nssTrustDomain_GetActiveSlots. r=rrelyea

Comment 2

[Daniel Veditz [:dveditz]]

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

Comment 3

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

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

Comment 4

[John Schanck [:jschanck]]

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

[Daniel Veditz [:dveditz]]

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

[Daniel Veditz [:dveditz]]

Comment on attachment 9264049 [details]
Bug 1755555 - Hold tokensLock through nssToken_GetSlot calls in nssTrustDomain_GetActiveSlots. r=rrelyea

sec-approval+ = dveditz

Comment 7

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/a36477f0ee5060bdfcedb69ce51b64ff32300747
https://hg.mozilla.org/mozilla-central/rev/1ecdb4c33adf

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.