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

Limit the length of the Referer header #903

Closed
annevk opened this issue May 8, 2019 · 10 comments · Fixed by w3c/webappsec-referrer-policy#122

Comments

@annevk
Copy link
Member

commented May 8, 2019

As it's attacker-controlled it can be used for cache-eviction or determining cookie size.

To quote @mikewest in https://bugs.chromium.org/p/chromium/issues/detail?id=959757 (it's worth reading this for some more relevant discussion):

As noted in https://github.com/xsleaks/xsleaks/wiki/Browser-Side-Channels#cache-and-error-events, servers will often behave in unexpected ways when presented with an overly-long Referer header. This is unfortunate, as Referer is one header whose length attackers generally retain control over when generating no-cors requests.

Perhaps we can cap it to something reasonable? This would be conceptually similar to the cap in https://fetch.spec.whatwg.org/#cors-safelisted-request-header for CORS requests...

cc @whatwg/security

@mikewest

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Based on the limited amount of data gathered in https://bugs.chromium.org/p/chromium/issues/detail?id=959757#c8, I plan to run a short experiment in Chrome Canary/Dev which would strip the referer down to an origin if it exceeded 2k (0.1% of requests) or 4k (0.01% of requests).

Two things to note:

  1. 4k feels like a reasonable limit to aim for. I'm curious about the effect of a tighter limit, but I doubt we'll see statistically relevant changes in any metric between the two. I'm told that some large services cap the referer header to 4k server-side, so I'm a bit biased towards that limit given an existence proof of it being acceptable.

  2. The patch I landed against Chromium in https://chromium-review.googlesource.com/c/chromium/src/+/1595872 implements the cap by stripping the referer header down to an origin (plus a trailing /) if it exceeds the limit, as @MattMenke2 raised reasonable concerns about just cutting the string off in the middle. This is fairly certain to be safe (especially given that some vendors strip headers to origins already) but may result in subtle breakage in path-based ACLs that would be hard for us to detect at scale. It might be worth coming back to this implementation question.

@annevk

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Thank you (and thank you @MattMenke2 for arguing for the model I also wanted to go with)! Per the "We Still Don’t Have Secure Cross-Domain Requests:an Empirical Study of CORS" paper a 4KiB limit would be quite good, but might still expose the length of large cookies on Tomcat servers given the 8KiB overall limit there.

Another thing we might consider is if it should influence https://fetch.spec.whatwg.org/#cors-unsafe-request-header-names somehow. That is, if your referrer is very large, we cap your budget for CORS-safelisted request-header values. This gets quite complex though so probably best avoided.

If we don't want to consider the interaction with CORS, I suspect we want to put this login in https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer.

@mikewest

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

There doesn't appear to be any statistically significant difference in error rates in Chrome's Canary/Dev population, and the overall header length percentiles are pretty stable. I'd suggest we run with a 4k cap, which affects ~1 out of 10,000 requests according to Chrome's data.

If y'all are on board, Anne, I'll send a PR against Referrer Policy and an Intent to Ship to blink-dev@.

@ericlaw1979: How does Edge feel about this?
@johnwilander: How about the WebKittens?

@ericlaw1979

This comment has been minimized.

Copy link

commented Jun 4, 2019

Sounds reasonable to me, although I'd also be fine with a 2k cap, given the scenario and the fact that for 15 years, public websites couldn't reliably use URLs over 2083 characters.

mikewest added a commit to w3c/webappsec-referrer-policy that referenced this issue Jun 5, 2019
If the `referer` header's value is a URL with a length of more than 4k, strip it down to an origin.

Fixes whatwg/fetch#903.
@mikewest

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I put a PR up at w3c/webappsec-referrer-policy#122 and poked public-webappsec@. Will send an intent to ship out later in the week when I'm not buried in meetings.

@dveditz

This comment has been minimized.

Copy link

commented Jun 6, 2019

Looks good to me. 4k seems overly generous, but if we know it's safe and everyone can agree on it then it works.

@mikewest Do you know anything else about the referrers other than length? I wonder if a lot of the long ones are data: URLs.

@mikewest

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Looks good to me. 4k seems overly generous, but if we know it's safe and everyone can agree on it then it works.

Well, we don't know that it's safe (1 in 10,000 requests isn't nothing!), but it seems like it ought to be safe. And affecting 1 out of 10,000 requests is certainly saf_er_ than affecting 1 out of 1,000. :) We can always ratchet things down further in the future if this isn't tight enough. But as a sanity check, this seems like a reasonable place to start.

Do you know anything else about the referrers other than length?

Nope. We're only collecting a simple histogram.

I wonder if a lot of the long ones are data: URLs.

We shouldn't be sending data: in a Referer header. See step 2 of https://w3c.github.io/webappsec-referrer-policy/#strip-url.

@mikewest

This comment has been minimized.

@annevk

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

It seems good to have a test for the actual limit (-1, at limit, +1), which is a little tricky given that the origin of WPT is variable, but should be doable. Note that you can also set referrer from fetch(), which might make it a little easier to test.

@mikewest

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

It seems good to have a test for the actual limit (-1, at limit, +1), which is a little tricky given that the origin of WPT is variable, but should be doable.

Fair. Added more tests that extend the URL based on the size of the origin. I should have done that to begin with, thanks for the poke. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.