Security/robustness hardening: handshake verification, message-size bounds, TLS IP SANs, mask entropy, liveness - #7
Conversation
The handshake was accepted whenever the response contained "HTTP/1.1 101" and a Sec-WebSocket-Accept header of any value; the accept digest computed from our nonce at construction was never compared (dead code). RFC 6455 §4.1 requires failing the connection unless Sec-WebSocket-Accept equals base64(SHA1(key + GUID)). Extract the header value (case-insensitive name, case-sensitive base64 value, trimmed) and compare it to the expected accept; fail the upgrade on mismatch or absence.
rxInflate appended inflate output with no size limit, so a hostile server that negotiated permessage-deflate could send a small compressed frame that expands to gigabytes, exhausting memory. Cap the reassembled/inflated output at MAX_MESSAGE_SIZE (16 MiB) and abort inflation with a protocol error once it would be exceeded. The overflow check is written as `produced > MAX - old` to avoid any additive overflow.
The receiver imposed no maximum on a single frame's declared payload length (a peer-controlled 64-bit value) nor on the total size of a fragmented message accumulated across continuation frames, so a hostile server could drive unbounded buffering (memory exhaustion). Reject any data/continuation frame whose payload exceeds MAX_MESSAGE_SIZE and abort reassembly once the accumulated fragmented message would exceed it, both with close code 1009 (message too big). Control frames were already capped at 125 bytes.
The per-frame WebSocket masking key came from a splitmix PRNG seeded once from time(nullptr) XOR a stack address, which is predictable. RFC 6455 §5.3 requires masks derived from a strong source of entropy (the handshake nonce already uses std::random_device). Seed the PRNG from random_device; the per-frame splitmix step is retained so masking stays syscall-free on the hot path.
Before the WebSocket upgrade completes, every read copied the entire input buffer and rescanned it from the start for the CRLFCRLF header terminator, with no cap on the pre-upgrade byte count. A server that streams header bytes without ever terminating could grow the buffer and impose quadratic rescan cost. Fail the connection once the un-terminated handshake response exceeds 64 KiB.
When permessage-deflate was negotiated the receiver accepted the RSV1 (compression) bit on any frame. RFC 7692 §6 permits RSV1 only on the first frame of a message; it is illegal on control frames and on continuation frames. Reject those as a 1002 protocol error.
For a wss:// endpoint given as an IP literal, peer verification used X509_VERIFY_PARAM_set1_host, which matches dNSName SANs, so a certificate with the correct iPAddress SAN could be rejected (or the IP not validated as intended). Use X509_VERIFY_PARAM_set1_ip_asc for IP-literal hosts and keep set1_host for hostnames.
The handshake request interpolated the configured URI, host, and custom header key/values directly into CRLF-terminated request lines. A CR or LF embedded in any of those (via local configuration) would inject arbitrary handshake headers or smuggle a second request. Strip CR/LF/NUL from these values before writing the request line.
Heartbeat pings were fire-and-forget: pongs were never tracked and the declared ErrorCode::PING_TIMEOUT was unused, so a peer that went silent while its TCP connection stayed up was never detected. Track pings sent since the last pong (event-thread-only state) and, once MAX_MISSED_PONGS (2) intervals pass with no pong, declare the connection dead using the same teardown path as other fatal errors. Gated on the upgrade and on ping_interval > 0, so behavior is unchanged when the heartbeat is off.
|
Thanks for the thorough review and for splitting the fixes into independent commits. I went through them individually. Most of the findings look valid and useful, and I am happy with the general direction of the PR. The following changes look good to me:
There are a few changes I would like to adjust before merging. Handshake response size limit The 64 KiB handshake limit is a good idea, but I think the current check happens too late. The code first does something equivalent to: const size_t len = evbuffer_get_length(input); std::vector snap(len); and only afterwards checks whether the HTTP header terminator was found and whether the accumulated handshake exceeds MAX_HANDSHAKE_BYTES. This means a hostile peer can already cause a large allocation before the limit is applied. At the same time, simply rejecting whenever the complete evbuffer is larger than 64 KiB would not be correct either, because the buffer may already contain a small valid HTTP upgrade response followed by WebSocket frame data. I would prefer to search for \r\n\r\n only within the first MAX_HANDSHAKE_BYTES bytes, ideally directly using the evbuffer API, and: terminator found within the limit -> process handshake no terminator and buffered handshake > limit -> fail otherwise -> wait for more data That would enforce the limit before copying/allocating based on an attacker-controlled buffer size. Frame masking PRNG I agree that time() plus an address is not an appropriate seed for WebSocket masking. However, I would like to look more carefully at the proposed approach of using std::random_device only to seed the existing PRNG. RFC 6455 requires each masking key to be unpredictable, including that observing previous masking keys should not make subsequent keys predictable. If the existing next_u32() generator is a small non-cryptographic PRNG, giving it a strong initial seed may still not satisfy that property once outputs are observable by the peer. I would therefore prefer either:
I would like to avoid relying only on a secure seed if the generator itself is predictable from its outputs. CR/LF sanitization I agree with the request/header injection issue, but I am not sure silently stripping CR/LF/NUL is the best API behavior. For example, turning: abc\r\ndef into: abcdef creates a value the caller did not actually provide. I would prefer rejecting invalid URI/host/header input when it contains forbidden CR/LF/NUL characters rather than modifying it silently. Header names may also deserve stricter validation than just CR/LF stripping. Sec-WebSocket-Accept parsing The actual accept-value verification is definitely needed. One minor improvement I would suggest is avoiding a substring search for: sec-websocket-accept: across the complete HTTP response. It would be more robust to parse individual HTTP header lines and compare the header name case-insensitively while keeping the Base64 value case-sensitive. This is not a blocker for the security fix itself, but it would make the handshake parser more precise. Overall, thanks for the work on this. The majority of the findings are real issues and I would like to merge them. I mainly want to tighten the handshake-size handling, masking-key generation, and input-validation behavior before taking the complete PR. Best, |
3924ab3 to
33aa144
Compare
This PR adds a set of security/robustness hardening fixes to the WebSocket
client transport, found during a code review of a downstream integration.
Each fix is an independent commit.
The client treats the server side as potentially hostile (untrusted input),
so several of these close remote DoS / integrity gaps:
Sec-WebSocket-Acceptagainstbase64(SHA1(key + GUID)). Thehandshake previously accepted on mere presence of the header (RFC 6455 §4.1
requires matching the computed value); the computed
acceptwas dead code.rxInflateappended inflate outputwith no cap, so a small compressed frame could expand without limit
(decompression bomb → OOM). Capped at a 16 MiB message ceiling.
a peer-declared 64-bit frame length or on fragments accumulated across
continuation frames. Reject with close code 1009.
bytes forever without the terminating CRLFCRLF, growing the buffer and
re-scanning it each read. Fail past 64 KiB.
masking key was seeded from
time()+ a stack address (predictable); RFC6455 §5.3 requires unpredictable masks. Now seeded from
std::random_device(as the handshake nonce already is), keeping per-frame masking syscall-free.
compression bit only on the first frame of a message.
X509_VERIFY_PARAM_set1_ip_ascfor IP hosts instead ofset1_host(whichmatches dNSName SANs).
prevent header/request injection from local configuration.
fire-and-forget and
PING_TIMEOUTwas unused; now the connection is closedafter N unanswered pings. Gated on
ping_interval > 0, so behavior isunchanged when the heartbeat is disabled.
Independent of, and non-conflicting with, the
_ctxthread-safety PR (#6);this branch is based on
mainand touches onlyWebSocketContext.*andWebSocketReceiver.*.