diff --git a/ircd/ircd_string.c b/ircd/ircd_string.c index 74f59f4f..2b1e45bc 100644 --- a/ircd/ircd_string.c +++ b/ircd/ircd_string.c @@ -62,6 +62,13 @@ int string_has_wildcards(const char* str) /** * Check if a given string is a valid UTF-8 encoded string. * + * This answers the encoding question only. C0 control characters are valid + * UTF-8 and are accepted: IRC carries CTCP delimiters (0x01) and the mIRC + * formatting codes (bold 0x02, colour 0x03, reset 0x0F, reverse 0x16, + * monospace 0x11, italic 0x1D, strikethrough 0x1E, underline 0x1F) as bare + * control bytes, and rejecting them here would strip them from every message. + * A caller that wants printable-only text must check for that separately. + * * @param str The string to check. * @return 1 if the string is valid UTF-8, 0 otherwise. */ @@ -73,12 +80,10 @@ int string_is_valid_utf8(const char * str) const unsigned char * bytes = (const unsigned char *)str; while(*bytes) { - if( (// ASCII - // use bytes[0] <= 0x7F to allow ASCII control characters - bytes[0] == 0x09 || - bytes[0] == 0x0A || - bytes[0] == 0x0D || - (0x20 <= bytes[0] && bytes[0] <= 0x7E) + if( (// ASCII: every byte 0x01-0x7F is a well-formed single-byte + // sequence, C0 control characters included. 0x00 cannot occur + // here -- it terminates the string and ends the loop above. + bytes[0] <= 0x7F ) ) { bytes += 1; @@ -159,6 +164,13 @@ int string_is_valid_utf8(const char * str) * The string is modified in place. Caller should ensure str has at least * BUFSIZE bytes available. * + * Note that "no modification was needed" is also what callers get when the + * invalid bytes lie beyond the working buffer: the scan stops at BUFSIZE + * while string_is_valid_utf8() scans the whole string, so a long line whose + * only bad byte is near the end validates as invalid yet sanitizes to -1. + * Callers must therefore treat -1 as "leave the string and its length + * alone", never as a length. + * * @param str The string to sanitize (will be modified). * @return Length of sanitized string, or -1 if no modification was needed. */ @@ -182,9 +194,8 @@ int string_sanitize_utf8(char *str) { seq_len = 0; - /* ASCII printable and common control characters */ - if (in[0] == 0x09 || in[0] == 0x0A || in[0] == 0x0D || - (0x20 <= in[0] && in[0] <= 0x7E)) + /* ASCII: any byte 0x01-0x7F, C0 control characters included */ + if (in[0] <= 0x7F) { seq_len = 1; } diff --git a/ircd/s_bsd.c b/ircd/s_bsd.c index 5975bc2f..d5c88237 100644 --- a/ircd/s_bsd.c +++ b/ircd/s_bsd.c @@ -384,7 +384,16 @@ unsigned int deliver_it(struct Client *cptr, struct MsgQ *buf) * using text messages. We replace invalid bytes with U+FFFD. */ if (text_mode && !string_is_valid_utf8(irc_line)) { - line_len = string_sanitize_utf8(irc_line); + /* string_sanitize_utf8() returns -1 for "nothing modified", + * which happens whenever the offending bytes sit past its + * BUFSIZE working window: the line is then unchanged and its + * original length still stands. Assigning that -1 into + * line_len would hand websocket_encode_frame() a data_len of + * -1, i.e. memcpy(..., SIZE_MAX). The inbound path at the + * bottom of this file already guards it; do the same here. */ + int sanitized_len = string_sanitize_utf8(irc_line); + if (sanitized_len >= 0) + line_len = sanitized_len; } /* Encode as WebSocket frame using client's negotiated/detected mode */ @@ -394,6 +403,9 @@ unsigned int deliver_it(struct Client *cptr, struct MsgQ *buf) Debug((DEBUG_DEBUG, "WebSocket deliver: line_len=%d, frame_len=%d, msg='%.50s'", line_len, frame_len, irc_line)); + if (frame_len <= 0) + break; /* refuse to write a frame we could not encode */ + #ifdef USE_SSL if (cli_socket(cptr).ssl) { int send_result = SSL_write(cli_socket(cptr).ssl, ws_frame, frame_len); diff --git a/ircd/test/ircd_string_cmocka.c b/ircd/test/ircd_string_cmocka.c index df33d0ff..f07eae2a 100644 --- a/ircd/test/ircd_string_cmocka.c +++ b/ircd/test/ircd_string_cmocka.c @@ -11,6 +11,7 @@ #include #include +#include "ircd_defs.h" #include "ircd_string.h" #include "ircd_chattr.h" @@ -558,6 +559,153 @@ static void test_utf8_clamp_null_safe(void **state) assert_int_equal(0, ircd_utf8_clamp(NULL, 5)); } +/* --- string_is_valid_utf8 / string_sanitize_utf8 --- */ + +/* A line as an IRC client actually sends it: CTCP delimiters and the mIRC + * formatting codes are bare C0 control bytes, and every one of them is a + * well-formed single-byte UTF-8 sequence. */ +#define CTCP_ACTION_LINE "\001ACTION waves\001" +#define FORMATTED_LINE "\002bold\002 \037under\037 \003" "04red\003 \035it\035 \017" + +static void test_utf8_valid_plain_ascii(void **state) +{ + (void)state; + assert_int_equal(1, string_is_valid_utf8("")); + assert_int_equal(1, string_is_valid_utf8("hello world")); + assert_int_equal(1, string_is_valid_utf8("tab\there\r\n")); +} + +static void test_utf8_valid_control_characters(void **state) +{ + (void)state; + /* CTCP delimiter and the mIRC formatting codes. */ + assert_int_equal(1, string_is_valid_utf8(CTCP_ACTION_LINE)); + assert_int_equal(1, string_is_valid_utf8(FORMATTED_LINE)); + /* Every byte 0x01-0x7F on its own. */ + { + char one[2] = {0, 0}; + int c; + for (c = 0x01; c <= 0x7F; c++) { + one[0] = (char)c; + assert_int_equal(1, string_is_valid_utf8(one)); + } + } +} + +static void test_utf8_valid_multibyte(void **state) +{ + (void)state; + assert_int_equal(1, string_is_valid_utf8("caf\303\251")); /* U+00E9 */ + assert_int_equal(1, string_is_valid_utf8("\342\202\254")); /* U+20AC */ + assert_int_equal(1, string_is_valid_utf8("\360\237\222\251")); /* U+1F4A9 */ + assert_int_equal(1, string_is_valid_utf8("\355\237\277")); /* U+D7FF */ +} + +static void test_utf8_invalid_sequences(void **state) +{ + (void)state; + assert_int_equal(0, string_is_valid_utf8("\200")); /* lone continuation */ + assert_int_equal(0, string_is_valid_utf8("\300\200")); /* overlong NUL */ + assert_int_equal(0, string_is_valid_utf8("\301\277")); /* overlong */ + assert_int_equal(0, string_is_valid_utf8("\340\200\200")); /* overlong 3-byte */ + assert_int_equal(0, string_is_valid_utf8("\355\240\200")); /* U+D800 surrogate */ + assert_int_equal(0, string_is_valid_utf8("\342\202")); /* truncated 3-byte */ + assert_int_equal(0, string_is_valid_utf8("\364\220\200\200")); /* > U+10FFFF */ + assert_int_equal(0, string_is_valid_utf8("\365\200\200\200")); /* 0xF5 lead */ + assert_int_equal(0, string_is_valid_utf8("\376\377")); /* never valid */ + assert_int_equal(0, string_is_valid_utf8("ok then \377 no")); /* mid-string */ +} + +static void test_utf8_sanitize_leaves_control_codes_alone(void **state) +{ + char buf[BUFSIZE]; + (void)state; + + strcpy(buf, FORMATTED_LINE); + assert_int_equal(-1, string_sanitize_utf8(buf)); /* -1 == nothing modified */ + assert_string_equal(buf, FORMATTED_LINE); + + strcpy(buf, CTCP_ACTION_LINE); + assert_int_equal(-1, string_sanitize_utf8(buf)); + assert_string_equal(buf, CTCP_ACTION_LINE); +} + +static void test_utf8_sanitize_replaces_invalid_bytes(void **state) +{ + char buf[BUFSIZE]; + (void)state; + + /* One bad byte becomes the 3-byte U+FFFD, so the string grows by two. */ + strcpy(buf, "bad\377end"); + assert_int_equal((int)strlen("bad") + 3 + (int)strlen("end"), + string_sanitize_utf8(buf)); + assert_string_equal(buf, "bad\357\277\275end"); + + /* Valid multibyte text survives untouched alongside a bad byte: + * "caf" (3) + U+00E9 (2) + U+FFFD for the 0x80 (3) + "!" (1) = 9. */ + strcpy(buf, "caf\303\251\200!"); + assert_int_equal(9, string_sanitize_utf8(buf)); + assert_string_equal(buf, "caf\303\251\357\277\275!"); +} + +static void test_utf8_sanitize_keeps_formatted_line_intact(void **state) +{ + /* The WebSocket text-frame path (s_bsd.c) validates and then sanitizes: + * a formatted line must come out of both steps byte-for-byte unchanged. */ + char buf[BUFSIZE]; + (void)state; + + strcpy(buf, "\002Rubin\002: Pong!"); + assert_int_equal(1, string_is_valid_utf8(buf)); + assert_int_equal(-1, string_sanitize_utf8(buf)); + assert_string_equal(buf, "\002Rubin\002: Pong!"); +} + + +static void test_utf8_sanitize_returns_minus_one_past_its_window(void **state) +{ + /* string_sanitize_utf8() only scans until its output buffer is nearly + * full (BUFSIZE), while string_is_valid_utf8() scans the whole string. + * A long line whose only bad byte sits past that window is therefore + * "invalid" but sanitizes to -1 == "nothing modified". s_bsd.c used to + * assign that straight into the WebSocket frame length, i.e. memcpy() of + * SIZE_MAX bytes; pin the contract here so it stays visible. */ + char buf[BUFSIZE * 4]; + size_t i; + (void)state; + + for (i = 0; i < BUFSIZE + 100; i++) + buf[i] = 'x'; + buf[i++] = (char)0xFF; /* the one malformed byte, past the window */ + buf[i] = '\0'; + + assert_int_equal(0, string_is_valid_utf8(buf)); + assert_int_equal(-1, string_sanitize_utf8(buf)); + assert_int_equal(BUFSIZE + 101, (int)strlen(buf)); /* left untouched */ +} + +static void test_utf8_sanitize_truncates_at_a_sequence_boundary(void **state) +{ + /* A bad byte inside the window is replaced, and the result is clamped to + * the working buffer without splitting a multibyte sequence. */ + char buf[BUFSIZE * 4]; + size_t i; + int n; + (void)state; + + buf[0] = (char)0x80; /* lone continuation, inside the window */ + for (i = 1; i < BUFSIZE * 2; i++) + buf[i] = 'z'; + buf[i] = '\0'; + + n = string_sanitize_utf8(buf); + assert_true(n > 0); + assert_true(n < BUFSIZE); + assert_int_equal(n, (int)strlen(buf)); + assert_int_equal(1, string_is_valid_utf8(buf)); +} + + int main(void) { const struct CMUnitTest tests[] = { @@ -630,6 +778,17 @@ int main(void) cmocka_unit_test(test_json_escape_passthrough), cmocka_unit_test(test_json_escape_specials), cmocka_unit_test(test_json_escape_truncates_cleanly), + + /* string_is_valid_utf8 / string_sanitize_utf8 */ + cmocka_unit_test(test_utf8_valid_plain_ascii), + cmocka_unit_test(test_utf8_valid_control_characters), + cmocka_unit_test(test_utf8_valid_multibyte), + cmocka_unit_test(test_utf8_invalid_sequences), + cmocka_unit_test(test_utf8_sanitize_leaves_control_codes_alone), + cmocka_unit_test(test_utf8_sanitize_replaces_invalid_bytes), + cmocka_unit_test(test_utf8_sanitize_keeps_formatted_line_intact), + cmocka_unit_test(test_utf8_sanitize_returns_minus_one_past_its_window), + cmocka_unit_test(test_utf8_sanitize_truncates_at_a_sequence_boundary), }; return cmocka_run_group_tests(tests, NULL, NULL); diff --git a/ircd/websocket.c b/ircd/websocket.c index 0b9dec27..7a344def 100644 --- a/ircd/websocket.c +++ b/ircd/websocket.c @@ -673,6 +673,11 @@ int websocket_encode_frame(const char *data, int data_len, int pos = 0; int opcode = text_mode ? WS_OPCODE_TEXT : WS_OPCODE_BINARY; + /* data_len feeds an unchecked memcpy() below; a negative value would be + * a SIZE_MAX copy. Refuse rather than trust the caller. */ + if (data_len < 0) + return -1; + /* First byte: FIN + opcode */ frame[pos++] = WS_FIN | opcode;