diff mbox series

[Openvpn-devel] Ensure n = 2 is set in key2 structer in tls_crypt_v2_unwrap_client_key

Message ID 20230309120031.3780130-1-arne@rfc2549.org
State Accepted
Headers
Series [Openvpn-devel] Ensure n = 2 is set in key2 structer in tls_crypt_v2_unwrap_client_key |

Commit Message

Arne Schwabe March 9, 2023, noon UTC 
  The ASSERT in xor_key2 assumes that all methods that load a key2 struct
correctly set n=2. However, tls_crypt_v2_unwrap_client_key loads a key
without setting n = 2, trigerring the assert.

Closes and reported in https://github.com/OpenVPN/openvpn/issues/272

Change-Id: Iaeb163d83b95818e0b26faf9d25e7737dc8ecb23
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/tls_crypt.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Antonio Quartulli March 9, 2023, 1:11 p.m. UTC | #1 
Hi,

On 09/03/2023 13:00, Arne Schwabe wrote:
> The ASSERT in xor_key2 assumes that all methods that load a key2 struct
> correctly set n=2. However, tls_crypt_v2_unwrap_client_key loads a key
> without setting n = 2, trigerring the assert.

trigerring -> triggering

> 
> Closes and reported in https://github.com/OpenVPN/openvpn/issues/272
> 
> Change-Id: Iaeb163d83b95818e0b26faf9d25e7737dc8ecb23
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

I can easily reproduce the issue ad verify that indeed the patch fixes it.

By checking the code I can see that we set 'n' whenever we initialize 
the keys member. However, this was not happening in 
tls_crypt_v2_unwrap_client_key() thus leading to the assert being triggered.

Acked-by: Antonio Quartulli <a@unstable.cc>

> ---
>   src/openvpn/tls_crypt.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
> index 8882d5de0..4f22f8af7 100644
> --- a/src/openvpn/tls_crypt.c
> +++ b/src/openvpn/tls_crypt.c
> @@ -533,6 +533,7 @@ tls_crypt_v2_unwrap_client_key(struct key2 *client_key, struct buffer *metadata,
>       }
>       memcpy(&client_key->keys, BPTR(&plaintext), sizeof(client_key->keys));
>       ASSERT(buf_advance(&plaintext, sizeof(client_key->keys)));
> +    client_key->n = 2;
>   
>       if (!buf_copy(metadata, &plaintext))
>       {
Gert Doering March 9, 2023, 6:55 p.m. UTC | #2 
Trivial enough :-) - wording fixes applied ("structer", "trigerring").

Interesting that Antonio could reproduce this while it "just works"
in my test beds.

Your patch has been applied to the master and release/2.6 branch.

commit 85832307fcb41c229ccb7ba83984726757eb32f7 (master)
commit ae6068842854278de70264218516b0e4fcdfc6d9 (release/2.6)
Author: Arne Schwabe
Date:   Thu Mar 9 13:00:31 2023 +0100

     Ensure n = 2 is set in key2 struct in tls_crypt_v2_unwrap_client_key

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


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index 8882d5de0..4f22f8af7 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -533,6 +533,7 @@  tls_crypt_v2_unwrap_client_key(struct key2 *client_key, struct buffer *metadata,
     }
     memcpy(&client_key->keys, BPTR(&plaintext), sizeof(client_key->keys));
     ASSERT(buf_advance(&plaintext, sizeof(client_key->keys)));
+    client_key->n = 2;
 
     if (!buf_copy(metadata, &plaintext))
     {