Skip to content

WebSocket client: mask outgoing frames (RFC 6455 5.1) - #198

Closed
Vizit0r wants to merge 1 commit into
winddriver:masterfrom
Vizit0r:fix/wsclient-masking-key
Closed

WebSocket client: mask outgoing frames (RFC 6455 5.1)#198
Vizit0r wants to merge 1 commit into
winddriver:masterfrom
Vizit0r:fix/wsclient-masking-key

Conversation

@Vizit0r

@Vizit0r Vizit0r commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

TCrossWebSocket.FMaskingKey is never initialised, so it stays 0 unless the caller assigns MaskingKey by hand. And MakeFrameData reads 0 as "do not mask":

if (AMaskKey <> 0) then
  Result[1] := Result[1] or $80;   // MASK bit -- skipped
...
if (AMaskKey <> 0) then ... else
  Move(AData^, Result[LHeaderSize], LDataSize);   // payload sent in the clear

RFC 6455 5.1 requires every client-to-server frame to be masked, and says the server MUST close the connection on an unmasked one. So the client does not work against a compliant server out of the box.

The failure is confusing to diagnose, because the handshake is fine: OnOpen fires (101 + Sec-WebSocket-Accept validated), and only then does the peer drop the connection — right after the first frame is sent. What you see is a connect -> open -> close loop with no error anywhere.

Fix

Add TCrossWebSocketParser.NewMaskingKey next to NewSecWebSocketKey — same unit, same CSPRNG (Utils.CryptRandom, already in its uses), same failure mode — and seed FMaskingKey from it in the constructor:

class function TCrossWebSocketParser.NewMaskingKey: Cardinal;
begin
  repeat
    if not TryFillCryptRandomBytes(Result, SizeOf(Result)) then
      raise ECrossSocket.Create('Failed to generate WebSocket masking key: CSPRNG unavailable');
  until (Result <> 0);
end;

Callers that assign MaskingKey themselves are unaffected.

One design note

The key is generated once per TCrossWebSocket, so a reconnect reuses it. Generating it in Open instead would give a fresh key per connection, which is closer to the spirit of 5.3 ("the masking key for a given frame MUST NOT be predicted by the server"). I kept it in the constructor to stay minimal — happy to move it if you prefer.

Checked

Compiles clean on Win64 (Delphi 37.0) — no new warnings or hints. Verified live against a WSS endpoint that was previously closing the connection ~50 ms after the first frame: with a non-zero key the server accepts the frame and replies.

TCrossWebSocket.FMaskingKey is never initialised, so it stays 0. MakeFrameData
treats 0 as "do not mask": the MASK bit is left clear and the payload goes out
unmasked. RFC 6455 5.1 requires every client-to-server frame to be masked and
says the server MUST close the connection on an unmasked one, so the client
does not work against a compliant server out of the box -- the handshake
succeeds, then the peer closes as soon as the first frame is sent.

Add TCrossWebSocketParser.NewMaskingKey next to NewSecWebSocketKey (same
CSPRNG, same failure mode) and seed FMaskingKey from it in the constructor.
Callers that assign MaskingKey themselves are unaffected.
@winddriver

Copy link
Copy Markdown
Owner

Thank you for identifying the issue with clients sending unmasked WebSocket frames by default.

This issue has been addressed on master by commit 0d51d163625e8b6dc41f26e8cb6737a2c459675b.

This PR was not merged directly because RFC 6455 Sections 5.3 and 10.3 require a client to select a new, unpredictable 32-bit masking key for every frame. The current patch generates the key when TCrossWebSocket is constructed and then reuses it for subsequent frames.

The replacement implementation:

  • removes the public MaskingKey property;
  • determines the outgoing masking behavior from the endpoint role;
  • generates a fresh key from a CSPRNG for every client frame;
  • ensures that server frames are never masked;
  • treats an all-zero masking key as a valid key rather than an “unmasked” sentinel;
  • keeps incoming frame parsing permissive for compatibility;
  • adds FPC frame-level regression tests and passes the Win64 WebSocket client and server builds.

The issue is therefore resolved by the replacement implementation, and this PR can be closed as superseded. The source branch does not need to be deleted.

Thank you again for the report and the initial implementation.

@winddriver winddriver closed this Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants