| Message ID | 20190816204945.7937-1-a@unstable.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] mbedtls: fix segfault by calling mbedtls_cipher_free() in cipher_ctx_free() | expand |
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
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>
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 --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); }
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(-)