Modify cookie service GetCookieString (etc) to provide necessary data for SameSite checks.

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: mgoodwin, Assigned: ckerschb)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox60 fixed, firefox61 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
A SameSite cookie implementation (see https://tools.ietf.org/html/draft-west-first-party-cookies-07) requires that nsCookieService has information  on the originating document to determine if it is same site.
Whiteboard: [necko-active]
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1

Updated

2 years ago
Priority: P1 → P3
Whiteboard: [necko-active]

Updated

2 years ago
Whiteboard: [necko-triaged]
(Reporter)

Updated

2 years ago
Assignee: mgoodwin → nobody
Hey Valentin, I checked the codebase and looked for 'isSecure' and updated the cookie code in a similar fashion which should cover all the cases. The only change I am not sure about is within RecordDocumentCookie().

Within this patch I added a test within TestCookie.cpp, but that test setup isn't create. Ultimately we also need a test using a top-level navigation using 'samesite=lax'.
Attachment #8965347 - Flags: review?(valentin.gosu)
(Assignee)

Updated

a year ago
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: P3 → P1
If it's a cross-site request (based on eTLD+1, not origin) SameSite=Strict cookies are always blocked

If it's SameSite=lax then 
 * subrequests are always blocked
 * top-level navigations are allowed for "safe" methods.
   use nsHttpRequestHead::IsSafeMethod() for this check

Instead of passing in another boolean you could combine them as aIsSafeTopLevelNav because we don't need the states independently.
(In reply to Daniel Veditz [:dveditz] from comment #3)
> Instead of passing in another boolean you could combine them as
> aIsSafeTopLevelNav because we don't need the states independently.

Yeah, that makes sense. Thanks for pointing that out Dan.
Attachment #8965347 - Attachment is obsolete: true
Attachment #8965347 - Flags: review?(valentin.gosu)
Attachment #8965492 - Flags: review?(valentin.gosu)
Comment on attachment 8965492 [details] [diff] [review]
bug_1286861_finish_same_site_cookies.patch

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

The patch looks good to me. Great job!
I'd love to see some web-platform-tests for this, to make sure we implement it in a way that doesn't break web-compat.
Attachment #8965492 - Flags: review?(valentin.gosu) → review+
Here are tests for subrequests, we still need tests for top-level loads though.
Attachment #8965616 - Flags: review?(mgoodwin)
(Reporter)

Comment 7

a year ago
Comment on attachment 8965616 [details] [diff] [review]
bug_1286861_same_site_subrequest.patch

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

LGTM!

::: dom/security/test/general/file_same_site_cookies_subrequest.sjs
@@ +16,5 @@
> +  <body>
> +    <img src = "http://mochi.test:8888/tests/dom/security/test/general/file_same_site_cookies_subrequest.sjs?checkCookie">
> +  </body>
> +  </html>`;
> +  

Nit: space

@@ +72,5 @@
> +    return;
> +  }
> +
> +  // we should never get here, but just in case return something unexpected
> +  response.write("do'h");

Nit: spelling "D'oh"
(Reporter)

Updated

a year ago
Attachment #8965616 - Flags: review?(mgoodwin) → review+
(In reply to Mark Goodwin [:mgoodwin] from comment #7)
> Nit: spelling "D'oh"

Done - carrying over r+ from mgoodwin!
Attachment #8965616 - Attachment is obsolete: true
Attachment #8965668 - Flags: review+
When writing tests for top-level navigation I realized that the flag 'aForeign' is never 'true' for top-level navigations. The reason for that is because we pass mURI to GetCookieStringFromHttp [1]. I don't think we can update that and pass some other value than mURI for top-level navigations. I don't even know what implication that might have. To play it safe and avoid possible regressions I think the safe bet is to pass a new flag around.

What we need specifically for the purpose of same site cookies is a comparison between the triggeringPrincipal and the new top-level URI, hence I created a new function NS_IsTopLevelForeign specifically to compute that value.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#3388
Attachment #8965672 - Flags: review?(valentin.gosu)
Mark, here is new set of tests for top-level navigations. I think with that we are covering all the cases we care about, right?
Attachment #8965673 - Flags: review?(mgoodwin)
(Reporter)

Comment 11

a year ago
Comment on attachment 8965673 [details] [diff] [review]
bug_1286861_same_site_toplevel.patch

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

Looks good!
Attachment #8965673 - Flags: review?(mgoodwin) → review+
Please note there is also a public test page for same-site cookies: http://rfc6265.biz/tests/

So far it seems we are failing tests because we are falling back to using 'strict' if the value is undefined. I think our fallback is acceptable and in reality not really a concern. Once those tests are baked into actual web platform tests, then we can reconsider. The reason those are not web-platform tests at the moment is because the test infrastructure can only provide one top-level domain, but in order to test same site cookies more top-level domains are needed. Someone is working on that however. I guess we just wait!
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> Please note there is also a public test page for same-site cookies:
> http://rfc6265.biz/tests/

mgoodwin pointed out that there is a piece of the spec we are missing, probably we should incorporate that change right away. Namely, if a samesite cookie has a bogus value, then the entire cookie needs to be ignored, see relevant phrase of the spec here:

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.

And then ultimately, we also shouldn't allow same-site cookies to be set on a cross origin page.
Valentin, as explained in the previous comment, we should ignore the cookie entirely in case the same site value is bogus. Here is the patch including the updated gtest.
Attachment #8965727 - Flags: review?(valentin.gosu)
I did another run on http://rfc6265.biz having all of the patches from this bug applied. It seems that CORS values are somehow off within those tests, because if I disable CORS we pass tests that otherwise timeout because of CORS errors. Anyway, with CORS disabled I get:

fetch:       10/12 PASS (2 timeouts)
form [GET]:  12 timeouts
form [POST]: 12 timeouts
iframe:      10/12 PASS (2 timeouts)
img:         12/12 PASS (1 timout)
window.open:  5/12 PASS (7 timeouts)

I don't know where all these timeouts are coming from. Probably worth spending yet a little more time debugging.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> Created attachment 8965727 [details] [diff] [review]
> bug_1286861_skip_bogus_values.patch
> 
> Valentin, as explained in the previous comment, we should ignore the cookie
> entirely in case the same site value is bogus. Here is the patch including
> the updated gtest.

Since that's what bug 1430803 is about, let's move that specific discussion there: https://bugzilla.mozilla.org/show_bug.cgi?id=1430803#c6
(In reply to Valentin Gosu [:valentin] from comment #5)
> I'd love to see some web-platform-tests for this, to make sure we implement
> it in a way that doesn't break web-compat.

The test page that Christoph mentioned in comment 12 is already using the wpt framework: https://github.com/mikewest/rfc6265-biz/tree/master/static/tests/samesite

so I would assume that Mike West intends to merge his tests there once a second implementation has shipped.
Comment on attachment 8965727 [details] [diff] [review]
bug_1286861_skip_bogus_values.patch

Dropping the review request for now since this patch will need to change after bug 1430803 lands.
Attachment #8965727 - Flags: review?(valentin.gosu)
Comment on attachment 8965672 [details] [diff] [review]
bug_1286861_finish_same_site_cookies_followup.patch

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

::: netwerk/cookie/CookieServiceChild.cpp
@@ +496,5 @@
>    } else {
>      if (!mIPCOpen) {
>        return NS_ERROR_NOT_AVAILABLE;
>      }
> +    GetCookieStringSyncIPC(aHostURI, !!isForeign, isSafeTopLevelNav,

nit: I think we can remove the !! for isForeign.
Attachment #8965672 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8965727 [details] [diff] [review]
bug_1286861_skip_bogus_values.patch

(In reply to François Marier [:francois] from comment #19)
> Dropping the review request for now since this patch will need to change
> after bug 1430803 lands.

Let's wait for the clarification in the spec. Personally I think our implementation is more correct. I don't see a reason we drop the entire cookie just because same-site has a bogus value.

Either way, I agree with that there is nothing we need to land within this patch. If at all, we can follow up on Bug 1430803.
Attachment #8965727 - Attachment is obsolete: true

Comment 22

a year ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04acf6387687
Update CookieService to enforce same site cookies. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/59818b273bff
Add tests for same site subrequests. r=mgoodwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/525e7c6d62de
Add tests for same site top-level. r=mgoodwin

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/04acf6387687
https://hg.mozilla.org/mozilla-central/rev/59818b273bff
https://hg.mozilla.org/mozilla-central/rev/525e7c6d62de
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8965492 [details] [diff] [review]
bug_1286861_finish_same_site_cookies.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bgg 1286861
[User impact if declined]: No enforcement for same-site cookies.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, 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?]: Because logic for same site cookies is not interfering with regular cookie code.
[String changes made/needed]: no
Attachment #8965492 - Flags: approval-mozilla-beta?
Comment on attachment 8965668 [details] [diff] [review]
bug_1286861_same_site_subrequest.patch

Approval Request Comment
Please see comment 24.
Attachment #8965668 - Flags: approval-mozilla-beta?
Comment on attachment 8965672 [details] [diff] [review]
bug_1286861_finish_same_site_cookies_followup.patch

Approval Request Comment
Please see comment 24.
Attachment #8965672 - Flags: approval-mozilla-beta?
Comment on attachment 8965673 [details] [diff] [review]
bug_1286861_same_site_toplevel.patch

Approval Request Comment
Please see comment 24.
Attachment #8965673 - Flags: approval-mozilla-beta?
Comment on attachment 8965492 [details] [diff] [review]
bug_1286861_finish_same_site_cookies.patch

samesite cookie support, beta60+
Attachment #8965492 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8965668 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8965672 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8965673 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.