From 61b9251a3aaa65e65c4aab3a6800e884bb3b82f9 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 18 Nov 2015 14:41:35 +0000 Subject: [PATCH 1/4] crypto: fix leak of gnutls_dh_params_t data on credential unload The QCryptoTLSCredsX509 object was not free'ing the allocated gnutls_dh_params_t data when unloading the credentials Signed-off-by: Daniel P. Berrange --- crypto/tlscredsx509.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index dc46bc40f7..c5d1a0de30 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -654,6 +654,10 @@ qcrypto_tls_creds_x509_unload(QCryptoTLSCredsX509 *creds) gnutls_certificate_free_credentials(creds->data); creds->data = NULL; } + if (creds->parent_obj.dh_params) { + gnutls_dh_params_deinit(creds->parent_obj.dh_params); + creds->parent_obj.dh_params = NULL; + } } From 6ef8cd7a4142049707b70b8278aaa9d8ee2bc5f5 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 18 Nov 2015 14:42:40 +0000 Subject: [PATCH 2/4] crypto: fix mistaken setting of Error in success code path The qcrypto_tls_session_check_certificate() method was setting an Error even when the ACL check suceeded. This didn't affect the callers detection of errors because they relied on the function return status, but this did cause a memory leak since the caller would not free an Error they did not expect to be set. Signed-off-by: Daniel P. Berrange --- crypto/tlssession.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/tlssession.c b/crypto/tlssession.c index ffc5c47949..373552942c 100644 --- a/crypto/tlssession.c +++ b/crypto/tlssession.c @@ -304,9 +304,9 @@ qcrypto_tls_session_check_certificate(QCryptoTLSSession *session, allow = qemu_acl_party_is_allowed(acl, session->peername); - error_setg(errp, "TLS x509 ACL check for %s is %s", - session->peername, allow ? "allowed" : "denied"); if (!allow) { + error_setg(errp, "TLS x509 ACL check for %s is denied", + session->peername); goto error; } } From 7b35030eedc26eff82210caa2b0fff2f9d0df453 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 18 Nov 2015 14:44:31 +0000 Subject: [PATCH 3/4] crypto: fix leaks in TLS x509 helper functions The test_tls_get_ipaddr() method forgot to free the returned data from getaddrinfo(). The test_tls_write_cert_chain() method forgot to free the allocated buffer holding the certificate data after writing it out to a file. Signed-off-by: Daniel P. Berrange --- tests/crypto-tls-x509-helpers.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/crypto-tls-x509-helpers.c b/tests/crypto-tls-x509-helpers.c index c5de67baaf..47b4c7ba53 100644 --- a/tests/crypto-tls-x509-helpers.c +++ b/tests/crypto-tls-x509-helpers.c @@ -153,6 +153,7 @@ test_tls_get_ipaddr(const char *addrstr, *datalen = res->ai_addrlen; *data = g_new(char, *datalen); memcpy(*data, res->ai_addr, *datalen); + freeaddrinfo(res); } /* @@ -465,6 +466,7 @@ void test_tls_write_cert_chain(const char *filename, if (!g_file_set_contents(filename, buffer, offset, NULL)) { abort(); } + g_free(buffer); } From 08cb175a24d642a40e41db2fef2892b0a1ab504e Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 18 Nov 2015 15:42:26 +0000 Subject: [PATCH 4/4] crypto: avoid passing NULL to access() syscall The qcrypto_tls_creds_x509_sanity_check() checks whether certs exist by calling access(). It is valid for this method to be invoked with certfile==NULL though, since for client credentials the cert is optional. This caused it to call access(NULL), which happens to be harmless on current Linux, but should none the less be avoided. Signed-off-by: Daniel P. Berrange --- crypto/tlscredsx509.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index c5d1a0de30..d080deb83e 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -485,7 +485,8 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, int ret = -1; memset(cacerts, 0, sizeof(cacerts)); - if (access(certFile, R_OK) == 0) { + if (certFile && + access(certFile, R_OK) == 0) { cert = qcrypto_tls_creds_load_cert(creds, certFile, isServer, errp);