* [PATCH] Relax X509 CA cert sanity checking
@ 2021-12-14 11:39 Henry Kleynhans
2021-12-14 13:04 ` Philippe Mathieu-Daudé
2021-12-14 13:16 ` Daniel P. Berrangé
0 siblings, 2 replies; 6+ messages in thread
From: Henry Kleynhans @ 2021-12-14 11:39 UTC (permalink / raw)
To: qemu-devel; +Cc: Henry Kleynhans, henry.kleynhans
From: Henry Kleynhans <hkleynhans@fb.com>
The sanity checking function attempts to validate all the certificates
in the provided CA file. These checks are performed on certificates
which may or may not be part of the signing chain and duplicates checks
that should be performed by the TLS library.
In real life this causes a problem if the certificate chain I want to
use is valid, but there exist another expired certificate in the CA
file.
This patch relaxes the sanity checks to only ensure we have at least one
valid certificate in the CA certificate file and leave the actual
validation to the TLS library.
Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
---
crypto/tlscredsx509.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 32948a6bdc..fb056f96a2 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -473,6 +473,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
gnutls_x509_crt_t cert = NULL;
gnutls_x509_crt_t cacerts[MAX_CERTS];
size_t ncacerts = 0;
+ size_t nvalidca = 0;
size_t i;
int ret = -1;
@@ -505,11 +506,15 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
for (i = 0; i < ncacerts; i++) {
if (qcrypto_tls_creds_check_cert(creds,
cacerts[i], cacertFile,
- isServer, true, errp) < 0) {
- goto cleanup;
+ isServer, true, errp) == 0) {
+ ++nvalidca;
}
}
+ if (nvalidca == 0) {
+ goto cleanup;
+ }
+
if (cert && ncacerts &&
qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
ncacerts, cacertFile,
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Relax X509 CA cert sanity checking
2021-12-14 11:39 [PATCH] Relax X509 CA cert sanity checking Henry Kleynhans
@ 2021-12-14 13:04 ` Philippe Mathieu-Daudé
2021-12-14 13:16 ` Daniel P. Berrangé
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-14 13:04 UTC (permalink / raw)
To: Henry Kleynhans, qemu-devel, Daniel P . Berrange
Cc: Henry Kleynhans, henry.kleynhans
Hi Henry,
Please Cc maintainers:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer
(doing it for you now).
On 12/14/21 12:39, Henry Kleynhans wrote:
> From: Henry Kleynhans <hkleynhans@fb.com>
>
> The sanity checking function attempts to validate all the certificates
> in the provided CA file. These checks are performed on certificates
> which may or may not be part of the signing chain and duplicates checks
> that should be performed by the TLS library.
>
> In real life this causes a problem if the certificate chain I want to
> use is valid, but there exist another expired certificate in the CA
> file.
>
> This patch relaxes the sanity checks to only ensure we have at least one
> valid certificate in the CA certificate file and leave the actual
> validation to the TLS library.
Since nobody complained so far, should we add this change as a boolean
property such "allow-expired-ca-certificate", default to false?
> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> ---
> crypto/tlscredsx509.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 32948a6bdc..fb056f96a2 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -473,6 +473,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> gnutls_x509_crt_t cert = NULL;
> gnutls_x509_crt_t cacerts[MAX_CERTS];
> size_t ncacerts = 0;
> + size_t nvalidca = 0;
> size_t i;
> int ret = -1;
>
> @@ -505,11 +506,15 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> for (i = 0; i < ncacerts; i++) {
> if (qcrypto_tls_creds_check_cert(creds,
> cacerts[i], cacertFile,
> - isServer, true, errp) < 0) {
> - goto cleanup;
> + isServer, true, errp) == 0) {
> + ++nvalidca;
> }
> }
>
> + if (nvalidca == 0) {
> + goto cleanup;
> + }
> +
> if (cert && ncacerts &&
> qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
> ncacerts, cacertFile,
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Relax X509 CA cert sanity checking
2021-12-14 11:39 [PATCH] Relax X509 CA cert sanity checking Henry Kleynhans
2021-12-14 13:04 ` Philippe Mathieu-Daudé
@ 2021-12-14 13:16 ` Daniel P. Berrangé
2021-12-14 14:55 ` Henry Kleynhans
1 sibling, 1 reply; 6+ messages in thread
From: Daniel P. Berrangé @ 2021-12-14 13:16 UTC (permalink / raw)
To: Henry Kleynhans; +Cc: Henry Kleynhans, qemu-devel, henry.kleynhans
On Tue, Dec 14, 2021 at 11:39:30AM +0000, Henry Kleynhans wrote:
> From: Henry Kleynhans <hkleynhans@fb.com>
>
> The sanity checking function attempts to validate all the certificates
> in the provided CA file. These checks are performed on certificates
> which may or may not be part of the signing chain and duplicates checks
> that should be performed by the TLS library.
>
> In real life this causes a problem if the certificate chain I want to
> use is valid, but there exist another expired certificate in the CA
> file.
>
> This patch relaxes the sanity checks to only ensure we have at least one
> valid certificate in the CA certificate file and leave the actual
> validation to the TLS library.
IIUC your scenario is that you have something like
Root CA -> Sub CA1 ---> Server Cert
\ \-> Client Cert
\---> Sub CA2
And the ca-cert.pem file given to QEMU contains all of 'Root CA',
'Sub CA1' and 'Sub CA2', despite 'Sub CA2' being irrelevant
from POV of QEMU's needs for the chain of trust.
Now 'Sub CA2' is expired so QEMU is rejecting the entire
'ca-cert.pem' at startup.
Your suggested change makes it so that we only validate that
one of these three certs is OK, so if 'Sub CA2' is expired
it dosn't block startup.
The trouble is that this equally ignores problems if 'Sub CA1'
is expired (or otherwise invalid), which is exactly the
scenario that we're aiming to detect.
TLS certificate config mistakes are an incredibly common
problem and the error reporting when this happens at time
of TLS session handshake is terrible. This leads to a
stream of support requests from users wondering why their
TLS sessions won't establish, which are very hard for us
to debug as maintainers. The validation made a significant
difference to this by giving users clearer error reports
upfront instead of letting QEMU startup with a broken
TLS cert setup.
Thus I'm loathe to relax the validation in the way proposed
here.
Is there a reason you aren't able to just set the
property 'validate=off' on the tls-creds-x509 object
to skip the validation in the case where you know your
CA bundle is valid overall, but contains broken certs ?
>
> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> ---
> crypto/tlscredsx509.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 32948a6bdc..fb056f96a2 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -473,6 +473,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> gnutls_x509_crt_t cert = NULL;
> gnutls_x509_crt_t cacerts[MAX_CERTS];
> size_t ncacerts = 0;
> + size_t nvalidca = 0;
> size_t i;
> int ret = -1;
>
> @@ -505,11 +506,15 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> for (i = 0; i < ncacerts; i++) {
> if (qcrypto_tls_creds_check_cert(creds,
> cacerts[i], cacertFile,
> - isServer, true, errp) < 0) {
> - goto cleanup;
> + isServer, true, errp) == 0) {
> + ++nvalidca;
> }
> }
>
> + if (nvalidca == 0) {
> + goto cleanup;
> + }
> +
> if (cert && ncacerts &&
> qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
> ncacerts, cacertFile,
> --
> 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 :|
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Relax X509 CA cert sanity checking
2021-12-14 13:16 ` Daniel P. Berrangé
@ 2021-12-14 14:55 ` Henry Kleynhans
2021-12-14 15:00 ` Daniel P. Berrangé
0 siblings, 1 reply; 6+ messages in thread
From: Henry Kleynhans @ 2021-12-14 14:55 UTC (permalink / raw)
To: Daniel P. Berrangé, Henry Kleynhans; +Cc: qemu-devel, henry.kleynhans
[-- Attachment #1: Type: text/plain, Size: 4544 bytes --]
Hi Daniel,
I agree that this would allow QEMU startup with a broken TLS setup. Maybe the better solution is to only validate the chain of trust. Would a patch that does that be acceptable?
Kind regards, Henry
From: Daniel P. Berrangé <berrange@redhat.com>
Date: Tuesday, 14 December 2021 at 13:16
To: Henry Kleynhans <henry.kleynhans@gmail.com>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>, Henry Kleynhans <hkleynhans@fb.com>, henry.kleynhans@fb.com <henry.kleynhans@fb.com>
Subject: Re: [PATCH] Relax X509 CA cert sanity checking
On Tue, Dec 14, 2021 at 11:39:30AM +0000, Henry Kleynhans wrote:
> From: Henry Kleynhans <hkleynhans@fb.com>
>
> The sanity checking function attempts to validate all the certificates
> in the provided CA file. These checks are performed on certificates
> which may or may not be part of the signing chain and duplicates checks
> that should be performed by the TLS library.
>
> In real life this causes a problem if the certificate chain I want to
> use is valid, but there exist another expired certificate in the CA
> file.
>
> This patch relaxes the sanity checks to only ensure we have at least one
> valid certificate in the CA certificate file and leave the actual
> validation to the TLS library.
IIUC your scenario is that you have something like
Root CA -> Sub CA1 ---> Server Cert
\ \-> Client Cert
\---> Sub CA2
And the ca-cert.pem file given to QEMU contains all of 'Root CA',
'Sub CA1' and 'Sub CA2', despite 'Sub CA2' being irrelevant
from POV of QEMU's needs for the chain of trust.
Now 'Sub CA2' is expired so QEMU is rejecting the entire
'ca-cert.pem' at startup.
Your suggested change makes it so that we only validate that
one of these three certs is OK, so if 'Sub CA2' is expired
it dosn't block startup.
The trouble is that this equally ignores problems if 'Sub CA1'
is expired (or otherwise invalid), which is exactly the
scenario that we're aiming to detect.
TLS certificate config mistakes are an incredibly common
problem and the error reporting when this happens at time
of TLS session handshake is terrible. This leads to a
stream of support requests from users wondering why their
TLS sessions won't establish, which are very hard for us
to debug as maintainers. The validation made a significant
difference to this by giving users clearer error reports
upfront instead of letting QEMU startup with a broken
TLS cert setup.
Thus I'm loathe to relax the validation in the way proposed
here.
Is there a reason you aren't able to just set the
property 'validate=off' on the tls-creds-x509 object
to skip the validation in the case where you know your
CA bundle is valid overall, but contains broken certs ?
>
> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> ---
> crypto/tlscredsx509.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 32948a6bdc..fb056f96a2 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -473,6 +473,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> gnutls_x509_crt_t cert = NULL;
> gnutls_x509_crt_t cacerts[MAX_CERTS];
> size_t ncacerts = 0;
> + size_t nvalidca = 0;
> size_t i;
> int ret = -1;
>
> @@ -505,11 +506,15 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> for (i = 0; i < ncacerts; i++) {
> if (qcrypto_tls_creds_check_cert(creds,
> cacerts[i], cacertFile,
> - isServer, true, errp) < 0) {
> - goto cleanup;
> + isServer, true, errp) == 0) {
> + ++nvalidca;
> }
> }
>
> + if (nvalidca == 0) {
> + goto cleanup;
> + }
> +
> if (cert && ncacerts &&
> qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
> ncacerts, cacertFile,
> --
> 2.34.1
>
>
Regards,
Daniel
--
|: https://berrange.com<https://berrange.com> -o- https://www.flickr.com/photos/dberrange<https://www.flickr.com/photos/dberrange> :|
|: https://libvirt.org<https://libvirt.org> -o- https://fstop138.berrange.com<https://fstop138.berrange.com> :|
|: https://entangle-photo.org<https://entangle-photo.org> -o- https://www.instagram.com/dberrange :|
[-- Attachment #2: Type: text/html, Size: 9330 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Relax X509 CA cert sanity checking
2021-12-14 14:55 ` Henry Kleynhans
@ 2021-12-14 15:00 ` Daniel P. Berrangé
0 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2021-12-14 15:00 UTC (permalink / raw)
To: Henry Kleynhans; +Cc: Henry Kleynhans, henry.kleynhans, qemu-devel
On Tue, Dec 14, 2021 at 02:55:02PM +0000, Henry Kleynhans wrote:
> Hi Daniel,
>
> I agree that this would allow QEMU startup with a broken
> TLS setup. Maybe the better solution is to only validate
> the chain of trust. Would a patch that does that be acceptable?
Yes, that would be fine. It was only ever intended to validate
the chain of trust needed for QEMU's usage. It simply never
occurred to me that someone who have extra redundant certs in
their bundle, so I didn't do anything special to handle that.
BTW, there's a decent amount of unit test coverage for this code
in tests/unit/test-crypto-tlscredsx509.c which could be fairly
easily extended to cover the scenarios of extra certs outside
the required chain of trust.
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 :|
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Relax X509 CA cert sanity checking
@ 2021-12-14 10:03 Henry Kleynhans
0 siblings, 0 replies; 6+ messages in thread
From: Henry Kleynhans @ 2021-12-14 10:03 UTC (permalink / raw)
To: qemu-devel; +Cc: Henry Kleynhans, henry.kleynhans
From: Henry Kleynhans <hkleynhans@fb.com>
The sanity checking function attempts to validate all the certificates
in the provided CA file. These checks are performed on certificates
which may or may not be part of the signing chain and duplicates checks
that should be performed by the TLS library.
In real life this causes a problem if the certificate chain I want to
use is valid, but there exist another expired certificate in the CA
file.
This patch relaxes the sanity checks to only ensure we have at least one
valid certificate in the CA certificate file and leave the actual
validation to the TLS library.
Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
---
crypto/tlscredsx509.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 32948a6bdc..fb056f96a2 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -473,6 +473,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
gnutls_x509_crt_t cert = NULL;
gnutls_x509_crt_t cacerts[MAX_CERTS];
size_t ncacerts = 0;
+ size_t nvalidca = 0;
size_t i;
int ret = -1;
@@ -505,11 +506,15 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
for (i = 0; i < ncacerts; i++) {
if (qcrypto_tls_creds_check_cert(creds,
cacerts[i], cacertFile,
- isServer, true, errp) < 0) {
- goto cleanup;
+ isServer, true, errp) == 0) {
+ ++nvalidca;
}
}
+ if (nvalidca == 0) {
+ goto cleanup;
+ }
+
if (cert && ncacerts &&
qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
ncacerts, cacertFile,
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-14 15:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-14 11:39 [PATCH] Relax X509 CA cert sanity checking Henry Kleynhans
2021-12-14 13:04 ` Philippe Mathieu-Daudé
2021-12-14 13:16 ` Daniel P. Berrangé
2021-12-14 14:55 ` Henry Kleynhans
2021-12-14 15:00 ` Daniel P. Berrangé
-- strict thread matches above, loose matches on Subject: below --
2021-12-14 10:03 Henry Kleynhans
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.