Add SameSite support for cookies

NEW
Assigned to

Status

()

Core
Networking: Cookies
5 years ago
22 days ago

People

(Reporter: mgoodwin, Assigned: mgoodwin, NeedInfo)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs, {dev-doc-needed, sec-want})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-would-take], URL)

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

5 years ago
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/
(Assignee)

Comment 1

5 years ago
Created attachment 665932 [details] [diff] [review]
Add SameDomain attribute to nsCookie and nsCookieAttributes

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

Comment 2

5 years ago
Created attachment 665934 [details] [diff] [review]
Add storage and schema migration for SameDomain cookie attribute

Add storage and schema migration for SameDomain cookie attribute
Attachment #665934 - Flags: feedback?(mmc)
(Assignee)

Comment 3

5 years ago
Created attachment 665937 [details] [diff] [review]
Check for SameDomain attr, compare with internal referer on cookie send to determine violations
Attachment #665937 - Flags: feedback?(mmc)
Keywords: sec-want
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
(Assignee)

Updated

5 years ago
Attachment #665932 - Attachment is obsolete: true
Attachment #665932 - Flags: feedback?(mmc)
(Assignee)

Updated

5 years ago
Attachment #665934 - Attachment is obsolete: true
Attachment #665934 - Flags: feedback?(mmc)
(Assignee)

Updated

5 years ago
Attachment #665937 - Attachment is obsolete: true
Attachment #665937 - Flags: feedback?(mmc)
(Assignee)

Comment 8

5 years ago
Created attachment 684738 [details] [diff] [review]
1 - parse attributes, etc.

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

Comment 9

5 years ago
Created attachment 684740 [details] [diff] [review]
2 - Clean up hardcoded column indexes

Make the code (a little) less opaque by setting constants for the cardcoded column indexes.
Attachment #684740 - Flags: feedback?(mmc)
(Assignee)

Comment 10

5 years ago
Created attachment 684741 [details] [diff] [review]
3 - storage (and migration)

Sort out persistence for cookie attributes. Add migration script from schema 5 to 6.
(Assignee)

Comment 11

5 years ago
Created attachment 684742 [details] [diff] [review]
4 - the actual samedomain patch

Check to see if a cookie has samedomain attribute set before off-origin send.
Attachment #684742 - Flags: feedback?(mmc)
(Assignee)

Updated

5 years ago
Attachment #684741 - Flags: feedback?(mmc)
(Assignee)

Updated

5 years ago
Attachment #684740 - Attachment description: Clean up hardcoded column indexes → 2 - Clean up hardcoded column indexes
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 13

5 years ago
(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
(Assignee)

Comment 19

5 years ago
Created attachment 686281 [details] [diff] [review]
Check attribute on send

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

Comment 21

4 years ago
(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?
(Assignee)

Comment 24

4 years ago
(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
(Assignee)

Comment 27

4 years ago
Created attachment 730390 [details] [diff] [review]
samedomain implementation using cookie tags

Still awaiting unit_ipc test
Attachment #686281 - Attachment is obsolete: true
Attachment #686281 - Flags: feedback?
(Assignee)

Comment 28

4 years ago
Created attachment 731464 [details] [diff] [review]
samedomain implementation using cookie tags

Got ipc tests working
Attachment #730390 - Attachment is obsolete: true
Attachment #731464 - Flags: feedback?(mmc)
(Assignee)

Comment 29

4 years ago
(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?
(Assignee)

Comment 31

4 years ago
(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)
(Assignee)

Updated

2 years ago
See Also: → bug 629325
Hey Mark, are you still working on this?
Whiteboard: [necko-would-take]

Comment 34

a year ago
Chrome Shipped same-site cookies https://bugs.chromium.org/p/chromium/issues/detail?id=459154#c32
Summary: Add SameDomain support for cookies → Add SameSite support for cookies
(Assignee)

Updated

10 months ago
No longer depends on: 792986
(Assignee)

Updated

10 months ago
Depends on: 1286858
(Assignee)

Updated

10 months ago
Depends on: 1286861
(Assignee)

Updated

10 months ago
Depends on: 1286870
Keywords: dev-doc-needed
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.
Blocks: 1298370
No longer blocks: 1298370
Blocks: 1298370
Comment hidden (me-too)
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)

Comment 40

2 months ago
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.

Comment 41

2 months ago
@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.

Updated

27 days ago
Blocks: 1351663
You need to log in before you can comment on or make changes to this bug.