Repository navigation
Read chunked trailers with the body read_timeout and the configured header limits - #738
Merged
Merged
Conversation
…eader limits
### The problem
The trailer section at the end of a chunked request body is read with the wrong settings. Two
of them, with the same cause.
The timeout. Every read of a chunked body uses the `read_timeout` passed to
`Plug.Conn.read_body/2` (15 seconds by default): the chunk sizes, the chunk data, the CRLF after
each chunk. The trailer section is the exception. It is read with the Thousand Island socket's own
`read_timeout`, 60 seconds by default, which is meant for idle keepalive connections. So a client
that sends `0\r\n` and then stops holds the connection for 60 seconds before its 408, where one
that stops in the middle of a chunk gets the 15 the plug asked for.
The header limits. `max_header_count` and `max_header_length` configured for the server don't
apply to the trailer section. The defaults of 50 and 10,000 do. A server configured with
`max_header_count: 5` still accepts a 50 line trailer section.
Repro on `main`, with any plug that reads the body:
```
POST / HTTP/1.1\r\nhost: x\r\ntransfer-encoding: chunked\r\n\r\n3\r\n123\r\n0\r\n
(then nothing) -> 408 after 60 s, not after the plug's read_timeout
...0\r\n followed by 50 trailer lines, on a server with max_header_count: 5
-> 200 OK
```
### Why it happens
`do_read_chunk!/4` reads the trailer section through a throwaway `%Bandit.HTTP1.Socket{}` that it
builds on the spot, so that `do_read_headers!/6` can parse the trailers the way it parses the
header section. That throwaway socket carried none of the real socket's settings: its Thousand
Island socket kept the server's `read_timeout`, and its options were `%{http_1: []}`, so
`do_read_headers!/6` fell back to its defaults for both limits.
### Why it matters
A limit an operator sets should hold for the whole request. Trailers are field lines like any
other ([RFC 9112 §7.1.2](https://www.rfc-editor.org/rfc/rfc9112.html#section-7.1.2)), and a
client gets to send them at the default limits however the server is configured. The exposure is
bounded (50 lines of 10,000 bytes, parsed once and thrown away), which is why this is an ordinary
PR and not a report. The timeout half is the one people would actually notice: a stalled client
costs four times the configured wait on this one path, and the option documented as the per-read
timeout for the body isn't honoured for the last piece of it.
### The fix
Build the throwaway socket from the real one's settings. `read_timeout` on its Thousand Island
socket is set to the body option, and the server's HTTP options are threaded down from
`read_data/2` through the three chunk-reading functions to where the socket is built, in place of
`%{http_1: []}`. `do_read_headers!/6` is unchanged; it picks up the configured limits exactly as it
does for the header section. Fifteen lines in `lib`, most of them the extra argument.
### Tests
Three, in the chunked bodies block, all failing on `main`. One starts a server with a 60 second
socket timeout, sends `3\r\n123\r\n0\r\n` to a plug reading with `read_timeout: 100`, and expects
the read timeout log and the 408 within half a second; on `main` the handler is still waiting on
the socket timeout when the assertion gives up. Two more configure `max_header_count: 3` and
`max_header_length: 100` and expect a 431 for a four line trailer section and for a 200 byte
trailer line, with the same error messages the header section produces. Suite, format, credo and
dialyzer are clean.
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.
When a chunked request body ends, Bandit reads the trailer section with the wrong settings. I
found this while going through how the chunked body reader applies its settings, and it turned out
to be two problems with one cause.
The first is the timeout. Reading a chunked body uses the
read_timeoutpassed toPlug.Conn.read_body/2(15 seconds by default) for every read: the chunk sizes, the chunk data,the CRLF after each chunk. Except the trailer section. That read uses the Thousand Island socket's
own
read_timeout, 60 seconds by default, which exists for idle keepalive connections. So aclient that sends
3\r\n123\r\n0\r\nand then goes quiet holds the connection for 60 secondsbefore it gets a 408, where a client that goes quiet in the middle of a chunk gets 15. If you've
raised the socket timeout for long-lived keepalive connections, say to five minutes, a stalled
trailer section holds the connection for five minutes, and the option you'd reach for to fix that,
read_timeoutonread_body, has no effect on it.The second is the header limits.
max_header_countandmax_header_lengthdon't apply to thetrailer section. Whatever you configure, trailers are parsed under the defaults of 50 lines and
10,000 bytes each. A server with
max_header_count: 5accepts a 50 line trailer section withoutcomplaint.
Both come from the same place.
do_read_chunk!/4reads the trailers through a throwaway%Bandit.HTTP1.Socket{}that it builds on the spot, so thatdo_read_headers!/6can parse them theway it parses the header section. That throwaway socket got none of the real socket's settings:
its Thousand Island socket still had the server's
read_timeout, and its options were%{http_1: []}, sodo_read_headers!/6fell back to its defaults for both limits.Why I think this is worth fixing. The timeout is the half people will actually hit.
read_timeoutis how an application bounds a slow or stalled upload, and on this path a client gets around that
bound by stalling in the trailer section instead of the body, holding a connection process four
times longer than intended by default, and for as long as the socket timeout allows if you've
raised it. Nothing in the configuration can close that gap today. The limits half matters less in
practice: the exposure is bounded at 50 lines of 10,000 bytes, parsed once and thrown away, which
is why this is an ordinary PR and not a report. But a limit an operator sets should hold for the
whole request. Trailers are field lines like any other
(RFC 9112 §7.1.2), and the header
section already honours both settings, so it's hard to see why trailers shouldn't.
The fix builds the throwaway socket from the real one's settings.
read_timeouton its ThousandIsland socket is set to the body option, and the server's HTTP options are threaded down from
read_data/2through the three chunk-reading functions to where the socket is built, in place of%{http_1: []}.do_read_headers!/6doesn't change; it picks up the configured limits the sameway it does for the header section. Fifteen lines in lib, most of them the extra argument.
Three tests in the chunked bodies block, all failing on
main. One starts a server with a 60second socket timeout, sends
3\r\n123\r\n0\r\nto a plug reading withread_timeout: 100, andexpects the read timeout log and the 408 within half a second; on
mainthe handler is stillwaiting on the socket timeout when the assertion gives up. The other two configure
max_header_count: 3andmax_header_length: 100and expect a 431 for a four line trailer sectionand for a 200 byte trailer line, with the same error messages the header section produces. Suite,
format, credo and dialyzer are clean.