366405 - PK11_DeleteTokenPrivateKey leaks cert when "force" is true

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

When PK11_DeleteTokenPrivateKey is invoked with the "force" parameter 
set to true, and a cert is found that corresponds to the private key,
PK11_DeleteTokenPrivateKey leaks the reference to the cert.  It should not.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch restructures the code so that there is only one return path
and it always destroys the in-memory objects.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Kai, presently, the only code that calls this function with force==PR_TRUE
is in PSM.  That path will definitely leak a cert if it finds one.

Can you tell us what PSM operation(s) invoke this, and how we might create 
a test case for this with FireFox or SeaMonkey?

Comment 3

[Alexei Volkov]

Comment on attachment 251293 [details] [diff] [review]
patch v1

r=alexei.volkov.bugs

Comment 4

[Wan-Teh Chang]

Comment on attachment 251293 [details] [diff] [review]
patch v1

The SECKEY_DestroyPrivateKey call should stay with the
PK11_DestroyTokenObject call, right?

We should remove "now, then" from the comment because
the comment is now the first (and only) comment in the
function, so "now, then" sounds strange.

Comment 5

[Wan-Teh Chang]

You should also think about whether we should call
SECKEY_DestroyPrivateKey only if the
PK11_DestroyTokenObject call succeeded.  I don't know
what the right answer is, but we need to make a decision
and document it.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, the old code had two exit paths and called SECKEY_DestroyPrivateKey
in both of them.  It always destroys the private key object (in memory) that
is passed to it, whether it destroys the token object or not.  The existing
comment says "this function also frees the privKey structure" and doesn't 
suggest that the freeing is conditional on the success of other events.

I wasn't trying to change that.  I was only trying to plug the leak of the 
cert reference.  The new code has only one exit path, and it calls 
SECKEY_DestroyPrivateKey, so it preserves the behavior of the old code with
respect to destroying the in-memory private key object.  

I think you're suggesting/asking whether the behavior of destroying the 
private key object when returning SECWouldBlock is incorrect or perhaps
undesirable.  Please ask Bob to comment on this question.

I guess one way to try to answer that is to study all the known existing 
uses of the function to see if they expect that behavior, or if they 
attempt to destroy the in-memory key object after this function fails.  

For now, I would like to leave the behavior of this function unchanged, 
with respect to the destruction of the in-memory private key object.  
If we later find that to be an error, we can file a separate bug.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

> Bug 366405. Fix PK11_DeleteTokenPrivateKey to not leak the cert when
> force is true.  r=alexei.volkov,wtchang

Checking in pk11akey.c; new revision: 1.9.2.5; previous revision: 1.9.2.4
Checking in pk11akey.c; new revision: 1.15; previous revision: 1.14

Comment 8

[Kai Engert [:KaiE:]]

(In reply to comment #2)
> Kai, presently, the only code that calls this function with force==PR_TRUE
> is in PSM.  That path will definitely leak a cert if it finds one.
> 
> Can you tell us what PSM operation(s) invoke this, and how we might create 
> a test case for this with FireFox or SeaMonkey?

This rather new code was introduced with enhancement bug 337733, in order to  implement an rc4 xpcom interface.

Quoting from that bug:
"Specifically, safe browsing would like to use it to encrypt urls on the client
side."

I never used that feature myself. It appears you must use install a Firefox extension in order to trigger calls to this code: http://www.google.com/tools/firefox/safebrowsing/

cc'ing Tony who worked on that patch.

Comment 9

[Tony Chang (Google)]

PK11_DeleteTokenPrivateKey is in security/manager/ssl/src/nsKeyModule.cpp, however, it's not actually called at any point in Firefox.  The safe browsing (anti-phishing) feature in Firefox only uses the symmetric keys for RC4, so the private key code path is never used.

So changing this behavior shouldn't impact Firefox.

Comment 10

[Robert Relyea]

wtc: the proposed patch is correct. The comment clearly documents that the private key structure is always freed, whether or not the delete was successful.