On Wed, Jul 20, 2011 at 02:12:47PM +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/