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..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 @@ -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,51 @@ 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. 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) { + 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); + boolean pending = false; + for (; i < s.length(); i++) { + final char ch = s.charAt(i); + 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 56897c9b81..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 @@ -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);