353714 - key search functions ignore the nickname argument

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Alexei Volkov]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 4

[Tom Mraz]

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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 6

[Wyllys Ingersoll]

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.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 8

[Wyllys Ingersoll]

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.

Comment 9

[Robert Relyea]

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

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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?  

Comment 11

[Wyllys Ingersoll]

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.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 13

[Wyllys Ingersoll]

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.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for trunk only, v1

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.

Comment 17

[Robert Relyea]

Comment on attachment 283656 [details] [diff] [review]
patch for trunk only, v1

r+ = relyea

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

committed on trunk
softoken/legacydb/lgfind.c; new revision: 1.3; previous revision: 1.2

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for branch only, v1

This is the equivalent patch for the branch.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 284270 [details] [diff] [review]
patch for branch only, v1

request review for branch

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

On branch
Checking in lib/softoken/pkcs11.c;
new revision: 1.112.2.24; previous revision: 1.112.2.23