Daniel Veditz [:dveditz] Assignee
	Description • 8 years ago

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

	Daniel Veditz [:dveditz] Assignee
	Comment 2 • 8 years ago

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.

	Valentin Gosu [:valentin] (he/him)
	Comment 3 • 8 years ago

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.

	Patrick McManus [:mcmanus]
	Comment 4 • 8 years ago

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!

	Amy Chung [:Amy]
	Comment 5 • 8 years ago

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?

	Amy Chung [:Amy]
	Comment 6 • 8 years ago

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!

	Amy Chung [:Amy]
	Comment 7 • 8 years ago

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

	Daniel Veditz [:dveditz] Assignee
	Comment 8 • 8 years ago

Fixed whitespace issue, carrying over r+

	Pulsebot
	Comment 9 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c60e672328ac
Implement cookie prefixes spec, r=valentin r=amchung

	Carsten Book [:Tomcat]
	Comment 10 • 8 years ago
bugherder

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

	Florian Scholz (Open Web Docs)
	Comment 11 • 8 years ago

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