1430803 - SameSite cookies are incorrectly being stored, when set with invalid SameSite values

Status: RESOLVED FIXED

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

Description

[April King [:April]]

If a site serves the following header:

> Set-Cookie: sstest=foo; Path=/; SameSite=Bar

According to the SameSite cookie specification,

https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00

These cookies should be ignored entirely:

> 1.  If "cookie-av"'s "attribute-value" is not a case-insensitive
>     match for "Strict" or "Lax", ignore the "cookie-av".

Currently these cookies are stored as having been set with SameSite=Strict.

I believe this bug is in firefox58 as well, but I don't really have any great way of seeing into the cookie store to tell what it's doing, since SameSite was added to the inspector in firefox59.

I can't think of any specific security concerns off the top of my head, but the browser is storing cookies it shouldn't, They are set to "Strict" so probably not an actual risk, but I'm going to mark this as security sensitive and let the folks on the Cookies team deflag it if they think it's unnecessary.

Comment 1

[Daniel Veditz [:dveditz]]

We don't yet support the SameSite specification--that's bug 795346. We did add some parsing in bug 1286858. From a security POV it's hard to argue with "fail strict", but it's not spec compliant. Tests are pretty minimal and don't include an invalid-value case.

https://searchfox.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#3942-3945

This does not need to be a hidden security bug.

Comment 2

[François Marier [:francois]]

(In reply to April King [:April] from comment #0)
> These cookies should be ignored entirely:
> 
> > 1.  If "cookie-av"'s "attribute-value" is not a case-insensitive
> >     match for "Strict" or "Lax", ignore the "cookie-av".

The way I read this, only the cookie _attribute_ is ignored. The cookie itself is not ignored, it's just not a samesite cookie.

(In reply to Daniel Veditz [:dveditz] from comment #1)
> From a security POV it's hard to argue with "fail strict", but it's not spec compliant.

The main issue I see is that our current implementation is not forward-compatible with any future values (not that I can think of any, but hey) that this attribute might take.

Comment 3

[François Marier [:francois]]

Attachment: Bug 1430803 - Ignore SameSite cookie attribute when value is empty or unrecognised.

Make the parsing the the attribute spec-compliant:
https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-5.3.7

Review commit: https://reviewboard.mozilla.org/r/234314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234314/

Comment 4

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

Comment on attachment 8965524 [details]
Bug 1430803 - Ignore SameSite cookie attribute when value is empty or unrecognised.

https://reviewboard.mozilla.org/r/234314/#review239986

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Please note that this patch doesn't really solve the problem. The spec says:

If the "SameSite" attribute's value is "Strict", the cookie will only
  be sent along with "same-site" requests.  If the value is "Lax", the
  cookie will be sent with same-site requests, and with "cross-site"
  top-level navigations, as described in Section 5.3.7.1.  If the
  "SameSite" attribute's value is neither of these, the cookie will be
  ignored.

So what we really need to do is make sure the cookie gets 'ignored' if the value is bogus. I don't have a strong opinion in which bug the fix lands, but I think the proper fix is:
  https://bugzilla.mozilla.org/attachment.cgi?id=8965727&action=diff

Comment 6

[François Marier [:francois]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> Please note that this patch doesn't really solve the problem. The spec says:
> 
> If the "SameSite" attribute's value is "Strict", the cookie will only
>   be sent along with "same-site" requests.  If the value is "Lax", the
>   cookie will be sent with same-site requests, and with "cross-site"
>   top-level navigations, as described in Section 5.3.7.1.  If the
>   "SameSite" attribute's value is neither of these, the cookie will be
>   ignored.

The above is from the Server Requirements of the spec (https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-4.1.2.7). It does seem like there is a conflict with the equivalent section under User Agent Requirements (https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-5.3.7), which is what April quoted in comment 0.

Namely, under 4.1.2.7, when faced with bogus values, we're supposed to ignore the _cookie_ whereas under 5.3.7, we're supposed to ignore the _cookie attribute_.

Since Section 4.1.2 is non-normative, I wonder whether that means that 5.3.7 is the one to follow.

Also, at the end of 4.1.2, it says:

  "User agents ignore unrecognized cookie attributes (but not the entire cookie)."

I wonder whether that means that the last sentence of 4.1.2.7:

  "If the "SameSite" attribute's value is neither of these, the cookie will be ignored."

is missing a word:

  "If the "SameSite" attribute's value is neither of these, the cookie ATTRIBUTE will be ignored."

As I said in comment 2, from a forward-compatibility point of view, I think that section 5.3.7 is the one that's right.

Comment 7

[François Marier [:francois]]

I discussed this with Christoph and since the phrasing in Section 5 is the least webcompat-risky, we'll go ahead and ship that now.

Once we clarify the spec, we can update our implementation if needed, but at least we won't be stuck with a spec "bug" in an ESR if this ends up getting uplifted.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd0991e92860
Ignore SameSite cookie attribute when value is empty or unrecognised. r=valentin

Comment 9

[François Marier [:francois]]

I started an email thread with Mike West about clarifying the spec.

Comment 10

[Cristian Brindusan [:cbrindusan]]

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

Comment 11

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #9)
> I started an email thread with Mike West about clarifying the spec.

Proposed spec change here: https://github.com/httpwg/http-extensions/pull/574

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Would this be good to uplift to beta 60?   Seems like it should be OK to wontfix for 59 but getting this change into ESR 60 might be a good thing.

Comment 13

[François Marier [:francois]]

Comment on attachment 8965524 [details]
Bug 1430803 - Ignore SameSite cookie attribute when value is empty or unrecognised.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1430803
[User impact if declined]: Spec compliance issue with SameSite cookie attribute.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: The automated tests pass.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No
[Why is the change risky/not risky?]: This fixes the parsing of the SameSite attribute. While we store the SameSite attribute, we don't actually apply it to cookies, so whether or not it's parsed correctly doesn't currently matter.
[String changes made/needed]: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8965524 [details]
Bug 1430803 - Ignore SameSite cookie attribute when value is empty or unrecognised.

Getting this in 60 sounds like a good idea. Approved for 60.0b12.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/57318d4d7ca9

Comment 16

[François Marier [:francois]]

Spec has been fixed: https://github.com/httpwg/http-extensions/commit/ebe8f8ea69c02d83e5dceb91fdae0e787845c7ba