From: Arne Schwabe Date: Mon, 27 May 2024 13:02:41 +0000 (+0200) Subject: Properly handle null bytes and invalid characters in control messages X-Git-Tag: v2.6.11~2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=90e7a858e5594d9a019ad2b4ac6154124986291a;p=thirdparty%2Fopenvpn.git Properly handle null bytes and invalid characters in control messages 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 Change-Id: I0d926f910637dabc89bf5fa919dc6beef1eb46d9 Signed-off-by: Arne Schwabe Acked-by: Antonio Quartulli 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 (cherry picked from commit 414f428fa29694090ec4c46b10a8aba419c85659) --- diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c index 2fab9e43f..78a13ef81 100644 --- a/src/openvpn/buffer.c +++ b/src/openvpn/buffer.c @@ -1113,6 +1113,23 @@ string_mod(char *str, const unsigned int inclusive, const unsigned int exclusive 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, diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h index cea4af6b3..d988ef256 100644 --- a/src/openvpn/buffer.h +++ b/src/openvpn/buffer.h @@ -945,6 +945,17 @@ bool string_class(const char *str, const unsigned int inclusive, const unsigned 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, diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 29e812ffd..ce7175246 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -230,6 +230,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. @@ -245,51 +290,41 @@ check_incoming_control_channel(struct context *c) 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")) + while (BLEN(&buf) > 1) { - 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)); + /* 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 diff --git a/tests/unit_tests/openvpn/test_buffer.c b/tests/unit_tests/openvpn/test_buffer.c index 92ac77a78..df1fdcb15 100644 --- a/tests/unit_tests/openvpn/test_buffer.c +++ b/tests/unit_tests/openvpn/test_buffer.c @@ -259,6 +259,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) @@ -289,6 +395,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);