Remove Cookie Name Decoding

[security-curious]

Per response from [email protected] posting as public PR. Proof of concept to demonstrate the vulnerability can be seen at https://github.com/security-curious/cookie-prefix-crystal-poc

---

Crystal automatically encodes and decodes cookie names. The cookie spec encourages the value to be encoded but says nothing about the cookie name (it could be encoded or not).

While encoding is allowed, a vulnerability can occur if the cookie name is decoded in such a way that a cookie not using a correctly formed cookie prefix will overwrite a cookie that is using a proper cookie prefix rendering the protections provided cookie prefixes void.

Since Crystal is encoding the entire cookie name using URL encoding two cookies can be sent by the browser and if in the correct order the server will read the cookie without the proper cookie prefix. An example cookie header value is:

__Secure-Name=safe; __%53ecure-Name=evil

The exact same issue was recently corrected in the rack library for the Ruby language as well as the werkzeug library used by Python's Flask web framework. In fact those corrects are what prompted me to look to see if Crystal had the same issue.

Although NIST rated the Rack vulnerability "high" the Rack maintainers consider that a mistake and consider is a low priority vulnerability. I agree.

The solution taken by both rack and werkzeug were to simply stop encoding the cookie name. If an application needs it encoded because they have a funky name that might mess with header parsing that application can do the encoding itself. This is the solution implemented on this commit.

An alternative solution that is probably more compatible but also more complex is to encode the part of the cookie name after
the cookie prefix if the cookie has a prefix that matches one of the official cookie prefixes (__Secure- and __Host-).
While this would preserve the encoding for the most part I don't feel the complexity introduced is worth the benefit.

[bcardiff]

For existing projects the following monkey-patch can be applied meanwhile after the require "http".

module HTTP
  class Cookie
    def to_cookie_header(io)
      io << @name
      io << '='
      URI.encode_www_form(value, io)
    end

    def parse_cookies(header)
      header.scan(CookieString).each do |pair|
        yield Cookie.new(pair["name"], URI.decode_www_form(pair["value"]))
      end
    end
  end
end

Thanks @security-curious

[straight-shoota]

Related: #9306