454437 - CERT_IsCACert should return PR_FALSE for X.509v3 certs with no extensions

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P5)

Description

[Kai Engert (:KaiE:)]

Attached to bug 453463 is attachment 337161 [details].
It lists a minimal cert, no extensions.

For that cert, function CERT_IsCACert returns PR_TRUE, meaning.

This bug suggests to change the behaviour and the function should return PR_FALSE.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

The rules for X.509v3 certs a re different than for v1 certs. 
v1 certs, of course, have no extensions.  
But v3 certs without extensions clearly should not be treated as CAs.

Comment 2

[Julien Pierre]

Kai,

I looked at this particular cert you referenced.
It is a v3 cert with no extensions.
Its subject and issuer fields are identical, ie. it is self-signed.
There is no AKID nor SKID, nor basic constraints.

Based on the fact that this is a v3 cert, and is lacking basic constraints, CERT_IsCACert should return PR_FALSE.

I traced through the code and what happens is that :

a) cert_IsRootCert considers it a root cert and sets the isRoot field to PR_TRUE.

b) CERT_IsCACert checks isRoot field, and assumes it is a CA cert because it is a root cert.

I am not certain where the fix belongs.

What cert_isRootCert does is really ask the question - is this cert self-signed ? That's the meaning of the "isRoot" flag. But that's not sufficient to make the cert a CA cert.

RFC3280 says that self-signed certs are allowed to omit the AKID extension, and thus cert_IsRootCert properly doesn't prevent this omission of AKID from considering the cert a root. This would not be a common case, but it is legal.

That's also legal for v1 certs.

I think the bug is that CERT_IsCACert automatically assumes that all certs that have the isRoot flag are also CAs. This is only true for v1 certs. v3 certs that are self-signed (and thus have isRoot) 

I think my recommendation would be for CERT_IsCACert to check the x509 version of the cert and do some different processing there.

Perhaps it's a simple as changing the code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/certdb.c&rev=1.92#2174
and add a v1 check in that if (cert->isRoot) test.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Before making any change, someone should review all the certs in nssckbi 
to see if there are any v3 certs that lack basic constraints there.

The proposed changes risk a break in binary compatibility, if for no other
reason than that the error code would be different when validating a cert 
whose root is a v3 without Basic Constraints.  We need to be sure that 
any proposed change will not break customers for which the present code
seems to be working without error.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Kai, I have attached a patch (attachment 348155 [details] [diff] [review]) to bug 464406.  
It contains a rewrite of CERT_IsCACert.  I think it addresses this bug.
Would you test it for me with PSM and let me know if the results are 
satisfactory and as expected?

Comment 5

[Benjamin Beurdouche [:beurdouche]]

Bob, could you please check and eventually update the triaging for this bug ? : ) Thanks !

Comment 6

[Robert Relyea]

Nelson's patch from comment 4 has been applied 15 years ago, and subsequently rewritten. We actually no longer handle v1 CACerts in NSS, so this patch is long fixed. Nelson would have closed it, but he was looking for verification first. I'm closing it now.