Bug 63434

Summary: Multiple Cookie headers combined to one comma-separated header line
Product: Apache httpd-2 Reporter: Armin Abfalterer <a.abfalterer>
Component: CoreAssignee: Apache HTTPD Bugs Mailing List <bugs>
Status: NEW ---    
Severity: normal Keywords: PatchAvailable
Priority: P2    
Version: 2.4.39   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: patch that turns multiple Cookie headers into single header
patch file with initialization fix

Description Armin Abfalterer 2019-05-15 08:42:19 UTC
RFC 6265, 5.4. The Cookie Header says

"When the user agent generates an HTTP request, the user agent MUST NOT attach more than one Cookie header field."

However, httpd combines multiple Cookie headers into on header line; e.g.

Cookie: foo1=bar1
Cookie: foo2=bar2
Cookie: foo3=bar3

becomes "Cookie: foo1=bar1, foo2=bar2, foo3=bar3" (which in turns violates syntax definition in RFC 6265, 4.2.1. Syntax).

The call of apr_table_compress() in in server/protocol.c:ap_get_mime_headers_core() leads to this misbehaviour

https://github.com/apache/httpd/blob/trunk/server/protocol.c#L1274

Cheers, Armin
Comment 1 Yann Ylavic 2019-05-15 09:05:15 UTC
(In reply to Armin Abfalterer from comment #0)
> RFC 6265, 5.4. The Cookie Header says
> 
> "When the user agent generates an HTTP request, the user agent MUST NOT
> attach more than one Cookie header field."

This says "there must not be multiple Cookie headers" (thus "multiple cookies must be in the same header"), which you translate as "there must not be multiple cookies in the same header"?

It seems to me that apr_table_compress() does the right thing, including for Cookie headers..
Comment 2 Armin Abfalterer 2019-05-15 10:43:23 UTC
(In reply to Yann Ylavic from comment #1)

> This says "there must not be multiple Cookie headers" (thus "multiple
> cookies must be in the same header"), which you translate as "there must not
> be multiple cookies in the same header"?

No, multiple cookies are fine in the same header... but separated by semicolon, not comma.
Comment 3 Yann Ylavic 2019-05-15 11:18:19 UTC
So, since comma in a header is equivalent to multiple headers, do you propose that httpd rejects (with status 4xx) any request with either multiple Cookie header or a single one containing comma(s)?

Because turning multiple Cookie headers into a single one with semicolon(s) is not the same HTTP request (while the comma preserves semantics), the only possible action would be to reject.

Also, it seems to me that Cookie is an application thingy, not an HTTP one, so why would httpd reject it if the HTTP header is valid?
With comma separated cookies, the application can detect and reject, not if httpd changes the semantics..
Comment 4 Armin Abfalterer 2019-05-15 12:19:04 UTC
(In reply to Yann Ylavic from comment #3)
> So, since comma in a header is equivalent to multiple headers, do you
> propose that httpd rejects (with status 4xx) any request with either
> multiple Cookie header or a single one containing comma(s)?
> 
> Because turning multiple Cookie headers into a single one with semicolon(s)
> is not the same HTTP request (while the comma preserves semantics), the only
> possible action would be to reject.

I'd propose either to reject a request with multiple Cookie headers or to turn multiple Cookie headers into one where each cookie-pair is separated by semicolon.

In any case I'd propose to reject a request with comma separated cookie-pairs in a Cookie header.

> Also, it seems to me that Cookie is an application thingy, not an HTTP one,
> so why would httpd reject it if the HTTP header is valid?
> With comma separated cookies, the application can detect and reject, not if
> httpd changes the semantics..

In my opinion separated cookie pairs are a HTTP protocol violation so httpd should not allow this at all; e.g. such request should not hit backend servers when mod_proxy is in use
Comment 5 Armin Abfalterer 2019-05-27 07:34:15 UTC
Created attachment 36600 [details]
patch that turns multiple Cookie headers into single header
Comment 6 Armin Abfalterer 2019-05-27 09:43:57 UTC
Created attachment 36601 [details]
patch file with initialization fix

(In reply to Armin Abfalterer from comment #5)
> Created attachment 36600 [details]
> patch that turns multiple Cookie headers into single header

there is a error in the patch, a wrong initialization

cookie_hdrs_state state = {r->pool, NULL, 0};

see new patch file
Comment 7 rgagnon24 2019-12-19 09:19:13 UTC
I can confirm I've seen this behavior as well.  The problem stems from bad clients under HTTP/1.1.

HTTP/2 allows for multiple "Cookie" headers, but the problem shows really when a HTTP/1.1 user-agent incorrectly sends multiple Cookie headers.

Apache is compressing them with ", " into one header.  This leads to an improper single Cookie value that some language like PHP (or others) will then not be able to properly parse (as commas are allowed characters within the value of a cookie), and because cookie parsers in most interpreters are going to "split" the key-value pairs out of the single Cookie: value by looking for a semi-colon to split on, and then creating the key-value pairs from those.

So RFC6265 (https://tools.ietf.org/html/rfc6265#page-7) shows how servers can send multiple "Set-Cookie" headers (to support proxies like load balancers, etc injecting their own cookies), but as the OP stated, 5.4 states the user agent MUST NOT attach more than one Cookie header field" (which is correct for HTTP/1.1)

The compression routine in apache is technically not at fault (sort of) because it is combining multiple headers as other RFCs allow, but the issue it would be nice if (for the Cookie header) that httpd could play nice with a bad user-agent, and fix the multiple "Cookie" header mistake the UA makes, so that the parser behind httpd can properly see the values.

Scenario 1: Similar example to the OP, imagine a bad UA sends back:

Cookie: foo1=hello, world
Cookie: foo2=How are you, sam?

Current httpd would compress this into:

Cookie: foo1=hello, world, foo2=How are you, sam?

Then some processor looking at the COOKIE value, would only see a single cookie with the key "foo1" and a value of "hello, world, foo2=How are you, sam?" 

If the processor were to try and split on the ", " that the apr_table_compress() inserted, it would not properly be able to parse the string because the parts would be:

foo1=hello
world
foo2=How are you
sam?

However, if the "Cookie" header were given special-case treatment (as would be needed under an HTTP/2 transaction anyhow, then the routine would create:

Cookie: foo1=hello, world;foo2=How are you, sam?

Which, current processors will properly split with semicolon delimiting that they expect in the Cookie value.

In the case of PHP, the current problem with the apr_table_compress() in regard to Cookie is that the resulting $_COOKIE array in that language will miss seeing Cookies that should be present, which can lead to broken SESSION's as the session-id used is stored in the Cookie header---like any other language.

We've seen this with incompatible UA's communicating with server banks behind load balancers especially.

Scenario 2: UA connects to a server through a proxy like a load balancer that adds its own cookie for state tracking, and a server that adds the cookie value "PHPSESSID" for session tracking.

When the server initiates the response back to the UA, it will add something like:

Set-Cookie: PHPSESSID=XXXXXXXXXXXX; path=/

On the way through the proxy, the proxy (like and F5 load balancer) adds its own cookie:

Set-Cookie: BIGipServer~XXXXX~XXXX=XXXXX; expires=Some date; path=/; Httponly

Now, the poor broken HTTP/1.1 User-agent (which is supposed to combine those 2 headers into one, but does not), sends back:

Cookie: PHPSESSID=XXXXXXXXXXXX
Cookie: BIGipServer~XXXXX~XXXX=XXXXX

apr_table_compress() now proceeds to combine those into one header before PHP gets it:

Cookie: PHPSESSID=XXXXXXXXXXXX, BIGipServer~XXXXX~XXXX=XXXXX

When PHP now looks to find it's session data, it can't find it because the value for "PHPSESSID" now appears as "XXXXXXXXXXXX, BIGipServer~XXXXX~XXXX=XXXXX"

Worse yet, if the bad user-agent sends the headers in a different order, PHP won't even find a value at all for "PHPSESSID" because there will be no Cookie that starts like that... for example:

Cookie: BIGipServer~XXXXX~XXXX=XXXXX
Cookie: PHPSESSID=XXXXXXXXXXXX

Would become:

Cookie: BIGipServer~XXXXX~XXXX=XXXXX, PHPSESSID=XXXXXXXXXXXX

And the only thing then present in $_COOKIE would be the key "BIGipServer~XXXXX~XXXX" with the value "XXXXX, PHPSESSID=XXXXXXXXXXXX"
Comment 8 Yann Ylavic 2019-12-19 11:32:28 UTC
(In reply to rgagnon24 from comment #7)
> 
> Apache is compressing them with ", " into one header.  This leads to an
> improper single Cookie value that some language like PHP (or others) will
> then not be able to properly parse (as commas are allowed characters within
> the value of a cookie),

From rfc6265 (which you refer to) it is defined as:
 cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace, DQUOTE, comma, semicolon,
                       ; and backslash

In older RFCs (because cookies have several RFCs and the latest is not necessarily the one used), the value was defined as a token (or quoted-string):
       token          = 1*<any CHAR except CTLs or separators>
       separators     = "(" | ")" | "<" | ">" | "@"
                      | "," | ";" | ":" | "\" | <">
                      | "/" | "[" | "]" | "?" | "="
                      | "{" | "}" | SP | HT

In the original Netscape's "Persistent Client State -- HTTP Cookies" (actually the one mostly used):
 NAME=VALUE
    This string is a sequence of characters excluding semi-colon, comma and
    white space. If there is a need to place such data in the name or value,
    some encoding method such as URL style %XX encoding is recommended,
    though no encoding is defined or required.

So no, commas and whitespaces are not allowed in a cookie value (unless quoted). The are allowed in the Expires attribute value, with no quoting (!), causing all sort of special case / workarounds for Set-Cookie headers (like never compressing them).

> and because cookie parsers in most interpreters are
> going to "split" the key-value pairs out of the single Cookie: value by
> looking for a semi-colon to split on, and then creating the key-value pairs
> from those.

These parsers need to be fixed IMHO, they should look for the comma too, a compressed Cookie header is a single Cookie header (though semantically equivalent in HTTP to as much headers as commas).
Yes they should not happen because, as you said, rfc6265 forbids multiple Cookie headers in the first place, but by the way I'm afraid there is no such thing as a cookie RFC which is honored by everyone...

> The compression routine in apache is technically not at fault (sort of)
> because it is combining multiple headers as other RFCs allow, but the issue
> it would be nice if (for the Cookie header) that httpd could play nice with
> a bad user-agent, and fix the multiple "Cookie" header mistake the UA makes,
> so that the parser behind httpd can properly see the values.

httpd enforces the HTTP protocol, not rfc6265 or any cookie specification which is an application thing (i.e. httpd has nothing to do with cookies besides validating that they are correct HTTP headers). If modules/applications need to make use of the cookies they should extract them from the HTTP header, and parse them according to HTTP (compressed-)headers semantics, and eventually reject them if a comma is found (meaning multiple headers were received).

> When PHP now looks to find it's session data, it can't find it because the
> value for "PHPSESSID" now appears as "XXXXXXXXXXXX,
> BIGipServer~XXXXX~XXXX=XXXXX"
> 
> Worse yet, if the bad user-agent sends the headers in a different order, PHP
> won't even find a value at all for "PHPSESSID" because there will be no
> Cookie that starts like that... for example:
> 
> Cookie: BIGipServer~XXXXX~XXXX=XXXXX
> Cookie: PHPSESSID=XXXXXXXXXXXX
> 
> Would become:
> 
> Cookie: BIGipServer~XXXXX~XXXX=XXXXX, PHPSESSID=XXXXXXXXXXXX
> 
> And the only thing then present in $_COOKIE would be the key
> "BIGipServer~XXXXX~XXXX" with the value "XXXXX, PHPSESSID=XXXXXXXXXXXX"

How about fixing (mod_)PHP(-fpm)? Either to reject the request (enforcing rfc6265, which wouldn't help in your case) or to deal with the comma correctly.
PHP cares about cookies since it provides $_COOKIE, httpd does not.

Please note that addressing this in httpd would require a quite painful treatment for every request: first set aside Cookie headers, compress the other headers with comma, compress the Cookie headers with semicolon and re-insert. This all would be before knowing whether the Cookies are needed/used later or not, so at the very least it would need to be optional.