Closed Bug 1430803 Opened 3 years ago Closed 2 years ago

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

Categories

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

59 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: April, Assigned: francois)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

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.
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
Priority: -- → P3
Whiteboard: [necko-triaged]
(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 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
(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.
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.
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
I started an email thread with Mike West about clarifying the spec.
https://hg.mozilla.org/mozilla-central/rev/bd0991e92860
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(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.
Flags: needinfo?(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
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+
You need to log in before you can comment on or make changes to this bug.