On Wed, Dec 22, 2021 at 03:06:00PM +0000, Henry Kleynhans wrote:
> From: Henry Kleynhans <hkleynh...@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 <hkleynh...@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é <berra...@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 :|
