Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web cache poisoning -- X-Forwarded-Scheme vs X-Forwarded-Proto #1809

Closed
directionless opened this issue Feb 15, 2022 · 8 comments
Closed

Web cache poisoning -- X-Forwarded-Scheme vs X-Forwarded-Proto #1809

directionless opened this issue Feb 15, 2022 · 8 comments

Comments

@directionless
Copy link

Hi! I believe there default rack behavior of forwarded_scheme creates a cache poisoning vulnerability in common configurations. This was initially disclosed almost a year ago in the blog post Cache Poisoning At Scale, and I am starting to see it used by pentesters. As I don't an issue or fix, I wanted to document it here.

In short, many CDNs use X-Forwarded-Proto to convey the original request protocol. But, rack prefers X-Forwarded-Scheme, this creates a simple opportunity for a cache poisoning attack.

This is a bit hard to show, but if you assume the rack URL has a CDN in front of it:

URL=https://rack.example.com/assets/image.png?cachebust=${RANDOM}

curl -i  $URL
# Observe that the `x-cache` response is MISS. 

curl -i  $URL
# Observe that the `x-cache` response is now HIT

##
## Attack
## 
URL=https://rack.example.com/assets/image.png?cachebust=${RANDOM}

curl -H 'x-forwarded-scheme: http' -i $URL
# Observe the 301 response, and `x-cache: MISS`

curl  -i $URL
# observe a cached 301

This is something I had to patch on our CDN, and it sounds like something many sites have had to patch as well. (The previously linked blog post lists several notable sites. Here is the hackerone discussion)

@ioquatix
Copy link
Member

Do you have any suggestions about how we can fix it?

@jeremyevans
Copy link
Contributor

The obvious approach to "fixing it" would be respecting x-forwarded-proto in preference to x-forwarded-scheme if both are present. Of course, doing that will break existing safe usage where the CDN/proxy uses x-forwarded-scheme (since then an attacker could specify x-forwarded-proto). There is no completely safe default. We would need to make this configurable, and probably choose whichever is more popular as the default (and I don't know which is more popular).

@matthewd
Copy link
Contributor

I think the long term answer is #1423, but that only helps when relevant upstreams have also adopted it.

@directionless
Copy link
Author

I was thinking about what an ideal solution is, unfortunately I suspect that any time you check multiple headers, the site
must set them all or it creates this kind of attack vector. That feels inherent in any kind of A or B or C situation.

This leads me to think that the best route is to decide what's more common, and drop support for the other. Possibly with a documented config/monkeypatch/etc to change the behavior.

@ioquatix
Copy link
Member

I also want to mention that there might be some issues regarding how we handle - and _ in headers. It could be feasible attack to write X_Forwarded_Proto or something similar.

jeremyevans added a commit to jeremyevans/rack that referenced this issue Mar 17, 2022
The Request.forwarded_priority accessor sets the priority.  Default to
considering Forwarded first, since it is now the official standard.

Also allow configuring whether X-Forwarded-Proto or X-Forwarded-Scheme
has priority, using the Request.x_forwarded_proto_priority
accessor.

Allowing configurable priorities for these headers is necessary,
because which headers should be checked depends on the environment
the application runs in.

Make Request#forwarded_authority use the last forwarded authority
instead of the first forwarded authority, since earlier forwarded
authorities can be forged by the client.

Fixes rack#1809
Fixes rack#1829
Implements rack#1423
Implements rack#1832
@dcheckoway
Copy link

Hello! Any chance of a patch release containing this fix? I see it's planned for 3.0.0, but my team and I would really prefer not to wait for 3.0.0 to land and stabilize. Any chance we could have a 2.2.3.2 with this fix included? That would be much appreciated if so!!

@jeremyevans
Copy link
Contributor

I think there is very little chance of backporting this change, it's way too disruptive for a tiny version change. It's not a bug fix, it's a completely new feature that may require configuration.

@dcheckoway
Copy link

Gotcha, thanks for the quick reply anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants