utf8: C0 control characters are valid UTF-8 — stop rewriting them to U+FFFD - #102
Open
rubinlinux wants to merge 2 commits into
Open
utf8: C0 control characters are valid UTF-8 — stop rewriting them to U+FFFD#102rubinlinux wants to merge 2 commits into
rubinlinux wants to merge 2 commits into
Conversation
…U+FFFD 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 "<U+FFFD>Nick<U+FFFD>: Pong!" where a services reply meant a bold nick, and its own /me left the server as "<U+FFFD>ACTION waves<U+FFFD>" — 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 <noreply@anthropic.com>
…length
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 <noreply@anthropic.com>
MrLenin
added a commit
to MrLenin/nefarious2
that referenced
this pull request
Aug 28, 2026
…jection evilnet#103 (Rubin, red-teaming evilnet#102). serialize_message frames records as [\x05target\x05][\x06tags\x06]content on the assumption those bytes never appear in payloads; nothing enforces it (check_utf8_text is a no-op unless FEAT_UTF8ONLY, default-off). So a client puts \x06+evil/tag=x\x06 in its own PRIVMSG body and the deserializer lifts it into client_tags, which m_chathistory splices verbatim into the replayed tag prefix -- attacker-controlled message tags forged onto another user's replayed message, plain TCP, no privs. The \x05 variant forges original_target on PM-pair-keyed records. Fix: escape \x04/\x05/\x06 in every VALUE field (content, client_tags, original_target) on serialize and reverse on deserialize; the structural brackets stay raw. Introducer \x04, escaped byte = \x04,(b^0x40) -- results 'D'/'E'/'F' are never sentinels. Legacy records carry no \x04 escapes so they unescape to themselves (backward compatible; already-stored poison can't be retro-cleaned but no NEW injection lands). bufsize grown to 2x the escapable fields. mIRC formatting / CTCP bytes are deliberately NOT escaped -- only the record sentinels are -- so evilnet#102's formatting restoration is unaffected. Also anchor history_pm_target_has_sessid's +afternet.org/sid= match to a tag-list boundary (start or ';'-preceded) instead of an unanchored strstr over the (now un-injectable) client_tags. Residue (separate analysis, per evilnet#103's own 'flagged not claimed'): - \x1E/\x1F multiline markers are structural in a DOWNSTREAM consumer and share this serialize path via ML_CONTENT_SENTINEL (0x1E); escaping them here would corrupt legitimate multiline records, so they need a coordinated escape+unescape across it. - a client can still legitimately NAME a client-only tag '+afternet.org/sid=...'; reserving that vendor namespace at store time is a distinct fix from the injection closed here. Upstream ircv3.2-upgrade has the same defect (fix targets it). Gate: full build; 21/21 cmocka suites (history suite +3 escape tests, incl. the C-hex-escape-greediness trap in the escape-byte literal). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
string_is_valid_utf8()(ircd/ircd_string.c) accepts only0x09,0x0A,0x0Dand0x20–0x7Eas single-byte sequences, so it reports every other C0 control byte as invalid UTF-8.string_sanitize_utf8()has the identical branch and overwrites each one with U+FFFD.Every byte
0x01–0x7Fis a well-formed single-byte UTF-8 sequence. The comment sitting in that very branch already says so —// 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 (bold0x02, colour0x03, reset0x0F, monospace0x11, reverse0x16, italic0x1D, strikethrough0x1E, underline0x1F) as bare control bytes, so the pair silently destroys formatting and CTCP in two places.1. WebSocket clients on the
text.ircv3.netsubprotocol — both directions.s_bsd.c:386sanitises every outbound line before framing it;s_bsd.c:1270sanitises every inbound frame before it reaches the IRC parser. So a browser client sees<U+FFFD>Nick<U+FFFD>: Pong!where services sent a bold nick, and its own/me wavesleaves the server as<U+FFFD>ACTION waves<U+FFFD>— mangled for the whole channel, not just for the sender.Fragmented text frames fare worse still:
s_bsd.c:1229kills the connection rather than sanitising, so a long enough formatted line from a browser is a disconnect.2. Every client, on any network running
FEAT_UTF8ONLY.ircd_relay.c(PRIVMSG/NOTICE),m_topic.c,m_kick.c,m_part.candm_quit.call route user text through the same pair. TurningUTF8ONLYon today strips bold and colour from native IRC clients too.Reproduction
Same client, same lines, only the subprotocol differs — against
ircv3.2-upgradeat 94e4fbf:Binary mode is untouched because
websocket_encode_frame(..., text_mode=0)writes raw bytes and the inbound sanitiser is gated onopcode == WS_OPCODE_TEXT. With this branch, text mode produces the second transcript too.The fix
Accept
0x01–0x7Fin the ASCII branch of both functions.0x00cannot reach it — both walk a NUL-terminated string and stop there.Rejection of genuinely malformed encodings is untouched: lone continuation bytes, overlongs, surrogates, truncated sequences and
0xF5–0xFFleads are all still invalid, and RFC 6455 §8.1 is still satisfied (control characters are valid UTF-8; the RFC constrains encoding, not code points).Behaviour elsewhere
m_join.c'sFEAT_VALID_UTF8_CHANNELS_ONLYgate goes throughstring_character_structure_is_sane(), which already admitted control bytes in pure-ASCII names via its!string_contains_non_ascii()arm.strIsIrcCh()runs first and remains the real filter. Only names mixing control bytes and valid non-ASCII change status.m_metadata.c— values may now contain control bytes. That is what "valid UTF-8" means, and it matches what topics and messages already allow. A caller wanting printable-only text should check for that explicitly rather than borrowing the encoding validator; the doc comment now says so.Tests
ircd/test/ircd_string_cmocka.cgains seven cases: the whole of0x01–0x7Fbyte by byte, the CTCP and formatting codes as real lines, valid 2/3/4-byte sequences, the malformed encodings that must still be rejected, andstring_sanitize_utf8()'s no-modification (-1) and replacement paths including the byte arithmetic of a U+FFFD substitution.Full build plus all 21 cmocka suites green, then verified end to end against a live ircd built from this branch — the
text.ircv3.nettranscript above now matches the binary one byte for byte.🤖 Generated with Claude Code
Second commit: an adjacent crash this makes easier to hit
A red-team pass over the first commit turned up a latent remote crash next door, which this branch now also fixes.
string_sanitize_utf8()returns-1for "nothing modified", and that is reachable on a string that failed validation: its copy loop stops once the output buffer is nearly full (outlen < sizeof(out) - 4, ~508 bytes) whilestring_is_valid_utf8()scans the whole string. A longer line whose only malformed byte sits past that window validates as invalid and sanitises to-1, unchanged.s_bsd.cassigned that straight into the frame length, andwebsocket_encode_frame()does an uncheckedmemcpy(frame + pos, data, data_len):That is the two functions plus
websocket_encode_framedriven verbatim from this tree with an ordinary 691-byte line — tags, 600 characters, one stray0xFF. The inbound sibling ats_bsd.c:1273already guards this (if (new_len > 0) ws_len = new_len;); the outbound path never did.This is reachable on
ircv3.2-upgradeas it stands — no formatting codes required, just a latin-1 client relaying a long line into a channel a WebSocket text client is sitting in. But the first commit widens it, so it belongs here rather than in a separate PR. Before it, any formatting byte in the first ~508 bytes setmodifiedand produced a real length, so formatted lines were accidentally safe. Same 691-byte line with a leading\002:0xFF0xFFsanitize = -1sanitize = 508sanitize = -1sanitize = -1Fixes: take the sanitised length only when
>= 0and refuse to write a frame that failed to encode (s_bsd.c); reject a negativedata_lenrather than trusting the caller with an uncheckedmemcpy(websocket.c); document that-1means "leave the string and its length alone", and why it can happen after a failed validation (ircd_string.c). Two more cmocka cases pin the contract.Out of scope, reported separately
The same review surfaced a pre-existing message-tag injection via
history.c's\x05/\x06record sentinels, filed as its own issue. It is not caused by this branch — I reproduced it against a stock build over plain TCP, no WebSocket involved — so it should not gate this PR, but note the interaction: WebSocket text clients were accidentally shielded from it by the U+FFFD rewriting, and this branch removes that accident. The real fix is escaping inserialize_message(), not keeping a broken UTF-8 validator.