mirror of
https://github.com/OpenVPN/openvpn.git
synced 2024-09-19 19:42:30 +02:00
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 <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>
This commit is contained in:
parent
b3a68b85a7
commit
414f428fa2
@ -1087,6 +1087,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,
|
||||
|
@ -943,6 +943,18 @@ 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);
|
||||
|
||||
/**
|
||||
* Returns a copy of a string with certain classes of characters of it replaced with a specified
|
||||
* character.
|
||||
|
@ -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);
|
||||
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));
|
||||
|
||||
if (buf_string_match_head_str(&buf, "AUTH_FAILED"))
|
||||
if (cmdlen < BLEN(&buf))
|
||||
{
|
||||
receive_auth_failed(c, &buf);
|
||||
}
|
||||
else if (buf_string_match_head_str(&buf, "PUSH_"))
|
||||
/* 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))
|
||||
{
|
||||
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);
|
||||
msg(D_PUSH_ERRORS, "WARNING: Received control with invalid characters: %s",
|
||||
format_hex(BPTR(&buf), BLEN(&buf), 256, &gc));
|
||||
}
|
||||
else
|
||||
{
|
||||
msg(D_PUSH_ERRORS, "WARNING: Received unknown control message: %s", BSTR(&buf));
|
||||
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
|
||||
|
@ -354,6 +354,29 @@ test_character_class(void **state)
|
||||
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)
|
||||
{
|
||||
@ -437,6 +460,7 @@ main(void)
|
||||
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)
|
||||
};
|
||||
|
||||
|
Loading…
Reference in New Issue
Block a user