[Bug 62978] New: RemoteIpValve: Multiple forwards in X-Forwarded-Proto header not supported

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 62978] New: RemoteIpValve: Multiple forwards in X-Forwarded-Proto header not supported

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978

            Bug ID: 62978
           Summary: RemoteIpValve: Multiple forwards in X-Forwarded-Proto
                    header not supported
           Product: Tomcat 9
           Version: unspecified
          Hardware: All
                OS: All
            Status: NEW
          Keywords: PatchAvailable
          Severity: normal
          Priority: P2
         Component: Catalina
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: -----

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.

Background:
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: 111.111.111.111,222.222.222.222' -H 'X-Forwarded-Proto:
https'
HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Set-Cookie: JSESSIONID=A1B6F171F11844A0FE8E857DBB8A0AA1;
Path=/some-application/; Secure; HttpOnly

vs

curl -kI https://localhost:8443/some-application/setting-secure-cookies -H
'X-Forwarded-For: 111.111.111.111,222.222.222.222' -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.
References:
Zuul's source code:
https://github.com/spring-cloud/spring-cloud-netflix/blob/master/spring-cloud-netflix-zuul/src/main/java/org/springframework/cloud/netflix/zuul/filters/pre/PreDecorationFilter.java#L221
Evidence/indication that this behaviour is, or has become recommended:
https://github.com/aspnet/BasicMiddleware/issues/18

Thanks for considering and kind regards,
tom

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 62978] RemoteIpValve: Multiple forwards in X-Forwarded-Proto header not supported

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978

Tom Groot <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #1 from Tom Groot <[hidden email]> ---
Created attachment 36291
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36291&action=edit
Patch incl. test cases to support multiple forwarded protocols in
X-Forwarded-Proto header value.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 62978] RemoteIpValve: Multiple forwards in X-Forwarded-Proto header not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978

Mark Thomas <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #2 from Mark Thomas <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 62978] RemoteIpValve: Multiple forwards in X-Forwarded-Proto header not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978

--- Comment #3 from Tom Groot <[hidden email]> ---
Created attachment 36314
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=36314&action=edit
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.
Sorry,
tom

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 62978] RemoteIpValve: Multiple forwards in X-Forwarded-Proto header not supported

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=62978

--- Comment #4 from Mark Thomas <[hidden email]> ---
Patch applied. Thanks for the attention to detail. It is much appreciated.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]