PK11_DeleteTokenCertAndKey does not delete key



16 years ago
16 years ago


(Reporter: Ian McGreer, Assigned: Ian McGreer)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



16 years ago
The key is still in the database after the call.

Comment 1

16 years ago
Even when all the certs have been deleted? (if the key has more than one cert
pointing to it, it won't get deleted).


Comment 2

16 years ago
I'm not sure about this one...  What happened is that I was running QA on a
build that had a bunch of printf's in it.  For one, it printed out every key in
the database after the call to SECKEY_OpenKeyDB.  I would use certutil to delete
the key, however, it showed up later on with pk12util.

But I think the problem may have been that I didn't rebuild certutil, and it is
statically linked.  I think certutil -F (Delete Key) may take advantage of the
patch I proposed for 95135, in that it is not finding the private key when the
public key has a 0 in front.

I'm rerunning QA now to see if I can prove this hypothesis.  The bug itself, I
think, is incorrect, because I verified that the key was no longer in the
database in question.  But I will wait for QA (I have to run the FIPS test about
15-20 times to get a failure, and that takes a while).

Comment 3

16 years ago
Okay, this is a bug.  I think it is related to the other.  It is only failing to
delete the key when it thinks the key has a leading 0.  Thus, it needs a similar
workaround.  I'm currently looking for where that would go, but if you know
offhand could you post it here?

Comment 4

16 years ago
Somewhere in the softoken... probably from some call off of NSC_DestroyObject.


Comment 5

16 years ago
Created attachment 45783 [details] [diff] [review]
patch that implements same workaround

Comment 6

16 years ago
with this patch and the patch for 95135, I run the FIPS QA 30 times
successively.  Five of those runs had keys with a leading zero that took
advantage of both patches.  Thus the patches appear to be correct, at least

The only question I had is what level the patches should be at.  Is it right for
them to be in the PKCS#11 layer, as I have it, or should they be in keydb.c?  I
decided this was right, since the key database code should be blind to what goes
into it, i.e., it shouldn't know that the accessor may be a public key with a
leading zero.  So I think the patches are what we want.

The only other question is whether similar workarounds need to happen in other
parts of the code.  I reviewed the entry points to keydb.c and looked at them
through LXR, I think this is it.

I'm ready for review then, for this and 95135.

Comment 7

16 years ago

Comment 8

16 years ago
The patch looks good Ian.

Yes it's at the right level. BTW, the comment may be wrong. RSA always has a
leading '0', but I think it always has a leading '0', so I don't think it's the
database that is stripping it off. I think that the input attribute
(CKA_NETSCAPE_DB) is not getting the '0' added back again before it is used in
setting the DB value. your fix is preferable to fixing the CKA_NETSCAPE_DB
because we have already poluted some databases with bad key.db values.

Check it in. r=relyea.


Comment 9

16 years ago
checked in.  waiting for tomorrow's QA before resolving.

Comment 10

16 years ago
Ian, please set the target milestone and mark this bug fixed.
Assignee: wtc → ian.mcgreer
Priority: -- → P1

Comment 11

16 years ago
fixed in 3.2.2
Last Resolved: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.2.2

Comment 12

16 years ago
The fix is not in 3.3 but is in 3.3.1.
You need to log in before you can comment on or make changes to this bug.