[libvirt] [PATCH 1/2] Add some basic sanity checking of certificates before use
If the libvirt daemon or libvirt client is configured with bogus certificates, it is very unhelpful to only find out about this when a TLS connection is actually attempted. Not least because the error messages you get back for failures are incredibly obscure. This adds some basic sanity checking of certificates at the time the virNetTLSContext object is created. This is at libvirt startup, or when creating a virNetClient instance. This checks that the certificate expiry/start dates are valid and that the certificate is actually signed by the CA that is loaded. * src/rpc/virnettlscontext.c: Add certificate sanity checks --- src/rpc/virnettlscontext.c | 149 ++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 145 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 4fa6fbb..b0dcfe5 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -67,6 +67,7 @@ struct _virNetTLSSession { bool handshakeComplete; + bool isServer; char *hostname; gnutls_session_t session; virNetTLSSessionWriteFunc writeFunc; @@ -95,6 +96,134 @@ static void virNetTLSLog(int level, const char *str) { VIR_DEBUG("%d %s", level, str); } + +static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, + const char *certFile) +{ + gnutls_datum_t data; + gnutls_x509_crt_t cert = NULL; + char *buf = NULL; + int ret = -1; + time_t now; + + if ((now = time(NULL)) == ((time_t)-1)) { + virReportSystemError(errno, "%s", + _("cannot get current time")); + goto cleanup; + } + + if (gnutls_x509_crt_init(&cert) < 0) { + virNetError(VIR_ERR_SYSTEM_ERROR, "%s", + _("Unable to initialize certificate")); + goto cleanup; + } + + if (virFileReadAll(certFile, (1<<16), &buf) < 0) + goto cleanup; + + data.data = (unsigned char *)buf; + data.size = strlen(buf); + + if (gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM) < 0) { + virNetError(VIR_ERR_SYSTEM_ERROR, isServer ? + _("Unable to import server certificate %s") : + _("Unable to import client certificate %s"), + certFile); + goto cleanup; + } + + if (gnutls_x509_crt_get_expiration_time(cert) < now) { + virNetError(VIR_ERR_SYSTEM_ERROR, isServer ? + _("The server certificate %s has expired") : + _("The client certificate %s has expired"), + certFile); + goto cleanup; + } + + if (gnutls_x509_crt_get_activation_time(cert) > now) { + virNetError(VIR_ERR_SYSTEM_ERROR, isServer ? + _("The server certificate %s is not yet active") : + _("The client certificate %s is not yet active"), + certFile); + goto cleanup; + } + + ret = 0; + +cleanup: + if (ret != 0) { + gnutls_x509_crt_deinit(cert); + cert = NULL; + } + VIR_FREE(buf); + return cert; +} + + +static int virNetTLSContextSanityCheckCredentials(bool isServer, + const char *cacertFile, + const char *certFile) +{ + gnutls_x509_crt_t cert = NULL; + gnutls_x509_crt_t cacert = NULL; + int ret = -1; + unsigned int status; + + if (access(certFile, R_OK) == 0) { + if (!(cert = virNetTLSContextSanityCheckCert(isServer, certFile))) + goto cleanup; + } + if (access(cacertFile, R_OK) == 0) { + if (!(cacert = virNetTLSContextSanityCheckCert(isServer, cacertFile))) + goto cleanup; + } + + if (cert && cacert) { + if (gnutls_x509_crt_list_verify(&cert, 1, + &cacert, 1, + NULL, 0, + 0, &status) < 0) { + virNetError(VIR_ERR_SYSTEM_ERROR, "%s", isServer ? + _("Unable to verify server certificate against CA certificate") : + _("Unable to verify client certificate against CA certificate")); + goto cleanup; + } + + if (status != 0) { + const char *reason = _("Invalid certificate"); + + if (status & GNUTLS_CERT_INVALID) + reason = _("The certificate is not trusted."); + + if (status & GNUTLS_CERT_SIGNER_NOT_FOUND) + reason = _("The certificate hasn't got a known issuer."); + + if (status & GNUTLS_CERT_REVOKED) + reason = _("The certificate has been revoked."); + +#ifndef GNUTLS_1_0_COMPAT + if (status & GNUTLS_CERT_INSECURE_ALGORITHM) + reason = _("The certificate uses an insecure algorithm"); +#endif + + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Our own certificate %s failed validation against %s: %s"), + certFile, cacertFile, reason); + goto cleanup; + } + } + + ret = 0; + +cleanup: + if (cert) + gnutls_x509_crt_deinit(cert); + if (cacert) + gnutls_x509_crt_deinit(cacert); + return ret; +} + + static int virNetTLSContextLoadCredentials(virNetTLSContextPtr ctxt, bool isServer, const char *cacert, @@ -217,6 +346,10 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert, goto error; } + if (requireValidCert && + virNetTLSContextSanityCheckCredentials(isServer, cacert, cert) < 0) + goto error; + if (virNetTLSContextLoadCredentials(ctxt, isServer, cacert, cacrl, cert, key) < 0) goto error; @@ -574,15 +707,21 @@ static int virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt, } if (gnutls_x509_crt_get_expiration_time(cert) < now) { - virNetError(VIR_ERR_SYSTEM_ERROR, "%s", - _("The client certificate has expired")); + /* Warning is reversed from what you expect, since with + * this code it is the Server checking the client and + * vica-verca */ + virNetError(VIR_ERR_SYSTEM_ERROR, "%s", sess->isServer ? + _("The client certificate has expired") : + _("The server certificate has expired")); gnutls_x509_crt_deinit(cert); goto authdeny; } if (gnutls_x509_crt_get_activation_time(cert) > now) { - virNetError(VIR_ERR_SYSTEM_ERROR, "%s", - _("The client certificate is not yet active")); + /* client/server order reversed. see above */ + virNetError(VIR_ERR_SYSTEM_ERROR, "%s", sess->isServer ? + _("The client certificate is not yet active") : + _("The server certificate is not yet active")); gnutls_x509_crt_deinit(cert); goto authdeny; } @@ -756,6 +895,8 @@ virNetTLSSessionPtr virNetTLSSessionNew(virNetTLSContextPtr ctxt, gnutls_transport_set_pull_function(sess->session, virNetTLSSessionPull); + sess->isServer = ctxt->isServer; + return sess; error: -- 1.7.4.4
Gnutls requires that certificates have basic constraints present to be used as a CA certificate. OpenSSL doesn't add this data by default, so add a sanity check to catch this situation. Also validate that the key usage and key purpose constraints contain correct data * src/rpc/virnettlscontext.c: Add sanity checking of certificate constraints --- src/rpc/virnettlscontext.c | 132 +++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 130 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index b0dcfe5..d2b1ded 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -98,6 +98,7 @@ static void virNetTLSLog(int level, const char *str) { static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, + bool isCA, const char *certFile) { gnutls_datum_t data; @@ -105,6 +106,14 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, char *buf = NULL; int ret = -1; time_t now; + int status; + int i; + char *buffer = NULL; + size_t size; + unsigned int usage; + + VIR_DEBUG("isServer %d isCA %d certFile %s", + isServer, isCA, certFile); if ((now = time(NULL)) == ((time_t)-1)) { virReportSystemError(errno, "%s", @@ -148,6 +157,124 @@ static gnutls_x509_crt_t virNetTLSContextSanityCheckCert(bool isServer, goto cleanup; } + status = gnutls_x509_crt_get_basic_constraints(cert, NULL, NULL, NULL); + VIR_DEBUG("Cert %s basic constraints %d", certFile, status); + + if (status > 0) { /* It is a CA cert */ + if (!isCA) { + virNetError(VIR_ERR_SYSTEM_ERROR, isServer ? + _("The certificate %s basic constraints show a CA, but we need one for a server") : + _("The certificate %s basic constraints show a CA, but we need one for a client"), + certFile); + goto cleanup; + } + } else if (status == 0) { /* It is not a CA cert */ + if (isCA) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("The certificate %s basic constraints do not show a CA"), + certFile); + goto cleanup; + } + } else if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) { /* Missing basicConstraints */ + if (isCA) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("The certificate %s is missing basic constraints for a CA"), + certFile); + goto cleanup; + } + } else { /* General error */ + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Unable to query certificate %s basic constraints %s"), + certFile, gnutls_strerror(status)); + goto cleanup; + } + + status = gnutls_x509_crt_get_key_usage(cert, &usage, NULL); + + VIR_DEBUG("Cert %s key usage status %d usage %d", certFile, status, usage); + if (status < 0) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Unable to query certificate %s basic constraints %s"), + certFile, gnutls_strerror(status)); + goto cleanup; + } + + if (usage & GNUTLS_KEY_KEY_CERT_SIGN) { + if (!isCA) { + virNetError(VIR_ERR_SYSTEM_ERROR, isServer ? + _("Certificate server usage is for certificate signing, but wanted a %s certificate") : + _("Certificate client usage is for certificate signing, but wanted a %s certificate"), + certFile); + goto cleanup; + } + } else { + if (isCA) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Certificate %s usage is for not certificate signing"), + certFile); + goto cleanup; + } + } + + for (i = 0 ; ; i++) { + size = 0; + status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, NULL); + + if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) { + VIR_DEBUG("No key purpose data available, skipping checks"); + break; + } + if (status != GNUTLS_E_SHORT_MEMORY_BUFFER) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Unable to query certificate %s key purpose %s"), + certFile, gnutls_strerror(status)); + goto cleanup; + } + + if (VIR_ALLOC_N(buffer, size) < 0) { + virReportOOMError(); + goto cleanup; + } + + status = gnutls_x509_crt_get_key_purpose_oid(cert, i, buffer, &size, NULL); + if (status < 0) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("Unable to query certificate %s key purpose %s"), + certFile, gnutls_strerror(status)); + goto cleanup; + } + + VIR_DEBUG("Key purpose %d %s", status, buffer); + if (STREQ(buffer, GNUTLS_KP_TLS_WWW_SERVER)) { + if (isCA || !isServer) { + virNetError(VIR_ERR_SYSTEM_ERROR, isCA ? + _("Certificate CA purpose is TLS server, but wanted a %s certificate") : + _("Certificate TLS client purpose is TLS server, but wanted a %s certificate"), + certFile); + goto cleanup; + } + } else if (STREQ(buffer, GNUTLS_KP_TLS_WWW_CLIENT)) { + if (isCA || isServer) { + virNetError(VIR_ERR_SYSTEM_ERROR, isCA ? + _("Certificate CA purpose is TLS client, but wanted a %s certificate") : + _("Certificate TLS server purpose is TLS client, but wanted a %s certificate"), + certFile); + goto cleanup; + } + } else if (STRNEQ(buffer, GNUTLS_KP_ANY)) { + virNetError(VIR_ERR_SYSTEM_ERROR, (isCA ? + _("Certificate CA purpose is wrong, wanted a %s certificate") : + (isServer ? + _("Certificate TLS server purpose is wrong, wanted a %s certificate") : + _("Certificate TLS client purpose is wrong, wanted a %s certificate"))), + certFile); + goto cleanup; + } + + VIR_FREE(buffer); + } + + ret = 0; cleanup: @@ -155,6 +282,7 @@ cleanup: gnutls_x509_crt_deinit(cert); cert = NULL; } + VIR_FREE(buffer); VIR_FREE(buf); return cert; } @@ -170,11 +298,11 @@ static int virNetTLSContextSanityCheckCredentials(bool isServer, unsigned int status; if (access(certFile, R_OK) == 0) { - if (!(cert = virNetTLSContextSanityCheckCert(isServer, certFile))) + if (!(cert = virNetTLSContextSanityCheckCert(isServer, false, certFile))) goto cleanup; } if (access(cacertFile, R_OK) == 0) { - if (!(cacert = virNetTLSContextSanityCheckCert(isServer, cacertFile))) + if (!(cacert = virNetTLSContextSanityCheckCert(isServer, true, cacertFile))) goto cleanup; } -- 1.7.4.4
On 07/19/2011 07:55 AM, Daniel P. Berrange wrote:
Gnutls requires that certificates have basic constraints present to be used as a CA certificate. OpenSSL doesn't add this data by default, so add a sanity check to catch this situation. Also validate that the key usage and key purpose constraints contain correct data
* src/rpc/virnettlscontext.c: Add sanity checking of certificate constraints --- src/rpc/virnettlscontext.c | 132 +++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 130 insertions(+), 2 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/19/2011 07:55 AM, Daniel P. Berrange wrote:
If the libvirt daemon or libvirt client is configured with bogus certificates, it is very unhelpful to only find out about this when a TLS connection is actually attempted. Not least because the error messages you get back for failures are incredibly obscure.
This adds some basic sanity checking of certificates at the time the virNetTLSContext object is created. This is at libvirt startup, or when creating a virNetClient instance.
This checks that the certificate expiry/start dates are valid and that the certificate is actually signed by the CA that is loaded.
* src/rpc/virnettlscontext.c: Add certificate sanity checks --- src/rpc/virnettlscontext.c | 149 ++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 145 insertions(+), 4 deletions(-)
@@ -574,15 +707,21 @@ static int virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt, }
if (gnutls_x509_crt_get_expiration_time(cert)< now) { - virNetError(VIR_ERR_SYSTEM_ERROR, "%s", - _("The client certificate has expired")); + /* Warning is reversed from what you expect, since with + * this code it is the Server checking the client and + * vica-verca */
s/vica-verca/vice-versa/ ACK with spelling nit fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Jul 19, 2011 at 08:40:50AM -0600, Eric Blake wrote:
On 07/19/2011 07:55 AM, Daniel P. Berrange wrote:
If the libvirt daemon or libvirt client is configured with bogus certificates, it is very unhelpful to only find out about this when a TLS connection is actually attempted. Not least because the error messages you get back for failures are incredibly obscure.
This adds some basic sanity checking of certificates at the time the virNetTLSContext object is created. This is at libvirt startup, or when creating a virNetClient instance.
This checks that the certificate expiry/start dates are valid and that the certificate is actually signed by the CA that is loaded.
* src/rpc/virnettlscontext.c: Add certificate sanity checks --- src/rpc/virnettlscontext.c | 149 ++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 145 insertions(+), 4 deletions(-)
@@ -574,15 +707,21 @@ static int virNetTLSContextValidCertificate(virNetTLSContextPtr ctxt, }
if (gnutls_x509_crt_get_expiration_time(cert)< now) { - virNetError(VIR_ERR_SYSTEM_ERROR, "%s", - _("The client certificate has expired")); + /* Warning is reversed from what you expect, since with + * this code it is the Server checking the client and + * vica-verca */
s/vica-verca/vice-versa/
ACK with spelling nit fixed.
Thanks, I've pushed these two Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Eric Blake