Bug 62978

Summary: RemoteIpValve: Multiple forwards in X-Forwarded-Proto header not supported
Product: Tomcat 9 Reporter: Tom Groot <t-groot>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Severity: normal CC: t-groot
Priority: P2 Keywords: PatchAvailable
Version: unspecified   
Target Milestone: -----   
Hardware: All   
OS: All   
Attachments: Patch incl. test cases to support multiple forwarded protocols in X-Forwarded-Proto header value.
Fixes typo / copy paste mistake in test method names.

Description Tom Groot 2018-12-05 12:52:41 UTC
When a request gets forwarded through multiple proxies, certain proxy implementations seem to apply the same logic as it is common with the X-Forwarded-For header, and likewise append the incoming protocol to the existing value of the X-Forwarded-Proto header in a comma separated values fashion.
Should such a request reach tomcat, it can never be considered secure, due to its implementation in the RemoteIpValve.

The appended patch modifies the RemoteIpValve to consider also multiple forwards to be secure, if it exclusively holds forwards of secure protocols.
I tried to stay in line with the existing code formatting and avoided any exotics so it could be backported, if desired. For the new test cases i opted to avoid the duplication of the repetitive assertion code as seen at existing test cases, and hope that it is fine.

In our setup we have a loadbalancer enforcing the https protocol with web clients and setting the X-Forwarded-* headers on the requests it forwards to our API gateway (spring-cloud-netflix Zuul). Zuul processes these X-Forwarded-* for headers and adds itself to the chain of proxies, and in its implementation it performs the CSV based appending also to the X-Forwarded-Proto header.
Therefore the request reaching our services running on tomcat exhibit the header like that:
'X-Forwarded-Proto: https,https'
Which obviously doesn't match ignoreCase 'https' and thus is considered insecure. Configuring the protocolHeaderHttpsValue to be 'https,https' is not an acceptable solution.
Although not completely certain, we have the suspicion that this is the reason why something strips the "Secure" flags from our session cookies, which is rather bad. Regardless of whether this is related or not, the support for handling "new" behaviour of standard proxy solutions correctly and backwards compatibly should be beneficial.

How to reproduce:
curl -kI https://localhost:8443/some-application/setting-secure-cookies -H 'X-Forwarded-For:,' -H 'X-Forwarded-Proto: https'
HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Set-Cookie: JSESSIONID=A1B6F171F11844A0FE8E857DBB8A0AA1; Path=/some-application/; Secure; HttpOnly


curl -kI https://localhost:8443/some-application/setting-secure-cookies -H 'X-Forwarded-For:,' -H 'X-Forwarded-Proto: https,https'
HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Set-Cookie: JSESSIONID=A5B7933C3AE78ACA2AD7A1767A4163B1; Path=/some-application/; HttpOnly

I failed to find a profound resource specifying the value of this non-standard header. However i have come to the opinion that Zuul's way of processing this header is reasonable it is not the only kind of proxy doing it this way.
Zuul's source code:
Evidence/indication that this behaviour is, or has become recommended:

Thanks for considering and kind regards,
Comment 1 Tom Groot 2018-12-05 12:56:11 UTC
Created attachment 36291 [details]
Patch incl. test cases to support multiple forwarded protocols in X-Forwarded-Proto header value.
Comment 2 Mark Thomas 2018-12-06 14:46:23 UTC
Thanks for the report, test cases and patch. As bug reports go, this one was ideal.

Fixed in:
- trunk for 9.0.14 onwards
- 8.5.x for 8.5.36 onwards
- 7.0.x for 7.0.93 onwards

I also applied a very similar patch to the RemoteIpFilter that uses almost identical code.
Comment 3 Tom Groot 2018-12-08 22:57:18 UTC
Created attachment 36314 [details]
Fixes typo / copy paste mistake in test method names.

Hi Mark,

Thanks a lot for the quick response and also for taking care of the filter i didn't know of.
I have to apologize, i noticed now that when copy-pasting these endlessly long names of the test methods i failed to adjust some of them properly, hence this small follow-up patch. In cases where the mock request is prepared as http, the test method name said it was https. It should not matter too much, it would just be to avoid confusion if someone wanted to make sense of the tests.
Comment 4 Mark Thomas 2018-12-11 20:13:47 UTC
Patch applied. Thanks for the attention to detail. It is much appreciated.