[Openvpn-devel] tls-crypt-v2: bail out if the client key is too small

Message ID 20220628094144.17471-1-a@unstable.cc
State Accepted
Headers show
Series [Openvpn-devel] tls-crypt-v2: bail out if the client key is too small | expand

Commit Message

Antonio Quartulli June 28, 2022, 9:41 a.m. UTC
The tls-crypt-v2 key should be at least 2 bytes long in order to read
the actual length.

Bail out if the key is too short.
Failing to do so will lead to a read out of the buffer boundary.

While at it improve the error message a bit.

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

Comments

Arne Schwabe June 28, 2022, 12:46 p.m. UTC | #1
Am 28.06.22 um 11:41 schrieb Antonio Quartulli:
> The tls-crypt-v2 key should be at least 2 bytes long in order to read
> the actual length.
> 
> Bail out if the key is too short.
> Failing to do so will lead to a read out of the buffer boundary.

Actually not. We read from BEND(), so this is defined for TCP since the 
minimum length there is 3 bytes (pkt len + opcode). For UDP we might 
read past the beginning of the packet but since they are buffers coming 
from the packet stack we have the headroom/tailroom, so might read some 
random data (but not out of bound!).

So we copy some more or less random number into net_len/wkc_len but 
without actually reading from undefined memory.

The next line will then almost definitively fail


     if (!buf_advance(&wrapped_client_key, BLEN(&wrapped_client_key) - 
wkc_len))

Since BLEN(wrapped_client_key) is 0 or 1 or wkc_len has to be 0 or 1 to 
not fail this check

and then in turn

  tls_crypt_v2_unwrap_client_key

will fail at

     if (BLEN(&wrapped_client_key) < sizeof(net_len))


> While at it improve the error message a bit.
> 

Acked-BY: Arne Schwabe <arne@rfc2549.org>
Gert Doering June 28, 2022, 3:33 p.m. UTC | #2
I have taken the "why is this not a problem?" text from Arne's ACK and
included it into the commit message.

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

commit 462339a45089ef655faf02232d7d792def9b8afb (master)
commit ce24bec7e2518d4ea7aa931021454d1191f4906b (release/2.5)
Author: Antonio Quartulli
Date:   Tue Jun 28 11:41:44 2022 +0200

     tls-crypt-v2: bail out if the client key is too small

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


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index 88730a99..2fc79111 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -557,7 +557,8 @@  tls_crypt_v2_extract_client_key(struct buffer *buf,
 
     if (BLEN(&wrapped_client_key) < sizeof(net_len))
     {
-        msg(D_TLS_ERRORS, "failed to read length");
+        msg(D_TLS_ERRORS, "Can not read tls-crypt-v2 client key length");
+        return false;
     }
     memcpy(&net_len, BEND(&wrapped_client_key) - sizeof(net_len),
            sizeof(net_len));