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.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
crypto/tlscredsx509.c | 57 ++++++++++++++++++++++++---
tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++-
2 files changed, 75 insertions(+), 7 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index cd1f504471..797854ac89 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -315,6 +315,51 @@ 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;
+}
static int
qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
@@ -499,12 +544,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 3c25d75ca1..a7ea5f422d 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.50.1
On Fri, Sep 19, 2025 at 11:10:17AM +0100, Daniel P. Berrangé 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.
I agree that relaxing this will permit more certs than before (and
possibly with less CPU cycles spent on the irrelevant portions of the
cert), without weakening the security of the chain we are actually
interested in.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> crypto/tlscredsx509.c | 57 ++++++++++++++++++++++++---
> tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++-
> 2 files changed, 75 insertions(+), 7 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index cd1f504471..797854ac89 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -315,6 +315,51 @@ 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;
Why is this int instead of bool? It's only assigned 1 or 0, and local
to the function.
That's a trivial cleanup, so I'm okay if you make that change and add:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
On Thu, Oct 16, 2025 at 09:37:51AM -0500, Eric Blake wrote:
> On Fri, Sep 19, 2025 at 11:10:17AM +0100, Daniel P. Berrangé 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.
>
> I agree that relaxing this will permit more certs than before (and
> possibly with less CPU cycles spent on the irrelevant portions of the
> cert), without weakening the security of the chain we are actually
> interested in.
>
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > crypto/tlscredsx509.c | 57 ++++++++++++++++++++++++---
> > tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++-
> > 2 files changed, 75 insertions(+), 7 deletions(-)
> >
> > diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> > index cd1f504471..797854ac89 100644
> > --- a/crypto/tlscredsx509.c
> > +++ b/crypto/tlscredsx509.c
> > @@ -315,6 +315,51 @@ 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;
>
> Why is this int instead of bool? It's only assigned 1 or 0, and local
> to the function.
No good reason.
>
> That's a trivial cleanup, so I'm okay if you make that change and add:
I'll change to bool.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization: qemu.org | libguestfs.org
>
With 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 :|
© 2016 - 2026 Red Hat, Inc.