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

Test control characters in Set-Cookie header values #28290

Open
wants to merge 2 commits into
base: annevk/cookie-non-utf-8
Choose a base branch
from

Conversation

englehardt
Copy link
Contributor

Adding to #26170. I didn't see any tests for control characters in set-cookie headers, but would like some to verify changes we're making in httpwg/http-extensions#1420. This can be merged into Anne's PR.

@annevk PTAL

@annevk
Copy link
Member

annevk commented Apr 12, 2021

I think there are some issues with httpwg/http-extensions#1420 and this PR:

  1. As per https://fetch.spec.whatwg.org/#concept-header-value a header value cannot contain 0x00, 0x0A, or 0x0D.
    1. Per Why does headers-normalize-response.htm expect null bytes to be allowed? whatwg/xhr#165 (comment) 0x00 in a response header block should result in a network error. So Chrome is correct there.
    2. 0x0A or 0x0D probably results in a new header, though not sure for 0x0D. Also not sure how the Python APIs behave or why they do not throw. I recommend using .asis files or writing out the header bytes manually (see https://github.com/web-platform-tests/wpt/blob/master/fetch/h1-parsing/lone-cr.window.js for instance).
  2. Truncating values is kinda bad for security as it allows attackers to mask injections and such. I don't think we want to standardize on that, but it might not matter depending on the outcome of 1.
  3. Bonus: it would be good to test document.cookie as well.

cc @chlily1

@chlily1
Copy link
Contributor

chlily1 commented Apr 13, 2021

I think there are some issues with httpwg/http-extensions#1420 and this PR:

  1. As per https://fetch.spec.whatwg.org/#concept-header-value a header value cannot contain 0x00, 0x0A, or 0x0D.

The PR in httpwg/http-extensions#1420 addresses this:

NOTE: As set-cookie-string may originate from a non-HTTP API, it is not
guaranteed to be free of CTL characters, so this algorithm handles them
explicitly.

I agree that a header value shouldn't contain any of these characters. The set-cookie-string is not always a header value, though. (i.e. when it's set by document.cookie).

  1. Truncating values is kinda bad for security as it allows attackers to mask injections and such. I don't think we want to standardize on that, but it might not matter depending on the outcome of 1.

Can you explain what you mean by "mask injections"? If the text after 0x00 or 0x0A or 0x0D is always ignored, I don't see how it gets injected anywhere.

  1. Bonus: it would be good to test document.cookie as well.

The original WPTs that I landed in #27886 did.

@annevk
Copy link
Member

annevk commented Apr 13, 2021

@chlily1 ah okay, I got the impression from this PR that it was mainly about the HTTP header.

I still think truncating is bad though. If a site naïvely does document.cookie = "before" + attackerControlled + "after" it's generally not great if an attacker can make it ignore "after" but honor "before".

(I do think we want to test 0x0D further as unlike 0x0A it alone might not be sufficient to terminate a header value.)

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

Successfully merging this pull request may close these issues.

6 participants