fix: prevent CRLF injection through chunk trailers and extensions (#1256) - #1270
Open
pjfanning wants to merge 1 commit into
Open
fix: prevent CRLF injection through chunk trailers and extensions (#1256)#1270pjfanning wants to merge 1 commit into
pjfanning wants to merge 1 commit into
Conversation
…ache#1256) Motivation: `RenderSupport.renderChunk` emitted two attacker-influenced parts of a chunked response without the CR/LF guard that the main header block relies on: - Trailer headers were rendered with `r ~~ trailer`, which resolves to the generic sequence renderer and calls `header.render` directly, bypassing the `~~(HttpHeader)` overload whose `check` scans the rendered bytes for CR/LF and drops the header. `HttpEntity.LastChunk` and `RawHeader` do no CR/LF validation, so a trailer built from user data (e.g. `RawHeader("X-Trace", "ok\r\nSet-Cookie: ..."))`) could split the response. The identical header placed in the main header block is caught; only the trailer path let it through. - The chunk extension was rendered raw into the chunk-size line (`r ~~ ';' ~~ extension`) with no CR/LF check, so a CR/LF in an app-set extension corrupted the chunk framing. Modification: Render each trailer header through the guarded `~~(HttpHeader)` overload (`trailer.foreach(r ~~ _)`), matching the main header block, and remove the now-unused `trailerRenderer` implicit so the unguarded path cannot be reintroduced by accident. Skip a chunk extension that contains CR/LF; the extension is optional metadata, so omitting an illegal one is safe. Byte output is unchanged for valid trailers and extensions. Result: CR/LF in a chunk trailer header value or a chunk extension can no longer reach the wire; the offending header/extension is dropped, as in the main header block, instead of splitting the response. Tests: - sbt "http-core/testOnly org.apache.pekko.http.impl.engine.rendering.ResponseRendererSpec org.apache.pekko.http.impl.engine.rendering.RequestRendererSpec" - pass (62 tests); two new tests assert a CRLF-bearing trailer header and a CRLF-bearing chunk extension are dropped. Verified both fail with the fix stashed (the injected bytes reach the output). - sbt http-core/mimaReportBinaryIssues - pass (internal impl.engine.rendering change, no public API). References: None - closes the CRLF-injection paths in chunked response rendering
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.
cherry pick b9a6a41 #1256
Motivation:
RenderSupport.renderChunkemitted two attacker-influenced parts of a chunked response without the CR/LF guard that the main header block relies on:r ~~ trailer, which resolves to the generic sequence renderer and callsheader.renderdirectly, bypassing the~~(HttpHeader)overload whosecheckscans the rendered bytes for CR/LF and drops the header.HttpEntity.LastChunkandRawHeaderdo no CR/LF validation, so a trailer built from user data (e.g.RawHeader("X-Trace", "ok\r\nSet-Cookie: ..."))) could split the response. The identical header placed in the main header block is caught; only the trailer path let it through.r ~~ ';' ~~ extension) with no CR/LF check, so a CR/LF in an app-set extension corrupted the chunk framing.Modification:
Render each trailer header through the guarded
~~(HttpHeader)overload (trailer.foreach(r ~~ _)), matching the main header block, and remove the now-unusedtrailerRendererimplicit so the unguarded path cannot be reintroduced by accident. Skip a chunk extension that contains CR/LF; the extension is optional metadata, so omitting an illegal one is safe. Byte output is unchanged for valid trailers and extensions.Result:
CR/LF in a chunk trailer header value or a chunk extension can no longer reach the wire; the offending header/extension is dropped, as in the main header block, instead of splitting the response.
Tests:
References:
None - closes the CRLF-injection paths in chunked response rendering