2017945 - Temp private key lifecycle is broken.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Robert Relyea]

Temp Private keys typically 'own' their own handles, so when you free a temp key, you destroy the handle. However, when we search temp keys from Find, that handle we get is owned by a different temp key. Destroying that key will destroy the underlying key. We ran into this problem with the Certificate Server, where the server would start up SSL and create a temporary key from the perm key (for performance reasons. In another thread, the server had code that would look up the private key and do some queries. In some tokens, like softoken, the query would return the original perm key first, but it's perfectly acceptable for the token to return the session key first. In that case when the key is freed, that thread will delete the handle returned. This deletes the underlying key still used in SSL and all SSL sessions stop working because the private key is now gone. It's clearly wrong to delete the handles returned from a find, the keys should not be owned by the find. We need to separately track both isTemp and isOwned.

Because SECKEYPrivateKeys are public (sigh), we can't just add new fields to the structure. Fortunately if IsOwned is set, then isTemp must also be set, so we can overload the PRBool as bit flags. Old code testing for isTemp will still succeed.

Comment 1

[Robert Relyea]

Attachment: Bug 2017945 Temp private key lifecycle is broken.

Temp Private keys typically 'own' their own handles, so when you free a temp key, you destroy the handle. However, when we search temp keys from Find, that handle we get is owned by a different temp key. Destroying that key will destroy the underlying key. We ran into this problem with the Certificate Server, where the server would start up SSL and create a temporary key from the perm key (for performance reasons. In another thread, the server had code that would look up the private key and do some queries. In some tokens, like softoken, the query would return the original perm key first, but it's perfectly acceptable for the token to return the session key first. In that case when the key is freed, that thread will delete the handle returned. This deletes the underlying key still used in SSL and all SSL sessions stop working because the private key is now gone. It's clearly wrong to delete the handles returned from a find, the keys should not be owned by the find. We need to separately track both isTemp and isOwned.

Because SECKEYPrivateKeys are public (sigh), we can't just add new fields to the structure. Fortunately if IsOwned is set, then isTemp must also be set, so we can overload the PRBool as bit flags. Old code testing for isTemp will still succeed.

Comment 2

[John Schanck [:jschanck]]

I want to carefully consider the implications of this for multithreaded access to softoken. Temporarily marking this as security sensitive until we perform that review.

Comment 3

[John Schanck [:jschanck]]

OK, I've reviewed this and haven't found any cause for concern.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/5cc3e16a946c
Temp private key lifecycle is broken.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/8cbbfa502487
Temp private key lifecycle is broken.

Comment 6

[Robert Relyea]

Turned out the test case with softoken was easier than I thought. ... create a session key, look that key up using Find, use the key and then free it. The original key is now clobbered.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/b6d396f57fec
Temp private key lifecycle is broken. -- fix clangformat