NSS_GetClientAuthData does not work with limited usage certs

RESOLVED FIXED in 3.9

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
15 years ago
12 years ago

People

(Reporter: Julien Pierre, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.18 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Wan-Teh Chang
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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.
(Reporter)

Updated

15 years ago
Blocks: 201260
Sounds to me like we need a PK11_FindCertFromNicknameAndUsage function.
Both code paths described above could use it, right?
(Reporter)

Comment 2

15 years ago
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.

(Reporter)

Comment 3

15 years ago
Created attachment 119919 [details] [diff] [review]
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.
(Reporter)

Updated

15 years ago
Attachment #119919 - Flags: superreview?(wtc)
Attachment #119919 - Flags: review?(nelsonb)
Comment on attachment 119919 [details] [diff] [review]
Include usage when searching for a client cert

r=nelsonb
Attachment #119919 - Flags: review?(nelsonb) → review+
(Assignee)

Comment 5

15 years ago
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?
(Reporter)

Comment 6

15 years ago
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.
(Assignee)

Comment 7

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

r=wtc.
Attachment #119919 - Flags: superreview?(wtc) → superreview+
(Reporter)

Comment 8

15 years ago
Targeting 3.9 .
Priority: -- → P1
Target Milestone: --- → 3.9
(Reporter)

Comment 9

15 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 10

15 years ago
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.
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.   
Summary: Default SSL client auth callback NSS_GetClientAuthData does not work with dual-key certs → NSS_GetClientAuthData does not work with dual-key certs

Comment 12

12 years ago
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.
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
Summary: NSS_GetClientAuthData does not work with dual-key certs → NSS_GetClientAuthData does not work with limited usage certs

Comment 14

12 years ago
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.
You need to log in before you can comment on or make changes to this bug.