Open Bug 795346 (samesite-cookies) Opened 7 years ago Updated 7 months ago

Add SameSite support for cookies

Categories

(Core :: Networking: Cookies, defect, P5)

defect

Tracking

()

Tracking Status
relnote-firefox --- -

People

(Reporter: mgoodwin, Unassigned, NeedInfo)

References

(Depends on 9 open bugs, )

Details

(Keywords: dev-doc-needed, parity-chrome, sec-want, Whiteboard: [necko-would-take])

Attachments

(10 obsolete files)

I've been working on an anti CSRF mechanism with Joe Walker. Following some discussions with some other people dealing with Security on Mozilla products, it seems there's good support for this idea.

You can read more about this idea here: https://people.mozilla.com/~mgoodwin/SameDomain/
Add SameDomain attribute to nsCookie and nsCookieAttributes.

This does not yet make use of tagging; I'm learning my way around cookie code and would like early feedback on the things I'm doing.
Attachment #665932 - Flags: feedback?(mmc)
Add storage and schema migration for SameDomain cookie attribute
Attachment #665934 - Flags: feedback?(mmc)
Comment on attachment 665932 [details] [diff] [review]
Add SameDomain attribute to nsCookie and nsCookieAttributes

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

::: netwerk/cookie/nsCookie.h
@@ +53,5 @@
>       , mCreationTime(aCreationTime)
>       , mIsSession(aIsSession != false)
>       , mIsSecure(aIsSecure != false)
>       , mIsHttpOnly(aIsHttpOnly != false)
> +     , mIsSameDomain(aIsSameDomain != false)

There's no need to check bool != false when initializing another bool.

::: netwerk/cookie/nsICookie2.idl
@@ +47,5 @@
>  
>      /**
> +     * true if the cookie is a Same Domain cookie
> +     */
> +    readonly attribute boolean isSameDomain;

Likewise, I'd be happier seeing a structure that supported multiple tag=values here.

::: netwerk/cookie/nsICookieManager2.idl
@@ +57,5 @@
>             in int64_t     aExpiry);
>  
>    /**
> +   * Add a cookie with sameDomain attribute specified. nsICookieService is the
> +   * normal way to do this. This method is something of a backdoor.

Why is this method needed?
Comment on attachment 665934 [details] [diff] [review]
Add storage and schema migration for SameDomain cookie attribute

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

::: netwerk/cookie/nsCookieService.cpp
@@ +475,5 @@
>        if (!row)
>          break;
>  
>        CookieDomainTuple *tuple = mDBState->hostArray.AppendElement();
> +      row->GetUTF8String(10, tuple->key.mBaseDomain);

All these constants are brittle. What does the constant 10 mean? (and 11, and 12 below)

@@ +994,5 @@
> +        // We need to add isSameDomain for the SameDomain cookie flag
> +        // Add the isSameDomain column to the table.
> +        rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +          "ALTER TABLE moz_cookies ADD isSameDomain INTEGER"));
> +        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);

I've already brought this up on dev-privacy, but I think adding a column to hold serialized tag=value pairs would be much more useful.
Comment on attachment 665937 [details] [diff] [review]
Check for SameDomain attr, compare with internal referer on cookie send to determine violations

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

This set of changes requires better documentation for adding the internal referrer checks. At the minimum, it should link to your doc or a wiki.

::: netwerk/cookie/PCookieService.ipdl
@@ +61,5 @@
>     *
>     * @return the resulting cookie string.
>     */
>    sync GetCookieString(URIParams host,
> +                       URIParams referrer,

How do you know you found all the call sites for this method?

::: netwerk/cookie/nsCookieService.cpp
@@ +2533,5 @@
>  
> +    // if the cookie is SameDomain and we've got an internal referrer, check
> +    // the referrer matches the cookie host
> +    if (cookie->IsSameDomain() && hostFromReferrer.Length() > 0) {
> +      if (cookie->RawHost() != hostFromReferrer && 

trailing whitespace. Do the same considerations as above with the subdomain match apply to SameDomain?
Hi Mark,

Sorry for the delay, I thought I had already hit publish on those comments. I think figuring out the right new fields for nsCookie will guide the storage question. Is there any other way to get the referrer?

Monica
Attachment #665932 - Attachment is obsolete: true
Attachment #665932 - Flags: feedback?(mmc)
Attachment #665934 - Attachment is obsolete: true
Attachment #665934 - Flags: feedback?(mmc)
Attachment #665937 - Attachment is obsolete: true
Attachment #665937 - Flags: feedback?(mmc)
Attached patch 1 - parse attributes, etc. (obsolete) — Splinter Review
Add a hashtable to nsCookie and nsCookieAttributes to allow arbitrary attributes to be stored. Alter nsCookieService::ParseAttributes to parse any new attributes into a hashtable.

Add hashtable to nsCookie ctor and nsCookie::Create.

Create method to get the attributes as string, to check for the presence of Attributes and get and set specific attributes. Include extended attributes in cookie debug messages. Expose AddWithAttributes to nsICookieManager2 (more to be done here - also would to be able to get an nsIArray of attr names and a value for a given name).
Attachment #684738 - Flags: feedback?(mmc)
Make the code (a little) less opaque by setting constants for the cardcoded column indexes.
Attachment #684740 - Flags: feedback?(mmc)
Attached patch 3 - storage (and migration) (obsolete) — Splinter Review
Sort out persistence for cookie attributes. Add migration script from schema 5 to 6.
Attached patch 4 - the actual samedomain patch (obsolete) — Splinter Review
Check to see if a cookie has samedomain attribute set before off-origin send.
Attachment #684742 - Flags: feedback?(mmc)
Attachment #684741 - Flags: feedback?(mmc)
Attachment #684740 - Attachment description: Clean up hardcoded column indexes → 2 - Clean up hardcoded column indexes
Attachment #684742 - Attachment description: the actual samedomain patch → 4 - the actual samedomain patch
Comment on attachment 684740 [details] [diff] [review]
2 - Clean up hardcoded column indexes

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

::: netwerk/cookie/nsCookieService.cpp
@@ +87,5 @@
> +#define COL_SECURE 7
> +#define COL_HTTPONLY 8
> +#define PARAM_BASE_DOMAIN 9
> +#define PARAM_APP_ID 10
> +#define PARAM_IBE 11

Thanks a lot for doing this. Can you add a comment that the first ones are columns, and the last ones are row keys (if I'm reading the code right)?
Attachment #684740 - Flags: feedback?(mmc) → feedback+
(In reply to Monica Chew from comment #12)
> Thanks a lot for doing this. Can you add a comment that the first ones are
> columns, and the last ones are row keys (if I'm reading the code right)?

Sure.

I'm actually about to upload new versions of these patches so you may want to hold of going through these a minute.
Comment on attachment 684738 [details] [diff] [review]
1 - parse attributes, etc.

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

::: netwerk/cookie/nsCookie.h
@@ +79,5 @@
>      inline bool IsDomain()                const { return *mHost == '.'; }
>      inline bool IsSecure()                const { return mIsSecure; }
>      inline bool IsHttpOnly()              const { return mIsHttpOnly; }
> +    inline bool HasAttributes() const { return mAttributes.Count() > 0; }
> +    inline PRBool GetAttribute(nsCString key, nsCString **value) const { return mAttributes.Get(key, value); }

For consistency and avoiding annoyance (and following https://developer.mozilla.org/en-US/docs/PRBool), please change this to bool.

::: netwerk/cookie/nsCookieService.cpp
@@ +1807,5 @@
>                       bool              aIsHttpOnly,
>                       bool              aIsSession,
>                       int64_t           aExpiry)
>  {
> +  return AddWithAttributes(aHost,aPath,aName,aValue,aIsSecure,aIsHttpOnly,aIsSession,aExpiry,nsCString(""));

please put spaces between params

@@ +1861,5 @@
> +    nsCString* key = new nsCString(tokenString);
> +    nsCString* value = new nsCString(tokenValue);
> +    attrs.Put(*key, value);
> +  }
> +

This is overlapping with your other storage patch, right?

@@ +3134,5 @@
>      // ignore any tokenValue for isHttpOnly (see bug 178993);
>      // just set the boolean
>      else if (tokenString.LowerCaseEqualsLiteral(kHttpOnly))
>        aCookieAttributes.isHttpOnly = true;
> +    

trailing whitespace

::: netwerk/cookie/nsICookieManager2.idl
@@ +57,5 @@
>             in int64_t     aExpiry);
>  
>    /**
> +   * Add a cookie. nsICookieService is the normal way to do this. This
> +   * method is something of a backdoor.

I don't understand this comment. Why is it a backdoor? Who's supposed to use this?
Comment on attachment 684741 [details] [diff] [review]
3 - storage (and migration)

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

::: netwerk/cookie/nsCookie.cpp
@@ +179,3 @@
>    mAttributes.EnumerateRead(AppendAttribute, &aAttrString); 
>    if (aAttrString.Length() > 0) {
> +    aAttrString.SetCharAt((char)0, aAttrString.Length() - 1);

NULL is a more conventional string termination than (char)0, I would revert this change.

::: netwerk/cookie/nsCookieService.cpp
@@ +85,5 @@
>  #define COL_LAST_ACCESSED 5
>  #define COL_CREATION_TIME 6
>  #define COL_SECURE 7
>  #define COL_HTTPONLY 8
> +#define COL_ATTRIBUTES 9

This doesn't look backwards compatible to me. What happens when DBs created under the old schema suddenly interpret 9 to be COL_ATTRIBUTES instead of PARAM_BASE_DOMAIN?

@@ +88,5 @@
>  #define COL_HTTPONLY 8
> +#define COL_ATTRIBUTES 9
> +#define PARAM_BASE_DOMAIN 10
> +#define PARAM_APP_ID 11
> +#define PARAM_IBE 12

This part is overlapping with your other patch to nsCookieService, right?

@@ +1050,5 @@
> +        // Add the extendedAttributes column to the table.
> +        rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +          "ALTER TABLE moz_cookies ADD extendedAttributes TEXT"));
> +        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
> +        COOKIE_LOGSTRING(PR_LOG_DEBUG, 

trailing whitespace

@@ +2060,5 @@
>                            creationTime,
>                            false,
>                            isSecure,
>                            isHttpOnly,
> +                         &attrs);

indentation

@@ +3188,5 @@
>    aCookieHeader.Rebind(cookieStart, cookieEnd);
>    return newCookie;
>  }
>  
> +// Parses attributes from a string of extended attributes

// and returns true if a new attribute is found.

This changes the semantics of ParseAttributes, which used to return true if there was a new cookie. Are you sure that all callers are ok with this?

The comment block that starts "The following comment block elucidates the function of ParseAttributes" also needs a re-write.
Hi Mark,

I am having trouble reviewing this patch since every patch covers the same files. Can you please make a single patch, omitting the sameDomain changes?

Thanks,
Monica
Hi Mark,

Thanks for chatting today. To recap the plan:

1) Send the cleanup of hardcoded column numbers for review to jdm, whom you can also ping about e10s
2) Test the schema migration (hopefully automatically). Newer schemas should also be readable by older versions of firefox.
3) Unittest the attribute parsing, probably in netwerk/test/TestCookie
4) sameDomain stuff goes in last

Good luck and have fun,
Monica
Hi Mark,

As far as I can tell there is no schema migration test for cookies. However, there are migration tests for the download DB in toolkit/components/downloads/test/schema_migration that you may be able to model a test on.

Thanks,
Monica
Attached patch Check attribute on send (obsolete) — Splinter Review
Have moved the cleanup and schema code to bug 815753 and bug 792986 respectively (this seems to make sense if we're splitting landing). I haven't combed for nits on any of this but comments on the rest welcome.
Attachment #684738 - Attachment is obsolete: true
Attachment #684740 - Attachment is obsolete: true
Attachment #684741 - Attachment is obsolete: true
Attachment #684742 - Attachment is obsolete: true
Attachment #684738 - Flags: feedback?(mmc)
Attachment #684741 - Flags: feedback?(mmc)
Attachment #684742 - Flags: feedback?(mmc)
Attachment #686281 - Flags: feedback?(mmc)
Hey Mark,

Are you still waiting on feedback for this? I was waiting for bug 792986.

Monica
(In reply to Monica Chew from comment #20)
> Hey Mark,
> 
> Are you still waiting on feedback for this? I was waiting for bug 792986.
> 
> Monica

I was still waiting on review of 815753 (blocks the blocker). I've not looked at this since December so I'll un-bitrot (there's been lots of work on cookies lately) and ping mconnor again tomorrow.
Comment on attachment 686281 [details] [diff] [review]
Check attribute on send

In that case I'll just clean up my bugzilla dash, please ping me when you're ready :)
Attachment #686281 - Flags: feedback?(mmc) → feedback?
1. The SameDomain proposal should be discussed on some security standards mailing list. Which mailing list has/is that discussion happened/happening on?

2. I know for CSS and other non-networking features we have some kind of policy (that I am not very familiar with) as far as when we expose features in releases and when we prefix them or not. Does that have any impact on this feature?
(In reply to Brian Smith (:bsmith) from comment #23)
> 1. The SameDomain proposal should be discussed on some security standards
> mailing list. Which mailing list has/is that discussion happened/happening
> on?

It hasn't yet; where would you suggest?
 
> 2. I know for CSS and other non-networking features we have some kind of
> policy (that I am not very familiar with) as far as when we expose features
> in releases and when we prefix them or not. Does that have any impact on
> this feature?

Historically, extensions to cookie haven't been prefixed so I see no precedent for this - that doesn't mean we shouldn't here. Does anyone know if there is such a policy that would apply to cookies?
(In reply to Mark Goodwin [:mgoodwin] from comment #24)
> (In reply to Brian Smith (:bsmith) from comment #23)
> > 1. The SameDomain proposal should be discussed on some security standards
> > mailing list. Which mailing list has/is that discussion happened/happening
> > on?
> 
> It hasn't yet; where would you suggest?

I think it would be good to start the discussion with a pointer to the proposed draft spec on webappsec.

> > 2. I know for CSS and other non-networking features we have some kind of
> > policy (that I am not very familiar with) as far as when we expose features
> > in releases and when we prefix them or not. Does that have any impact on
> > this feature?
> 
> Historically, extensions to cookie haven't been prefixed so I see no
> precedent for this - that doesn't mean we shouldn't here. Does anyone know
> if there is such a policy that would apply to cookies?

When we were developing CSP, we did use the Moz- prefix to the header.

I don't know the scope of the prefixing proposal. Is it just about CSS and Javascript APIs, or is it about every not-yet-standardized thing we do? It would be good to discuss this on dev-platform.

I read the draft spec. I am confused about the terminology. I consider https://www.mozilla.org and http://www.mozilla.org:80 to be "same domain" but not "same origin". But, the proposal seems to consider "domain" to be a synonym for origin. Or, perhaps I am grossly misunderstanding the proposal. If my understanding is correct, then we should replace all instances of "Domain" with "Origin", including in the name of the SameDomain cookie attribute itself. If my understanding is wrong, then the terminology section of the draft spec should define "domain" and explain the difference between "domain" and "origin."
It would also be great to see an explicit comparison against Adam Barth's Origin cookie proposal:
http://tools.ietf.org/html/draft-abarth-cake-01
http://abortz.net/papers/session-integrity.pdf
Still awaiting unit_ipc test
Attachment #686281 - Attachment is obsolete: true
Attachment #686281 - Flags: feedback?
Got ipc tests working
Attachment #730390 - Attachment is obsolete: true
Attachment #731464 - Flags: feedback?(mmc)
(In reply to Brian Smith (:bsmith) from comment #26)
> It would also be great to see an explicit comparison against Adam Barth's
> Origin cookie proposal:

Brian, I've updated the spec document to reference Adam's origin cookie proposal. These ideas complement each other; one is designed to prevent related-domain modification and the other to prevent cross-site request forgery.

Anyway, I'm now maintaining the document here: https://github.com/mozmark/SameDomain-cookies/blob/master/samedomain.txt
Can we wait until the tagging patch is landed before doing a review cycle on this?
(In reply to Monica Chew [:mmc] from comment #30)
> Can we wait until the tagging patch is landed before doing a review cycle on
> this?

absolutely.
Comment on attachment 731464 [details] [diff] [review]
samedomain implementation using cookie tags

Great, thank you!
Attachment #731464 - Flags: feedback?(mmc)
See Also: → 629325
Hey Mark, are you still working on this?
Whiteboard: [necko-would-take]
Summary: Add SameDomain support for cookies → Add SameSite support for cookies
No longer depends on: 792986
Depends on: 1286858
Depends on: 1286861
Depends on: 1286870
Duplicate of this bug: 629325
This proposal has been adopted for consideration by the httpbis working group. The current version of the spec has been renamed to draft-ietf-httpbis-cookie-same-site.
Given yesterday's popularity of https://scotthelme.co.uk/csrf-is-dead/, is any work in progress to land this (in any form) as early as possible? It's been quite a while since Chrome implemented this, and even quite a while since the spec became draft, ideally we would have been the first to implement this to push the web forward again, but failing that, are we landing this soon?
Mark, is this ready to land?
Flags: needinfo?(mgoodwin)
So... how exactly is this attribute to be used. The available documentation on the feature (here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie) currently echos the ietf entry (https://tools.ietf.org/html/draft-west-first-party-cookies-07) which only permits the access control be set as a bool with string entries "Strict" and "Lax" however what domain scope is this enforced against? The origin of the request in which the "Set-Cookie" response header was defined or the scope outlined in the "Domain" attribute defined within each individual "Set-Cookie" header?

I am trying to understand how any environment utilizing any form of shared session identifier can utilize this access control to whitelist a set of approved domains instead of the individual domain entry.
@Daniel, I don't think this is the place to ask about how to use SameSite.

But as a quick(ish) answer, it's just for the individual domain only (no whitelists yet); so any cookies marked as SameSite will only be sent in requests that were started by the "same site", where this is based on the origin (not the Domain attribute).

With an environment using a shared session identifier, that will depend on the handover... if you do a full authenticated handover every time the user changes domains, where each domain keeps it's own session cookie, then you might be able to use "Strict"; if it uses simple links between domains, then you will need to use "Lax"; but if you are using POST requests between the domains, then the current implementation of SameSite won't work for you.

See https://scotthelme.co.uk/csrf-is-dead/ for more info.
Blocks: 1351663
Shouldn't this bug have a parity-chrome flag?
(In reply to Bartosz Piec from comment #42)
> Shouldn't this bug have a parity-chrome flag?

Yes, added it to the whiteboard.

Ping, Mark, there's a half-a-year-old pending feedback request asking if your patch is ready to land. If not, I suggest you unassign yourself.

Sebastian
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [necko-would-take] → [necko-would-take][parity-chrome]
php7 already supported SameSite.

Expecting firefox update.
Assignee: mgoodwin → nobody
Flags: needinfo?(mgoodwin)
(In reply to Songhongbao from comment #44)
> php7 already supported SameSite.

Uh? According to publicly accessible information and discussion, it will be available in 7.3, i.e. in more than one year from now.
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
Comment on attachment 731464 [details] [diff] [review]
samedomain implementation using cookie tags

Obsoleting old patch - the tagging approach is no longer appropriate and the same site attribute storage is already taken care of with bug 1286858
Attachment #731464 - Attachment is obsolete: true
Hello, does the resolution of 1286858 means that Firefox 58 will ship support for SameSite cookies (FYI that's what Caniuse suggests: https://caniuse.com/#feat=same-site-cookie-attribute), or some more work is needed?

If it's resolved, could someone update the metadata of the current bug?

Jakub
It's not working in Firefox Developer Edition 58.0b6 ... so I assume it will be in a later beta?
(In reply to jakub.g from comment #48)
> Hello, does the resolution of 1286858 means that Firefox 58 will ship
> support for SameSite cookies (FYI that's what Caniuse suggests:
> https://caniuse.com/#feat=same-site-cookie-attribute), or some more work is
> needed?
> 
> If it's resolved, could someone update the metadata of the current bug?
> 
> Jakub

That would be bug 1298370
Depends on: 1430803
Annevk, it seems that same-site cookies are getting more important for Firefox to implement. Question is, is that model somehow captured within the fetch spec at all at the moment? And, if not, can we capture it?
Flags: needinfo?(annevk)
Hey Mark, it seems sameSite cookie support was added within Bug 1286858. Can we mark this bug as a duplicate or is there something missing within Bug 1286858 that needs to be implemented here?
Flags: needinfo?(mgoodwin)
Cookies need to be integrated with Fetch (and HTML) to some extent, yes. I'm not personally working on that. Mike touched cookies last so hopefully he can answer.
Flags: needinfo?(annevk) → needinfo?(mike)
Christoph, I don't think this can be closed given the other dependencies listed here. It seems they need to be fixed first.
Flags: needinfo?(mgoodwin)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #52)
> Hey Mark, it seems sameSite cookie support was added within Bug 1286858. Can
> we mark this bug as a duplicate or is there something missing within Bug
> 1286858 that needs to be implemented here?

Yeah, there's a missing part: bug 1286861 (bug 1286858 on deals with storage and interface changes)

It's probably not a huge piece of work but my time's allocated to crypto eng things at the moment.
Depends on: 1452715
Actual SameSite support has been enabled with bug 1286861.
Blocks: 1452715
No longer depends on: 1452715
Alias: samesite-cookies
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [necko-would-take][parity-chrome] → [necko-would-take]
Depends on: 1459321
Can someone please clarify the status of SameSite support in Firefox 60?

I notice that https://caniuse.com/#feat=same-site-cookie-attribute and https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie state that Firefox 60 supports SameSite, but it is not mentioned in the Firefox 60 release notes, nor "Firefox 60 for developers", nor "Firefox 60 Site Compatibility", and this bug's status is still "New".

https://www.mozilla.org/en-US/firefox/60.0/releasenotes/
https://developer.mozilla.org/en-US/Firefox/Releases/60
https://www.fxsitecompat.com/en-CA/versions/60/

I think it would be good to clarify things. Thank you.
relnote-firefox: --- → ?
Firefox 60 definitely has SameSite support. What needs to happen here?
Could we have someone familiar with SameSite update the MDN page linked above [2] to mention SameSite cookies? The release notes are already released and I don’t believe we control FxSiteCompat, but we could certainly improve MDN content.

[2] https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/60
The dev-doc-needed flag should get this on the mdn team's radar when this bug gets resolved.  Can we do that yet Christoph?
Flags: needinfo?(ckerschb)
(In reply to Atoll R. Soderberg  [:atoll, :rsoderberg] from comment #60)
> Could we have someone familiar with SameSite update the MDN page linked
> above [2] to mention SameSite cookies?

Sure.

(In reply to Julien Cristau [:jcristau] from comment #61)
> The dev-doc-needed flag should get this on the mdn team's radar when this
> bug gets resolved.  Can we do that yet Christoph?

We can.
Flags: needinfo?(ckerschb)
You need to log in before you can comment on or make changes to this bug.