221638 - NSS fails NIST tests for certs with DSA signatures

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

NSS fails PKITS tests 4, 5, and 6 of section 4.1.  
Those tests all use certs with DSA signatures.  
All tests report error -8102, inadequate key usage.

Assuming this is not an error in the test setup somehow, this is apparently a
regression.

This bug is against the "libraries" component of NSS, under the assumption
that this is not an error in the test setup.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

The test commands are:

Case 4:

vfychain -d d:/tmp/pkits -u 4 \
  ValidDSASignaturesTest4EE.crt \
  DSACACert.crt \
  TrustAnchorRootCertificate.crt

Case 5: 

vfychain -d d:/tmp/pkits -u 4 \
  ValidDSAParameterInheritanceTest5EE.crt \
  DSAParametersInheritedCACert.crt \
  DSACACert.crt \
  TrustAnchorRootCertificate.crt

Case 6: 

vfychain -d d:/tmp/pkits -u 4 \
  InvalidDSASignatureTest6EE.crt 
  DSACACert.crt \
  TrustAnchorRootCertificate.crt

These tests all pass, as far as I can tell.  
The reason they failed before is that the -u 4 argument was not given.

However, in researching this, I found a related but not causal bug in 
NSS, and will attach a patch for it here.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

This patch adds dsaKey to the list of key types that require Key Agreement
usage, rather than Key Encipherment usage.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Add reviewers to CC list.

Comment 4

[Robert Relyea]

Comment on attachment 139173 [details] [diff] [review]
patch v1

I like the change from if to switch, but I'd like to understand way we are
trying to do a key exchange with a DSA key?

On the other hand the required key Usage shouldn't be KU_KEY_ENCIPHERMENT
either.

It seems that dsaKeys should always fail if KU_KEY_AGREEMENT_OR_ENCIPHERMENT is
specified.
The only exception would be a dual use key (dsa/dh). If we support this
combination, then this patch would be fine.

bob

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Here's the story.  

We're verifying a cert chain from an SSL server for validity for SSL server auth.
The cert contains a DSA public key.  

NSS is asking whether the CERT is good for KU_KEY_AGREEMENT_OR_ENCIPHERMENT
(which is what it always checks for SSL server auth), but there is no 
agreement-or-encipherment extended key usage.  There is key agreement, and 
there is key encipherment.  NSS decides to check for one or the other of those
extended keys usages in the cert, based on the type of key.  

Perhaps the test should be:
  If keyType == keyRSA, 
     then check for encipherment, 
     else check for agreement.
because AFAIK, the only type of public key with which we do key encipherment
is RSA.  

The way that SSL/TLS use a signature only type key (such as DSA) is to sign
a temporal key agreement key, such as a DH key.  That key and the signature
on it are sent in the server key exchange.  the DSA (or whatever) signature
validates the DH (or whatever) key in the server key exchange, and so it 
part of the larger key agreement operation.  

Does that change your mind about this patch?  

Would you prefer that the patch work as suggested by the above pseudo code?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139173 [details] [diff] [review]
patch v1

Bob, please re-review in light of my most recent comments in this bug.
I will also add another attachment with an alternative implementation.
Perhaps you can choose one.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 - bigger, more elegant (?)

This is an alternative to patch v1.  I like this one better.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139287 [details] [diff] [review]
patch v2 -  bigger, more elegant (?)

Bob, please review this patch too, and if possible, choose one of the two
patches to this bug.

Comment 9

[Robert Relyea]

Comment on attachment 139287 [details] [diff] [review]
patch v2 -  bigger, more elegant (?)

I like this version better.  A couple of comments:

1) There is a function called 'CERT_GetCertKeyType' which does essentially the
same thing as CERT_ExtractPublicKeyType, except it doesn't require actually
extracting the whole Public Key, and it takes a PublicKeyInfo rather than a
Certificate.

2) it seems to me that in the case where we signed a key, that we should be
looking for Signature, not KeyAgreement or KeyEncipherment. This patch is
better than the current code, which would just assume keyEncipherment for the
DSA key.

This leaves the following question, though: are either of the following cases
possible (or will the every be possible)?

1) DH algorithm where the DH key is signed by RSA.
2) RSA algorithm where the RSA key is signed by a DSA cert.

If either of these are acceptable, then it seems we SSL needs to determine if
the key is a signature key and check for that extension.

Since this is an SSL question, this patch is acceptable to me as is.

bob

Comment 10

[Robert Relyea]

Comment on attachment 139173 [details] [diff] [review]
patch v1

already granted '+' to v2

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v3

This patch incorporates Bob's feedback, and also the information about 
key usage extensions on page 38 of RFC 2246 (TLS 1.0).

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139633 [details] [diff] [review]
patch v3 

Bob, please review.

Comment 13

[Robert Relyea]

Comment on attachment 139633 [details] [diff] [review]
patch v3 

r=relyea, yes, this patch looks good.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

patch v3 checked in.  
Thanks for the reviews.

Comment 15

[Bishakha Banerjee]

Marking VERIFIED.

DSA Signatures tests pass. Test 4.1.4, 4.1.5 and 4.1.6 of the NIST PKITS test suite.