Security/robustness hardening: handshake verification, message-size bounds, TLS IP SANs, mask entropy, liveness#7
Open
voicetel wants to merge 9 commits into
Open
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.
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
2 participants
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.
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.*.