Web cache poisoning - semicolon as a separator

[AdamGold]

The request.rb file treats semicolon as a separator (6af5f92) - whereas most proxies today only take ampersands as separators. Link to a blog post explaining this vulnerability: https://snyk.io/blog/cache-poisoning-in-popular-open-source-packages/

When the attacker can separate query parameters using a semicolon (;), they can cause a difference in the interpretation of the request between the proxy (running with default configuration) and the server. This can result in malicious requests being cached as completely safe ones, as the proxy would usually not see the semicolon as a separator, and therefore would not include it in a cache key of an unkeyed parameter - such as utm_* parameters, which are usually unkeyed. Let’s take the following example of a malicious request:

GET /?link=http://google.com&utm_content=1;link='><t>alert(1)</script> HTTP/1.1

Host: somesite.com

Upgrade-Insecure-Requests: 1		

User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,imag e/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9 Accept-Encoding: gzip, deflate			

Accept-Language: en-US,en;q=0.9 Connection: close			

The backend sees 3 parameters here: link, utm_content and then link again. On the other hand, the proxy considers this full string: 1;link='><t>alert(1)</script> as the value of utm_content, which is why the cache key would only contain somesite.com/?link=http://google.com.

A possible solution could be to allow developers to specify a separator, defaulting to an ampersand.

[jeremyevans]

Developers can already specify a separator when parsing queries directly (second argument to Rack::QueryParser#parse_query), but it looks like Request#GET always supports either ampersand or semicolon. I agree about switching the default to just ampersand, but I'm guessing that has backwards compatibility issues for people currently relying on splitting on semicolon, so this may have to be done in Rack 3. I added #1733 for the change.

@tenderlove is this change likely to affect Rails?

[tenderlove]

Ya, I totally agree we should switch the default. I don't think it will impact Rails, but I'm sure this will impact someone. Should we consider making this change in a major release?

[jamgregory]

Snyk keeps regularly reminding me that rack is currently vulnerable to this issue. I was wondering if an update was in that works that would include this change?

[causztic]

Snyk keeps regularly reminding me that rack is currently vulnerable to this issue. I was wondering if an update was in that works that would include this change?

hi @jamgregory

I patched this in my own Rails application by adding this into config/intiializers/patches/rack.rb, while waiting for rack 3.0

module Rack
  class Request
    Helpers.module_eval do
      def GET
        if get_header(RACK_REQUEST_QUERY_STRING) == query_string
          get_header(RACK_REQUEST_QUERY_HASH)
        else
          query_hash = parse_query(query_string, '&')
          set_header(RACK_REQUEST_QUERY_STRING, query_string)
          set_header(RACK_REQUEST_QUERY_HASH, query_hash)
        end
      end
    end
  end
end

[jamgregory]

Thanks for that @causztic, we might look at using it. Obviously it'd be ideal to get a rack release with this change included, but that might work in the meantime.

[SpyMachine]

Seems like the answer is no, but just to confirm, should we ever expect a fix for this on Rack 2? My project uses Sinatra which is pegged to Rack 2 so I'm getting dinged on this by Snyk. I assume we'll just need to wait for Sinatra to support Rack 3 👀

[jeremyevans]

Correct, this is considered too breaking to backport to Rack 2.

[ioquatix]

Have we ever actually seen an attack in the wild? i.e. is this a little bit of a false flag?

[mame]

Is this incompatibility really necessary?

I understand the problem as follows:

1. Reverse proxy generally exclude utm_content=xxx from the query_string as a cache key.
2. If an attacker passes a query_string like a=foo&utm_content=xxx;a=BOO, Rack 2 may return a result as a=BOO (because it handles the semicolon as a separater), and the reverse proxy wrongly caches the result as a=foo (because it ignores utm_content=xxx;a=BOO).

My question here is: is 1 true? I looked for information on NGINX and Varnish and couldn't find any mention of ignoring utm_content etc. by default. (Let me know if I am wrong.)

For NGINX, I found the following example configuration.

https://gist.github.com/a-vasyliev/de8ffc6c6aa74cdeadfe

This manually removes utm_content=[^&]* from the URL. But I don't think it is vulnerable because it actually sends the erased query_string to the backend as well.
Also, the fact that such a configuration example is found, means that NGINX provides no easy configuration to exclude utm_content, etc., I think.

For Varnish, I found the following discussion.

https://serverfault.com/questions/591860/ignore-utm-values-with-varnish

This response says:

Yes, but to do this, you must override the default vcl_hash. this is a dangerous thing to do only because people forget how Varnish works.

This indicates that it is (possible but) never easy to configure Varnish to ignore utm_content key.
Also, note that the answer here is regsuball(req.url,"([? &])utm_[^&? ;]*","\1"), which takes into account that the semicolon is treated as a separator. In other words, the old behavior of Rack 2 is rather preferable in this example.

So, I think this is an unnecessary incompatibility introduced against a problem that in reality rarely exists or is the configuration bug of a reverse proxy.

What do you think? @AdamGold @jeremyevans @tenderlove

[jeremyevans]

@mame Treating semicolon as separator is certainly incorrect behavior in terms of parameter handling (see decoding part of https://www.w3.org/TR/2014/REC-html5-20141028/forms.html#url-encoded-form-data). Because fixing Rack to only split on ampersand is incompatible, we did not backport the fix to Rack 2. However, we should certainly keep the fix in Rack 3.

[mame]

@jeremyevans Thanks for your quick reply.

Interestingly, ; was actually preferred over & in the old HTML spec.

https://www.w3.org/TR/1999/REC-html401-19991224/appendix/notes.html#h-B.2.2

We recommend that HTTP server implementors, and in particular, CGI implementors support the use of ";" in place of "&" to save authors the trouble of escaping "&" characters in this manner.

If the change is not for security reasons, but to comply with the new specification, I understand your decision.

IHMO, the incompatibility is too big just for that purpose, though. I hope that the W3C be cautious about unnecessarily changing what it has recommended in the past...