[Openvpn-devel] Fix use-after-free with EVP_CIPHER_free

Message ID 20230601095721.4065834-1-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel] Fix use-after-free with EVP_CIPHER_free | expand

Commit Message

Arne Schwabe June 1, 2023, 9:57 a.m. UTC
In many scenerios the context will still have a reference to the cipher, so
this use-after-free does not explode but it is still wrong.

Change-Id: I59002d6613eaef36d5a47b20b56073e399cfa1df
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto_openssl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Antonio Quartulli June 1, 2023, 10:01 a.m. UTC | #1
Hi,

On 01/06/2023 11:57, Arne Schwabe wrote:
> In many scenerios the context will still have a reference to the cipher, so

scenerios -> scenarios

> this use-after-free does not explode but it is still wrong.

Good catch - glad we're so lucky :-)

> 
> Change-Id: I59002d6613eaef36d5a47b20b56073e399cfa1df
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   src/openvpn/crypto_openssl.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index c2ac80b74..8fe56fc78 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -839,11 +839,12 @@ cipher_ctx_init(EVP_CIPHER_CTX *ctx, const uint8_t *key,
>           crypto_msg(M_FATAL, "EVP cipher init #2");
>       }
>   
> -    EVP_CIPHER_free(kt);
>       /* make sure we used a big enough key */
>       ASSERT(EVP_CIPHER_CTX_key_length(ctx) <= EVP_CIPHER_key_length(kt));
> +    EVP_CIPHER_free(kt);
>   }
>   
> +

This is not required - please remove it before merging.

>   int
>   cipher_ctx_iv_length(const EVP_CIPHER_CTX *ctx)
>   {

Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering June 3, 2023, 8:03 a.m. UTC | #2
I have not tested this beyond a simple "does it compile" (it does) - and
the fix is quite "obviously correct" - good find.

I have also removed the spurious extra blank line on merge.

Your patch has been applied to the master and release/2.6 branch
(relevant code is not part of 2.5).

commit 13f5e615310ea64ab69f521e622a10f2d0ad3f4e (master)
commit 205c66bd0ed2661c47b9fe7317089fbb09cc7aa4 (release/2.6)
Author: Arne Schwabe
Date:   Thu Jun 1 11:57:21 2023 +0200

     Fix use-after-free with EVP_CIPHER_free

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index c2ac80b74..8fe56fc78 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -839,11 +839,12 @@  cipher_ctx_init(EVP_CIPHER_CTX *ctx, const uint8_t *key,
         crypto_msg(M_FATAL, "EVP cipher init #2");
     }
 
-    EVP_CIPHER_free(kt);
     /* make sure we used a big enough key */
     ASSERT(EVP_CIPHER_CTX_key_length(ctx) <= EVP_CIPHER_key_length(kt));
+    EVP_CIPHER_free(kt);
 }
 
+
 int
 cipher_ctx_iv_length(const EVP_CIPHER_CTX *ctx)
 {