From 90e7a858e5594d9a019ad2b4ac6154124986291a Mon Sep 17 00:00:00 2001
From: Arne Schwabe <arne@rfc2549.org>
Date: Mon, 27 May 2024 15:02:41 +0200
Subject: [PATCH] Properly handle null bytes and invalid characters in control
 messages
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This makes OpenVPN more picky in accepting control message in two aspects:
- Characters are checked in the whole buffer and not until the first
  NUL byte
- if the message contains invalid characters, we no longer continue
  evaluating a fixed up version of the message but rather stop
  processing it completely.

Previously it was possible to get invalid characters to end up in log
files or on a terminal.

This also prepares the logic a bit in the direction of having a proper
framing of control messages separated by null bytes instead of relying
on the TLS framing for that. All OpenVPN implementations write the 0
bytes between control commands.

This patch also include several improvement suggestion from Reynir
(thanks!).

CVE: 2024-5594

Reported-By: Reynir Björnsson <reynir@reynir.dk>
Change-Id: I0d926f910637dabc89bf5fa919dc6beef1eb46d9
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <a@unstable.cc>

Message-Id: <20240619103004.56460-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28791.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit 414f428fa29694090ec4c46b10a8aba419c85659)
---
 src/openvpn/buffer.c                   |  17 ++++
 src/openvpn/buffer.h                   |  11 +++
 src/openvpn/forward.c                  | 121 ++++++++++++++++---------
 tests/unit_tests/openvpn/test_buffer.c | 109 ++++++++++++++++++++++
 4 files changed, 215 insertions(+), 43 deletions(-)

--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -1115,6 +1115,23 @@ string_mod(char *str, const unsigned int
     return ret;
 }
 
+bool
+string_check_buf(struct buffer *buf, const unsigned int inclusive, const unsigned int exclusive)
+{
+    ASSERT(buf);
+
+    for (int i = 0; i < BLEN(buf); i++)
+    {
+        char c = BSTR(buf)[i];
+
+        if (!char_inc_exc(c, inclusive, exclusive))
+        {
+            return false;
+        }
+    }
+    return true;
+}
+
 const char *
 string_mod_const(const char *str,
                  const unsigned int inclusive,
--- a/src/openvpn/buffer.h
+++ b/src/openvpn/buffer.h
@@ -945,6 +945,17 @@ bool string_class(const char *str, const
 
 bool string_mod(char *str, const unsigned int inclusive, const unsigned int exclusive, const char replace);
 
+/**
+ * Check a buffer if it only consists of allowed characters.
+ *
+ * @param buf The buffer to be checked.
+ * @param inclusive The character classes that are allowed.
+ * @param exclusive Character classes that are not allowed even if they are also in inclusive.
+ * @return True if the string consists only of allowed characters, false otherwise.
+ */
+bool
+string_check_buf(struct buffer *buf, const unsigned int inclusive, const unsigned int exclusive);
+
 const char *string_mod_const(const char *str,
                              const unsigned int inclusive,
                              const unsigned int exclusive,
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -232,6 +232,51 @@ check_tls(struct context *c)
     }
 }
 
+static void
+parse_incoming_control_channel_command(struct context *c, struct buffer *buf)
+{
+    if (buf_string_match_head_str(buf, "AUTH_FAILED"))
+    {
+        receive_auth_failed(c, buf);
+    }
+    else if (buf_string_match_head_str(buf, "PUSH_"))
+    {
+        incoming_push_message(c, buf);
+    }
+    else if (buf_string_match_head_str(buf, "RESTART"))
+    {
+        server_pushed_signal(c, buf, true, 7);
+    }
+    else if (buf_string_match_head_str(buf, "HALT"))
+    {
+        server_pushed_signal(c, buf, false, 4);
+    }
+    else if (buf_string_match_head_str(buf, "INFO_PRE"))
+    {
+        server_pushed_info(c, buf, 8);
+    }
+    else if (buf_string_match_head_str(buf, "INFO"))
+    {
+        server_pushed_info(c, buf, 4);
+    }
+    else if (buf_string_match_head_str(buf, "CR_RESPONSE"))
+    {
+        receive_cr_response(c, buf);
+    }
+    else if (buf_string_match_head_str(buf, "AUTH_PENDING"))
+    {
+        receive_auth_pending(c, buf);
+    }
+    else if (buf_string_match_head_str(buf, "EXIT"))
+    {
+        receive_exit_message(c);
+    }
+    else
+    {
+        msg(D_PUSH_ERRORS, "WARNING: Received unknown control message: %s", BSTR(buf));
+    }
+}
+
 /*
  * Handle incoming configuration
  * messages on the control channel.
@@ -247,51 +292,41 @@ check_incoming_control_channel(struct co
     struct buffer buf = alloc_buf_gc(len, &gc);
     if (tls_rec_payload(c->c2.tls_multi, &buf))
     {
-        /* force null termination of message */
-        buf_null_terminate(&buf);
-
-        /* enforce character class restrictions */
-        string_mod(BSTR(&buf), CC_PRINT, CC_CRLF, 0);
 
-        if (buf_string_match_head_str(&buf, "AUTH_FAILED"))
-        {
-            receive_auth_failed(c, &buf);
-        }
-        else if (buf_string_match_head_str(&buf, "PUSH_"))
-        {
-            incoming_push_message(c, &buf);
-        }
-        else if (buf_string_match_head_str(&buf, "RESTART"))
-        {
-            server_pushed_signal(c, &buf, true, 7);
-        }
-        else if (buf_string_match_head_str(&buf, "HALT"))
-        {
-            server_pushed_signal(c, &buf, false, 4);
-        }
-        else if (buf_string_match_head_str(&buf, "INFO_PRE"))
-        {
-            server_pushed_info(c, &buf, 8);
-        }
-        else if (buf_string_match_head_str(&buf, "INFO"))
+        while (BLEN(&buf) > 1)
         {
-            server_pushed_info(c, &buf, 4);
-        }
-        else if (buf_string_match_head_str(&buf, "CR_RESPONSE"))
-        {
-            receive_cr_response(c, &buf);
-        }
-        else if (buf_string_match_head_str(&buf, "AUTH_PENDING"))
-        {
-            receive_auth_pending(c, &buf);
-        }
-        else if (buf_string_match_head_str(&buf, "EXIT"))
-        {
-            receive_exit_message(c);
-        }
-        else
-        {
-            msg(D_PUSH_ERRORS, "WARNING: Received unknown control message: %s", BSTR(&buf));
+            /* commands on the control channel are seperated by 0x00 bytes.
+             * cmdlen does not include the 0 byte of the string */
+            int cmdlen = (int)strnlen(BSTR(&buf), BLEN(&buf));
+
+            if (cmdlen < BLEN(&buf))
+            {
+                /* include the NUL byte and ensure NUL termination */
+                int cmdlen = (int)strlen(BSTR(&buf)) + 1;
+
+                /* Construct a buffer that only holds the current command and
+                 * its closing NUL byte */
+                struct buffer cmdbuf = alloc_buf_gc(cmdlen, &gc);
+                buf_write(&cmdbuf, BPTR(&buf), cmdlen);
+
+                /* check we have only printable characters or null byte in the
+                 * command string and no newlines */
+                if (!string_check_buf(&buf, CC_PRINT | CC_NULL, CC_CRLF))
+                {
+                    msg(D_PUSH_ERRORS, "WARNING: Received control with invalid characters: %s",
+                        format_hex(BPTR(&buf), BLEN(&buf), 256, &gc));
+                }
+                else
+                {
+                    parse_incoming_control_channel_command(c, &cmdbuf);
+                }
+            }
+            else
+            {
+                msg(D_PUSH_ERRORS, "WARNING: Ignoring control channel "
+                    "message command without NUL termination");
+            }
+            buf_advance(&buf, cmdlen);
         }
     }
     else
--- a/tests/unit_tests/openvpn/test_buffer.c
+++ b/tests/unit_tests/openvpn/test_buffer.c
@@ -261,6 +261,112 @@ test_buffer_gc_realloc(void **state)
     gc_free(&gc);
 }
 
+static void
+test_character_class(void **state)
+{
+    char buf[256];
+    strcpy(buf, "There is \x01 a nice 1234 year old tr\x7f ee!");
+    assert_false(string_mod(buf, CC_PRINT, 0, '@'));
+    assert_string_equal(buf, "There is @ a nice 1234 year old tr@ ee!");
+
+    strcpy(buf, "There is \x01 a nice 1234 year old tr\x7f ee!");
+    assert_true(string_mod(buf, CC_ANY, 0, '@'));
+    assert_string_equal(buf, "There is \x01 a nice 1234 year old tr\x7f ee!");
+
+    /* 0 as replace removes characters */
+    strcpy(buf, "There is \x01 a nice 1234 year old tr\x7f ee!");
+    assert_false(string_mod(buf, CC_PRINT, 0, '\0'));
+    assert_string_equal(buf, "There is  a nice 1234 year old tr ee!");
+
+    strcpy(buf, "There is \x01 a nice 1234 year old tr\x7f ee!");
+    assert_false(string_mod(buf, CC_PRINT, CC_DIGIT, '@'));
+    assert_string_equal(buf, "There is @ a nice @@@@ year old tr@ ee!");
+
+    strcpy(buf, "There is \x01 a nice 1234 year old tr\x7f ee!");
+    assert_false(string_mod(buf, CC_ALPHA, CC_DIGIT, '.'));
+    assert_string_equal(buf, "There.is...a.nice......year.old.tr..ee.");
+
+    strcpy(buf, "There is \x01 a 'nice' \"1234\"\n year old \ntr\x7f ee!");
+    assert_false(string_mod(buf, CC_ALPHA|CC_DIGIT|CC_NEWLINE|CC_SINGLE_QUOTE, CC_DOUBLE_QUOTE|CC_BLANK, '.'));
+    assert_string_equal(buf, "There.is...a.'nice'..1234.\n.year.old.\ntr..ee.");
+
+    strcpy(buf, "There is a \\'nice\\' \"1234\" [*] year old \ntree!");
+    assert_false(string_mod(buf, CC_PRINT, CC_BACKSLASH|CC_ASTERISK, '.'));
+    assert_string_equal(buf, "There is a .'nice.' \"1234\" [.] year old .tree!");
+}
+
+
+static void
+test_character_string_mod_buf(void **state)
+{
+    struct gc_arena gc = gc_new();
+
+    struct buffer buf = alloc_buf_gc(1024, &gc);
+
+    const char test1[] =  "There is a nice 1234\x00 year old tree!";
+    buf_write(&buf, test1, sizeof(test1));
+
+    /* allow the null bytes and string but not the ! */
+    assert_false(string_check_buf(&buf, CC_ALNUM | CC_SPACE | CC_NULL, 0));
+
+    /* remove final ! and null byte to pass */
+    buf_inc_len(&buf, -2);
+    assert_true(string_check_buf(&buf, CC_ALNUM | CC_SPACE | CC_NULL, 0));
+
+    /* Check excluding digits works */
+    assert_false(string_check_buf(&buf, CC_ALNUM | CC_SPACE | CC_NULL, CC_DIGIT));
+    gc_free(&gc);
+}
+
+static void
+test_snprintf(void **state)
+{
+    /* we used to have a custom openvpn_snprintf function because some
+     * OS (the comment did not specify which) did not always put the
+     * null byte there. So we unit test this to be sure.
+     *
+     * This probably refers to the MSVC behaviour, see also
+     * https://stackoverflow.com/questions/7706936/is-snprintf-always-null-terminating
+     */
+
+    /* Instead of trying to trick the compiler here, disable the warnings
+     * for this unit test. We know that the results will be truncated
+     * and we want to test that */
+#if defined(__GNUC__)
+/* some clang version do not understand -Wformat-truncation, so ignore the
+ * warning to avoid warnings/errors (-Werror) about unknown pragma/option */
+#if defined(__clang__)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wunknown-warning-option"
+#endif
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-truncation"
+#endif
+
+    char buf[10] = { 'a' };
+    int ret = 0;
+
+    ret = snprintf(buf, sizeof(buf), "0123456789abcde");
+    assert_int_equal(ret, 15);
+    assert_int_equal(buf[9], '\0');
+
+    memset(buf, 'b', sizeof(buf));
+    ret = snprintf(buf, sizeof(buf), "- %d - %d -", 77, 88);
+    assert_int_equal(ret, 11);
+    assert_int_equal(buf[9], '\0');
+
+    memset(buf, 'c', sizeof(buf));
+    ret = snprintf(buf, sizeof(buf), "- %8.2f", 77.8899);
+    assert_int_equal(ret, 10);
+    assert_int_equal(buf[9], '\0');
+
+#if defined(__GNUC__)
+#pragma GCC diagnostic pop
+#if defined(__clang__)
+#pragma clang diagnostic pop
+#endif
+#endif
+}
 
 int
 main(void)
@@ -291,6 +397,9 @@ main(void)
         cmocka_unit_test(test_buffer_free_gc_one),
         cmocka_unit_test(test_buffer_free_gc_two),
         cmocka_unit_test(test_buffer_gc_realloc),
+        cmocka_unit_test(test_character_class),
+        cmocka_unit_test(test_character_string_mod_buf),
+        cmocka_unit_test(test_snprintf)
     };
 
     return cmocka_run_group_tests_name("buffer", tests, NULL, NULL);