391454 - libPKIX does not honor NSS's override trust flags

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Steve Parkinson]

libpkix fails to validate the paypal cert chain.

The failure occurs in pkix_CertSelector_Match_BasicConstraint.

Full stack trace:

#0  pkix_CertSelector_Match_BasicConstraint (params=0x89aa88c, cert=0x89aebbc, pResult=0xbfa59b20, 
    plContext=0x89505f0) at pkix_certselector.c:189
#1  0x008e33f4 in pkix_CertSelector_DefaultMatch (selector=0x89a8834, cert=0x89aebbc, pResult=0xbfa59bc0, 
    plContext=0x89505f0) at pkix_certselector.c:1351
#2  0x00b170da in pkix_pl_Pk11CertStore_GetCert (store=0x8950d84, selector=0x89a8834, 
    pNBIOContext=0xbfa59c50, pCertList=0xbfa59c58, plContext=0x89505f0) at pkix_pl_pk11certstore.c:532
#3  0x009a9db3 in pkix_Build_GatherCerts (state=0x89aa79c, certSelParams=0x89aa88c, pNBIOContext=0xbfa59d20, 
    plContext=0x89505f0) at pkix_build.c:2109
#4  0x009af340 in pkix_BuildForwardDepthFirstSearch (pNBIOContext=0xbfa59ea8, pState=0xbfa59e4c, 
    pValResult=0xbfa59e5c, plContext=0x89505f0) at pkix_build.c:2667
#5  0x009c03de in pkix_Build_InitiateBuildChain (procParams=0x8950bf4, pNBIOContext=0xbfa59f80, 
    pState=0xbfa59f88, pBuildResult=0xbfa59f84, pVerifyNode=0x0, plContext=0x89505f0) at pkix_build.c:4098
#6  0x009c1ccd in PKIX_BuildChain (procParams=0x8950bf4, pNBIOContext=0xbfa5a00c, pState=0xbfa5a008, 
    pBuildResult=0xbfa5a010, pVerifyNode=0x0, plContext=0x89505f0) at pkix_build.c:4281



The cert in question chains to:

"Builtin Object Token:Verisign Class 3 Public Primary Certification Authority"

which is a x509v1 cert without a basic constraints extension (or any other
extension.

Comment 1

[Steve Parkinson]

Attachment: Treat x509v1 certs as valid for issuing basic constraints

This patch treats as x095v1 cert as good for issuing certs, even though it does not (and cannot) have a basic constraints.

The change to PKIX_PL_Cert_GetVersion was to fix a crash when reading the
version number out of nssCert->version, since nssCert->version.len == 0 in the
case of a x509v1 cert. (perhaps that should be fixed elsewhere)

Comment 2

[Steve Parkinson]

Attachment: Treat x509v1 certs as valid for issuing basic constraints

This patch treats as x509v1 cert as good for issuing certs, even though it does not (and cannot) have a basic constraints.

The change to PKIX_PL_Cert_GetVersion was to fix a crash when reading the
version number out of nssCert->version, since nssCert->version.len == 0 in the
case of a x509v1 cert. (perhaps that should be fixed elsewhere)

Comment 3

[Steve Parkinson]

Comment on attachment 275873 [details] [diff] [review]
Treat x509v1 certs as valid for issuing basic constraints

oops - double posted

Comment 5

[Alexei Volkov]

Comment on attachment 275874 [details] [diff] [review]
Treat x509v1 certs as valid for issuing basic constraints

One comment: I think the code of PKIX_PL_Cert_GetBasicConstraints should also be modified to make sure that only if CERT_FindBasicConstraintExten returns SEC_ERROR_EXTENSION_NOT_FOUND , pkix would claim that it has not found an extensions. The library should report an error in all other cases.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275874 [details] [diff] [review]
Treat x509v1 certs as valid for issuing basic constraints

IMO, we definitely do NOT want to create a blanket exception, allowing
all X.509v1 and v2 certs to be CA certs.  I seem to remember that same thing 
causing a big embarrassment for Microsoft years ago.  Let's not repeat it.

NSS has a trust flag known as "Valid CA".  It means "this cert is a valid
CA cert, even though it lacks any extension that would make it a valid CA".
It is precisely for this purpose.  It is an override.  It overrides the 
content of the cert itself.

AFAIK, at present the only certs for which this is an issue are trusted 
root CA certs.  Of course, no self-issued cert is honored AT ALL unless 
it is explicitly trusted (marked with a trust flag).  

I would recommend modifying PKIX_PL_Cert_GetBasicConstraints so that it
checks the trust flags, and if the VALID_CA or TRUSTED_CA trust flag is 
set, it then returns a value that says "yes, this is a CA cert", even if
the actual basic constraints is absent or says it's not a CA.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

NSS presently defines ten "trust flags".  Individual flags are used to 
perform one or more of the following classes of functionality:

1)Indicate a trust anchor (trusted CA) or trusted peer (not a CA)
  These flags stop the process of path building, because they indicate
  that the cert is the top of a chain.

  The trust anchor flags include
  - CERTDB_TRUSTED            (means trusted peer, not trusted CA)
  - CERTDB_TRUSTED_CA         (is a trust anchor, a CA)
  - CERTDB_NS_TRUSTED_CA      (equivalent to CERTDB_TRUSTED_CA)
  - CERTDB_TRUSTED_CLIENT_CA  (for SSL usage only)

2)error overrides, allowing a cert to be used as valid for some purpose
  even though it may not appear on its face to be valid for that purpose.
  This does not imply trust, and does not stop path building, but allows
  path building to continue past this cert in cases where it would 
  otherwise stop path building with an error at this cert.

  The override flags include:
  - CERTDB_VALID_PEER     (OK to use as an EE cert for this usage, *)
  - CERTDB_VALID_CA       (OK to use as a  CA cert for this usage, *)
  Note (*): if and only if the cert chains up to a trust anchor.
  ALL the trust anchor flags are also override flags.

3)UI behavior indicators, such as flags that say not to display the cert
  in lists of certs, or to warn the user before relying on a chain using
  the cert.  These include:
  - CERTDB_SEND_WARN      (warn user before relying on chain with this cert)
  - CERTDB_INVISIBLE_CA   (cert should not appear in cert displays)

4)indicators of other state
  - CERTDB_USER               (means we have the private key for this cert)
  - CERTDB_GOVT_APPROVED_CA   (only meaningful if TRUSTED_CA is also set,
                               only meaningful for SSL, means CA is authorized
                               to issue SSL step up certs.)

libPKIX does not honor the error overrides, at all.  

In a separate comment, to follow, I will attempt to describe exactly what 
each override flag means.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

A slight clarification:  
CERTDB_NS_TRUSTED_CA is not used by NSS internally, and is deprecated.  
Applications should use CERTDB_TRUSTED_CA instead.

Likewise, the UI indicators are not, and never have been, used by NSS
internally.  They exist to benefit the browser.  Whether the browser 
makes any use of them is unknown (to me).

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

PKIX_PL_Cert_IsCertTrusted checks the wrong flag.
It checks CERTDB_VALID_CA, and treats any cert with that flag as trusted.
That's a P1 bug.  Let's fix it as part of this bug.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

pkix_pl_Pk11CertStore_CheckTrust checks two flags as indicators of trust.
It checks the wrong flags.  It checks to see if either of these flags is set: CERTDB_TRUSTED_CA | CERTDB_VALID_CA
Another P1.

Comment 11

[Steve Parkinson]

What do you think about how it uses ((PKIX_PL_NssContext*)plContext)->certificateUsage

It seems really odd to get the usage flags from the nss context like that,
especially since it was set up in PKIX_PL_Initialize() like this:

        PKIX_CHECK(PKIX_PL_NssContext_Create
                (0x10, useArenas, NULL, &plContext),
                PKIX_NSSCONTEXTCREATEFAILED);

The 0x10 constant is what gets put in certificateUsage, but this seems
to map to certificateUsageEmailSigner. Does that make any sense?

Comment 12

[Alexei Volkov]

Global PKIX_PL_NssContext should not be used for validation session. Each validation attempt should create new context and set required certificate usage in it.

But there are still issues reported in bug 391244.

Comment 13

[Kai Engert (:KaiE:)]

It took me quite a bit of time to produce a testing environment.
I failed to use vfyserv, because it has problems looking up DNS.
I failed to use "pkixutil validate_chain", because it crashes.
I failed to compile the pkixutil tests with BUILD_LIBPKIX_TESTS and the patches from 390888 and 390502 applied, I got syntax errors and multiple defined symbols, so I gave up.

I was able to do the following:
- use ssltap to and "client" to dump out the certs and intermediates used by www.paypal.com
- used certutil to import paypal cert and the intermediates with empty trust ",,"

With this setup, I can test the chain with:
$ certutil -d . -V -n paypal -u V
certutil: certificate is valid

Now I wanted to test using the paypal code.
I figured I need to apply the patch from bug 390888.
I got errors complaining about CRLs, so I figured I also need the patch from bug 390502.

I merged both patches to the trunk and made them coexist.
For testing this bug, I used:
- bug 390888 patch v4 c
- bug 390502 patch v1 b

With this setup, I can use an environment variable to enable PKIX verification code. Now I get this:
$ NSS_ENABLE_PKIX_VERIFY=1 certutil -d . -V -n paypal -u V
BUILD ERROR:
*** CERTVFYPKIX Error- cert_BuildAndVerifyChain: Unable to build chain
*** Cause (2): *** BUILD Error- PKIX_BuildChain: pkix_Build_InitiateBuildChain failed
*** Cause (3): *** BUILD Error- pkix_Build_InitiateBuildChain: Unable to build chain
certutil: certificate is invalid: This certificate is not valid.


Now, have I reproduced Steve's environment?
Steve reported the chain can not be verified.
But in my test case, the chain fails earlier, at chain build time???

Have I found a valid starting point for looking at the code?

Comment 14

[Kai Engert (:KaiE:)]

(In reply to comment #13)
> Now I wanted to test using the paypal code.

libpkix verification code...


> But in my test case, the chain fails earlier, at chain build time???

the verification fails earlier... need to proofread before hitting submit...

Comment 15

[Kai Engert (:KaiE:)]

Ok, I understand that Steve's latest patch is not what we want, but I tried it, and with his patch applied, I no longer get a verification failure. I get a positive verification result.

So I conclude I have a good testing environment.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Kai wrote:
> I failed to use vfyserv, because it has problems looking up DNS.

I have never had any DNS troubles with it.  If you have, then
Please file a bug explaining (documenting) the failures.  Thanks.

Comment 17

[Kai Engert (:KaiE:)]

(In reply to comment #6)
> I would recommend modifying PKIX_PL_Cert_GetBasicConstraints so that it
> checks the trust flags, and if the VALID_CA or TRUSTED_CA trust flag is 
> set, it then returns a value that says "yes, this is a CA cert", even if
> the actual basic constraints is absent or says it's not a CA.  


PKIX_PL_Cert_GetBasicConstraints produces a data structure, which other functions operate on. That data structure is then cached for later operations. Is it a good idea to synthesize such a data structure based on our overrides?

I think I'd prefer a slight variation of your proposal:

- Let's keep that function's current behaviour, which is to 
  return the raw data.

- Instead, let's modify the function that makes decisions 
  based on the constraints:
  pkix_CertSelector_Match_BasicConstraint

Comment 18

[Kai Engert (:KaiE:)]

(In reply to comment #16)
> Kai wrote:
> > I failed to use vfyserv, because it has problems looking up DNS.
> 
> I have never had any DNS troubles with it.  If you have, then
> Please file a bug explaining (documenting) the failures.  Thanks.

Filed bug 394013

Comment 19

[Kai Engert (:KaiE:)]

(In reply to comment #9)
> PKIX_PL_Cert_IsCertTrusted checks the wrong flag.
> It checks CERTDB_VALID_CA, and treats any cert with that flag as trusted.
> That's a P1 bug.  Let's fix it as part of this bug.

Nelson, is it sufficient to simply replace the flag?

@@ -3204,7 +3256,7 @@ PKIX_PL_Cert_IsCertTrusted(
         if (rv == SECSuccess) {

                 certFlags = SEC_GET_TRUST_FLAGS((&trust), trustType);
-                if ((certFlags & CERTDB_VALID_CA) &&
+                if ((certFlags & CERTDB_TRUSTED_CA) &&
                     ((certFlags & requiredFlags) == requiredFlags)) {
                         trusted = PKIX_TRUE;
                 }

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 17 (which is a reply to comment 6):
Kai, I don't know how many callers to PKIX_PL_Cert_GetBasicConstraints
exist, but it occurs to me that if we DO change it to synthesize a 
basic-constraints based on flags, then we don't need to change any of
its callers.  OTOH, if we don't change that function, then we must 
potentially change all its callers.  I'm in favor of fixing this in 
the way that requires the least number of functions to be changed.
I believe that approach minimizes complexity.

Comment 21

[Kai Engert (:KaiE:)]

(In reply to comment #10)
> pkix_pl_Pk11CertStore_CheckTrust checks two flags as indicators of trust.

It also seems to use an imprecise definition of trust. It seems to say "if this cert is trusted for any of ssl/email/object-signing, then it's trusted"...


> It checks the wrong flags.  It checks to see if either of these flags is set:
> CERTDB_TRUSTED_CA | CERTDB_VALID_CA
> Another P1.

You're not saying which would be the right flags, so I'm trying to deduce from your other comments.

Given the interface of this function, I conclude, "any trust bit is sufficient for a positive result", and I'm proposing this change:


@@ -84,7 +84,9 @@ pkix_pl_Pk11CertStore_CheckTrust(
         PKIX_NULLCHECK_THREE(store, cert, pTrusted);
         PKIX_NULLCHECK_ONE(cert->nssCert);

-        trustedValues = CERTDB_TRUSTED_CA | CERTDB_VALID_CA;
+        trustedValues = CERTDB_TRUSTED_CA
+                        | CERTDB_TRUSTED_CLIENT_CA
+                        | CERTDB_TRUSTED_PEER;

         PKIX_CERT_DEBUG("\t\tCalling CERT_GetCertTrust).\n");
         rv = CERT_GetCertTrust(cert->nssCert, &nssTrusted);

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 19:
Function CERT_TrustFlagsForCACertUsage returns the required set of flags.
The correct patch for this code is:
         if (rv == SECSuccess) {

                 certFlags = SEC_GET_TRUST_FLAGS((&trust), trustType);
-                if ((certFlags & CERTDB_VALID_CA) &&
-                    ((certFlags & requiredFlags) == requiredFlags)) {
+                if ((certFlags & requiredFlags) == requiredFlags) {
                         trusted = PKIX_TRUE;
                 }

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

I wonder:  Why is the world do we have BOTH functions
pkix_pl_Pk11CertStore_CheckTrust and PKIX_PL_Cert_IsCertTrusted ?
This seems really wrong.

In reply to comment 21:
The function is supposed to return an answer about trust.
Despite being called a "trust flag", CERTDB_VALID_CA does not imply trust.

RFC 3280 has no concept of being trusted for one purpose but not another.
In its model, there is a set of trusted certs, called the trust anchors.
Its algorithm presumes that all the trust anchors provided as inputs 
are trusted for whatever purpose (usage) it is validating.  IOW, it 
presumes that the caller has pre-selected the trust anchors to include 
only the ones that are trusted for the caller's purpose.  In that model,
trust is purely binary.  A cert either is a trust anchor, or it's not.

That's undoubtedly why pkix_pl_Pk11CertStore_CheckTrust ORs together the 
trust flags from all 3 trust categories.  But that's seriously incorrect, 
and it will cause CAs that issue DV SSL certs to also effectively be Code 
signing CAs. So, this function must be changed.  This is a P1 issue.

Perhaps a new input parameter needs to be added.  The function needs to 
be told enough information that it can determine which trust flags need
to be checked, and in which category.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

I don't know who are all the callers of pkix_pl_Pk11CertStore_CheckTrust,
so I don't know if they ever check for EE cert trust.  But it is wrong
to assume that the CERTDB_TRUSTED_CA flag is always the right one.
The function CERT_TrustFlagsForCACertUsage returns the required set of flags.
pkix_pl_Pk11CertStore_CheckTrust should call it.  Then one really must wonder
why we have two nearly identical functions:
pkix_pl_Pk11CertStore_CheckTrust and PKIX_PL_Cert_IsCertTrusted

Comment 25

[Kai Engert (:KaiE:)]

(In reply to comment #20)
> In reply to comment 17 (which is a reply to comment 6):
> Kai, I don't know how many callers to PKIX_PL_Cert_GetBasicConstraints
> exist, but it occurs to me that if we DO change it to synthesize a 
> basic-constraints based on flags, then we don't need to change any of
> its callers.  OTOH, if we don't change that function, then we must 
> potentially change all its callers.

There are multiple callers. If the trust override should be effective whenever someone asks for the basic constraint extensions, then I agree it makes sense to create a synthetic constraint extensions.

But, what happens if someone is really interested in the list of explicit constraints? Will we cause side effects by producing a synthetic extension?


> I'm in favor of fixing this in 
> the way that requires the least number of functions to be changed.
> I believe that approach minimizes complexity.

Let's assume we really want to produce the synthetic extension: Should we do it one stop above PKIX, and do it in CERT_FindBasicConstraintExten?

Comment 26

[Kai Engert (:KaiE:)]

(In reply to comment #23)
> That's undoubtedly why pkix_pl_Pk11CertStore_CheckTrust ORs together the 
> trust flags from all 3 trust categories.  But that's seriously incorrect, 
> and it will cause CAs that issue DV SSL certs to also effectively be Code 
> signing CAs. So, this function must be changed.  This is a P1 issue.
> 
> Perhaps a new input parameter needs to be added.  The function needs to 
> be told enough information that it can determine which trust flags need
> to be checked, and in which category.

If you really want to add a new parameter, I guess this should be done soon. The interface of that function is defined as a callback-function-type, and there is code which stores a function pointer. I conclude all code that stores that function pointer will have to get extended to store/specify that additional parameter...

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 25, 
All the PKIX_PL_* functions should only be called by libPKIX itself.  
I think it is OK for all libPKIX functions to see the synthesized extension.

In contrast, CERT_FindBasicConstraintExten is used by code in NSS and in 
applications to inquire (among other things) if the extension is really 
there, or not (e.g. for displaying extensions) and IMO we don't want to 
display fake extensions in that case.  So I'd suggest that we synthesize the 
extension in the PKIX_PL_ function and not in CERT_FindBasicConstraintExten.

Comment 28

[Kai Engert (:KaiE:)]

I think this bug becomes overloaded and I have trouble to see where I should start.
I am filing invidiual bugs for the several separate problems that we have identified.

Bug 394182: Eliminate pkix_pl_Pk11CertStore_CheckTrust
            or        PKIX_PL_Cert_IsCertTrusted

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Perhaps this bug should become an umbrella bug, tying the other related
bugs together.  I think you're already doing that, which is good.

Comment 30

[Kai Engert (:KaiE:)]

(In reply to comment #29)
> Perhaps this bug should become an umbrella bug, tying the other related
> bugs together.  I think you're already doing that, which is good.


Filed bug 394182 for the "eliminate one of the trust functions".
But I believe we want that bug invalid.
Follow up there.

Bug 391595 is for testing the right trust flags in pkix_pl_Pk11CertStore_CheckTrust
Follow up there.

Bug 394199 is for a bug for always seeting out params in the trust function.
Follow up there.

Bug 394206 is for testing the right trust flags in PKIX_PL_Cert_IsCertTrusted.
Follow up there.


I believe I have produced separate bugs for all problems mentioned in this one, so I can return to the original problem now...

Comment 31

[Kai Engert (:KaiE:)]

Attachment: Patch v2

I agree with Nelson's proposal to create a synthetic basic constraint within function PKIX_PL_Cert_GetBasicConstraints, which will only be visible to callers from within libPKIX.

Nelson proposed we check the trust flags, and if we find a trusted_ca/valid_ca flag, then we conclude it's a CA.

I assume that we want to call CERT_GetCertTrust.

I assume we want to look at any of the usages. That is, if any of the ssl/email/object-signing usages has the CA flag, we decide it's a CA cert.

I assume, if we decide it's a CA based on trust flags, we want to allow any path len and set it to unlimited.

I assume, if we don't find any CA flags, then we will not create a synthetic basic constraint.

This patch implements that proposal.


In my tree, in addition to the patch from this bug, I have patches for the following other bugs applied:

- 394206
- 394199
- 391595
- 390888
- 390502


My test case produces a "valid CA" result with this combination of patches.


If anybody of you wonders how I maintain that many patches in my tree, I recently found and learned to use the "quilt" tool, which is a real help for mental sanity.

Comment 32

[Kai Engert (:KaiE:)]

Attachment: Patch v3

renamed BS to BC to better describe Basic-Constraint

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278837 [details] [diff] [review]
Patch v3

This is not a complete review, but there is one problem that must be 
corrected that will probably cause a big change to this patch, so 
I'll review the next patch in more depth.

>+                                int anyWantedFlag = CERTDB_TRUSTED_CA | CERTDB_VALID_CA;
>+                                if ((trust.sslFlags & anyWantedFlag) 
>+                                    || (trust.emailFlags & anyWantedFlag) 
>+                                    || (trust.objectSigningFlags & anyWantedFlag)) {

The criteria for being a valid code signing CA are (presently) different (more 
restrictive) than for the other types.  It is quite reasonable for a CA cert to have
the valid override flag set for SSL and SMIME but not for code signing.
Therefore, we really need to know what type of CA we're looking for here in order to 
decide if the _relevant_ trust flag is set.

>+                }
>+                else {

Please collapse that into a single line, as is the style elsewhere in this file
(and in this patch).

Comment 34

[Kai Engert (:KaiE:)]

Nelson, I think I disagree and I would like to ask you reconsider. I will also try to chat to you directly.


This function is "get me the basic constraint extension".

If I understand correctly, the purpose of this extension is "find out if this cert is technically a CA cert".

This function will not make any statements about trust.


You also say, it is harder for a CA to get a the object-signing-trust flags.

I think you are worried that a caller might be interested in a object-signing-CA, but with the proposed patch we would return "yes this is a CA cert" even if there is only the ssl-CA flag set.

But why is that a problem?

This function does not claim any trust.


I think we are looking for the equivalent of saying "this cert has been marked by the issuer as a valid CA cert".

You had proposed we accept our own trust as an override to that, and IMHO, as soon as we have assigned our own trust for any CA, I think this means, we declare the cert as a CA cert.


I looked at the x.509 standards document ISO/IEC 9594-8:0205 section 8.4.2.1
It says:
"This field indicates if the subject may act as a CA, with the certified public key being used to verify certificate signatures."

It does not make any claims about what kind of certificates may be validated.


I believe it is NOT the responsibility of the get-basic-constraint function to check usages.


Finally, we were discussing that we want to cache the synthesized basic constraint. Now, if that synthesized extension were derived from a particular usage, wouldn't it be bad to cache it? A future call might ask for a different usage and thus, if the extension were derived from the usage, the cached might be incorrect. (But I still believe we want the extension to be independent of usage.)


Nelson, please reconsider after reading this comment.
But maybe I was completely missing your point?

Comment 35

[Kai Engert (:KaiE:)]

Attachment: Patch v4

New in this patch:

- now on a single line: } else if { 

- moved the "unlock" call to the very end of the function.

Some more comments on my changes around locking:

First, I find the unlock code difficult to read. I find it confusing that  PKIX_OBJECT_LOCK(cert) and PKIX_OBJECT_UNLOCK(lockedObject) seem to unlock the same object. I think it would be much more readable if the call to PKIX_OBJECT_UNLOCK(lockedObject) would also use "cert" as the argument. I changed that.

Second, I removed that unlock call in the middle of the function. I noticed this code keeps the lock while calling into NSS anyway, so I decided to keep that logic even when calling two NSS functions.

I think it's necessary to have all that logic happen while we are locked, even the final assignment to the output parameter. We must ensure no other thread changes cert->certBasicConstraints before we have assigned it to the out parameter.

In addition, based on my understanding of common multithreading, we must increment the reference counter while holding the lock, too. If we don't, another thread might decide to dereference it, arrive at zero and free the object...

This function does not yet address Nelson's request about checking-for-usage, as we are still discussing.

Comment 36

[Kai Engert (:KaiE:)]

Nelson, maybe your point is: "There are other places where libPKIX must look at the correct trust overrides."

Maybe you are right, but so far I have not yet tried to identify and fix those other places. In order to fix the original issue with x.509 v1 certs, it is sufficient to fix the is-general-CA-cert question.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Kai, I gather that your comment 36 was a reply to my comment 33.
Sorry if my point wasn't clear.  It was that this code:

>+                 if ((trust.sslFlags & anyWantedFlag) 
>+                  || (trust.emailFlags & anyWantedFlag) 
>+                  || (trust.objectSigningFlags & anyWantedFlag)) {

is wrong because it treats all 3 classes of usage as interchangeable.
It says, in effect, "if this cert is a valid CA for any of these 
usages, then it is a valid CA for all of them."  That's wrong.  

I saw some code that does this the right way in a patch for another bug
recently.  I'm trying to find that.  

I know the use of "locked object" seems strange, but it's right. 
There's some very subtle stuff in there.  Please leave it as "locked object"
for now, so it will be consistent with the rest of libPKIX.  If we decide
to change that logic, we should do so for all of libPKIX and not only for
this one function.  (this is not a full review of this patch.)

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279101 [details] [diff] [review]
Patch v4

Here's the wrong code:

>+            CERTCertTrust trust;
>+            rv = CERT_GetCertTrust(nssCert, &trust);
>+            if (rv == SECSuccess) {
>+                int anyWantedFlag = CERTDB_TRUSTED_CA | CERTDB_VALID_CA;
>+                if ((trust.sslFlags & anyWantedFlag) 
>+                 || (trust.emailFlags & anyWantedFlag) 
>+                 || (trust.objectSigningFlags & anyWantedFlag)) {

The right code would replace those last 3 lines with something like this:

+                unsigned int certFlags;
+                certFlags = SEC_GET_TRUST_FLAGS((&trust), trustType);
+                if (certFlags & anyWantedFlag)) {

Of course, it needs "trustType" which is missing in this function,
but can be derived, I think, from available information.  
It is output by a call to 
+    rv = CERT_TrustFlagsForCACertUsage(certUsage, &requiredFlags, &trustType);
which requires certUsage. I'm pretty sure you can get certUsage here.

The lines above are derived from the patch v2 for bug 391595.

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279101 [details] [diff] [review]
Patch v4

OK, comment 37 and comment 38 constitute my full 
review of this patch.

Comment 40

[Kai Engert (:KaiE:)]

I have decided that I will implement Nelson's proposal from comment 36 and 37, and I will attach a patch now.


But I I would appreciate a response to my comment 34.
I would like to understand why my assumptions in that comment are wrong.


Basically my point is, an x509v3 cert can have a basic constraint extension, and this does not mean anything for trust. Only additional separate trust will enable the cert to be trusted.

That's the reason why I think: A synthetic basic constraint means nothing wrt trust. It only causes other functions checking for the basic constraint not to fail. The other functions will still check the trust - I hope!

Comment 41

[Kai Engert (:KaiE:)]

The decision to base the "synthetic basic constraint" on the cert usage means: A synthetic basic constraint can not be cached.

Comment 42

[Kai Engert (:KaiE:)]

The existing old code in PKIX_PL_Cert_IsCertTrusted, 
the recently added code in pkix_pl_Pk11CertStore_CheckTrust,
and the new code I'm adding in this patch do the following:

if CERT_TrustFlagsForCACertUsage fails,
they'll conclude: requiredFlags = 0 and trustType = trustSSL

Is this really a good idea?
Or should all 3 functions pass the failure up to the caller?

Comment 43

[Kai Engert (:KaiE:)]

Attachment: Patch v5 - ignore whitespace in this patch - easier to review

This is a
  cvs diff -w
patch that ignores changes to the whitespace.

I personally find this easier to review, because it lists less changes to the code.

Please do not care for whitespace in this patch.

Comment 44

[Kai Engert (:KaiE:)]

Attachment: Patch v5 - real patch

This is the "real" patch that includes the changes to whitespace.
I find this more confusing to review, because it lists changes that are caused by whitespace changes.

Comment 45

[Kai Engert (:KaiE:)]

Comment on attachment 279619 [details] [diff] [review]
Patch v5 - real patch

I'm requesting review on the real patch, but you might prefer to read the whitespace-ignore patch version.

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

Kai, your comment 41 (with which I agree) makes me think that now is the 
time to drop that old non-standard definition of "valid CA" for object
signing certs, and think it more strongly than before.  

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Kai, In answer to the question of comment 42:
You're right.  If the usage is unrecognized, we should not silently change
it to mean SSL server usage.

Comment 48

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279619 [details] [diff] [review]
Patch v5 - real patch

In Thursday's meeting, we agreed to change our policy on valid CAs,
and stop requiring extra extensions to indicate valid CAs for issuers
of object signing certs.  Consequently, patch v4 is much closer to 
what we want than patch v5.

Comment 49

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279101 [details] [diff] [review]
Patch v4

The only remaining issue with patch v4 is this change:

-	PKIX_OBJECT_UNLOCK(lockedObject);
+	PKIX_OBJECT_UNLOCK(cert);

Undo that change, and then patch v4 will be r+

Comment 50

[Kai Engert (:KaiE:)]

Attachment: Patch v4 b

Thanks for the review Nelson. This is patch v4 plus the requested change removed. I'll check it in asap.

Comment 51

[Kai Engert (:KaiE:)]

fixed on trunk

/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c,v  <--  pkix_pl_cert.c
new revision: 1.10; previous revision: 1.9

Comment 52

[Kai Engert (:KaiE:)]

Attachment: Addon patch v5

I see that I have not yet addressed my comment 42 and Nelson's comment 47.
Probably because my latest patch does no longer call function CERT_TrustFlagsForCACertUsage...

In fact, I can see only a single occurrence in the code that calls it.

Do you want this change?

Comment 53

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 281265 [details] [diff] [review]
Addon patch v5

Yes, I think this is better.  Even better might be to "throw" an error, telling the user "you asked me to do the impossible".

Comment 54

[Kai Engert (:KaiE:)]

Attachment: Addon patch v6

Sorry, patch v5 was against an older snapshot of the trunk, this is why I had missed the second place where we draw the same bad conclusion.

This patch addresses both occurrences.

Nelson, you're proposing to possibly use throw to signal the error to the caller. I'm not sure how this would be done. I was looking through the NSS sources and found a macro named PKIX_THROW, but to my surprise, I found only a single place where this is being called. And what surprises me even more, it refers to a symbol PKIX_OBJECTWITHNONPOSITIVEREFERENCES which doesn't seem to be defined anywhere in the tree.

Comment 55

[Kai Engert (:KaiE:)]

Nelson, this bug is already resolved, but do you want addon patch v6?
If yes, please review, thanks!

(You already had reviewed v5, but another change was necessary)

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

reopening.  This bug still has unreviewed patches.

Comment 57

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 281290 [details] [diff] [review]
Addon patch v6

It appears that this patch will have the effect that if the requested usage is unknown, the answer for "is it trusted" will always be false.  That seems right to me.
r=nelson

Comment 58

[Kai Engert (:KaiE:)]

Patch v6, attachment 281290 [details] [diff] [review] checked in, marking fixed.