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

RESOLVED FIXED in Firefox 60

Status

()

P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: April, Assigned: francois)

Tracking

(Blocks: 1 bug)

59 Branch
mozilla61
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed, firefox61 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Blocks: 795346
Group: core-security → network-core-security
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.
Group: network-core-security

Updated

a year ago
Priority: -- → P3
Whiteboard: [necko-triaged]
(Assignee)

Comment 2

a year ago
(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.
Assignee: nobody → francois
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)

Comment 4

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

https://reviewboard.mozilla.org/r/234314/#review239986
Attachment #8965524 - Flags: review?(valentin.gosu) → review+
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
(Assignee)

Comment 6

a year ago
(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.
(Assignee)

Comment 7

a year ago
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

a year ago
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd0991e92860
Ignore SameSite cookie attribute when value is empty or unrecognised. r=valentin
(Assignee)

Comment 9

a year ago
I started an email thread with Mike West about clarifying the spec.
https://hg.mozilla.org/mozilla-central/rev/bd0991e92860
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
status-firefox60: --- → affected
(Assignee)

Comment 11

a year ago
(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
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.
status-firefox59: affected → wontfix
Flags: needinfo?(francois)
(Assignee)

Comment 13

a year ago
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
Flags: needinfo?(francois)
Attachment #8965524 - Flags: approval-mozilla-beta?
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.
Attachment #8965524 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 15

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/57318d4d7ca9
status-firefox60: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.