[Qemu-devel] [PULL v1 (for 2.5) 0/4] Fix misc memory leaks & bugs in crypto code

[Qemu-devel] [PULL v1 (for 2.5) 0/4] Fix misc memory leaks & bugs in crypto code

[Daniel P. Berrange]

Running test-crypto-tlscredsx509 & test-crypto-tlssession under
valgrind identified 3 memory leaks and one other bug. This short
series fixes them.

The following changes since commit c27e9014d56fa4880e7d741275d887c3a5949997:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-vnc-20151116-1' into staging (2015-11-17 12:34:07 +0000)

are available in the git repository at:

  git://github.com/berrange/qemu.git tags/qcrypto-fixes-20151118-1

for you to fetch changes up to 08cb175a24d642a40e41db2fef2892b0a1ab504e:

  crypto: avoid passing NULL to access() syscall (2015-11-18 15:42:26 +0000)

----------------------------------------------------------------
Pull qcrypto fixes 2015/11/18 v1

----------------------------------------------------------------

Daniel P. Berrange (4):
  crypto: fix leak of gnutls_dh_params_t data on credential unload
  crypto: fix mistaken setting of Error in success code path
  crypto: fix leaks in TLS x509 helper functions
  crypto: avoid passing NULL to access() syscall

 crypto/tlscredsx509.c           | 7 ++++++-
 crypto/tlssession.c             | 4 ++--
 tests/crypto-tls-x509-helpers.c | 2 ++
 3 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.5.0

[Qemu-devel] [PULL v1 (for 2.5) 1/4] crypto: fix leak of gnutls_dh_params_t data on credential unload

[Daniel P. Berrange]

The QCryptoTLSCredsX509 object was not free'ing the allocated
gnutls_dh_params_t data when unloading the credentials

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/tlscredsx509.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index dc46bc4..c5d1a0d 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;
+    }
 }
 
 
-- 
2.5.0

[Qemu-devel] [PULL v1 (for 2.5) 2/4] crypto: fix mistaken setting of Error in success code path

[Daniel P. Berrange]

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 <berrange@redhat.com>
---
 crypto/tlssession.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index ffc5c47..3735529 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;
                 }
             }
-- 
2.5.0

[Qemu-devel] [PULL v1 (for 2.5) 3/4] crypto: fix leaks in TLS x509 helper functions

[Daniel P. Berrange]

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 <berrange@redhat.com>
---
 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 c5de67b..47b4c7b 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);
 }
 
 
-- 
2.5.0

[Qemu-devel] [PULL v1 (for 2.5) 4/4] crypto: avoid passing NULL to access() syscall

[Daniel P. Berrange]

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 <berrange@redhat.com>
---
 crypto/tlscredsx509.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index c5d1a0d..d080deb 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);
-- 
2.5.0

Re: [Qemu-devel] [PULL v1 (for 2.5) 0/4] Fix misc memory leaks & bugs in crypto code

[Peter Maydell]

On 18 November 2015 at 15:47, Daniel P. Berrange <berrange@redhat.com> wrote:
> Running test-crypto-tlscredsx509 & test-crypto-tlssession under
> valgrind identified 3 memory leaks and one other bug. This short
> series fixes them.
>
> The following changes since commit c27e9014d56fa4880e7d741275d887c3a5949997:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/pull-vnc-20151116-1' into staging (2015-11-17 12:34:07 +0000)
>
> are available in the git repository at:
>
>   git://github.com/berrange/qemu.git tags/qcrypto-fixes-20151118-1
>
> for you to fetch changes up to 08cb175a24d642a40e41db2fef2892b0a1ab504e:
>
>   crypto: avoid passing NULL to access() syscall (2015-11-18 15:42:26 +0000)
>
> ----------------------------------------------------------------
> Pull qcrypto fixes 2015/11/18 v1
>
> ----------------------------------------------------------------
>
> Daniel P. Berrange (4):
>   crypto: fix leak of gnutls_dh_params_t data on credential unload
>   crypto: fix mistaken setting of Error in success code path
>   crypto: fix leaks in TLS x509 helper functions
>   crypto: avoid passing NULL to access() syscall
>
>  crypto/tlscredsx509.c           | 7 ++++++-
>  crypto/tlssession.c             | 4 ++--
>  tests/crypto-tls-x509-helpers.c | 2 ++
>  3 files changed, 10 insertions(+), 3 deletions(-)

Applied, thanks.

-- PMM