Implement cookie prefixes spec

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dveditz, Assigned: dveditz)

Tracking

({dev-doc-complete})

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 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
(Assignee)

Comment 1

3 years ago
Posted patch Implement cookie prefix spec (obsolete) — Splinter Review
Attachment #8766669 - Flags: review?(mcmanus)
(Assignee)

Comment 2

3 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.
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.
Attachment #8766669 - Flags: review+
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!
Attachment #8766669 - Flags: review?(mcmanus) → review?(amchung)
Whiteboard: [necko-active]

Comment 5

3 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?
Flags: needinfo?(mcmanus)

Comment 6

3 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!
Attachment #8766669 - Flags: review?(amchung) → review+

Comment 7

3 years ago
Hi Patrick,
I have finished to review the patch after studied spec and tested code.
Thanks!
Flags: needinfo?(mcmanus)
(Assignee)

Comment 8

3 years ago
Fixed whitespace issue, carrying over r+
Attachment #8766669 - Attachment is obsolete: true
Attachment #8768184 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8768184 - Flags: checkin?

Comment 9

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c60e672328ac
Implement cookie prefixes spec, r=valentin r=amchung
Keywords: checkin-needed
Attachment #8768184 - Flags: checkin? → checkin+

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c60e672328ac
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Updated

3 years ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.