[dxbjavid]

RFC6265CookieSpec accepted and wrote out cookies whose name or value carried control characters. The HTTP/1.1 session buffer already keeps CR, LF and NUL from reaching the wire in the default configuration (TextUtils.castAsByte), and the explicit charset encoder path is being closed in httpcomponents-core (apache/httpcomponents-core#725), so this is not a header injection on its own. Given the sensitive nature of cookies it still seems better to refuse such cookies outright than to carry them around or quietly rewrite them.

This change rejects a cookie whose name or value contains a control character (%x00-1F or %x7F), which RFC 6265 permits in neither cookie-name nor cookie-value. validate() throws MalformedCookieException, so a Set-Cookie carrying one is rejected before it reaches the cookie store (ResponseProcessCookies already logs and drops those), and formatCookies() throws IllegalArgumentException, per its documented contract, rather than copying such a cookie into the Cookie request header. The earlier sanitising approach is dropped.

[arturobernalg]

One issue with the test: replacing CR and LF individually produces three spaces, including the original one, not one. Please adjust the expected value.

Also, replacing CR, LF, and NUL with spaces prevents this header injection vector, but does not guarantee compliance with the strict RFC 6265 cookie syntax.

[dxbjavid]

good catch on the spaces. i've changed it so a maximal run of CR/LF/NUL collapses to a single space, so a folded CRLF now leaves one space rather than a run of them, and updated the expected value to match (sid="abc Injected: evil").

on the second point, agreed, this only neutralises the write-side injection vector and isn't meant to fully validate the name against the token grammar or the value against cookie-octet. i kept the change narrow to the CR/LF/NUL that split the header, matching what BufferedHeader.getValue() already does on read-back. happy to follow up with stricter cookie-name/cookie-value validation as a separate change if you'd like it.

[ok2c]

@dxbjavid @arturobernalg Can we take a step back and take look at the problem from a broader view.

But first, I really do not like this

On the classic transport the header is serialised from the raw buffer of the BufferedHeader rather than its getValue()

Does that mean the async transport behaves differently in that regard? If so, this needs to be fixed first and foremost.

Then, is this problem really specific to RFC6265 cookie spec? Should not it be fixed at at the HTTP/1.1 connection layer for headers of any type?

[ok2c]

@dxbjavid As far as I can see the behavior of the classic and the async message writers looks consistent

https://github.com/apache/httpcomponents-core/blob/master/httpcore5/src/main/java/org/apache/hc/core5/http/impl/io/AbstractMessageWriter.java#L86
https://github.com/apache/httpcomponents-core/blob/master/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/AbstractMessageWriter.java#L87

[dxbjavid]

you're right that the two writers are consistent. both take the FormattedHeader's raw CharArrayBuffer and write it verbatim, so this isn't a classic-vs-async difference, and my wording in the description was misleading on that. the asymmetry is really within the header itself: BufferedHeader.getValue() substitutes CR/LF/NUL with a space, but the buffer the writers serialise keeps them.

agreed too that the proper place for a write-side guard is the HTTP/1.1 header serialisation, so it covers headers of any type rather than just RFC6265 cookies. that lives in httpcore (AbstractMessageWriter), so it'd be a change over there, not here. happy to raise it against httpcomponents-core, either stripping the control characters at serialisation or rejecting a header that carries them, whichever you'd prefer. if you'd rather fix it centrally i can close this one, or leave it as a narrow cookie-spec stopgap until the core change lands.

[arturobernalg]

@dxbjavid @arturobernalg Can we take a step back and take look at the problem from a broader view.

But first, I really do not like this

On the classic transport the header is serialised from the raw buffer of the BufferedHeader rather than its getValue()

Does that mean the async transport behaves differently in that regard? If so, this needs to be fixed first and foremost.

Then, is this problem really specific to RFC6265 cookie spec? Should not it be fixed at at the HTTP/1.1 connection layer for headers of any type?

Agreed. FormattedHeader bypasses the normal header formatting, so this is not cookie-specific. The fix belongs in HttpCore's HTTP/1 serialization layer, covering both classic and async transports.

[ok2c]

you're right that the two writers are consistent. both take the FormattedHeader's raw CharArrayBuffer and write it verbatim, so this isn't a classic-vs-async difference, and my wording in the description was misleading on that. the asymmetry is really within the header itself: BufferedHeader.getValue() substitutes CR/LF/NUL with a space, but the buffer the writers serialise keeps them.

agreed too that the proper place for a write-side guard is the HTTP/1.1 header serialisation, so it covers headers of any type rather than just RFC6265 cookies. that lives in httpcore (AbstractMessageWriter), so it'd be a change over there, not here. happy to raise it against httpcomponents-core, either stripping the control characters at serialisation or rejecting a header that carries them, whichever you'd prefer. if you'd rather fix it centrally i can close this one, or leave it as a narrow cookie-spec stopgap until the core change lands.

@dxbjavid I do not mind having an extra safe-guard in RFC6265 cookie policy, but I want to understand if there is a bigger problem at the low transport level. As far as I understand there is already a safe-guard to ensure no non-printable characters can end up in the session buffer. See TextUtils#castAsByte and its usage.

I am actually not sure there is a problem at all.

[ok2c]

@dxbjavid It looks like @arturobernalg has found the root cause of the issue. The unchecked conversion from char to byte can only occur when the user has explicitly specified a non-standard charset for HTTP message elements. I personally do not see this as a very severe problem, but if we decide to fix it it should be fixed in core (the session buffer layer).

[dxbjavid]

@ok2c @arturobernalg that matches what i see now. with no charset configured, SessionOutputBufferImpl.writeLine copies the header buffer through ByteArrayBuffer.append, which runs every char through TextUtils.castAsByte, so CR/LF/NUL go out as '?' and the Cookie line can't be split. my test only looked at the FormattedHeader buffer rather than the bytes that actually reach the socket, so the description overstated it for the default setup, sorry about that.

the gap that's left looks to be the encoder path (writeEncoded), which only applies when a non-standard charset has been set explicitly and hands the chars to the CharsetEncoder unchecked. agreed that's low severity and belongs in the session buffer layer in core. i can open a small PR against httpcomponents-core that applies the same substitution there if that's useful. as for this one, i'm happy to keep it as an extra cookie-level safeguard or close it, whichever you prefer.

[arturobernalg]

@ok2c @arturobernalg that matches what i see now. with no charset configured, SessionOutputBufferImpl.writeLine copies the header buffer through ByteArrayBuffer.append, which runs every char through TextUtils.castAsByte, so CR/LF/NUL go out as '?' and the Cookie line can't be split. my test only looked at the FormattedHeader buffer rather than the bytes that actually reach the socket, so the description overstated it for the default setup, sorry about that.

the gap that's left looks to be the encoder path (writeEncoded), which only applies when a non-standard charset has been set explicitly and hands the chars to the CharsetEncoder unchecked. agreed that's low severity and belongs in the session buffer layer in core. i can open a small PR against httpcomponents-core that applies the same substitution there if that's useful. as for this one, i'm happy to keep it as an extra cookie-level safeguard or close it, whichever you prefer.

@dxbjavid
Thanks for confirming. This is already addressed by apache/httpcomponents-core#725, which covers the explicit charset encoder path in both classic and async transports.

[ok2c]

@dxbjavid What we can still do in client is to reject cookies with non-printable chars in them as invalid, given sensitive nature of cookies.. Please do feel to re-spin your change-set and please do continue contributing.

[dxbjavid]

re-spun as suggested. instead of rewriting the name and value, the spec now rejects a cookie carrying a control character (%x00-1F or %x7F): validate() throws MalformedCookieException, so a Set-Cookie with one is dropped before it reaches the store, and formatCookies() throws IllegalArgumentException per its documented contract rather than writing it into the Cookie header. the sanitising code is gone and the description is updated to match.

one thing worth a sanity check: on the write side this fails the request rather than quietly skipping the cookie, which seemed right for something that should never have made it into the store, but i can switch it to skipping with a warning if you'd prefer. 1079 tests and checkstyle pass locally.