Skip to content

Conversation

@pluknet
Copy link
Contributor

@pluknet pluknet commented Dec 4, 2025

Chunked transfer encoding, since originally introduced in HTTP/1.1 in RFC 2068, is specified to use CRLF as the only line terminator.

Although tolerant applications may recognize a single LF (discussed previously at [1]), formally this covers the start line and fields, and doesn't apply to chunks. Strict chunked parsing is reaffirmed as intentional in RFC errata ID 7633, notably "because it does not have to retain backwards compatibility with 1.0 parsers".

A general RFC 2616 recommendation to tolerate deviations interpreted unambiguously barely applies to chunks as they are used to determine HTTP message framing, and ambiguity may result in potentially broken delimitation. In contrast, accepting a single LF may pose an extra condition for potential request smuggling in complex scenarios. For instance, this is possible when receiving chunks from intermediates that misinterpret LF as part of a valid chunk-ext-name and pass it further without re-coding. The change aims to address this as well.

[1] https://mailman.nginx.org/pipermail/nginx-devel/2024-January/5CQQCHFYQMXTBAK7H2FITLVQQS5ECFFM.html

@pluknet pluknet added this to the nginx-1.29.4 milestone Dec 4, 2025
@pluknet pluknet self-assigned this Dec 4, 2025
@ac000
Copy link
Member

ac000 commented Dec 4, 2025

Change itself looks straightforward.

switch (state) {

case sw_chunk_start:
ctx->length = 3 /* "0" LF LF */;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ctx->length be corrected after bare LF is disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it needs to be adjusted. Also updated initial u->pipe->length in the proxy module,
and disabled bare LF in a trailer section for similar reasons; see 1c79a4e.

ctx->length is used to hint how much data we expect to receive.
It is important to not over-estimate as this may result in blocked request processing
as fixed in 88fc0f7.
In contrast, underestimating ctx->length may result in less efficient chunked processing,
because parser will be called more often than it might be.

On the client-facing side, ctx->length sets the remaining request body (rb->rest).
This is now largely irrelevant since after 150cbb0 we're prepared to read additional data
from the next pipelined request. Also, the chunked parser is called here whenever new data is received.
On the upstream side, ctx->length is used to buffer data in the event pipe when reading
from upstream until u->pipe->length is received, before invoking input filter to parse chunks.

@ac000
Copy link
Member

ac000 commented Dec 4, 2025 via email

@VadimZhestikov
Copy link

VadimZhestikov commented Dec 5, 2025

Hmm, don't we just bail out and return NGX_ERROR in such cases now?

ctx->length is minimal amount of data we want to see (this value is used in callers), and now looks like this value need to be increased, since in place of LF we expect always CR LF

Chunked transfer encoding, since originally introduced in HTTP/1.1
in RFC 2068, is specified to use CRLF as the only line terminator.

Although tolerant applications may recognize a single LF, formally
this covers the start line and fields, and doesn't apply to chunks.
Strict chunked parsing is reaffirmed as intentional in RFC errata
ID 7633, notably "because it does not have to retain backwards
compatibility with 1.0 parsers".

A general RFC 2616 recommendation to tolerate deviations whenever
interpreted unambiguously doesn't apply here, because chunked body
is used to determine HTTP message framing; a relaxed parsing may
cause various security problems due to a broken delimitation.
For instance, this is possible when receiving chunked body from
intermediates that blindly parse chunk-ext or a trailer section
until CRLF, and pass it further without re-coding.
Copy link

@VadimZhestikov VadimZhestikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@pluknet pluknet merged commit f405ef1 into nginx:master Dec 6, 2025
2 checks passed
@pluknet pluknet deleted the chunked branch December 6, 2025 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants