utf8: C0 control characters are valid UTF-8 — stop rewriting them to U+FFFD

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;
         }

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);

ircd/test/ircd_string_cmocka.c

@@ -11,6 +11,7 @@
 #include <string.h>
 #include <cmocka.h>
 
+#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);

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;