From d8c6de406940e8f224ebbf2239cdb3a94309d80c Mon Sep 17 00:00:00 2001 From: Funda Wang Date: Sat, 20 Jul 2024 12:04:44 +0800 Subject: [PATCH] Fix CVE-2024-5594 (cherry picked from commit d56c66f44d89dce4720b590a3e98cb72c2d7ffaf) --- CVE-2024-5594.patch | 341 ++++++++++++++++++++++++++++++++++++++++++++ openvpn.spec | 6 +- 2 files changed, 346 insertions(+), 1 deletion(-) create mode 100644 CVE-2024-5594.patch diff --git a/CVE-2024-5594.patch b/CVE-2024-5594.patch new file mode 100644 index 0000000..13b5fec --- /dev/null +++ b/CVE-2024-5594.patch @@ -0,0 +1,341 @@ +Backport of: + +From 90e7a858e5594d9a019ad2b4ac6154124986291a Mon Sep 17 00:00:00 2001 +From: Arne Schwabe +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 +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) +--- + 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 +@@ -1085,6 +1085,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 +@@ -933,6 +933,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 +@@ -184,6 +184,43 @@ check_tls_errors_nco(struct context *c) + + #if P2MP + ++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 ++ { ++ msg(D_PUSH_ERRORS, "WARNING: Received unknown control message: %s", BSTR(buf)); ++ } ++} ++ + /* + * Handle incoming configuration + * messages on the control channel. +@@ -199,43 +236,40 @@ 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); ++ while (BLEN(&buf) > 1) ++ { ++ /* 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)); + +- /* enforce character class restrictions */ +- string_mod(BSTR(&buf), CC_PRINT, CC_CRLF, 0); ++ if (cmdlen < BLEN(&buf)) ++ { ++ /* include the NUL byte and ensure NUL termination */ ++ int cmdlen = (int)strlen(BSTR(&buf)) + 1; + +- 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 +- { +- msg(D_PUSH_ERRORS, "WARNING: Received unknown control message: %s", BSTR(&buf)); ++ /* 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 +@@ -242,6 +242,113 @@ test_buffer_free_gc_two(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) + { +@@ -273,6 +380,9 @@ main(void) + test_buffer_list_teardown), + cmocka_unit_test(test_buffer_free_gc_one), + cmocka_unit_test(test_buffer_free_gc_two), ++ 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); diff --git a/openvpn.spec b/openvpn.spec index f920a32..66fa717 100644 --- a/openvpn.spec +++ b/openvpn.spec @@ -1,6 +1,6 @@ Name: openvpn Version: 2.5.5 -Release: 3 +Release: 4 Summary: A full-featured open source SSL VPN solution License: GPL-2.0-or-later and OpenSSL and SSLeay URL: https://community.openvpn.net/openvpn @@ -8,6 +8,7 @@ Source0: https://swupdate.openvpn.org/community/releases/openvpn-%{version}.tar. # https://github.com/OpenVPN/openvpn/commit/af3e382 Patch0: CVE-2022-0547.patch Patch1: CVE-2024-28882.patch +Patch2: CVE-2024-5594.patch BuildRequires: openssl-devel lz4-devel systemd-devel lzo-devel gcc BuildRequires: iproute pam-devel pkcs11-helper-devel >= 1.11 @@ -124,6 +125,9 @@ fi %{_mandir}/man5/openvpn-examples.5.gz %changelog +* Sat Jul 20 2024 Funda Wang - 2.5.5-4 +- Fix CVE-2024-5594 + * Tue Jul 09 2024 zhangxianting - 2.5.5-3 - Fix CVE-2024-28882 -- Gitee