1702031 - Ensure cookies that contain control characters are handled according the the spec

Status: RESOLVED FIXED

(Core :: Networking: Cookies, enhancement, P2)

Description

[Steven Englehardt [:englehardt]]

The cookie spec has been updated to specify how cookies containing control characters should be handled: https://github.com/httpwg/http-extensions/pull/1420. Specifically we've added the following language regarding CTL characters:

1.  If the set-cookie-string contains a %x0D (CR), %x0A (LF), or %x00 (NUL)
    octet, then set the set-cookie-string equal to all the characters of
    set-cookie-string up to, but not including, the first such octet.

2.  If the set-cookie-string contains a %x00-1F / %x7F (CTL) character:
    Abort these steps and ignore the set-cookie-string entirely.

The behavior of Firefox at the time of filing is as follows:

For cookies set with document.cookie: Firefox truncates at the first CR/LF/NUL and rejects cookie names with and other CTL character but if there's a CTL anywhere else (e.g., in the cookie value) it gets stripped out.

For cookies set in a Set-Cookie header: Firefox truncates at the first CR/LF/NUL and rejects cookies with any other CTL character (including in the value). However, the remainder of the cookie-string after a CR is processed and set as a second cookie, which seems like a bug.

I dug into our cookie parsing code a bit, and I see that we reject when there are "invalid" characters in the cookie name all the time, but only check for invalid characters in the value when the cookie is coming from a Set-Cookie header.

The start of some wpts for this are available here: https://github.com/web-platform-tests/wpt/pull/28290

Comment 1

[Andrew Williams]

The cookie spec has changed recently such that tab characters are allowed but all other characters should cause the whole cookie line to be discarded:

1. If the set-cookie-string contains a %x00-08 / %x0A-1F / %x7F character (CTL  
   characters excluding HTAB): Abort these steps and ignore the                  
   set-cookie-string entirely.                                                  

Also, from some testing with document.cookie, it looks like:

• 0x00 in the cookie value causes the rest of the value to be truncated (but subsequent attributes are preserved)
• 0x00 in the cookie name causes the rest of the name and the value to be truncated (but subsequent attributes are preserved)
• 0x0d and 0x0a cause the entire cookie line to be truncated (attributes ignored)
• 0x01 through 0x09, 0x0b through 0x0c, and 0x0e through 0x1f cause the cookie to be rejected if they are present in the name, but are allowed in the cookie value
• 0x7f is allowed in the cookie name and cookie value

For reference, I used variations of the following for testing (which also demonstrates the motivation behind not having control characters truncate):

function getCtlCharacters() {
  const ctlCodes = [...Array(0x20).keys()]
                       .concat([0x7F]);
  return ctlCodes.map(i => ({ code: i, chr: String.fromCharCode(i) }))
}
const CTLS = getCtlCharacters();
for (const ctl of CTLS) {
    malicious_value = `haha${ctl.chr}haha`;
    // See whether HttpOnly gets ignored for easier testing, but in
    // an attack scenario it could be bad if Secure gets ignored
    document.cookie = `test${ctl.code}=` + malicious_value + "; HttpOnly";
}
document.cookie

Comment 2

[Andrea Marchesini [:baku]]

The current char validation is compatible with the other browser vendors and it's almost in sync with the spec.
Ed, to me we are ready to close this bug. Thoughts?

Comment 3

[edgul]

The spec has changed significantly since this bug was filed and since it is sufficiently vague more recently I have been effectively using it as a meta (though I did not label it as such).

I think going forward we can deal with CTLs on an individual basis and we can close this bug. FWIW for future readers, you can see related known (at the time of closing) CTL bugs in the see also field of this bug.

Thanks!