Re: [PATCH] Relax X509 CA cert sanity checking

[Philippe Mathieu-Daudé]

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,
>