95150 - PK11_DeleteTokenCertAndKey does not delete key

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Ian McGreer]

The key is still in the database after the call.

Comment 1

[Robert Relyea]

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

bob

Comment 2

[Ian McGreer]

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

[Ian McGreer]

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

[Robert Relyea]

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

bob

Comment 5

[Ian McGreer]

Attachment: patch that implements same workaround

Comment 6

[Ian McGreer]

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

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

[Wan-Teh Chang]

r=wtc.

Comment 8

[Robert Relyea]

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.

bob

Comment 9

[Ian McGreer]

checked in.  waiting for tomorrow's QA before resolving.

Comment 10

[Wan-Teh Chang]

Ian, please set the target milestone and mark this bug fixed.
Thanks.

Comment 11

[Ian McGreer]

fixed in 3.2.2

Comment 12

[Wan-Teh Chang]

The fix is not in 3.3 but is in 3.3.1.