Skip to contentSkip to conversation

Plug memory leak in mbedTLS backend

PATCH In review Ivo Manca 5 participants 5 messages May 31, 2016

lore
Share & tools
lore

Ivo Manca

author
#0 lore raw

PATCH 1/1 Plug memory leak in mbedTLS backend

Signed-off-by Ivo Manca <[email protected]...>
---
 src/openvpn/ssl_verify_mbedtls.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
src/openvpn/ssl_verify_mbedtls.c +5 -2
index 9c4b51a..522ff68 100644
@@ -162,7 +162,7 @@ backend_x509_get_serial (mbedtls_x509_crt *cert, struct gc_arena *gc)
       cert->serial.len)))
     {
       msg(M_WARN, "Failed to retrieve serial from certificate.");
-      return NULL;
+      goto end;
     }
 
   /* Determine decimal representation length, allocate buffer */

@@ -173,9 +173,12 @@ backend_x509_get_serial (mbedtls_x509_crt *cert, struct gc_arena *gc)
   if (!mbed_ok(mbedtls_mpi_write_string(&serial_mpi, 10, buf, buflen, &buflen)))
     {
       msg(M_WARN, "Failed to write serial to string.");
-      return NULL;
+      buf = NULL;
+      goto end;
     }
 
+end:
+  mbedtls_mpi_free(&serial_mpi);
   return buf;
 }
Signature · 1 line
2.7.4
#1 lore raw
Quoted 21 lines · Ivo Manca
Signed-off-by Ivo Manca <[email protected]...>
---
 src/openvpn/ssl_verify_mbedtls.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
src/openvpn/ssl_verify_mbedtls.c +1 -1
index 9c4b51a..522ff68 100644
@@ -162,7 +162,7 @@ backend_x509_get_serial (mbedtls_x509_crt *cert, struct gc_arena *gc)
       cert->serial.len)))
     {
       msg(M_WARN, "Failed to retrieve serial from certificate.");
-      return NULL;
+      goto end;
     }
 
   /* Determine decimal representation length, allocate buffer */

@@ -173,9 +173,12 @@ backend_x509_get_serial (mbedtls_x509_crt *cert, struct gc_arena *gc)
   if (!mbed_ok(mbedtls_mpi_write_string(&serial_mpi, 10, buf, buflen, &buflen)))
     {

src/openvpn/ssl_verify_mbedtls.c backend_x509_get_serial (mbedtls_x509_crt *cert, struct gc_arena *gc) · line 183

Quoted 10 lines
src/openvpn/ssl_verify_mbedtls.c +4 -1
       msg(M_WARN, "Failed to write serial to string.");
-      return NULL;
+      buf = NULL;
+      goto end;
     }
 
+end:
+  mbedtls_mpi_free(&serial_mpi);
   return buf;
 }

ACK - *but* Ivo did this work for us (Fox-IT), so some external eyes before applying are welcome.

-Steffan

#2 lore raw

Am 31.05.16 um 14:21 schrieb Steffan Karger:

Quoted 28 lines · Ivo Manca
Quoted 27 lines
Signed-off-by Ivo Manca <[email protected]...>
---
 src/openvpn/ssl_verify_mbedtls.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
src/openvpn/ssl_verify_mbedtls.c +3 -2
index 9c4b51a..522ff68 100644
@@ -162,7 +162,7 @@ backend_x509_get_serial (mbedtls_x509_crt *cert, struct gc_arena *gc)
       cert->serial.len)))
     {
       msg(M_WARN, "Failed to retrieve serial from certificate.");
-      return NULL;
+      goto end;
     }
 
   /* Determine decimal representation length, allocate buffer */

@@ -173,9 +173,12 @@ backend_x509_get_serial (mbedtls_x509_crt *cert, struct gc_arena *gc)
   if (!mbed_ok(mbedtls_mpi_write_string(&serial_mpi, 10, buf, buflen, &buflen)))
     {
       msg(M_WARN, "Failed to write serial to string.");
-      return NULL;
+      buf = NULL;
+      goto end;
     }
Quoted 7 lines

src/openvpn/ssl_verify_mbedtls.c backend_x509_get_serial (mbedtls_x509_crt *cert, struct gc_arena *gc) · line 183

Quoted 4 lines
src/openvpn/ssl_verify_mbedtls.c +2 -0
+end:
+  mbedtls_mpi_free(&serial_mpi);
   return buf;
 }

ACK - *but* Ivo did this work for us (Fox-IT), so some external eyes before applying are welcome.

ACK, looks good to me, too.

Arne

#3 lore raw
In reply to Steffan Karger

Hi,

Quoted 7 lines · Steffan Karger

src/openvpn/ssl_verify_mbedtls.c backend_x509_get_serial (mbedtls_x509_crt *cert, struct gc_arena *gc) · line 183

Quoted 4 lines
src/openvpn/ssl_verify_mbedtls.c +2 -0
+end:
+  mbedtls_mpi_free(&serial_mpi);
   return buf;
 }

ACK - *but* Ivo did this work for us (Fox-IT), so some external eyes before applying are welcome.

I was about to write a NAK, spent some time reading mbedtls/bignum.c, and indeed this is needed (zeroize and free X->p), so another ACK here :-)

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
#4 lore raw
In reply to Ivo Manca

From: David Sommerseth <[email protected]...>

Your patch has been applied to the master branch.

commit cd538f2c7a128395284d23f9c30741217075503f

Author: Ivo Manca
Date:   Tue May 31 13:42:00 2016 +0200
     Plug memory leak in mbedTLS backend

     Signed-off-by: Ivo Manca <[email protected]...>
     Acked-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11751
     Signed-off-by: David Sommerseth <[email protected]...>

--
kind regards,

David Sommerseth