Bug 229088

Summary: Ensure cookies that contain control characters are handled according the the spec
Product: WebKit Reporter: Andrew Williams <awillia>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED MOVED    
Severity: Normal CC: achristensen, beidson, bfulgham, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: Safari 14   
Hardware: Unspecified   
OS: Unspecified   

Description Andrew Williams 2021-08-13 11:12:09 PDT
The latest draft of RFC6265bis has changed recently such that tab characters are allowed in cookies but all other characters should cause the whole cookie line to be discarded.  From 'The Set-Cookie Header Field' section:

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.

and from the 'Storage Model' section:

3. If the cookie-name or the cookie-value contains a %x00-08 / %x0A-1F / %x7F   
   character (CTL characters excluding HTAB), abort these steps and ignore the  
   cookie entirely.                                                             

From some testing using document.cookie in Safari, it looks like all control characters except the tab character cause the cookie to be rejected if present in the name and cause the rest of the cookie line to be truncated if present in the value.  To conform to the spec, it'd be ideal if all control characters except tabs anywhere in a cookie line (for cookie lines from document.cookie or in Set-Cookie headers) caused the cookie to be rejected.

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 = `te${ctl.chr}st${ctl.code}=asdf`;
}
document.cookie
Comment 1 John Wilander 2021-08-13 17:08:43 PDT
Thanks for filing! Cookies are for the most part not implemented or handled by WebKit but by the HTTP framework used underneath, such as CFNetwork, libcurl, and libsoup. I will open an issue with CFNetwork for Apple's platforms.
Comment 2 John Wilander 2021-08-13 17:10:27 PDT
<rdar://81922594>
Comment 3 John Wilander 2021-08-13 17:59:21 PDT
Andrew, can you please link to the change you are referring to? Thanks!
Comment 4 Andrew Williams 2021-08-13 18:39:08 PDT
Ah, my apologies.  Thank you so much for opening the CFNetwork issue for this, though!

The PRs that implement this specific RFC change are linked to at the bottom of the RFC document: https://github.com/httpwg/http-extensions/blob/main/draft-ietf-httpbis-rfc6265bis.md#draft-ietf-httpbis-rfc6265bis-09

One thing you'll notice at the link above is that size limits on the cookie name + value and also on the attribute value were added recently... I'm not sure whether an issue was opened with Apple for that change - do you think it'd be worth opening one for that as well?

Thanks again!
Comment 5 Brent Fulgham 2022-06-30 11:01:00 PDT
The fix for this bug needed to be made outside of WebKit software. Consequently, marking this as "RESOLVED | MOVED".

We believe this is fixed in:
iOS 16.0 Beta 1 (and newer)
macOS Ventura Beta 1 (and newer).