Re: [libvirt] [PATCH] 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 | 144 ++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 140 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 1120e1e..b778550 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,131 @@ 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, + _("Unable to import %s certificate %s"), + isServer ? "server" : "client", certFile);

+ goto cleanup; + } + + if (gnutls_x509_crt_get_expiration_time(cert) < now) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("The %s certificate %s has expired"), + isServer ? "server" : "client", certFile); + goto cleanup; + } + + if (gnutls_x509_crt_get_activation_time(cert) > now) { + virNetError(VIR_ERR_SYSTEM_ERROR, + _("The %s certificate %s is not yet active"), + isServer ? "server" : "client", 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, + _("Unable to verify %s certificate against CA certificate"), + isServer ? "server": "client"); + 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 +343,9 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert, goto error; } + if (virNetTLSContextSanityCheckCredentials(isServer, cacert, cert) < 0) + goto error; + if (virNetTLSContextLoadCredentials(ctxt, isServer, cacert, cacrl, cert, key) < 0) goto error; @@ -574,15 +703,20 @@ 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, + _("The %s certificate has expired"), + sess->isServer ? "client" : "server"); 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")); + virNetError(VIR_ERR_SYSTEM_ERROR, + _("The %s certificate is not yet active"), + sess->isServer ? "client" : "server"); gnutls_x509_crt_deinit(cert); goto authdeny; } @@ -756,6 +890,8 @@ virNetTLSSessionPtr virNetTLSSessionNew(virNetTLSContextPtr ctxt, gnutls_transport_set_pull_function(sess->session, virNetTLSSessionPull); + sess->isServer = ctxt->isServer; + return sess; error: