Skip to content

Correct minor security issue for cookie prefixes #1965

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

Merged
merged 1 commit into from
Jan 10, 2021

Conversation

eric1234
Copy link
Contributor

@eric1234 eric1234 commented Nov 7, 2020

NOTE: I did not see any special contact address for security issues. Seeing as this is a quite low severity issue I am just opening a public ticket. Feel free to delete if this is not the right thing to do.

Browsers assign special meaning to cookies prefixed with either __Secure- or __Host-. Due to the unquoting of cookie keys done by Werkzeurg this protection can be bypassed by creating a cookie with these prefixes inside quotes. The browses will not enforce the requirements needed by those prefixes while the server will not be able to differentiate between the unsecure quoted cookie and the more secure unquoted cookie.

While the cookie spec encourages encoding the cookie value it does not specify anything with regard to cookie name and the character set for the cookie name is more restricted.

This issue was brought up for the Rails platform recently which is what prompted me to see what other platforms have the same or similar issues. See that investigation for more information:

https://hackerone.com/reports/895727

I have created what I hope to be a POC of this issue at:

https://github.com/eric1234/werkzeug-cookie-prefix-poc

This commit changes the code so that only the value is unquoted and the key is not. If someone provides a quoted key then the
quote will become part of the cookie name.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@davidism
Copy link
Member

davidism commented Nov 7, 2020

Please use security@palletsprojects.com to responsibly report security issues. Your assessment of the severity shouldn't factor into that. For any project, if you're not sure, use another means of contact to determine the correct one. Additionally, definitely don't open a PR over an issue, as we can't delete PRs like we can issues.

@davidism
Copy link
Member

davidism commented Nov 7, 2020

If someone provides a quoted key then the quote will become part of the cookie name.

That doesn't seem correct.

@davidism
Copy link
Member

davidism commented Nov 7, 2020

I'm not clear how this is an issue that should be addressed by Werkzeug. If browsers are using the encoded/quoted name rather than the real name for this check then the issue should be reported to and fixed by browsers.

@eric1234
Copy link
Contributor Author

eric1234 commented Nov 8, 2020

Please use security@palletsprojects.com to responsibly report security issues. Your assessment of the severity shouldn't factor into that. For any project, if you're not sure, use another means of contact to determine the correct one. Additionally, definitely don't open a PR over an issue, as we can't delete PRs like we can issues.

Thank you for that feedback and I apologize for not doing things right. I wasn't aware PRs couldn't be deleted and couldn't find any published address but will try better next time.

If someone provides a quoted key then the quote will become part of the cookie name.

That doesn't seem correct.

Per the spec linked above:

cookie-name       = token
...
token             = <token, defined in [RFC2616], Section 2.2>
...[in RFC2616]...
        token          = 1*<any CHAR except CTLs or separators>
       separators     = "(" | ")" | "<" | ">" | "@"
                      | "," | ";" | ":" | "\" | <">
                      | "/" | "[" | "]" | "?" | "="
                      | "{" | "}" | SP | HT

So a cookie name cannot contain "separators" which would include double quote characters. You can also see this in the MDN documentation at:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#Attributes

I'm not clear how this is an issue that should be addressed by Werkzeug. If browsers are using the encoded/quoted name rather than the real name for this check then the issue should be reported to and fixed by browsers.

I may not have explained it well so I will try again. This is my best understanding of the issue. Feel free to correct any misunderstandings I have:

  1. Cookies are allowed to have a looser security model than the typical single-origin policy. Specifically they can be shared across subdomains and between http and https.
  2. This looser security model can be tightened via cookie attributes such as Domain, Secure and SameSite.
  3. While these cookie attributes do protect the confidentiality of the cookies (i.e. a http or subdomain site cannot read them) they do not protect the integrity of them (an attacker can still overwrite them with a desired value). This is achieved by the fact that when cookies are transferred between client and server there is a lack of symmetry. Set-Cookie includes the attributes but when the browser sends the cookie value back it does not include the attributes. Therefore the cookie the server receives may not be from a cookie in the browser that has the desired attributes.
  4. Cookie prefixes is a backwards compatible way of fixing this by embedding the attributes in cookie name. The browser will not set the cookie unless the attributes of the cookie match the embedded attributes in the name.
  5. werkzeug is allowing this protection to be bypassed by the fact that it is renaming the cookie when it is unquoted. An attacker could create a quoted version of the prefixed cookie and the browser would not enforce the restrictions as it is not properly prefixed. Due to the unquoting the cookie is renamed to the same as the more secure cookie overwriting the value therefore removing the protection that cookie prefixes provide.

Hopefully my understanding of the issue is correct enough that it helps you see the issue. Perhaps also an example attack might help.

  1. mybank.com wants its cookies as secure as possible so it is using all the necessary attributes as well as cookie prefixes.
  2. fundraiser.mybank.com is setup as a separate site for a fundraiser the bank is running. Since it's just a brochure site it is just Wordpress site with minimal security.
  3. An attacker compromises fundraiser.mybank.com so they have control over a subdomain.
  4. The attacker figured out a cookie on the main site where if they can change to a desired value it gives them some sort of foothold on the main site.
  5. The attacker uses the compromised subdomain to create a cookie by the same name as the desired cookie but encloses it with quotes (so the browser does not enforce the cookie prefix) and sets the cookie attributes so it is shared between the main site and the subdomain.
  6. A user visiting both sites would have both cookies (the unquoted real cookie set by the main site and the quoted attack cookie set by the compromised subdomain) set in their browser. Both cookies would be sent to the server but the server will unquote the attack cookie renaming it to be the same as the real cookie. This can cause the attack cookie value to overwrite the real cookie value causing the server to operate on the attackers value.

There are other attack scenarios (such as MITM for http vs https) but the end result is werkzeug's modification of the cookie name allows an attacker to bypass the integrity protections offered by cookie prefixes

This was against RFC 6265 and potentially allowed setting __Secure
prefixed cookies.
@davidism davidism merged commit 491fc6a into pallets:master Jan 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants