1283368 - Implement cookie prefixes spec

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Daniel Veditz [:dveditz]]

Implement restrictive behavior/requirements for cookies that start with the prefixes __Secure- and __Host- as specified in 
https://tools.ietf.org/html/draft-ietf-httpbis-cookie-prefixes-00

Cookies with a name starting with __Secure- must be set with the secure flag and from a secure page. Cookies with a name starting with __Host- must meet the above requirements and in addition must NOT have a domain specified and the path must be "/".

This is already implemented in Chrome 49 and Opera 36
https://www.chromestatus.com/feature/4952188392570880

Comment 1

[Daniel Veditz [:dveditz]]

Attachment: Implement cookie prefix spec

Comment 2

[Daniel Veditz [:dveditz]]

The __Secure- version is less interesting than the more general proposal to extend those protections to all secure cookies in https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone. A site that adopts HSTS with includesubdomains from its base domain also effectively gets this protection. But that spec has a long way to go before adoption (web compat worries) and switching an entire domain to HSTS can be non-trivial so this does have a use.

The more interesting __Host- version protects against cookie fixation attacks (should a sibling domain be compromised) that sites don't really have an alternative for at the moment.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8766669 [details] [diff] [review]
Implement cookie prefix spec

Review of attachment 8766669 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks good, and the tests seem to cover all the cases in the draft. Thanks!

::: netwerk/test/TestCookie.cpp
@@ +605,5 @@
> +      SetACookie(cookieService, "https://prefixed.test/", nullptr, "__Secure-test=test", nullptr);
> +      SetACookie(cookieService, "https://prefixed.test/", nullptr, "__Host-test=test", nullptr);
> +      GetACookie(cookieService, "https://prefixed.test/", nullptr, getter_Copies(cookie));
> +      rv[1] = CheckResult(cookie.get(), MUST_BE_NULL);
> +      

Trailing whitespace.

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8766669 [details] [diff] [review]
Implement cookie prefix spec

thanks valentin - I'd also like amy to review as she's been looking at cookie implementation issues.

and thanks dan!

Comment 5

[Amy Chung [:Amy]]

Hi Patrick,
I have traced code from Daniel, and my comment is same as Valentin.
But I need more time to study spec and test, would I review the path first?

Comment 6

[Amy Chung [:Amy]]

Comment on attachment 8766669 [details] [diff] [review]
Implement cookie prefix spec

Review of attachment 8766669 [details] [diff] [review]:
-----------------------------------------------------------------

I tried some cookies as below:
1. document.cookie = '__Secure-SID=12345; Secure; Domain=example.com'
2. document.cookie = '__Secure-SID2=12345; Domain=example.com'
3. document.cookie = '__Host-SID3=12345; Secure; Path=/'
4. document.cookie = '__Host-SID4=12345; Secure; Domain=example.com; Path=/' 
And I got the right results that following spec on Nightly after applied path.

Thanks!

Comment 7

[Amy Chung [:Amy]]

Hi Patrick,
I have finished to review the patch after studied spec and tested code.
Thanks!

Comment 8

[Daniel Veditz [:dveditz]]

Attachment: Implement cookie prefix spec [+whitespace fix]

Fixed whitespace issue, carrying over r+

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c60e672328ac
Implement cookie prefixes spec, r=valentin r=amchung

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c60e672328ac

Comment 11

[Florian Scholz (Open Web Docs)]

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