diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 71b71672801..40b7cc4286b 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -290,41 +290,14 @@ check_incoming_control_channel(struct context *c) struct buffer buf = alloc_buf_gc(len, &gc); if (tls_rec_payload(c->c2.tls_multi, &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)); - - if (cmdlen < BLEN(&buf)) - { - /* include the NUL byte and ensure NUL termination */ - int cmdlen = (int)strlen(BSTR(&buf)) + 1; + struct buffer cmdbuf = extract_command_buffer(&buf, &gc); - /* 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 + if (cmdbuf.len > 0) { - msg(D_PUSH_ERRORS, "WARNING: Ignoring control channel " - "message command without NUL termination"); + parse_incoming_control_channel_command(c, &cmdbuf); } - buf_advance(&buf, cmdlen); } } else diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c index 2ec0b2ff776..689cd7f99f9 100644 --- a/src/openvpn/ssl_pkt.c +++ b/src/openvpn/ssl_pkt.c @@ -557,3 +557,43 @@ check_session_id_hmac(struct tls_pre_decrypt_state *state, } return false; } + +struct buffer +extract_command_buffer(struct buffer *buf, struct gc_arena *gc) +{ + /* 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)) + { + buf_advance(buf, cmdlen); + /* Return empty buffer */ + struct buffer empty = { 0 }; + return empty; + } + + /* include the NUL byte and ensure NUL termination */ + cmdlen += 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); + + /* Remove \r and \n at the end of the buffer to avoid + * problems with scripts and other that add extra \r and \n */ + buf_chomp(&cmdbuf); + + /* check we have only printable characters or null byte in the + * command string and no newlines */ + if (!string_check_buf(&cmdbuf, CC_PRINT | CC_NULL, CC_CRLF)) + { + msg(D_PUSH_ERRORS, "WARNING: Received control with invalid characters: %s", + format_hex(BPTR(&cmdbuf), BLEN(&cmdbuf), 256, gc)); + cmdbuf.len = 0; + } + + buf_advance(buf, cmdlen); + return cmdbuf; +} diff --git a/src/openvpn/ssl_pkt.h b/src/openvpn/ssl_pkt.h index 88b9e8c38aa..c8a27fba9d7 100644 --- a/src/openvpn/ssl_pkt.h +++ b/src/openvpn/ssl_pkt.h @@ -230,6 +230,20 @@ tls_reset_standalone(struct tls_wrap_ctx *ctx, uint8_t header, bool request_resend_wkc); + +/** + * Extracts a control channel message from buf and adjusts the size of + * buf after the message has been extracted + * @param buf The buffer the message should be extracted from + * @param gc gc_arena to be used for the returned buffer and displaying + * diagnostic messages + * @return A buffer with a control channel message or a buffer with + * with length 0 if there is no message or the message has + * invalid characters. + */ +struct buffer +extract_command_buffer(struct buffer *buf, struct gc_arena *gc); + static inline const char * packet_opcode_name(int op) { diff --git a/tests/unit_tests/openvpn/test_pkt.c b/tests/unit_tests/openvpn/test_pkt.c index 1084d66d598..741c982ac14 100644 --- a/tests/unit_tests/openvpn/test_pkt.c +++ b/tests/unit_tests/openvpn/test_pkt.c @@ -626,6 +626,40 @@ test_generate_reset_packet_tls_auth(void **ut_state) free_tas(&tas_server); } +static void +test_extract_control_message(void **ut_state) +{ + struct gc_arena gc = gc_new(); + struct buffer input_buf = alloc_buf_gc(1024, &gc); + + /* This message will have a \0x00 at the end since it is a C string */ + const char input[] = "valid control message\r\n\0\0Invalid\r\none\0valid one again"; + + buf_write(&input_buf, input, sizeof(input)); + struct buffer cmd1 = extract_command_buffer(&input_buf, &gc); + struct buffer cmd2 = extract_command_buffer(&input_buf, &gc); + struct buffer cmd3 = extract_command_buffer(&input_buf, &gc); + struct buffer cmd4 = extract_command_buffer(&input_buf, &gc); + struct buffer cmd5 = extract_command_buffer(&input_buf, &gc); + + assert_string_equal(BSTR(&cmd1), "valid control message"); + /* empty message with just a \0x00 */ + assert_int_equal(cmd2.len, 1); + assert_string_equal(BSTR(&cmd2), ""); + assert_int_equal(cmd3.len, 0); + assert_string_equal(BSTR(&cmd4), "valid one again"); + assert_int_equal(cmd5.len, 0); + + const uint8_t nonull[6] = { 'n', 'o', ' ', 'N', 'U', 'L'}; + struct buffer nonull_buf = alloc_buf_gc(1024, &gc); + + buf_write(&nonull_buf, nonull, sizeof(nonull)); + struct buffer nonullcmd = extract_command_buffer(&nonull_buf, &gc); + assert_int_equal(nonullcmd.len, 0); + + gc_free(&gc); +} + int main(void) { @@ -640,6 +674,7 @@ main(void) cmocka_unit_test(test_verify_hmac_tls_auth), cmocka_unit_test(test_generate_reset_packet_plain), cmocka_unit_test(test_generate_reset_packet_tls_auth), + cmocka_unit_test(test_extract_control_message) }; #if defined(ENABLE_CRYPTO_OPENSSL)