Repository navigation
CAMEL-25704: camel-undertow - refuse a request target or header value with control characters - #27944
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 46 of 706 tested, 26 compile-only — current: 46 all testedMaveniverse Scalpel detected 46 affected modules (current approach: 46). Skip-tests mode would test 46 modules (2 direct + 45 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (46)
Modules with tests skipped (26)
Build reactor — dependencies compiled but only changed modules were tested (2 modules, 47.8s total)Total reactor time: 47.8s
Top 20 slowest modules:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Good security fix — the checkWriteable helper and its integration are clean and well-tested.
Two observations worth addressing:
1. Host header bypasses checkWriteable when preserveHostHeader=true
When endpoint.isPreserveHostHeader() is true, the caller-supplied host header value is written directly to requestHeaders without validation (UndertowProducer.java, lines 138-140):
final String host = message.getHeader(UndertowConstants.HOST_STRING, String.class);
if (endpoint.isPreserveHostHeader()) {
requestHeaders.put(Headers.HOST, Optional.ofNullable(host).orElseGet(uri::getAuthority));A CR/LF in the incoming Host header bypasses the fix entirely. Calling UndertowHelper.checkWriteable("Host header", host) before the requestHeaders.put call would close this gap.
2. Missing test for the char-narrowing attack vector (ch > 0xFF → LF)
The PR description and Javadoc explicitly call out U+010A → LF narrowing as a key motivation for the fix, but no test exercises the ch > 0xFF branch of checkWriteable (line 174 in UndertowHelper.java). All three rejection tests use %0D%0A or literal \r\n, which hit the ch < 0x20 guard. A test passing U+010A directly in a query or header value would verify the narrowing branch is caught. See the inline comment on the test file for a suggested addition.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
Thanks for the review. Both points are addressed in 333f977:
Full Claude Code on behalf of oscerd |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest commits — all prior findings are addressed. ✅
- Host header bypass (
preserveHostHeader=true):checkWriteable("Host header", hostHeader)is now called before theput, correctly closing the injection vector. - Narrowing-character test:
rejectsNarrowingCharacterInQuerysends U+010A (>0xFF) and confirms the first branch ofcheckWriteablefires before the byte is narrowed. - Content-Type propagation:
checkWriteableis called insidetoHttpRequest, which is wrapped in theIllegalArgumentExceptioncatch block in the producer — the exception path is correct. rejectsCrlfInPreservedHostHeader: validates the fix end-to-end with CRLF injection intoHost.
The dismissed items (cookie values, header key injection, consumer-side, IAE propagation) were all correctly scoped as pre-existing or already handled by the call stack. No further concerns.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @oscerd, this looks good. The Undertow 2.4.4 client conduit writes each char with a plain byte cast and does no validation, so validating the decoded request target and every header value (including the preserved Host and Content-Type) is the right fix, and it keeps the CAMEL-15410 rest round trip intact. Allowing HTAB and Latin-1 obs-text while refusing controls, DEL and anything above 0xFF is a sensible line, and the upgrade-guide note covers the behaviour change. CI is green.
One optional suggestion inline.
Claude Code on behalf of Claus Ibsen
… with control characters Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…ers too The producer writes the Host header (the caller's own when preserveHostHeader is set) and the Content-Type header verbatim, outside the header-copy loop, so run them through UndertowHelper.checkWriteable as well. Adds tests for the above-0xFF narrowing branch and for a CR/LF in a preserved Host header. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
…der-value characters Pins the allowed set: a header value with a tab and a Latin-1 (obs-text) character is sent, not refused. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
7640e02 to
6cbc02b
Compare
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Previous findings are all addressed. The new commit adds sendsAHeaderWithTabAndLatin1 — a positive test pinning the allowed boundary: a header value with a tab (exempted by ch != '\t') and a Latin-1 obs-text character (é, 0xE9 ≤ 0xFF) is correctly not refused by checkWriteable. The test reaches the validation path through toHttpRequest's header loop, so it is not vacuous. No new issues.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @oscerd. Re-reviewed from 333f977 (the commit approved earlier) to the current head. Apart from the rebase onto main, the only change is the new sendsAHeaderWithTabAndLatin1 test; UndertowHelper, UndertowProducer and DefaultUndertowHttpBinding are byte-for-byte the same as what was approved.
The optional point from the earlier review is addressed: the test sends MyHeader: café<TAB>au lait, which goes through the header loop in toHttpRequest (the default UndertowHeaderFilterStrategy does not filter MyHeader), so it really exercises checkWriteable and pins SP, HTAB and Latin-1 as allowed. Because throwExceptionOnFailure is on by default, assertFalse(out.isFailed()) also confirms the Undertow server accepted the request. The earlier gnodet-bot points (the preserved Host header and coverage for the narrowing branch) are still fixed, the upgrade-guide note is unchanged, and CI is green.
Good to merge.
Claude Code on behalf of Claus Ibsen
davsclaus
added
port/camel-4.18.x
|
Added Claude Code on behalf of Claus Ibsen |
🔄 Backport BotConflicts (🤖 agent dispatched to resolve):
ℹ️ If you push additional commits after |
|
✅ Backport to |
|
✅ Backport to |
CAMEL-25704: camel-undertow - refuse a request target or header value with control characters
What
The undertow producer writes the path, query and header values of the outgoing HTTP request to the socket one
character at a time (
io.undertow.client.http.HttpRequestConduitdoesbuffer.put((byte) string.charAt(i))), with novalidation. So:
0xFFwas narrowed to a single, different byte (for exampleU+010Awas written as a line feed).The producer decodes a percent-encoded
CamelHttpPath/CamelHttpQuerybefore building the request target(
URISupport.pathAndQueryOfuses the decodedURI.getPath()/URI.getQuery()), so a percent-encoded controlcharacter in those headers became a real control character at that point.
Change (camel-undertow only)
UndertowHelper.checkWriteable(what, value): refuses a value that holds a control character,DEL, or acharacter above
0xFF.SPandHTAB, which do not change the request framing, are allowed.UndertowProducer.processvalidates the request target (afterpathAndQueryOf) and fails the exchange with anIllegalArgumentExceptionif it is not writeable, instead of handing it to the client. ThetoHttpRequestcall iswrapped so a rejected header value fails the exchange the same way (rather than escaping the async callback).
DefaultUndertowHttpBinding.toHttpRequestvalidates every outgoing header value with the same helper (Undertowvalidates header names but not values), including the
Content-Typeheader it puts directly.Hostheader is also validated: whenpreserveHostHeaderis set the producer writes the caller's ownHostheader verbatim, so it is run through
checkWriteablebefore the put.The decoding of
CamelHttpPath/CamelHttpQueryis deliberately kept: the rest producer double-encodes its queryparameters and relies on the undertow producer decoding once (CAMEL-15410,
RestUndertowProducerEncodingTest).Changing the producer to send the raw, still-encoded target would regress that round trip, so this change keeps the
existing decoding and only refuses characters that cannot be written as they are.
Tests
UndertowProducerEncodedUriTest: a percent-encoded CR/LF inCamelHttpQueryorCamelHttpPath, a character above0xFFin the query (the narrowing case), a CR/LF in a header value, and a CR/LF in a preservedHostheader all makethe exchange fail; an ordinary query is still sent, and a header value with a tab and a Latin-1 character is
accepted (pinning the allowed set).
RestUndertowProducerEncodingTestkeeps passing (the rest round trip is unchanged).camel-undertowmodule suite is green (232 run, 0 failures, 1 skipped).Docs
camel-4x-upgrade-guide-4_24.adoc.Relates to CAMEL-25704.
Claude Code on behalf of oscerd
🤖 Generated with Claude Code