1756271 - Crash in nssSlot_IsTokenPresent | nssSlot_GetToken

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[John Schanck [:jschanck]]

Sample crash report: https://crash-stats.mozilla.org/report/index/c3fe5250-2c45-45cf-9413-8b0280220208

This is another lifetime violation for an NSSToken. This time when accessed from an NSSSlot rather than a PK11SlotInfo. This will be easy to fix once the patch for Bug 1745667 lands, since an NSSSlot owns a reference to a PK11SlotInfo and that patch gives us a safe way to access an NSSToken through a PK11SlotInfo.

Note that some crashes with this signature are fixed by the patches for Bug 1745667 and Bug 1755555. In particular, reports that reference devslot.c line 133, like this one, rather than line 214, are due to the other bugs.

Comment 1

[John Schanck [:jschanck]]

Attachment: Bug 1756271 - Remove token member from NSSSlot struct. r=rrelyea

Comment 2

[Robert Relyea]

So the patch is right, and I'm willing to think about it, but this is a move away from 'Stan'. So I want to discuss this a bit first.
The long term goal was to move to to the 'stan' versions of the code rather than the traditional. So at some point the application and NSS api would be only NSSSlot and NSSToken. The design point was that if the token wasn't present in the slot, the slot entry would be NULL. With this change we loos that. We also loose access to NSSToken from NSSSlot if the PK11SlotInfo structure is finally deprecated.

Given that history we can deside:

1. Stan is never going to happen. It's been around for 10 years, but we still have only exposed the API's internally, so this change is OK.
2. When Stan happens, we'll deal with the fallout then, in the meantime, having two ways to get the the same data is causing too many issues internally, so this change is again OK.
3. No we want to keep the goals of Stan around and should rethink how we fix this issue.

bob

Comment 3

[John Schanck [:jschanck]]

I view this as (2) a temporary fix while we decide whether, and how, to finish the work on Stan.

The design point was that if the token wasn't present in the slot, the slot entry would be NULL. With this change we loos that.

We still have the property that if the token is not present (modulo polling delay) then nssSlot_GetToken returns NULL, so I think this is moot.

We also loose access to NSSToken from NSSSlot if the PK11SlotInfo structure is finally deprecated

We can't deprecate PK11SlotInfo without major changes to both NSSToken and NSSSlot. If someday we change STAN_InitTokenForSlotInfo(NSSTrustDomain *td, PK11SlotInfo *slot) to STAN_InitTokenForSlotInfo(NSSTrustDomain *td, NSSSlot *slot) and / nssToken_CreateFromPK11SlotInfo to nssToken_CreateFromNSSSlot, then we can reintroduce a token field so that an NSSSlot can manage the lifetime of an NSSToken.

Comment 4

[John Schanck [:jschanck]]

Comment on attachment 9265263 [details]
Bug 1756271 - Remove token member from NSSSlot struct. r=rrelyea

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. The UAF here involves the destruction of an NSSToken, which usually happens at shutdown. The patch uses the PK11_GetNSSToken function that was improved in Bug 1370866, which indicates an NSSToken lifetime violation. A careful reader will find a fixed-offset pointer-sized read and a fixed-offset single-byte \x00 write. The latter is difficult to trigger, and it's not clear that the read is useful at all.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• 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?: Low risk; easy to create.
• How likely is this patch to cause regressions; how much testing does it need?: The patch is not likely to cause regressions.

Comment 5

[Tom Ritter [:tjr]]

Comment on attachment 9265263 [details]
Bug 1756271 - Remove token member from NSSSlot struct. r=rrelyea

Approved to land and do whatever uplift things need to be requested

Comment 6

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/55052f78244cb4a39ef2a19aebd19fbface06829

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Are we planning to backport this to 3.76 for Fx99 also?

Comment 8

[John Schanck [:jschanck]]

Yes, we'll prepare a 3.76.1 release on Monday. I've opened Bug 1761497 for tracking.

Comment 9

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

The patch landed in nightly and beta is affected.
:jschanck, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[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.