[libvirt] [PATCH 3/3] Honour key usage/purpose criticality flag

[Daniel Veillard]

On Wed, Jul 20, 2011 at 02:12:47PM +0100, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> If a key purpose or usage field is marked as non-critical in the
> certificate, then a data mismatch is not (ordinarily) a cause for
> rejecting the connection
> 
> * src/rpc/virnettlscontext.c: Honour key usage/purpose criticality
> ---
>  src/rpc/virnettlscontext.c |   78 ++++++++++++++++++++++++++++++-------------
>  1 files changed, 54 insertions(+), 24 deletions(-)
> 
> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index 1ee5ab2..1622bad 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -111,6 +111,7 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
>      char *buffer = NULL;
>      size_t size;
>      unsigned int usage;
> +    unsigned int critical;
>      bool allowClient = false, allowServer = false;
>  
>      VIR_DEBUG("isServer %d isCA %d certFile %s",
> @@ -190,9 +191,9 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
>          goto cleanup;
>      }
>  
> -    status = gnutls_x509_crt_get_key_usage(cert, &usage, NULL);
> +    status = gnutls_x509_crt_get_key_usage(cert, &usage, &critical);

  Okay, I was wondering if we were missing something by passing NULL and
we did :-)

> -    VIR_DEBUG("Cert %s key usage status %d usage %d", certFile, status, usage);
> +    VIR_DEBUG("Cert %s key usage status %d usage %d critical %u", certFile, status, usage, critical);
>      if (status < 0) {
>          if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) {
>              usage = isCA ? GNUTLS_KEY_KEY_CERT_SIGN :
> @@ -207,28 +208,45 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
>  
>      if (isCA) {
>          if (!(usage & GNUTLS_KEY_KEY_CERT_SIGN)) {
> -            virNetError(VIR_ERR_SYSTEM_ERROR,
> -                        _("Certificate %s usage does not permit certificate signing"),
> -                        certFile);
> -            goto cleanup;
> +            if (critical) {
> +                virNetError(VIR_ERR_SYSTEM_ERROR,
> +                            _("Certificate %s usage does not permit certificate signing"),
> +                            certFile);
> +                goto cleanup;
> +            } else {
> +                VIR_WARN(_("Certificate %s usage does not permit certificate signing"),
> +                         certFile);
> +            }
>          }
>      } else {
>          if (!(usage & GNUTLS_KEY_DIGITAL_SIGNATURE)) {
> -            virNetError(VIR_ERR_SYSTEM_ERROR,
> -                        _("Certificate %s usage does not permit digital signature"),
> -                        certFile);
> -            goto cleanup;
> +            if (critical) {
> +                virNetError(VIR_ERR_SYSTEM_ERROR,
> +                            _("Certificate %s usage does not permit digital signature"),
> +                            certFile);
> +                goto cleanup;
> +            } else {
> +                VIR_WARN(_("Certificate %s usage does not permit digital signature"),
> +                         certFile);
> +            }
>          }
>          if (!(usage & GNUTLS_KEY_KEY_ENCIPHERMENT)) {
> -            virNetError(VIR_ERR_SYSTEM_ERROR,
> -                        _("Certificate %s usage does not permit key encipherment"),
> -                        certFile);
> -            goto cleanup;
> +            if (critical) {
> +                virNetError(VIR_ERR_SYSTEM_ERROR,
> +                            _("Certificate %s usage does not permit key encipherment"),
> +                            certFile);
> +                goto cleanup;
> +            } else {
> +                VIR_WARN(_("Certificate %s usage does not permit key encipherment"),
> +                         certFile);
> +            }
>          }
>      }
>  
> +    critical = 0;
>      for (i = 0 ; ; i++) {
>          size = 0;
> +        unsigned int purposeCritical;
>          status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, NULL);
>  
>          if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) {
> @@ -251,15 +269,17 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
>              goto cleanup;
>          }
>  
> -        status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, NULL);
> +        status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, &purposeCritical);
>          if (status < 0) {
>              virNetError(VIR_ERR_SYSTEM_ERROR,
>                          _("Unable to query certificate %s key purpose %s"),
>                          certFile, gnutls_strerror(status));
>              goto cleanup;
>          }
> +        if (purposeCritical)
> +            critical = true;
>  
> -        VIR_DEBUG("Key purpose %d %s", status, buffer);
> +        VIR_DEBUG("Key purpose %d %s critical %u", status, buffer, purposeCritical);
>          if (STREQ(buffer, GNUTLS_KP_TLS_WWW_SERVER)) {
>              allowServer = true;
>          } else if (STREQ(buffer, GNUTLS_KP_TLS_WWW_CLIENT)) {
> @@ -273,16 +293,26 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer,
>  
>      if (!isCA) { /* No purpose checks required for CA certs */
>          if (isServer && !allowServer) {
> -            virNetError(VIR_ERR_SYSTEM_ERROR,
> -                        _("Certificate %s purpose does not allow use for with a TLS server"),
> -                        certFile);
> -            goto cleanup;
> +            if (critical) {
> +                virNetError(VIR_ERR_SYSTEM_ERROR,
> +                            _("Certificate %s purpose does not allow use for with a TLS server"),
> +                            certFile);
> +                goto cleanup;
> +            } else {
> +                VIR_WARN(_("Certificate %s purpose does not allow use for with a TLS server"),
> +                         certFile);
> +            }
>          }
>          if (!isServer && !allowClient) {
> -            virNetError(VIR_ERR_SYSTEM_ERROR,
> -                        _("Certificate %s purpose does not allow use for with a TLS client"),
> -                        certFile);
> -            goto cleanup;
> +            if (critical) {
> +                virNetError(VIR_ERR_SYSTEM_ERROR,
> +                            _("Certificate %s purpose does not allow use for with a TLS client"),
> +                            certFile);
> +                goto cleanup;
> +            } else {
> +                VIR_WARN(_("Certificate %s purpose does not allow use for with a TLS client"),
> +                         certFile);
> +            }
>          }
>      }

  Okay, the only extra caution I would do are initialize critical and
purposeCritical to 0 at declaration time to make sure they are
initialized all the time though the gnutls calls should really set them.

  ACK

Daniel


-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/