Closed Bug 1755555 Opened 9 months ago Closed 9 months ago

Crash in nssTrustDomain_GetActiveSlots

Categories

(NSS :: Libraries, defect, P2)

All
Unspecified

Tracking

(firefox-esr9199+ fixed, firefox97 wontfix, firefox98- wontfix, firefox99+ fixed)

RESOLVED FIXED
Tracking Status
firefox-esr91 99+ 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)

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.

Keywords: sec-high

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

See Also: → CVE-2022-1097

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

Priority: P3 → P2
Blocks: 1626047

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.
Attachment #9264049 - Flags: sec-approval?

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 on attachment 9264049 [details]
Bug 1755555 - Hold tokensLock through nssToken_GetSlot calls in nssTrustDomain_GetActiveSlots. r=rrelyea

sec-approval+ = dveditz

Attachment #9264049 - Flags: sec-approval? → sec-approval+
Group: crypto-core-security → core-security-release

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.

Flags: needinfo?(jschanck)
Whiteboard: [nss-fx] → [nss-fx][sec-survey]
Flags: needinfo?(jschanck)
Flags: qe-verify-
Whiteboard: [nss-fx][sec-survey] → [nss-fx][sec-survey][post-critsmash-triage]
Blocks: 1760872
Whiteboard: [nss-fx][sec-survey][post-critsmash-triage] → [nss-fx][sec-survey][post-critsmash-triage][adv-main99-]
Whiteboard: [nss-fx][sec-survey][post-critsmash-triage][adv-main99-] → [nss-fx][sec-survey][post-critsmash-triage][adv-main99-][adv-esr91.8-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.