Three robustness/security fixes in the frame path

[marcocarnut]

Three robustness/security fixes in the frame path

While hardening a wsServer-based service ahead of wider exposure, a security review turned up three issues in the frame-handling path. All three are small, self-contained, and independent — feel free to take, drop, or split any of them. Against current master:

1. Off-by-one out-of-bounds write on a 125-byte control frame (00622ff)
read_single_frame() writes a terminating NUL at msg[*msg_idx] when it finishes a FIN frame. For a control frame msg is the fixed msg_ctrl buffer and a control payload may be a full 125 bytes (the guard only rejects > 125), after which *msg_idx == 125 — so msg[*msg_idx] = '\0'; writes msg_ctrl[125], one past the 125-byte array.
Reproduce with a masked 125-byte PING: 0x89 0xFD <mask×4> <125 bytes>. Usually masked by struct padding, but genuine UB and layout-fragile. Fix: size the buffer 125 + 1; no behaviour change for any valid frame.

2. Reject unmasked client frames — RFC 6455 §5.1 (a71fe68)
The MASK bit is read into fsd.mask but never checked. RFC 6455 §5.1 requires every client→server frame to be masked and the server to fail the connection otherwise. Without the check an unmasked frame is mis-parsed (the first payload bytes are consumed as the absent masking key). Fix: close with 1002 on an unmasked frame. Conforming clients always mask, so none are affected; also fixes the corresponding Autobahn cases.

3. Bound blocking receives with SO_RCVTIMEO (3b276db)
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 connects and then sends nothing (or one byte at a time) parks its handler thread forever; with the MAX_CLIENTS pool a handful of idle sockets deny service to everyone else (a trivial slowloris). Fix: set SO_RCVTIMEO to the same value, gated on the same timeout_ms, so it is opt-in and deployments that leave timeout_ms at 0 are unaffected.

Each commit is Signed-off-by. Thanks for wsServer!

[Theldus]

Hi @marcocarnut, thanks for the PR.
The first two commits are fine and fix real issues, thanks for bringing them.
Regarding the third commit I need some clarifications: adding a SO_RCVTIMEO in the socket means that the client must always send new messages in at most 1 second. This breaks the vtouchpad for example, which is a long-running process.

Since many WebSocket use-cases are meant to be long running sessions of many minutes (or even hours), it's unnaceptable to close a connection in such short period.

[marcocarnut]

Hi @marcocarnut, thanks for the PR. The first two commits are fine and fix real issues, thanks for bringing them. Regarding the third commit I need some clarifications: adding a SO_RCVTIMEO in the socket means that the client must always send new messages in at most 1 second. This breaks the vtouchpad for example, which is a long-running process.

Since many WebSocket use-cases are meant to be long running sessions of many minutes (or even hours), it's unnaceptable to close a connection in such short period.

You're entirely right, I overdid it -- my usecase streams continuously, but many (most?) don't. I was thinking of avoiding cheap resource exhaustion attacks like slowloris.

But the timeout only ever needs to cover the handshake -- that's the window a zero-effort slowloris exploits (connect, send nothing, park a thread; repeat MAX_CLIENTS times). After the handshake there's no reason to keep it.

So I've revised the third commit to scope it: ws_accept() still sets SO_RCVTIMEO, but ws_establishconnection() clears it the instant do_handshake() succeeds. An established connection then blocks on recv() indefinitely, exactly as before -- vtouchpad is unaffected. Only the pre-handshake read is bounded, and still only when timeout_ms is set.

I tested it both ways: a client that connects and stays silent is closed at the handshake timeout, while an established session left idle for well over the timeout still sends and receives normally.

Abuse after a completed handshake I've left out of scope here — as you say, that's the app's call (heartbeats / ws_ping()), and it's a much higher bar for an attacker than the pre-handshake case anyway.

Branch is force-pushed with the revised commit. Thanks for the review!

[Theldus]

Hi @marcocarnut,
Thanks for your review.

Just a few things:

• SO_RCVTIME0 on Windows receives a DWORD of milliseconds, not a struct timeval, where '0' also means blocking. Could you review this? I think an #ifdef _WIN32 is enough.

• Your second commit mentions that it is fixing Autobahn tests where it sends unmasked frames. However, Autobahn do not tests for unmasked frames. Could you reword this message?

• I see that you declaring 'struct timeval' mid-block, could you move to the top of the function? to follow the remaining coding-style.