Repository navigation
Apply max_fragmented_message_size to the first fragment of a message - #739
Merged
Merged
Conversation
`max_fragmented_message_size` isn't checked against the first fragment of a message. A client can open a fragmented message with a frame far larger than the limit and Bandit will hold it, unchecked, until the client decides to send the next frame. Only then is the limit applied and the connection closed. The way it works today: `dispatch_frame/3` checks the limit in both `Frame.Continuation` clauses, adding the new frame to the running `fragment_size` and closing with 1009 if the total is over. The two clauses for a text or binary frame with FIN clear, the ones that start a fragmented message, store the frame and record its size without a check. So with `max_fragmented_message_size: 1024` and the default 8 MB `max_frame_size`, a 7 MB opening fragment is accepted and sits in `fragment_frame` for as long as the client cares to leave it there. The first continuation frame, even a one byte one, triggers the 1009. Why fix it. The limit's job is to bound the memory a fragmented message can occupy while it's being reassembled, and a frame with FIN clear is by definition the start of such a message, so "already over the limit" is known the moment the frame arrives. Deferring the check until the next frame means the one party who decides when the check happens is the client, and a client that never sends the next frame is never checked at all. There's also a plain correctness point: the option is documented as the maximum size of a message delivered across multiple frames, and a message of 7 MB and counting is over a 1 KB maximum whether or not the second frame has shown up yet. I don't think it rises to the security process. The opening fragment is still capped by `max_frame_size`, which the extractor enforces before the frame reaches this code, and an operator who allows a frame of that size allows it unfragmented too, so the memory a client can park per connection is a bound the operator already chose. What's wrong is the timing and the fact that one configured limit silently doesn't apply to the first frame. That's a bug fix, so it's a PR. The change is small. The two FIN-clear clauses now go through a `start_fragmented_message/3` that runs the existing `oversize_message?/2` on the first fragment. Over the limit it closes with 1009 and the same "Received oversize fragmented message" error the continuation path uses; otherwise it stores the fragment exactly as before. The continuation clauses are untouched, and so is the non-fragmented path. Tests: two new ones alongside the existing `max_fragmented_message_size` tests, one text and one binary, each sending a 1,010 byte opening fragment against a 1,000 byte limit and expecting the 1009 straight away, with the WebSock's terminate seeing the same reason as for an oversize continuation. Both fail on `main`, where nothing happens until a continuation arrives. Suite, format, credo and dialyzer are clean.
Owner
|
Good catch! |
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
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.
max_fragmented_message_sizeis not applied to the first frame of a fragmented message. The checkonly runs when a continuation frame arrives, so a client can open a message with a frame far over
the limit and Bandit keeps it in memory, unchecked, until the client chooses to send another frame.
If it never does, the frame is never checked.
Some background on what the option is for, since that is what makes this a problem. A WebSocket
message can be split across many frames (RFC 6455 §5.4), and the server has to hold the pieces until the final one arrives before it can hand the WebSock a complete message. That
buffer is per connection, it lives as long as the message stays incomplete, and the client decides
both how big it gets and how long it lives.
max_fragmented_message_sizeis the operator's cap onit. It is a different knob from
max_frame_sizeon purpose: a single frame is handed to theWebSock and freed, so it costs memory only while it's being processed, where a fragment is retained.
RFC 6455 §10.4 asks implementations to
limit "the total message size after reassembly from multiple frames" for exactly this reason, and
Bandit has had three advisories about the edges of this buffer, so the limit matters here.
Here is what the gap looks like. Take an app that expects small messages and sets
max_fragmented_message_size: 64_000to keep reassembly buffers small, leavingmax_frame_sizeat its 8 MB default. A client sends one 8 MB text frame with FIN clear, then nothing but pings.
Bandit stores the 8 MB in
fragment_frame, the pings keep the idle timeout from firing, and the 64KB cap is never consulted, because
dispatch_frame/3only checks it in the twoFrame.Continuationclauses. The two clauses that start a fragmented message (a text or binaryframe with FIN clear) store the frame and record its size without a check. Open a thousand such
connections and the server is holding 8 GB that the operator believed was capped at 64 MB. The
first continuation frame, even a one byte one, would trigger the 1009, but a client doing this on
purpose won't send one.
Why this is a bug to fix rather than a limitation to document. The option's docs say it is "the
maximum size of a WebSocket message delivered across multiple continuation frames", and a frame
with FIN clear is by definition the start of such a message, so a first fragment over the limit is
already a message over the limit; nothing about the second frame changes that. The condition is
known the instant the frame arrives, and deferring the check hands the client control over whether
it ever happens. 1009 is defined as closing because the endpoint "has received a message that is
too big for it to process", which describes the opening fragment as well as it describes a later
one. Cowboy, for comparison, checks every fragment including the first against its limit.
The change is small. The two FIN-clear clauses now go through a
start_fragmented_message/3thatruns the existing
oversize_message?/2on the first fragment. Over the limit it closes with 1009and the same "Received oversize fragmented message" error the continuation path uses; otherwise it
stores the fragment exactly as before. The continuation clauses and the single-frame path are
untouched.
Tests: two new ones next to the existing
max_fragmented_message_sizetests, one text and onebinary. Each sends a 1,010 byte opening fragment against a 1,000 byte limit and expects the 1009
immediately, with the WebSock's terminate seeing the same reason as for an oversize continuation.
Both fail on
main, where nothing happens until a continuation arrives. Suite, format, credo anddialyzer are clean.