Closed
Bug 366405
Opened 18 years ago
Closed 18 years ago
PK11_DeleteTokenPrivateKey leaks cert when "force" is true
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.5
People
(Reporter: nelson, Assigned: nelson)
References
Details
(Keywords: memory-leak)
Attachments
(1 file)
1.32 KB,
patch
|
alvolkov.bgs
:
review+
wtc
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•18 years ago
|
Version: 3.0 → 3.4
Assignee | ||
Comment 1•18 years ago
|
||
This patch restructures the code so that there is only one return path and it always destroys the in-memory objects.
Assignee: biswatosh.chakraborty → nelson
Status: NEW → ASSIGNED
Attachment #251293 -
Flags: superreview?(wtchang)
Attachment #251293 -
Flags: review?(alexei.volkov.bugs)
Assignee | ||
Comment 2•18 years ago
|
||
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•18 years ago
|
||
Comment on attachment 251293 [details] [diff] [review] patch v1 r=alexei.volkov.bugs
Attachment #251293 -
Flags: review?(alexei.volkov.bugs) → review+
Comment 4•18 years ago
|
||
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•18 years ago
|
||
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.
Updated•18 years ago
|
Attachment #251293 -
Flags: superreview?(wtchang) → superreview+
Assignee | ||
Comment 6•18 years ago
|
||
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.
Assignee | ||
Comment 7•18 years ago
|
||
> 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
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 8•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•