Closed Bug 1286861 Opened 10 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
Networking: Cookies
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: mgoodwin, Assigned: ckerschb)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(4 files, 3 obsolete files)

bug_1286861_finish_same_site_cookies.patch
26.54 KB, patch
valentin
: review+
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review
bug_1286861_same_site_subrequest.patch
7.77 KB, patch
ckerschb
: review+
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review
bug_1286861_finish_same_site_cookies_followup.patch
24.12 KB, patch
valentin
: review+
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review
bug_1286861_same_site_toplevel.patch
8.05 KB, patch
mgoodwin
: review+
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review
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
Priority: P1 → P3
Whiteboard: [necko-active]
Whiteboard: [necko-triaged]
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: 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)
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"
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)
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
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 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.

Attachment

General

Creator:
Created:
Updated:
Size: