Re: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Dec 22, 2021 at 03:06:00PM +0000, Henry Kleynhans wrote:
> From: Henry Kleynhans <hkleynhans@fb.com>
> 
> The CA file provided to qemu may contain CA certificates which do not
> form part of the chain of trust for the specific certificate we are
> sanity checking.
> 
> This patch changes the sanity checking from validating every CA
> certificate to only checking the CA certificates which are part of the
> chain of trust (issuer chain).  Other certificates are ignored.
> 
> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> ---
>  crypto/tlscredsx509.c                 | 50 +++++++++++++++++++++++----
>  tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++++-
>  2 files changed, 68 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index d061c68253..841f80aac6 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -315,6 +315,44 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
>      return 0;
>  }
>  
> +static int
> +qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
> +                                        gnutls_x509_crt_t cert,
> +                                        gnutls_x509_crt_t *cacerts,
> +                                        unsigned int ncacerts,
> +                                        const char *cacertFile,
> +                                        bool isServer,
> +                                        bool isCA,
> +                                        Error **errp)
> +{
> +    gnutls_x509_crt_t *cert_to_check = &cert;
> +    int checking_issuer = 1;
> +    int retval = 0;
> +
> +    while (checking_issuer) {
> +        checking_issuer = 0;
> +
> +        if (gnutls_x509_crt_check_issuer(*cert_to_check, *cert_to_check)) {
> +            /* The cert is self-signed indicating we have reached the root of trust. */
> +            return qcrypto_tls_creds_check_cert(creds, *cert_to_check, cacertFile,
> +                                                isServer, isCA, errp);
> +        }
> +        for (int i = 0; i < ncacerts; i++) {
> +            if (gnutls_x509_crt_check_issuer(*cert_to_check, cacerts[i])) {
> +                retval = qcrypto_tls_creds_check_cert(creds, cacerts[i], cacertFile,
> +                                                      isServer, isCA, errp);
> +                if (retval < 0) {
> +                    return retval;
> +                }
> +                cert_to_check = &cacerts[i];
> +                checking_issuer = 1;
> +                break;
> +            }
> +        }
> +    }
> +
> +    return -1;

I have a feeling this should be 'return 0'.  eg consider the case
where the cacert.pem does not contain the issuer of clientcert.pem.
we will only do one iteration of the while(checking_issuer) loop,
not hitting any of the 'return' statements. This is ok, so should
report success I think.

> +}
>  
>  static int
>  qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
> @@ -500,12 +538,12 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>          goto cleanup;
>      }
>  
> -    for (i = 0; i < ncacerts; i++) {
> -        if (qcrypto_tls_creds_check_cert(creds,
> -                                         cacerts[i], cacertFile,
> -                                         isServer, true, errp) < 0) {
> -            goto cleanup;
> -        }
> +    if (cert && 
> +        qcrypto_tls_creds_check_authority_chain(creds, cert, 
> +                                                cacerts, ncacerts,
> +                                                cacertFile, isServer,
> +                                                true, errp) < 0) {
> +        goto cleanup;
>      }
>  
>      if (cert && ncacerts &&
> diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c
> index aab4149b56..e4d657ba61 100644
> --- a/tests/unit/test-crypto-tlscredsx509.c
> +++ b/tests/unit/test-crypto-tlscredsx509.c
> @@ -589,6 +589,12 @@ int main(int argc, char **argv)
>                   true, true, GNUTLS_KEY_KEY_CERT_SIGN,
>                   false, false, NULL, NULL,
>                   0, 0);
> +    TLS_CERT_REQ(cacertlevel1creq_invalid, cacertrootreq,
> +                 "UK", "qemu level 1c invalid", NULL, NULL, NULL, NULL,
> +                 true, true, true,
> +                 true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> +                 false, false, NULL, NULL,
> +                 360, 400);
>      TLS_CERT_REQ(cacertlevel2areq, cacertlevel1areq,
>                   "UK", "qemu level 2a", NULL, NULL, NULL, NULL,
>                   true, true, true,
> @@ -617,16 +623,32 @@ int main(int argc, char **argv)
>          cacertlevel2areq.crt,
>      };
>  
> +
>      test_tls_write_cert_chain(WORKDIR "cacertchain-ctx.pem",
>                                certchain,
>                                G_N_ELEMENTS(certchain));
>  
> +    gnutls_x509_crt_t certchain_with_invalid[] = {
> +        cacertrootreq.crt,
> +        cacertlevel1areq.crt,
> +        cacertlevel1breq.crt,
> +        cacertlevel1creq_invalid.crt,
> +        cacertlevel2areq.crt,
> +    };
> +
> +    test_tls_write_cert_chain(WORKDIR "cacertchain-with-invalid-ctx.pem",
> +                              certchain_with_invalid,
> +                              G_N_ELEMENTS(certchain_with_invalid));
> +
>      TLS_TEST_REG(chain1, true,
>                   WORKDIR "cacertchain-ctx.pem",
>                   servercertlevel3areq.filename, false);
>      TLS_TEST_REG(chain2, false,
>                   WORKDIR "cacertchain-ctx.pem",
>                   clientcertlevel2breq.filename, false);
> +    TLS_TEST_REG(certchainwithexpiredcert, false,
> +                 WORKDIR "cacertchain-with-invalid-ctx.pem",
> +                 clientcertlevel2breq.filename, false);
>  
>      /* Some missing certs - first two are fatal, the last
>       * is ok
> @@ -640,7 +662,6 @@ int main(int argc, char **argv)
>      TLS_TEST_REG(missingclient, false,
>                   cacert1req.filename,
>                   "clientcertdoesnotexist.pem", false);
> -
>      ret = g_test_run();
>  
>      test_tls_discard_cert(&cacertreq);
> @@ -694,10 +715,12 @@ int main(int argc, char **argv)
>      test_tls_discard_cert(&cacertrootreq);
>      test_tls_discard_cert(&cacertlevel1areq);
>      test_tls_discard_cert(&cacertlevel1breq);
> +    test_tls_discard_cert(&cacertlevel1creq_invalid);
>      test_tls_discard_cert(&cacertlevel2areq);
>      test_tls_discard_cert(&servercertlevel3areq);
>      test_tls_discard_cert(&clientcertlevel2breq);
>      unlink(WORKDIR "cacertchain-ctx.pem");
> +    unlink(WORKDIR "cacertchain-with-invalid-ctx.pem");
>  
>      test_tls_cleanup(KEYFILE);
>      rmdir(WORKDIR);
> -- 
> 2.34.1
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|