Closed Bug 353714 Opened 14 years ago Closed 12 years ago
key search functions ignore the nickname argument
Talked to Nelson about this problem, but we didn't get to the conclusion. And so I'm opening the bug. From a customer: ----------- It seems that a couple of NSS routines that are supposed to search the key database for keys with a specific "nickname" are actually ignoring the nickname and returning all keys regardless of nickname (label) value. Example: PK11_ListPublicKeysInSlot(slot, nickname). My DB has 1 key in it with a nickname of "test". However, if I search the DB using the above routine and give a nickname of "blahblahblah", it still returns the "test" key. --------- I've found couple problems with search function: 1) PK11_ListPrivKeysInSlot and PK11_ListPublicKeysInSlot make assumption that nickname parameter always has extra character at the end. 2) sftk_searchTokenList calls sftk_searchKeys with mustStrict set equal to true only when classFlags has NSC_KEY bit set AND name length is not zero. I think, OR should be used instead.
I wish there was a way to return this bug to "UNCONFIRMED" status. This bug alleges that PK11_ListPublicKeysInSlot ignores its nickname argument and returns all public keys in the slot. It's clear that PK11_ListPublicKeysInSlot does not ignore the nickname and makes it part of the attribute template that it sends to the PKCS#11 module as part of C_FindObjects. I have reason to suspect that the customer was searching a slot that was in a non-NSS PKCS#11 module. I think it is possible that that slot/module was ignoring the CKA_LABEL attribute in the template. IMO, we should not treat this as a confirmed NSS bug until we know that it is. (OTOH, there is that off-by-1 error in the functions that Alexei mentioned above, but that should be in a separate bug, since this bug alleges that nickname is simply ignored.)
Alexei, in comment 0, you identified two bugs in the source code. Please write a patch to fix them, and attach it to this bug. Even if we don't ever solve (or even understand) the customer's complaint, we should fix the bugs we DO know about.
Assignee: nobody → alexei.volkov.bugs
I've reopened the bug 381718 for the off by one issue in the ListKeys functions and attached patch there leaving this bug for the remaining issues.
OK, so, the off-by-one problem is now out of scope for this bug, and so the only remaining part of this bug that remains in scope here is the allegation that PK11_ListPublicKeysInSlot is "returning all keys regardless of nickname (label) value." I don't recall ever experiencing that problem, and I think the next step for this bug is to try to reproduce the alleged problem. This bug can be marked invalid if that symptom cannot be reproduced.
I have seen this problem (PK11_List***KeysInSlot ignores the CKA_LABEL attribute and returns everything) using 3.11.7 (Solaris Nevada build 75). Example: $ certutil -K -d . Enter Password or Pin for "NSS Certificate DB": <0> testlabel $ certutil -K -d . -n FOOBAR Enter Password or Pin for "NSS Certificate DB": <0> testlabel It returns the key labeled "testlabel" and ignores the fact that I specified a key label of "FOOBAR" on the command line. I believe certutil is just calling PK11_List***KeysInSlot. I can also duplicate this using the new Solaris 'pktool' utility, which definitely uses PK11_List***KeysInSlot and passes in the key label to do the search: $ pktool list keystore=nss dir=. objtype=key Enter PIN for internal: Found 1 private keys. Key #1 - RSA private key: testlabel Found 1 public keys. Key #1 - RSA public key: testlabel Now, see what happens when I specify a key nickname: $ pktool list keystore=nss dir=. objtype=key nickname=FOOBAR Enter PIN for internal: Found 1 private keys. Key #1 - RSA private key: testlabel Found 1 public keys. Key #1 - RSA public key: testlabel It should not have returned any hits for that nickname. I traced this down to the PK11_List***KeyInSlot function, there is definitely something wrong.
Wyllys, the certutil QA test tool's -K command ignores the -n option string, and calls the underlying NSS libraries without using that string value. That's a bug in certutil, the QA test program, not a fault of the NSS shared library. There is a separate NSS bug filed about certutil -K, bug 291384. This bug reports a problem with the shared library key search functions, and specifically with function PK11_ListPublicKeysInSlot. But no reproducible test case involving that function was ever given, and the function by that name clearly does not ignore nicknames passed to it. If the basis of this bug report was, in fact, the output of certutil -K, then this bug should be made a duplicate of bug 291384. If there is a reproducible test case that proves the claim in comment 0, that test case should be made available.
I have definitely seen the PK11_ListPublicKeysInSlot function ignore the nickname argument. I can try to write a reproduceable test case for it, because I've seen it happen a lot lately. The Solaris KMF code that calls NSS uses those APIs and it is definitely ignoring the nickname. The "pktool" example above is one way to get to it. The bug is somewhere in the NSS PKCS11 object searching code because I see that the actual PK11_ListPublicKeysInSlot function definitely sets the CKA_LABEL argument, but the underlying code that searches the objects seems to be ignoring it.
It's likely a bug in the old database code. There likely several cases where the code returns more objects than match the criteria. Most of these cases are not tripped over by NSS itself. The new database code stores and handle things more like PKCS #11 expects. It would be interesting to see if the test change fails in 3.12 using shared db. bob
Wyllys, could it be the PKCS#11 module's implomentation of C_FindObjects or C_FindObjectsInit that is ignoring the CKA_LABEL attribute in the array of templates passed as arguments to C_FindObjectsInit?
Yes, that is what I suspect. The NSS PKCS#11 code is ignoring the CKA_LABEL and returning all of the keys that if finds.
I didn't know that pktool used NSS. Does it use NSS for access to all PKCS#11 modules? or only for access to NSS's own PKCS#11 module?
pktool can use 1 of 3 keystores - file-based (OpenSSL), NSS, or PKCS#11. When it uses NSS, it defaults to the NSS "internal" DB. When pkcs#11 is selected (it is the default), it will call the Solaris libpkcs11 library directly and avoid NSS. The example I gave above: "pktool list keystore=nss dir=. objtype=key nickname=FOOBAR" is an example of pktool using NSS APIs to search for keys. Ultimately it uses the PK11_ListPublicKeysInSlot() API to find matching keys. PK11_ListPublicKeysInSlot calls the internal NSS PKCS#11 stuff, which is where I think the error lies. The NSS C_FindObject implementation seems to be broken.
I'm reassigning this bug to Julien for investigation for NSS 3.11.8. Julien, the working hypothesis is stated in comment 10. I have a patch for certutil that makes the -K command actually use the nickName string. That patch should help with this investigation. I will attach it to bug 291384 soon (the current patch addresses a different problem).
Assignee: alexei.volkov.bugs → julien.pierre.boogz
Priority: -- → P2
Target Milestone: --- → 3.11.8
Version: 3.12 → unspecified
I attached the patch for certutil to bug 291384. It is only for the trunk, but shouldn't be too difficult to back port to the 3.11 branch. It fixes certutil to only fetch keys with the given nickname, and it asserts that every key returned by PK11_ListPrivKeysInSlot has the right nickname. With it, I was able to easily confirm this bug. I found a bug in function lg_searchTokenList (which only exists on the trunk) that causes it to set the value of the variable classFlags to an incorrect value. But fixing that did not solve the problem. I will file a separate bug on that.
I mentioned this issue to Bob in IM, and he spotted some problems pretty quickly, and recommended some changes. This patch captures the changes that I made, mostly at Bob's suggestion. With this patch in place, my certutil -K test passes (doesn't assert). I'm about to run all.sh with it.
Attachment #283656 - Flags: review?(rrelyea)
Comment on attachment 283656 [details] [diff] [review] patch for trunk only, v1 r+ = relyea
Attachment #283656 - Flags: review?(rrelyea) → review+
committed on trunk softoken/legacydb/lgfind.c; new revision: 1.3; previous revision: 1.2
This is the equivalent patch for the branch.
Assignee: julien.pierre.boogz → nelson
Status: NEW → ASSIGNED
Comment on attachment 284270 [details] [diff] [review] patch for branch only, v1 request review for branch
Attachment #284270 - Flags: review?(alexei.volkov.bugs)
Comment on attachment 284270 [details] [diff] [review] patch for branch only, v1 Bob reviewed the patch for the trunk. Julien, please review this patch for the branch.
Attachment #284270 - Flags: review?(alexei.volkov.bugs) → review?(julien.pierre.boogz)
Attachment #284270 - Flags: review?(julien.pierre.boogz) → review+
On branch Checking in lib/softoken/pkcs11.c; new revision: 220.127.116.11; previous revision: 18.104.22.168
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: 3.11.8 → 3.11.9
You need to log in before you can comment on or make changes to this bug.