391595 - verify usages of NSS trust flags and overrides in libpkix

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P1)

Description

[Alexei Volkov]

One of noticed problem(from bug 391454):
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.

Another one is to make sure overrides CERTDB_VALID_CA are used for v1 certificates that have no basic constraints extension.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

After some trial-and-error in bug 394182, I'm learning about this bug.

I'm learning that PKIX_PL_Cert_IsCertTrusted has a better way to obtain the right flags.

Since we already have a dependency between the two functions, I'm cloning the good code into the function which needs to get fixed.

Here's a patch.

Comment 2

[Kai Engert [:KaiE:]]

Attachment: Patch v1 b

Same code, old block removed, patch looks different.
Maybe you'll like this patch better for reviewing.

Comment 3

[Alexei Volkov]

Let me add the comment that I've sent as a patch of discussion over this problem:

 libpkix has notion of a cert store. If a store is capable to verify
cert trust, then it should have a valid function pointer to a trust
validation function. This function for libpkix pkcs11 store is
pkix_pl_Pk11CertStore_CheckTrust.

At the same time each cert also has a function that can be used to
answer the question if the cert has a trust. This function is
PKIX_PL_Cert_IsCertTrusted. It is correctly checks assigned cert
store(store member of PKIX_PL_Cert) of a cert and validates the trust
if store is not NULL.

There are problems in both functions. I think pkix_pl_Pk11CertStore_CheckTrust
should remain to be a main function that validates trust(should have the code
that does validation). The only thing that PKIX_PL_Cert_IsCertTrusted
should be is to call pkix_pl_Pk11CertStore_CheckTrust (through trust
callback) if cert store is assigned.

I also think that trust call back should be expanded and include
SECCertUsage.

Comment 4

[Alexei Volkov]

Kai, do you also plan to change the PKIX_PL_Cert_IsCertTrusted function? If your v1 patch we will have duped code.

I hate that we obtaining required certificate usages through PKIX_PL_NssContext. I think it is ok for now and for this patch, but we should change the way this data get delivered to trust validation function.

Comment 5

[Kai Engert [:KaiE:]]

Let me rename the functions for discussion, since one calls the other.

MAIN: PKIX_PL_Cert_IsCertTrusted

SUB: pkix_pl_Pk11CertStore_CheckTrust


MAIN calls SUB if there is a cert store.


Alexei, let me try to repeat and simplify your proposal:

SUB should do the real work and be correct.
(and my patch in this bug fixes it)

When MAIN executes, when there is a cert store, then it will call SUB.
Then MAIN will end.
MAIN will not check the trust again.
MAIN will use the result of SUB.
Correct?

What should happen if MAIN does not see a cert store?
Then MAIN will check the trust, using the same code?

Comment 6

[Kai Engert [:KaiE:]]

(In reply to comment #4)
> Kai, do you also plan to change the PKIX_PL_Cert_IsCertTrusted function? If
> your v1 patch we will have duped code.

I don't know yet HOW to change the MAIN function (PKIX_PL_Cert_IsCertTrusted).

If you all tell me, only SUB should do the real trust checking, then I agree, we can remove the calls to cert_gettrust etc. from MAIN, and only have SUB do that work.

But as I say in my previous comment, what happens if there is no cert store? 

Maybe I do not see the full picture.
If you do, please make a proposal.


> I hate that we obtaining required certificate usages through
> PKIX_PL_NssContext. I think it is ok for now and for this patch, but we should
> change the way this data get delivered to trust validation function.

I propose to file a bug on this, so that we don't forget.

Comment 7

[Alexei Volkov]

(In reply to comment #5)
> MAIN: PKIX_PL_Cert_IsCertTrusted
> SUB: pkix_pl_Pk11CertStore_CheckTrust
> 
> 
> MAIN calls SUB if there is a cert store.
> 
> 
> Alexei, let me try to repeat and simplify your proposal:
> 
> SUB should do the real work and be correct.
> (and my patch in this bug fixes it)
This correct. We have many cert stores. Cert trust should be verified using a function from a particular store from which a cert was fetched. pkcs11 cert store know about trust of it's certs, and so it should be able to provide an answer. On the other hand, ldap or http cert store don't have such function defined. That actually means they don't know anything about cert trusts. 

> 
> When MAIN executes, when there is a cert store, then it will call SUB.
> Then MAIN will end.
> MAIN will not check the trust again.
> MAIN will use the result of SUB.
> Correct?
I agree.

> What should happen if MAIN does not see a cert store?
IMO, no store no trust. But you may assume, that a cert
was fetched from the local pkcs11 cert store.

> Then MAIN will check the trust, using the same code?
No. Only stores can provide an info about trusts that they have. 

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

> No. Only stores can provide an info about trusts that they have. 

Not true.  NSS's cert DB may have trust for ANY cert, including certs
that are not stored in that store.  The trust in NSS's PKCS#11 module
that uses the cert DB may override all other trust info.  
For example, when a user decides to override the trust on a cert from
nssckbi, the new trust information is stored in the cert DB.  Thereafter
that trust information overrides the trust information in nssckbi 
(which does have its own trust information).  

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 278798 [details] [diff] [review]
Patch v1 b

Ok, after reading your thoughts, we absolutely want my patch (or a derivative), because it changes SUB, which we agree should check the correct trust flags.

In addition we will need a patch for function MAIN. I think we can work on function MAIN as a separate patch, so I'm requesting review for the existing patch to SUB.

Comment 10

[Kai Engert [:KaiE:]]

I still don't see clear how function MAIN should be changed.

Nelson, if I understand your comment correctly, you're saying:

- the cert store may have trust associated

- in addition there may be trust stored elsewhere, 
  that overrides the store (nssckbi as an example)


We formerly said
  "if there is a store, then MAIN calls SUB, and is done".
Alexei agreed to that.

What does the new statement from Nelson mean?

Does it mean:

- if there is a cert store, MAIN calls SUB, and uses SUB's result
  as its preliminary result

- MAIN goes on to look for overrides.
  If it finds an override, it will use it.
  If there is no override, MAIN will return what SUB returned.


I don't know how to do the latter, so I'd like to ask for proposals.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278798 [details] [diff] [review]
Patch v1 b

>-        trustedValues = CERTDB_TRUSTED_CA | CERTDB_VALID_CA;
>+        certificateUsage = ((PKIX_PL_NssContext*)plContext)->certificateUsage;

Kai, I think your logic is correct, PROVIDED that not more than one bit
is set in certificateUsage we get from the plcontext.  Put an assertion here, 
asserting that at most one bit is set, e.g.

    PORT_Assert(!(certificateUsage & (certificateUsage - 1)));

>+        PKIX_PL_NSSCALLRV
>+                (CERT,
>+                rv,
>+                CERT_TrustFlagsForCACertUsage,
>+                (certUsage, &requiredFlags, &trustType));

We're trying to get rid of PKIX_PL_NSSCALLRV.  See bug 394020. 
It's one of several macros that completely obfuscate the code, and render 
mozilla's lxr and mxr tools pretty useless on it.  So, there should be no 
new uses of PKIX_PL_NSSCALLRV in new code, and as you edit code that has 
that macro, please get rid of it.  

>+        PKIX_PL_NSSCALLRV(CERT, rv, CERT_GetCertTrust, (cert->nssCert, &trust));

There it is again.

>+        if (rv == SECSuccess) {

The variable certFlags is only used within this basic block.  
Please move its declaration down into this block.

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

r=nelson when those changes are made.

Comment 12

[Kai Engert [:KaiE:]]

(In reply to comment #11)
> (From update of attachment 278798 [details] [diff] [review])
> >-        trustedValues = CERTDB_TRUSTED_CA | CERTDB_VALID_CA;
> >+        certificateUsage = ((PKIX_PL_NssContext*)plContext)->certificateUsage;
> 
> Kai, I think your logic is correct, PROVIDED that not more than one bit
> is set in certificateUsage we get from the plcontext.  Put an assertion here, 
> asserting that at most one bit is set, e.g.
> 
>     PORT_Assert(!(certificateUsage & (certificateUsage - 1)));


ok, added. Since I have stolen this code from the IsTrusted function, I will make the same change over there, too.


> >+        PKIX_PL_NSSCALLRV
> >+                (CERT,
> >+                rv,
> >+                CERT_TrustFlagsForCACertUsage,
> >+                (certUsage, &requiredFlags, &trustType));
> 
> We're trying to get rid of PKIX_PL_NSSCALLRV.  See bug 394020. 
> It's one of several macros that completely obfuscate the code, and render 
> mozilla's lxr and mxr tools pretty useless on it.  So, there should be no 
> new uses of PKIX_PL_NSSCALLRV in new code, and as you edit code that has 
> that macro, please get rid of it.  
> 
> >+        PKIX_PL_NSSCALLRV(CERT, rv, CERT_GetCertTrust, (cert->nssCert, &trust));
> 
> There it is again.


Ok, both changes made, and again I'm changing the IsTrusted function, too.


> >+        if (rv == SECSuccess) {
> 
> The variable certFlags is only used within this basic block.  
> Please move its declaration down into this block.

Ok, done. I also found two variables that were reported as "unused" by the compiler, that I had copied over from the IsTrusted function. I'm removing these two unnecessary variables.


> r=nelson when those changes are made.

Thanks.
I'll attach my updated patch and check it in.

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Patch v2 [checked in]

Updated patch as described in my previous comment.

trunk:

/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_pk11certstore.c,v  <--  pkix_pl_pk11certstore.c
new revision: 1.6; previous revision: 1.5

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

Comment 14

[Kai Engert [:KaiE:]]

Keeping bug open, need to address my questions from comment 10.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 10 (perhaps not directly answering that question):

The existing policy in NSS 3.4 through 3.11.x is that any overrides in
cert Db override everything else.  Cert DB trumps all.  

Now, I'm accustomed to thinking of "stores" as PKCS#11 slots.  And libPKIX
has a different definition of store.  I don't have a clear understanding of 
the set of stores presently available to libPKIX.  
A list of libPKIX's stores would be helpful.  
Also, in which of the stores do certs in NSS's "temp cert DB" exist?  
Are those part of the PKCS#11 store?

If libPKIX has a single store that is comprised of the collected set of 
PKCS#11 modules/slots (including the "temp" certs, basically everything 
formerly known to NSS in its default trust domain), then perhaps it suffices
for the code for that store to know how to get trust from the cert DB.  
But a cert coming from the ldap store should still be checked against the 
trust info in the cert DB, *I THINK*.  

Perhaps this is an issue for Bob, Julien and me to discuss.  

I think the reasonable choices are:
a) The cert DB is consulted for trust on all certs from all of libPKIX's stores

b) The cert DB is consulted for trust on all certs except those from libPKIX's
http and ldap stores, and NO certs in those two stores ever have any trust.
(However, in this model, the certDB cannot apply explicit distruct to certs 
from those other stores.  But I'm not sure we presently handle explicit 
distrust, so maybe that's irrelevant.)  

Comment 16

[Julien Pierre]

Nelson,

libpkix implements the following stores :

collection cert store (one DER cert per file)
HTTP cert store
LDAP cert store
PKCS#11 cert store (ie. NSS as we know it, which gives one trust answer per cert, regardless of which PKCS#11 token it lives in)

Most of those stores do not support trust. Only the later one does.
In fact, in the original libpkix design, trust was completely separate from cert stores. One had to pass in a list of trust anchors to the libpkix function. I decided this was not going to work for us, and made it possible for libpkix cert stores to optionally support trust. In practice, this is only used by the PKCS#11 cert store.

I think right now the code will only check for the trust in the libpkix cert store in which it was fetched from, provided the libpkix cert store supports trust.

Regarding comment 8, I believe NSS treats PKCS#11 trust objects as affecting any DER cert regardless of its location. However, in the present implementation, I don't think it's possible to have just the trust object without the cert. Each PKCS#11 token has to have both. Then there is a priority list to see which trust object is the right one, configured in secmod.db .

libpkix does not have such a mechanism for trust priorities like secmod.db. And I don't believe one is presently needed, because only one cert store, the NSS PKCS#11 cert store, currently supports trust. If a cert is fetched from HTTP or LDAP and also has a copy in the PKCS#11 cert store with trust, the code should find both of them, and choose to use the trusted one.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 16, it has been the practice of the programs that use
NSS to store the cert and the trust info together, but it is not a general
requirement.

Comment 18

[Julien Pierre]

re: comment 11, the choice of SECCertificateUsage vs SECCertUsage in the plContext was made in order to allow multiple usages to be requested by the application, like CERT_VerifyCertificate does. We should come up a way to handle that. But we must choose one chain that works for all, and it may be complicated.

Comment 19

[Alexei Volkov]

Nothing is needed to be fixed for NSS312Beta. Moving it into 3.12.1.