diff mbox series

[Openvpn-devel,v2] Fix message for too long tls-crypt-v2 metadata

Message ID 20221214153414.12671-1-maximilian.fillinger@foxcrypto.com
State Accepted
Headers show
Series [Openvpn-devel,v2] Fix message for too long tls-crypt-v2 metadata | expand

Commit Message

Maximilian Fillinger Dec. 14, 2022, 3:34 p.m. UTC 
The current code only checks if the base64-encoded metadata is at most
980 character. However, that can encode up to 735 bytes of data, while
only up to 733 bytes are allowed. When passing 734 or 735 bytes, openvpn
prints a misleading error message saying that the base64 cannot be
decoded.

This patch checks the decoded length to show an accurate error message.

v2: Remove now-unused macro and fix an off-by-one error.

Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
---
 src/openvpn/base64.h    |  4 ++++
 src/openvpn/tls_crypt.c | 18 +++++++++++-------
 src/openvpn/tls_crypt.h |  2 --
 3 files changed, 15 insertions(+), 9 deletions(-)

Comments

Gert Doering Dec. 15, 2022, 7:30 a.m. UTC | #1 
Acked-by: Arne Schwabe <arne@rfc2549.org>

Thanks for the v2.  It's identical to v1, except for the off-by-one,
and removing the now-obsolete macro, so I've taken the ACK from Arne on v1.

I have only test-compiled (and looked at the diffs).

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

commit 860bf4bf9248077259690a518925ecc14da4b320 (master)
commit f8e576eba078acb1279506d8b6f334667dea1585 (release/2.6)
Author: Max Fillinger
Date:   Wed Dec 14 16:34:14 2022 +0100

     Fix message for too long tls-crypt-v2 metadata

     Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20221214153414.12671-1-maximilian.fillinger@foxcrypto.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25694.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/base64.h b/src/openvpn/base64.h
index f49860fc..7b4224a5 100644
--- a/src/openvpn/base64.h
+++ b/src/openvpn/base64.h
@@ -38,6 +38,10 @@ 
 #define OPENVPN_BASE64_LENGTH(binary_length) \
     ((((8 * binary_length) / 6) + 3) & ~3)
 
+/** Compute the maximal number of bytes encoded in a base64 string. */
+#define OPENVPN_BASE64_DECODED_LENGTH(base64_length) \
+    ((base64_length / 4) * 3)
+
 int openvpn_base64_encode(const void *data, int size, char **str);
 
 int openvpn_base64_decode(const char *str, void *data, int size);
diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index 2fc79111..1e461fcf 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -627,15 +627,11 @@  tls_crypt_v2_write_client_key_file(const char *filename,
     }
     ASSERT(buf_write(&dst, client_key.keys, sizeof(client_key.keys)));
 
-    struct buffer metadata = alloc_buf_gc(TLS_CRYPT_V2_MAX_METADATA_LEN, &gc);
+    struct buffer metadata;
     if (b64_metadata)
     {
-        if (TLS_CRYPT_V2_MAX_B64_METADATA_LEN < strlen(b64_metadata))
-        {
-            msg(M_FATAL,
-                "ERROR: metadata too long (%d bytes, max %u bytes)",
-                (int)strlen(b64_metadata), TLS_CRYPT_V2_MAX_B64_METADATA_LEN);
-        }
+        size_t b64_length = strlen(b64_metadata);
+        metadata = alloc_buf_gc(OPENVPN_BASE64_DECODED_LENGTH(b64_length) + 1, &gc);
         ASSERT(buf_write(&metadata, &TLS_CRYPT_METADATA_TYPE_USER, 1));
         int decoded_len = openvpn_base64_decode(b64_metadata, BEND(&metadata),
                                                 BCAP(&metadata));
@@ -644,10 +640,18 @@  tls_crypt_v2_write_client_key_file(const char *filename,
             msg(M_FATAL, "ERROR: failed to base64 decode provided metadata");
             goto cleanup;
         }
+        if (decoded_len > TLS_CRYPT_V2_MAX_METADATA_LEN - 1)
+        {
+            msg(M_FATAL,
+                "ERROR: metadata too long (%d bytes, max %u bytes)",
+                decoded_len, TLS_CRYPT_V2_MAX_METADATA_LEN - 1);
+            goto cleanup;
+        }
         ASSERT(buf_inc_len(&metadata, decoded_len));
     }
     else
     {
+        metadata = alloc_buf_gc(1 + sizeof(int64_t), &gc);
         int64_t timestamp = htonll((uint64_t)now);
         ASSERT(buf_write(&metadata, &TLS_CRYPT_METADATA_TYPE_TIMESTAMP, 1));
         ASSERT(buf_write(&metadata, &timestamp, sizeof(timestamp)));
diff --git a/src/openvpn/tls_crypt.h b/src/openvpn/tls_crypt.h
index 928ff547..d5c73752 100644
--- a/src/openvpn/tls_crypt.h
+++ b/src/openvpn/tls_crypt.h
@@ -101,8 +101,6 @@ 
 #define TLS_CRYPT_V2_MAX_METADATA_LEN (unsigned)(TLS_CRYPT_V2_MAX_WKC_LEN \
                                                  - (TLS_CRYPT_V2_CLIENT_KEY_LEN + TLS_CRYPT_V2_TAG_SIZE \
                                                     + sizeof(uint16_t)))
-#define TLS_CRYPT_V2_MAX_B64_METADATA_LEN \
-    OPENVPN_BASE64_LENGTH(TLS_CRYPT_V2_MAX_METADATA_LEN - 1)
 
 /**
  * Initialize a key_ctx_bi structure for use with --tls-crypt.