Skip to content

fix websocket: apply control frame length limit to ping and pong - #1308

Open
netliomax25-code wants to merge 2 commits into
userver-framework:developfrom
netliomax25-code:websocket-control-frame-len
Open

fix websocket: apply control frame length limit to ping and pong#1308
netliomax25-code wants to merge 2 commits into
userver-framework:developfrom
netliomax25-code:websocket-control-frame-len

Conversation

@netliomax25-code

Copy link
Copy Markdown
Contributor
  1. is_data_frame in ReadWSFrameImpl classifies the opcode with opcode & (kText | kBinary), so ping (0x9) and pong (0xA) both test non-zero and count as data frames. The RFC 6455 5.5 guard just below it, which rejects a control frame with payload_len > 125, never fires for them, and a client can send a ping carrying up to max-remote-payload bytes.
  2. That payload is echoed back in a pong. MakeControlFrame assigns the size into a 7-bit field while SendFrame writes the payload in full, so a 200-byte ping is answered with 8A 48 followed by 200 bytes. The peer takes 72 of them as the pong and parses the remaining 128 as further frames.
  3. Compared the opcode against the three data opcodes instead of masking, which puts ping and pong back under the existing control frame check.
  4. Control frames may legally arrive inside a fragmented message, so the pong now echoes only the ping's own bytes rather than the whole accumulated buffer, which can still hold an unfinished data frame.

Added protocol_test.cpp covering the oversized ping and pong, a 125-byte ping, and a ping interleaved with a fragmented text message. The three new negative cases fail on the current tree (kNone instead of kProtocolError) and pass with the change; the rest of userver-core-unittest is unaffected.

@apolukhin apolukhin closed this Aug 6, 2026
@apolukhin apolukhin reopened this Aug 6, 2026
@apolukhin

Copy link
Copy Markdown
Member

LGTM

@robot-magpie

robot-magpie Bot commented Aug 6, 2026

Copy link
Copy Markdown

Many thanks for the PR! @apolukhin is now importing your pull request into our internal upstream repository.

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