Requests with multiple `X-Forwarded-For` headers are handled incorrectly

[Turbo87]

Required Terms

• [X] I agree to follow this project's Code of Conduct
• [X] I have read and accept the Salesforce Program Agreement

What service(s) is this request for?

Heroku router/proxy

Tell us about what you're trying to solve. What challenges are you facing?

When a request with multiple X-Forwarded-For headers is received by the Heroku router/proxy it should handle the values as one continuous list, as specified by the HTTP spec:

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one “field-name: field-value” pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

Additionally, when dealing the X-Forwarded-For headers, the connecting IP address must always be appended at the end.

Example

When the original request includes:

X-Forwarded-For: 1.1.1.1
X-Forwarded-For: 2.2.2.2

then the Heroku router/proxy has three valid options of replying:

X-Forwarded-For: 1.1.1.1, 2.2.2.2, <connecting-IP>

X-Forwarded-For: 1.1.1.1
X-Forwarded-For: 2.2.2.2, <connecting-IP>

X-Forwarded-For: 1.1.1.1
X-Forwarded-For: 2.2.2.2
X-Forwarded-For: <connecting-IP>

This is unfortunately not what the Heroku router/proxy currently does. It will instead reply with:

X-Forwarded-For: 1.1.1.1, <connecting-IP>
X-Forwarded-For: 2.2.2.2

Given the HTTP spec, this means the value of the header is now being interpreted as 1.1.1.1, <connecting-IP>, 2.2.2.2. This makes it impossible for spec-compliant tooling to rely on the right-most IP address being set by the Heroku router/proxy and makes usage of this header completely unsafe. It is however the only way of figuring out what IP address connected to Heroku for rate limiting or request blocking purposes.

It would be great if this security-relevant bug could be fixed in the near future.

Supporting documentation for this issue can also be found at https://adam-p.ca/blog/2022/03/x-forwarded-for/#multiple-headers.

[elimchaysengSF]

Thanks for raising this item - I'll be talking with the Heroku Networking team about how we might handle this. I'll update here with any specific steps forward!

[Turbo87]

One other bug I've noticed is that when multiple X-Forwarded-For headers are received by the Heroku router the fwd field in the log also only seems to show the value of the first header.

[Turbo87]

@elimchaysengSF any news on this? :)

[Turbo87]

@elimchaysengSF would be great to get an update eventually. is Heroku taking these security issues seriously?

[SHxKM]

Would be great to hear if there's any intention to tackle this

[elimchaysengSF]

Hey both - sorry for the delay. Wanted to update you with good news, we were able to prioritize this change and will be working it into the the public beta of our Router 2.0 to update the headers and formally add the Forwarded header and the expected IP ordering. I don't have an exact update date, but it's prioritized for this quarter and will likely be worked on within the next sprint or two. We'll likely post a changelog like we just did for the HTTP Headers update for Router 2.0 when its completed!

[elimchaysengSF]

Following up, after prioritizing the ticket and some discovery by our networking team, there is some complexity in formally adding the Forwarded header as I mentioned above. However, we did ensure that on our new Router, our X-Forwarded-For header is now behaving according to the HTTP Spec in the acceptable ways you mentioned in the original comment. We've done some work to validate this on our end, but if you feel comfortable opt-ing into our Router 2.0 public beta (if you haven't already) I'd like to know if this fully addresses the security bug from your side!

[Turbo87]

thanks for the update. we might try this out on the crates.io staging environment. depending on how things work there we might opt-in on the production service too then.

[elimchaysengSF]

Sweet - appreciate your help here raising this bug and helping us improve the new Router going forward. Looking forward to hearing more if you flip on your staging and/or prod env.

[Turbo87]

@elimchaysengSF we tested this out in our staging environment and the bug indeed appears to be fixed with the Router 2.0 version. is there a timeline for when this Router 2.0 can be considered stable enough to be used in production?

[elimchaysengSF]

Awesome, glad our fix addressed it, and thanks again for raising this - we're working through a few open investigations and our last feature (session affinity) for parity with our old router, and then we will be announcing GA status. So, ideally in the next month or so I would say, after we do some more detailed performance testing!