Repository navigation
Conversation
Motivation: renderChunk writes a chunk extension raw into the chunk-size line and drops it if it contains CR or LF, but not NUL. Since apache#1260 the header and trailer renderers drop all three via Rendering.isIllegalHeaderChar, so a NUL in an application-supplied chunk extension was the one place it still reached the wire. Review of the threat model in apache#1295 found the gap: P2 had to be narrowed to exclude chunk extensions from its NUL claim. Modification: Check the extension with Rendering.isIllegalHeaderChar, the predicate the header and trailer paths use. Add a ResponseRendererSpec case beside the existing CRLF one. Result: A chunk extension containing CR, LF or NUL is dropped on both the server response and client request paths, which share renderChunk. Tests: - sbt "http-core / Test / testOnly ...ResponseRendererSpec -- -z NUL": failed before the fix (rendered "7;ok\0bad") - sbt "http-core / Test / testOnly ...ResponseRendererSpec ...RequestRendererSpec": 63 passed - scalafmt --mode diff-ref=upstream/main: no changes References: Refs apache#1256, apache#1260, apache#1295
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
1 participant
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.
Motivation
RenderSupport.renderChunkwrites a chunk extension raw into the chunk-size line and drops it if it contains CR or LF — but not NUL. Since #1260 the header and trailer renderers drop all three viaRendering.isIllegalHeaderChar, so a NUL in an application-supplied chunk extension was the one rendering path where it still reached the wire.Found while reviewing the threat model in #1295, which had to narrow P2 so its NUL claim excludes chunk extensions.
Modification
renderChunkchecks the extension withRendering.isIllegalHeaderChar, the same predicate as the header and trailer paths.ResponseRendererSpec: a NUL case beside the existing CRLF one.Result
A chunk extension containing CR, LF or NUL is dropped.
renderChunkis shared, so this covers both server responses and client requests. Once this merges, the threat model's P2 can claim NUL for chunk extensions again.Tests
sbt "http-core / Test / testOnly ...ResponseRendererSpec -- -z NUL": failed before the fix (rendered7;ok\0bad)sbt "http-core / Test / testOnly ...ResponseRendererSpec ...RequestRendererSpec": 63 passedscalafmt --mode diff-ref=upstream/main: no changesprivate[http]method, no binary shape changeReferences
Refs #1256, #1260, #1295