From aef2f295aa6ae39878918b538a7e6f40fcf5f793 Mon Sep 17 00:00:00 2001 From: Alex Schumann Date: Fri, 28 Aug 2026 11:15:56 -0700 Subject: [PATCH 1/2] =?UTF-8?q?utf8:=20C0=20control=20characters=20are=20v?= =?UTF-8?q?alid=20UTF-8=20=E2=80=94=20stop=20rewriting=20them=20to=20U+FFF?= =?UTF-8?q?D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit string_is_valid_utf8() accepted only 0x09, 0x0A, 0x0D and 0x20-0x7E as single-byte sequences, so it reported every other C0 control byte as invalid UTF-8, and string_sanitize_utf8() then overwrote each one with U+FFFD. Every byte 0x01-0x7F is a well-formed single-byte UTF-8 sequence. The comment sitting in that very branch already said as much — "use bytes[0] <= 0x7F to allow ASCII control characters" — it was just never applied. IRC carries the CTCP delimiter (0x01) and the mIRC formatting codes (bold 0x02, colour 0x03, reset 0x0F, monospace 0x11, reverse 0x16, italic 0x1D, strikethrough 0x1E, underline 0x1F) as bare control bytes, so the pair silently destroyed formatting and CTCP in two places: - WebSocket clients on the text.ircv3.net subprotocol, both directions (s_bsd.c:386 outbound, :1270 inbound). A browser saw "Nick: Pong!" where a services reply meant a bold nick, and its own /me left the server as "ACTION waves" — mangled for the whole channel, not just for the sender, because the damage happens before the line reaches the IRC parser. Fragmented text frames fared worse still: s_bsd.c:1229 kills the connection instead of sanitizing, so a long enough formatted line from a browser was a disconnect. - Every client, on any network running FEAT_UTF8ONLY: ircd_relay.c (PRIVMSG/NOTICE), m_topic.c, m_kick.c, m_part.c and m_quit.c all route user text through the same pair. Fix the ASCII branch in both functions to accept 0x01-0x7F. 0x00 cannot reach it — both walk a NUL-terminated string and stop there. Behaviour elsewhere is unchanged in practice. m_join.c's FEAT_VALID_UTF8_CHANNELS_ONLY gate goes through string_character_structure_is_sane(), which already admitted control bytes in pure-ASCII names via its !string_contains_non_ascii() arm, and strIsIrcCh() remains the real filter on channel names. m_metadata.c now accepts control bytes in values, which is what "valid UTF-8" means and matches what topics and messages already allow. Rejection of genuinely malformed encodings — lone continuations, overlongs, surrogates, truncated sequences, F5-FF leads — is untouched. Verified against a browser WebSocket client on text.ircv3.net: before, "\002bold\002 \037under\037" and "\001ACTION waves\001" came back with every control byte replaced by U+FFFD; after, both round-trip byte for byte. ircd/test/ircd_string_cmocka.c gains seven cases covering the whole of 0x01-0x7F, the CTCP and formatting codes, valid multibyte text, the malformed encodings that must still be rejected, and sanitize()'s no-modification (-1) and replacement paths. Full build plus all 21 cmocka suites green. Co-Authored-By: Claude Opus 5 --- ircd/ircd_string.c | 22 ++++--- ircd/test/ircd_string_cmocka.c | 113 +++++++++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 9 deletions(-) diff --git a/ircd/ircd_string.c b/ircd/ircd_string.c index 74f59f4f..12aa0fb6 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; @@ -182,9 +187,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/test/ircd_string_cmocka.c b/ircd/test/ircd_string_cmocka.c index df33d0ff..90d4ea3f 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,109 @@ 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!"); +} + + int main(void) { const struct CMUnitTest tests[] = { @@ -630,6 +734,15 @@ 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), }; return cmocka_run_group_tests(tests, NULL, NULL); From 6400a4f4093f600ac13032767fb7a6d097f299ac Mon Sep 17 00:00:00 2001 From: Alex Schumann Date: Fri, 28 Aug 2026 11:35:14 -0700 Subject: [PATCH 2/2] websocket: don't pass string_sanitize_utf8()'s -1 through as a frame length MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit string_sanitize_utf8() returns -1 for "nothing modified". That is not only the trivially-clean case: its copy loop stops once the output buffer is nearly full (`outlen < sizeof(out) - 4`, i.e. ~508 bytes) while string_is_valid_utf8() scans the entire string. A line longer than that whose only malformed byte sits past the window therefore validates as invalid and sanitizes to -1, unchanged. The outbound WebSocket text path assigned that straight into the frame length: if (text_mode && !string_is_valid_utf8(irc_line)) line_len = string_sanitize_utf8(irc_line); /* may be -1 */ frame_len = websocket_encode_frame(irc_line, line_len, ...); websocket_encode_frame() then stores (unsigned char)-1 == 0xFF as the short payload length and runs memcpy(frame + pos, data, (size_t)-1) — a SIZE_MAX copy in a single-process daemon. The inbound sibling a thousand lines down already guards this (`if (new_len > 0) ws_len = new_len;`); the outbound one never did. Reproduced by driving the two functions and websocket_encode_frame verbatim with a 691-byte line — ordinary tags plus 600 characters plus one stray 0xFF — which yields is_valid=0, sanitize=-1, and a memcpy length of 18446744073709551615. A malformed byte late in a long line needs no special effort: any latin-1 client relaying into a channel that a WebSocket text client is sitting in will do it. This is reachable on ircv3.2-upgrade as it stands. The preceding commit widens it: before it, any mIRC formatting byte in the first ~508 bytes set `modified` and returned a real length, so formatted lines were accidentally safe; now only genuinely malformed bytes do, and a formatted long line with a late bad byte hits the -1 path too. Same 691-byte line with a leading \002: sanitize returns 508 before that commit, -1 after. - s_bsd.c: take the sanitized length only when it is >= 0, and refuse to write a frame that failed to encode. - websocket.c: reject a negative data_len rather than trusting the caller with an unchecked memcpy. - ircd_string.c: document that -1 means "leave the string and its length alone", and why it can happen on a string that failed validation. Two cmocka cases pin the contract: the past-the-window -1, and that an in-window replacement still clamps to a whole-sequence boundary. Co-Authored-By: Claude Opus 5 --- ircd/ircd_string.c | 7 ++++++ ircd/s_bsd.c | 14 ++++++++++- ircd/test/ircd_string_cmocka.c | 46 ++++++++++++++++++++++++++++++++++ ircd/websocket.c | 5 ++++ 4 files changed, 71 insertions(+), 1 deletion(-) diff --git a/ircd/ircd_string.c b/ircd/ircd_string.c index 12aa0fb6..2b1e45bc 100644 --- a/ircd/ircd_string.c +++ b/ircd/ircd_string.c @@ -164,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. */ 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 90d4ea3f..f07eae2a 100644 --- a/ircd/test/ircd_string_cmocka.c +++ b/ircd/test/ircd_string_cmocka.c @@ -662,6 +662,50 @@ static void test_utf8_sanitize_keeps_formatted_line_intact(void **state) } +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[] = { @@ -743,6 +787,8 @@ int main(void) 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;