From 11538d85581447fb6b3f99bb6af5d87ba58f08e0 Mon Sep 17 00:00:00 2001 From: Javid Khan Date: Tue, 20 Oct 2026 11:01:40 +0530 Subject: [PATCH 1/2] strip control characters when writing the cookie header --- .../http/impl/cookie/RFC6265CookieSpec.java | 37 ++++++++++++++++++- .../impl/cookie/TestRFC6265CookieSpec.java | 24 ++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/RFC6265CookieSpec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/RFC6265CookieSpec.java index 590cc5a319..9f1887a913 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/RFC6265CookieSpec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/RFC6265CookieSpec.java @@ -236,8 +236,8 @@ public List
formatCookies(final List cookies) { buffer.append(PARAM_DELIMITER); buffer.append(' '); } - buffer.append(cookie.getName()); - final String s = cookie.getValue(); + buffer.append(sanitize(cookie.getName())); + final String s = sanitize(cookie.getValue()); if (s != null) { buffer.append(EQUAL_CHAR); if (containsSpecialChar(s)) { @@ -278,4 +278,37 @@ boolean containsChars(final CharSequence s, final Tokenizer.Delimiter chars) { return false; } + /** + * Neutralises characters that must never appear in a cookie name or value when the + * {@code Cookie} header is written out. RFC 6265 excludes control characters from both the + * cookie-name (a token) and the cookie-value (cookie-octet), yet {@link #formatCookies} used to + * copy them through verbatim. A bare CR or LF in a cookie set from attacker-influenced data + * would then be written straight into the request header on the classic transport, splitting it + * and letting further headers be injected. Replace CR, LF and NUL with a space, which also keeps + * the serialised header in step with {@code BufferedHeader#getValue()} (it applies the same + * substitution when the value is read back). + */ + private static String sanitize(final String s) { + if (s == null) { + return null; + } + int i = 0; + for (; i < s.length(); i++) { + final char ch = s.charAt(i); + if (ch == '\r' || ch == '\n' || ch == '\0') { + break; + } + } + if (i == s.length()) { + return s; + } + final StringBuilder buf = new StringBuilder(s.length()); + buf.append(s, 0, i); + for (; i < s.length(); i++) { + final char ch = s.charAt(i); + buf.append(ch == '\r' || ch == '\n' || ch == '\0' ? ' ' : ch); + } + return buf.toString(); + } + } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/cookie/TestRFC6265CookieSpec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/cookie/TestRFC6265CookieSpec.java index 56897c9b81..a20cb70959 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/cookie/TestRFC6265CookieSpec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/cookie/TestRFC6265CookieSpec.java @@ -35,6 +35,7 @@ import org.apache.hc.client5.http.cookie.Cookie; import org.apache.hc.client5.http.cookie.CookieOrigin; import org.apache.hc.client5.http.cookie.MalformedCookieException; +import org.apache.hc.core5.http.FormattedHeader; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.message.BasicHeader; import org.junit.jupiter.api.Assertions; @@ -291,6 +292,29 @@ void testFormatCookiesIllegalCharsInValue() { Assertions.assertEquals("name1=value; name2=\"some value\"; name3=\"\\\"\\\\\\\"\"", header.getValue()); } + @Test + void testFormatCookiesStripsControlCharsFromValue() { + final Cookie cookie = new BasicClientCookie("sid", "abc\r\nInjected: evil"); + final RFC6265CookieSpec cookiespec = new RFC6265CookieSpec(); + final List
headers = cookiespec.formatCookies(Collections.singletonList(cookie)); + // The classic transport writes the raw buffer of a FormattedHeader, not the sanitised + // getValue(), so the buffer itself must be free of CR/LF. + final String wire = ((FormattedHeader) headers.get(0)).getBuffer().toString(); + Assertions.assertFalse(wire.indexOf('\r') >= 0 || wire.indexOf('\n') >= 0, + "CR/LF leaked into the Cookie header buffer"); + Assertions.assertEquals("Cookie: sid=\"abc Injected: evil\"", wire); + } + + @Test + void testFormatCookiesStripsControlCharsFromName() { + final Cookie cookie = new BasicClientCookie("sid\r\nInjected: evil", "v"); + final RFC6265CookieSpec cookiespec = new RFC6265CookieSpec(); + final List
headers = cookiespec.formatCookies(Collections.singletonList(cookie)); + final String wire = ((FormattedHeader) headers.get(0)).getBuffer().toString(); + Assertions.assertFalse(wire.indexOf('\r') >= 0 || wire.indexOf('\n') >= 0, + "CR/LF leaked into the Cookie header buffer via the cookie name"); + } + @Test void testParseCookieMultipleAttributes() throws Exception { final CommonCookieAttributeHandler h1 = Mockito.mock(CommonCookieAttributeHandler.class); From 5440ddfa112a67dfcf6bda2ac467ddf340812694 Mon Sep 17 00:00:00 2001 From: Javid Khan Date: Tue, 20 Oct 2026 15:50:58 +0530 Subject: [PATCH 2/2] collapse a run of control characters to a single space in the cookie header --- .../http/impl/cookie/RFC6265CookieSpec.java | 22 +++++++++++++++---- .../impl/cookie/TestRFC6265CookieSpec.java | 2 +- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/RFC6265CookieSpec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/RFC6265CookieSpec.java index 9f1887a913..147e8ca19f 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/RFC6265CookieSpec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/cookie/RFC6265CookieSpec.java @@ -284,9 +284,11 @@ boolean containsChars(final CharSequence s, final Tokenizer.Delimiter chars) { * cookie-name (a token) and the cookie-value (cookie-octet), yet {@link #formatCookies} used to * copy them through verbatim. A bare CR or LF in a cookie set from attacker-influenced data * would then be written straight into the request header on the classic transport, splitting it - * and letting further headers be injected. Replace CR, LF and NUL with a space, which also keeps - * the serialised header in step with {@code BufferedHeader#getValue()} (it applies the same - * substitution when the value is read back). + * and letting further headers be injected. A maximal run of CR, LF and NUL is collapsed to a + * single space so a folded {@code CRLF} leaves one space rather than a run of them, matching the + * substitution {@code BufferedHeader#getValue()} already applies when the value is read back. + * This closes the injection vector; it is not meant to fully validate the name against the + * token grammar or the value against cookie-octet. */ private static String sanitize(final String s) { if (s == null) { @@ -304,9 +306,21 @@ private static String sanitize(final String s) { } final StringBuilder buf = new StringBuilder(s.length()); buf.append(s, 0, i); + boolean pending = false; for (; i < s.length(); i++) { final char ch = s.charAt(i); - buf.append(ch == '\r' || ch == '\n' || ch == '\0' ? ' ' : ch); + if (ch == '\r' || ch == '\n' || ch == '\0') { + pending = true; + } else { + if (pending) { + buf.append(' '); + pending = false; + } + buf.append(ch); + } + } + if (pending) { + buf.append(' '); } return buf.toString(); } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/cookie/TestRFC6265CookieSpec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/cookie/TestRFC6265CookieSpec.java index a20cb70959..da706150ad 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/cookie/TestRFC6265CookieSpec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/cookie/TestRFC6265CookieSpec.java @@ -302,7 +302,7 @@ void testFormatCookiesStripsControlCharsFromValue() { final String wire = ((FormattedHeader) headers.get(0)).getBuffer().toString(); Assertions.assertFalse(wire.indexOf('\r') >= 0 || wire.indexOf('\n') >= 0, "CR/LF leaked into the Cookie header buffer"); - Assertions.assertEquals("Cookie: sid=\"abc Injected: evil\"", wire); + Assertions.assertEquals("Cookie: sid=\"abc Injected: evil\"", wire); } @Test