From 384d96b530254e99d2606af2a3448ba451b379b3 Mon Sep 17 00:00:00 2001 From: Marco Carnut Date: Wed, 16 Sep 2026 09:04:37 -0300 Subject: [PATCH 1/3] Fix off-by-one out-of-bounds write on a 125-byte control frame read_single_frame() writes a terminating NUL at msg[*msg_idx] whenever it finishes a FIN frame (the `if (fsd->is_fin && *frame_size > 0)` block). For a control frame, `msg` is the fixed-size `msg_ctrl` buffer and `*msg_idx` is `msg_idx_ctrl`. A control frame is allowed to carry a full 125-byte payload (the guard in next_complete_frame() only rejects frame_length > 125), after which the mask-copy loop has advanced *msg_idx to 125. The subsequent `msg[*msg_idx] = '\0';` therefore writes msg_ctrl[125] -- one byte past the 125-byte array. Reproduce with a masked 125-byte PING: `0x89 0xFD <125 payload bytes>`. On common struct layouts msg_ctrl[125] lands in alignment padding, so the stray NUL is usually harmless -- but it is a genuine out-of-bounds write / undefined behaviour and is fragile: reordering or packing the struct, or changing the buffer size, turns it into corruption of the following field. Fix by sizing the buffer 125 + 1 so there is always room for the terminator. No behavioural change for any valid frame. Signed-off-by: Marco Carnut Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FCfAt4kFaCVqEc5Mjax5kz --- src/ws.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/ws.c b/src/ws.c index b705926..cc3b628 100644 --- a/src/ws.c +++ b/src/ws.c @@ -164,8 +164,13 @@ struct ws_frame_data unsigned char *msg; /** * @brief Control frame payload + * + * Sized 125 (the RFC 6455 maximum control-frame payload) + 1 for the + * terminating NUL that read_single_frame() writes at msg[*msg_idx] on a + * FIN frame. A control frame may carry a full 125-byte payload, after + * which *msg_idx == 125, so the buffer must have room for index 125. */ - unsigned char msg_ctrl[125]; + unsigned char msg_ctrl[125 + 1]; /** * @brief Current byte position. */ From bc928f33383a2011945315ae9ef7d4b68f5af22c Mon Sep 17 00:00:00 2001 From: Marco Carnut Date: Wed, 16 Sep 2026 09:06:10 -0300 Subject: [PATCH 2/3] Reject unmasked client frames (RFC 6455 section 5.1) read_single_frame() reads the mask/length byte into fsd.mask and derives frame_length from its low 7 bits, but never checks the high MASK bit. RFC 6455 section 5.1 requires every client-to-server frame to be masked and requires the server to fail the connection otherwise. Without the check an unmasked frame is silently mis-parsed: the next 4 payload bytes are consumed as a masking key and the remainder is unmasked against them, desynchronising the frame stream. Close with 1002 (protocol error) on an unmasked frame. Browsers and conforming clients always mask, so no correct client is affected; this also fixes the relevant Autobahn masking cases. Signed-off-by: Marco Carnut Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FCfAt4kFaCVqEc5Mjax5kz --- src/ws.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/ws.c b/src/ws.c index cc3b628..5083d3b 100644 --- a/src/ws.c +++ b/src/ws.c @@ -1691,6 +1691,20 @@ static int next_complete_frame(struct ws_frame_data *wfd) fsd.frame_size = 0; fsd.msg_idx_ctrl = 0; + /* + * RFC 6455 section 5.1: the server MUST close the connection on + * receiving a frame that is not masked. Without this check an + * unmasked frame is mis-parsed, since the first payload bytes are + * then consumed as the (absent) 4-byte masking key. + */ + if (!(fsd.mask & 0x80)) + { + DEBUG("Client sent an unmasked frame!\n"); + do_close(wfd, WS_CLSE_PROTERR); + wfd->error = 1; + break; + } + /* * We should deny non-FIN control frames or that have * more than 125 octets. From 55a0cfa22258f368b8ebd9098aac82922088ac83 Mon Sep 17 00:00:00 2001 From: Marco Carnut Date: Thu, 17 Sep 2026 08:06:15 -0300 Subject: [PATCH 3/3] Bound the handshake read with SO_RCVTIMEO (slowloris mitigation) ws_accept() sets SO_SNDTIMEO from timeout_ms but never a receive timeout, so every recv() on a client socket blocks indefinitely. A client that completes the TCP connection and then sends nothing (or dribbles bytes) parks its handler thread forever inside do_handshake(); with the fixed MAX_CLIENTS pool a handful of such idle sockets consume every slot and deny service to all other clients at essentially zero cost. Set SO_RCVTIMEO on the accepted socket, but only for the handshake: ws_establishconnection() clears it the moment do_handshake() succeeds, so an established connection may stay idle on the read side indefinitely -- long-lived sessions that receive rarely (a remote touchpad, a notifier, etc.) are a first-class use case and must not be closed just for being quiet. Only the pre-handshake window, where no legitimate client has any reason to stall, is bounded. Gated on the existing timeout_ms, so deployments that leave it at 0 are unaffected. Abuse *after* a completed handshake is out of scope here and is best handled with application heartbeats (ws_ping()). Signed-off-by: Marco Carnut Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FCfAt4kFaCVqEc5Mjax5kz --- src/ws.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/ws.c b/src/ws.c index 5083d3b..d158256 100644 --- a/src/ws.c +++ b/src/ws.c @@ -1805,6 +1805,19 @@ static void *ws_establishconnection(void *vclient) if (do_handshake(&wfd) < 0) goto closed; + /* + * Handshake done: drop the receive timeout that ws_accept() set, so an + * established session may stay idle on the read side indefinitely -- + * long-running WebSocket connections are a first-class use case. The + * timeout only needs to bound the handshake against a slowloris client. + */ + if (timeout) + { + struct timeval zero = {0, 0}; + setsockopt(client->client_sock, SOL_SOCKET, SO_RCVTIMEO, + (const char *)&zero, sizeof(zero)); + } + /* Read next frame until client disconnects or an error occur. */ while (next_complete_frame(&wfd) >= 0) { @@ -1922,6 +1935,19 @@ static void *ws_accept(void *data) */ setsockopt(new_sock, SOL_SOCKET, SO_SNDTIMEO, (const char*)&time, sizeof(struct timeval)); + + /* + * Bound the *handshake* read too. A client that connects and + * then sends nothing parks its handler thread indefinitely in + * do_handshake(); with the MAX_CLIENTS pool a few such idle + * sockets deny service to everyone else (a trivial slowloris). + * ws_establishconnection() clears this the moment the handshake + * completes, so an established long-running session is free to + * stay idle on the read side for as long as it likes. Opt-in via + * timeout_ms, so behaviour is unchanged when it is left at 0. + */ + setsockopt(new_sock, SOL_SOCKET, SO_RCVTIMEO, (const char*)&time, + sizeof(struct timeval)); } /* Adds client socket to socks list. */