390888 - CERT_Verify* functions should be able to use libPKIX

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

This bug is the complement to Bug 294531, which requests the development 
of new enhanced interfaces for cert verification.

Existing programs that use NSS's six existing cert verification functions
need to also be able to work with libPKIX, without them having to be 
recoded to use those new interfaces.  

NSS's six existing cert verification functions are:
- CERT_VerifyCACertForUsage
- CERT_VerifyCert
- CERT_VerifyCertChain
- CERT_VerifyCertNow         *
- CERT_VerifyCertificate
- CERT_VerifyCertificateNow  *

Note that the two *Now functions do not need to be changed, because they 
merely call the corresponding non-Now functions, so only the other four
functions need to be changed.

These functions should be able to work with either the old NSS cert library
implementation, or with the new libPKIX implementation.  We envision a 
"switch" that allows the behavior of these functions to be chosen globally
for the process, or perhaps on a thread-by-thread basis.  

Alexei has been working on this, and has a patch that is mostly functional.
The current state is that error codes returned for unsuccessful validations
is not nearly as good as for the old cert library.  That work is planned.

Comment 1

[Alexei Volkov]

Does not look like CERT_VerifyCertChain needs to have a companion function that verifies a chain through libPkix. Although having CERT_VerifyCertChain to work though libPkix would be a good opportunity for development of additional set of tests for libPkix, but currently CERT_VerifyCertChain is not a part of exported API and only get called within certvfy.c by functions that are listed above.

Nelson, do you plan to use this function as a part of enhancement for bug 324867?   

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Alexei, given that "CERT_VerifyCertChain is not a part of exported API 
and only get called within certvfy.c by functions that are listed above",
I agree that we can exclude it from the list of functions to be converted.

Comment 3

[Alexei Volkov]

Attachment: initial wrapper patch

See bug dependency list for the list of bugs that need to be fixed to pass all.sh

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275916 [details] [diff] [review]
initial wrapper patch

Alexei,  

There is a large number of problems with this patch, 
mostly minor ones, but still a large number,
too many for me to type them all up here.
So, let's go over them by phone or in person ASAP.

Comment 5

[Alexei Volkov]

Attachment: patch v2

patch is limited to only things that need for pkix wrapper function.

Comment 6

[Alexei Volkov]

Attachment: patch v3

Fixed most of the problems found at review time.

Comment 7

[Alexei Volkov]

>Review comments from Nelson:
> 1) CERT_VerifyCACertForUsage now seems to be identical to CERT_VerifyCert
> when libPKIX is enabled.  Both functions make the same exact call in
> that case:
>         return cert_VerifyCertPkix(cert, checkSig, certUsage,
>                                    t, wincx, log);
> I think that can't be right.  (Can it?)
>
> CERT_VerifyCert asks "Is this cert valid for this usage?"
> CERT_VerifyCACertForUsage asks "Is this CA cert valid to be an issuer of
> an EE cert with this usage?"
>
> I don't think the same code can do both.  CERT_VerifyCACertForUsage is
> more like CERT_VerifyCertChain than it is like CERT_VerifyCert.

The API should allow "verify as CA" flag to be passed then caller wants to validate a cert as a CA cert. Currently, the code make assumption about caller intentions by checks the cert for it's a CA flags. I agree, that current behavior is incorrect.

> 2) This version of the patch changes the behavior of cert_NssKeyUsagesToPkix.
>
> The previous version converted the nssKeyUsage bits to PKIX key usage bits
> and then OR'ed them into the caller's pkixKeyUsage variable, so that
> the caller's variable contained the union of whatever bits were set before
> the call and the bits that were set by the call to this function.
>
> The patched version converts the nssKeyUsage bits to PKIX key usage bits
> and then STORES them into the caller's pkixKeyUsage variable, so that
> bits in the caller's variable before the call are lost by this function
> call.
>
> I don't know if this change is significant.  Perhaps it is never
> required to preserve the bits already set in the caller's pkixKeyUsage
> variable, but if it is, this can be fixed by changing one line near
> the end of the function cert_NssKeyUsagesToPkix:
> -    *pPkixKeyUsage = pkixKeyUsage;
> +    *pPkixKeyUsage |= pkixKeyUsage;

This is easy to fix, but unnecessary. *pPkixKeyUsage does not have initial
key usage information and should be overwritten.

> 3) In the new file certhigh/certvfypkix.c, please make all functions
> static that can be static (that are only called from within the same
> source file).  Please add a large comment near the front of the file
> saying that these interfaces are private and are not stable, and may
> change in the very next release!!
Done

> 
> 4) in function cert_PkixToNssCertsChain, it looks like the code will
> call PKIX_DECREF twice on the last certItem.
It is valid. PKIX_DECREF(POINTER) checks the POINTER if it is NULL before
proceeding and sets a POINTER to NULL after it is done. No memory menagement
problems here. The reason it is done twice is to cleanup  certItem if we jumped from the loop to "cleanup" with some an error.

> 
> 5) There are two calls to cert_GetBuildResults.  Both of them pass
> NULL for the ptrustedRoot and pvalidChain arguments in optimized
> builds, and pass non-NULL pointers in debug builds.  But in the
> debug builds, the callers make no use of the returned root and
> valid chain.  So, why pass non-NULL values in debug builds?

This is done to printout trusted root and build chain in the debug
builds. If any of these pointer are not equal to NULL, the  cert_GetBuildResults will extract a root or a chain from PKIX_BuildResult and will print
it out. 
 
> 6) in function cert_GetBuildResult   we see this code:
>       
> >     if (trustedRoot) {
> >         *ptrustedRoot = trustedRoot;
> >     }
> >     if (validChain) {
> >         *pvalidChain = validChain;
> >     }
> >     
> > cleanup:
> >     if (PKIX_ERROR_RECEIVED) {
> >         if (trustedRoot) {
> >             CERT_DestroyCertificate(trustedRoot);
> >         }
> >         if (validChain) {
> >             CERT_DestroyCertList(validChain);
> >         }
> >     }
Again, this is an after error clean up. If we got error we will never get to
the first two of if statements. On the other hand, if error has occurred,
PKIX_ERROR_RECEIVED is set, and the code will destroy the chain and root cert. 

> 
> That's bad because it gives the pointers to the trustedRoot and
> validChain to the caller, and then destroys them, leaving the caller
> with pointers to freed memory.  It should only give those values to
> the caller when it is not going to destroy them.
> 
> Some questions:
> 
> A) There is a bug that says that libPKIX doesn't check the first cert
> (the "leaf" or "EE" cert) for validity before trying to build the chain.
> Is this patch supposed to fix that? Or will that be in a separate patch?
No. This patch does not have fix for this issue. Will be in separate patch.

> 
> B) This new patch changes one line in pkix_Build_InitiateBuildChain in
> file libpkix/pkix/top/pkix_build.c.  That file was unchanged in the
> previous patch.  Is this change related to this bug?
This changes for memory corruption that been found. This patch can not work without it.

Comment 8

[Alexei Volkov]

Attachment: patch v4

changes according to last review.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: patch v4 b

this is patch v4 merged to the latest trunk

Comment 10

[Kai Engert [:KaiE:]]

Attachment: patch v4 c with 390502

This is patch v4 b plus the code for bug 390502 enabled.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 277966 [details] [diff] [review]
patch v4

r+ once the following have been fixed/addressed:
(using the numbers from my previous review comments in comment 7)

1. Regarding CERT_VerifyCACertForUsage, there is a separate bug that says
that libPKIX does not adequate check the first cert in the chain (the
EE cert or leaf).  The problems with CERT_VerifyCACertForUsage can be 
fixed later when that bug is fixed.

2. I think there is a comment for cert_NssKeyUsagesToPkix that says that
it combines key usage flags with the caller's.  If so, that comment should
be fixed to state that it replaces the caller's pkix key usage flags.

6. This code MUST ensure that it can never execute either of these two
lines if PKIX_ERROR_RECEIVED is true:

> >         *ptrustedRoot = trustedRoot;
> >         *pvalidChain = validChain;

That can be done by putting an assert into the code before those lines,
or by putting those lines inside of an enclosing "if (!PKIX_ERROR_RECEIVED) {"
or (perhaps) by rearranging the code after label cleanup to store those 
values in an "else" case for "if (PKIX_ERROR_RECEIVED) {"
I don't care which of those methods is used, but the code MUST make sure
it never outputs pointers to destroyed objects.

Comment 12

[Alexei Volkov]

Attachment: CERT_VerifyCert patch v5

fixes for last review comments.

Comment 13

[Alexei Volkov]

Attachment: cert_VerifyCertChain wrapper. patch v6

Implementation of the wrapper for cert_VerifyCertChain: do validation of leaf cert by old nss validation functions, and continue chain building and validation by libPkix.

Comment 14

[Alexei Volkov]

v5 committed.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278672 [details] [diff] [review]
CERT_VerifyCert patch v5

This is good enough to commit, I believe.  Please commit it.  If you want to make more changes, please submit a new patch that doesn't include all the changes in this patch.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278675 [details] [diff] [review]
cert_VerifyCertChain wrapper. patch v6

After you've committed patch v5, please submit a new patch that is the difference between v5 and v6.

Comment 17

[Alexei Volkov]

Attachment: Smaller v6 patch

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 278683 [details] [diff] [review]
Smaller v6 patch

Thank you for this smaller patch.  It was much easier to review!

One question.  Is the XXX comment shown below still true?  
Or does using libPKIX cure it?  
If libPKIX cures it, then the comment should change to state that 
cert path length constraints are only enforced by libPKIX.

>+/*
>+ * verify that a CA can sign a certificate with the requested usage.
>+ * XXX This function completely ignores cert path length constraints!
>+ */
> SECStatus
>-cert_VerifyCACertForUsage(CERTCertDBHandle *handle, CERTCertificate *cert,
>+CERT_VerifyCACertForUsage(CERTCertDBHandle *handle, CERTCertificate *cert,

Comment 19

[Alexei Volkov]

I'm not sure why this comment is still here. Yes, the body of the function CERT_VerifyCACertForUsage ignores path length of basic constraints extension. But the length is checked in CERT_VerifyCertChain, the function that CERT_VerifyCACertForUsage calls.

libPkix also validate cert chain taking path length in to consideration.

In my opinion the comment should be removed.  

Comment 20

[Alexei Volkov]

the comment is removed. v6 patch is integrated into trunk.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 18 and comment 19:
When the leaf is a CA cert, and we're validating it to see if it can issue 
another cert (as CERT_VerifyCACertForUsage does), we should add 1 to the 
required path length.  It's not enough for the path to be valid down to 
the leaf CA cert, it must also be valid down through the cert that the 
(leaf) CA might issue.

Comment 22

[Alexei Volkov]

libpkix, I guess as well as NSS, always start validation from counting the leaf cert as zero, regardless if it is CA cert or not. New bug 396606 is filed to track the fix for incorrect behavior mentioned in comment 21.