diff mbox series

[Openvpn-devel] mbedtls: fix segfault by calling mbedtls_cipher_free() in cipher_ctx_free()

Message ID 20190816204945.7937-1-a@unstable.cc
State Accepted
Headers
Series [Openvpn-devel] mbedtls: fix segfault by calling mbedtls_cipher_free() in cipher_ctx_free() |

Commit Message

Antonio Quartulli Aug. 16, 2019, 10:49 a.m. UTC 
  Commit ("openssl: Fix compilation without deprecated OpenSSL 1.1 APIs")
has removed the cipher_ctx_cleanup() API, as it is not anymore required
to be a distinct call. However, while doing so it also touched the
mbedtls backend in a wrong way causing a systematic segfault upon connection.

Basically mbedtls_cipher_free(ctx) was moved from the defunct cipher_ctx_cleanup()
to md_ctx_free(), while it was supposed to go into cipher_ctx_free().
This was clearly wrong as also the type of the ctx variable was not
correct anymore.

Fix this mistake by actually moving mbedtls_cipher_free(ctx) to
cipher_ctx_free().

Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 src/openvpn/crypto_mbedtls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Gert Doering Aug. 16, 2019, 8:48 p.m. UTC | #1 
Hi,

On Fri, Aug 16, 2019 at 10:49:45PM +0200, Antonio Quartulli wrote:
> Commit ("openssl: Fix compilation without deprecated OpenSSL 1.1 APIs")
> has removed the cipher_ctx_cleanup() API, as it is not anymore required
> to be a distinct call. However, while doing so it also touched the
> mbedtls backend in a wrong way causing a systematic segfault upon connection.
> 
> Basically mbedtls_cipher_free(ctx) was moved from the defunct cipher_ctx_cleanup()
> to md_ctx_free(), while it was supposed to go into cipher_ctx_free().
> This was clearly wrong as also the type of the ctx variable was not
> correct anymore.
> 
> Fix this mistake by actually moving mbedtls_cipher_free(ctx) to
> cipher_ctx_free().

This looks reasonable, the explanation makes sense and it pacifies
buildbot (gave this a test run on all the bots, and we're back to
"t_client test 1 fails on ubuntu+fedora29", and no more crashes).

So I'm good with it, though I'd welcome a confirmation from Arne
or Steffan.

gert
Arne Schwabe Aug. 17, 2019, 7:56 a.m. UTC | #2 
Am 17.08.2019 um 08:48 schrieb Gert Doering:
> Hi,
>
> On Fri, Aug 16, 2019 at 10:49:45PM +0200, Antonio Quartulli wrote:
>> Commit ("openssl: Fix compilation without deprecated OpenSSL 1.1 APIs")
>> has removed the cipher_ctx_cleanup() API, as it is not anymore required
>> to be a distinct call. However, while doing so it also touched the
>> mbedtls backend in a wrong way causing a systematic segfault upon connection.
>>
>> Basically mbedtls_cipher_free(ctx) was moved from the defunct cipher_ctx_cleanup()
>> to md_ctx_free(), while it was supposed to go into cipher_ctx_free().
>> This was clearly wrong as also the type of the ctx variable was not
>> correct anymore.
>>
>> Fix this mistake by actually moving mbedtls_cipher_free(ctx) to
>> cipher_ctx_free().
> This looks reasonable, the explanation makes sense and it pacifies
> buildbot (gave this a test run on all the bots, and we're back to
> "t_client test 1 fails on ubuntu+fedora29", and no more crashes).
>
> So I'm good with it, though I'd welcome a confirmation from Arne
> or Steffan.

Yeah, sorry that looks like a very stupid mistake from my side :(

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering Aug. 17, 2019, 8:09 a.m. UTC | #3 
Your patch has been applied to the master branch.

As already written, the explanation makes sense, and it fully fixes the
problems observed on the buildslaves -> so, another ACK from me.

Thanks for the quick investigation & fix.

commit 2a74fc3f66bb9f73fc957719d187256922ca003f
Author: Antonio Quartulli
Date:   Fri Aug 16 22:49:45 2019 +0200

     mbedtls: fix segfault by calling mbedtls_cipher_free() in cipher_ctx_free()

     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20190816204945.7937-1-a@unstable.cc>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18781.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index f924323d..648a988e 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -591,6 +591,7 @@  cipher_ctx_new(void)
 void
 cipher_ctx_free(mbedtls_cipher_context_t *ctx)
 {
+    mbedtls_cipher_free(ctx);
     free(ctx);
 }
 
@@ -855,7 +856,6 @@  md_ctx_new(void)
 void
 md_ctx_free(mbedtls_md_context_t *ctx)
 {
-    mbedtls_cipher_free(ctx);
     free(ctx);
 }