201259 - NSS_GetClientAuthData does not work with limited usage certs

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

NSS_GetClientAuthData is the default callback provided by NSS to select a client
authentication certificate. Applications that don't want to write extra code for
client auth can choose to use it. It is unknown how many shipping products use it.
However, nearly all of the NSS command-line tools use it.

The problem is that this function searches for client auth certificates by
nickname. It does not check the cert usage. If dual-key certs are deployed,
there are multiple certificates matching the nickname. There is a 50% chance
that the callback will pick the encryption cert instead of the SSL client auth cert.

This happened to me while I was developing tests for smartcards - the test
picked the encryption cert on the smartcard, and the SSL server refused it
(inadequate key usage).

There are two code paths in this default cert callback :
1) Searches by nickname provided in the void* argument
2) Searches in a list of the nicknames of all user certs that mach the list of
valid CAs provided by the server

Both code paths use PK11_FindCertFromNickname, which does not take a usage,
therefore they both have this problem.

The fix is to filter certs. The function CERT_FindUserCertByUsage can help for
the first case. In the second case we may need to build a list of certs by
subject and filter by usage ourselves.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Sounds to me like we need a PK11_FindCertFromNicknameAndUsage function.
Both code paths described above could use it, right?

Comment 2

[Julien Pierre]

Nelson, I don't think we need a PK11_FindCertFromNicknameAndUsage .
CERT_FindUserCertByUsage accomplishes what we need . It also has the advantage
that it only returns user certs. I will generate a patch shortly.

Comment 3

[Julien Pierre]

Attachment: Include usage when searching for a client cert

I verified that this code correctly picks the signing cert on my smartcard for
logging in to an SSL server.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 119919 [details] [diff] [review]
Include usage when searching for a client cert

r=nelsonb

Comment 5

[Wan-Teh Chang]

Comment on attachment 119919 [details] [diff] [review]
Include usage when searching for a client cert

Why do we pass PR_FALSE as the validOnly argument to
CERT_FindUserCertByUsage?

Comment 6

[Julien Pierre]

Wan-Teh,

This is to be compatible with the previous usage.
PK11_FindCertFromNickname returns a cert, but it isn't guaranteed to be valid.
So when I pass PR_FALSE to CERT_FindUserCertByUsage, I have the same behavior as
before.

Note that in the second code path, when the callback automatically searches for
the cert, there is code to only check for unexpired certs.

However, in the first path, where the nickname is passed as a string, we don't
check for validity, as the application forced the use of that certificate, even
if it is expired.

Comment 7

[Wan-Teh Chang]

Comment on attachment 119919 [details] [diff] [review]
Include usage when searching for a client cert

r=wtc.

Comment 8

[Julien Pierre]

Targeting 3.9 .

Comment 9

[Julien Pierre]

Fixed on the tip of NSS (3.9).

Checking in authcert.c;
/cvsroot/mozilla/security/nss/lib/ssl/authcert.c,v  <--  authcert.c
new revision: 1.3; previous revision: 1.2
done

CC'ing Rich Megginson & Mark Smith from DS, in case parts of it use this default
callback for client auth.

Comment 10

[Mark Smith [:mcs]]

We will have to check our proprietary DS code. Thanks for the "heads up."

The Mozilla LDAP C SDK code does not use NSS_GetClientAuthData but the code it
does use suffers from this bug as well and needs a similar fix. I just opened
bug 201483 to track that problem.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Changed the summary line because NSS_GetClientAuthData is not a "default" 
client auth callback.  There is no "default" client auth selection callback.
The application must specifically register a callback if any is to be used.

NSS apparently supplies a number of functions those purpose seems to be to 
implement a client auth cert selection callback, or at least a large part 
of one.   NSS_GetClientAuthData calls NSS_CmpCertChainWCANames in a loop to
determine if a potential user cert was issued by one of the named CAs.  
But NSS also offers function CERT_FilterCertListByCANames which seems to do
a very similar function to the loop in NSS_GetClientAuthData.  NSS also
offers CERT_MatchUserCert, which seems to do nearly all of what is needed
to automatically select certs for client auth.  mozilla uses
CERT_FilterCertListByCANames, but none of the other functions named above.  

I'm wondering if NSS needs so many different implementations of the client
auth cert selection callback.  Perhaps NSS_GetClientAuthData should call
CERT_MatchUserCert instead of the code it does now for automatic cert 
selection.   

Comment 12

[Rich Megginson]

LDAP C SDK is investigating switching from using PK11_FindCertFromNickname() to using CERT_FindUserCertByUsage().  However, it doesn't work quite the same way.  Apparently, CERT_FindUserCertByUsage() _requires_ the use of the proto_win parameter.  If this is not given, no certs will be found, and there is this comment in the code:
273     /* XXX - why is this restricted? */
274     if ( proto_win != NULL ) {
275         cert = PK11_FindCertFromNickname(nickname,proto_win);
276     }

I would also like to know why this is restricted, because the LDAP C SDK has been using PK11_FindCertFromNickname(nick,NULL) for quite some time and it seems to work fine.  If we must use CERT_FindUserCertByUsage(), then we will have to either figure out what to pass in for the proto_win argument, or we will simply just copy/paste the code from NSS to remove the restriction against  NULL proto_win.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Eliminating the meaningless phrase "dual key", since there is no such thing
as a cert with dual keys. 

Rich, AFAIK, NSS does nothing with this "proto_win" (a.k.a wincx) value other
than pass it to callback functions.  I think this is what's commonly called a
"completion" argument.  

I think this test for non-NULL can be eliminated.  In the mean time, let me 
suggest that, instead of NULL, you pass (void *)0xdeadbeef or some other 
recognizable but meaningless value, and see if that works OK.  
Other suggestions:  (void *)1  or (void *)0x0ddc0ffeebadf00d

Comment 14

[Mark Smith [:mcs]]

Looking at the code in /mozilla/directory/c-sdk/ldap/libraries/libssldap/ldapsinit.c:get_keyandcert() that Rich is trying to fix, we set a PK11 password callback function but not until after we make the call to find the certificate (I guess we assume the password will be needed only when retrieving the key).  I suggest the code be changed to set call PK11_SetPasswordFunc() earlier and then we can just pass our LDAPSSLSessionInfo pointer (ssip) as the completion parameter in all cases.