Add pref to disable same-site cookies

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: ckerschb, Assigned: francois)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox60 fixed, firefox61 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(2 attachments)

Comment hidden (empty)
(Reporter)

Updated

a year ago
Assignee: nobody → francois
Status: NEW → ASSIGNED
Component: DOM: Security → Networking: Cookies
Priority: -- → P1
I think it's best to add that pref here [1] to simply ignore the attribute.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#3951-3955
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
The function that Christoph suggested, nsCookieService::ParseAttributes(), is a static function so we unfortunately can't use the same pref-caching mechanism as for the other prefs.

I chose to put the pref-checking code there anyways though because:

- Preventing the attribute from being parsed is the simplest way to make sure that none of the SameSite codepaths are ever executed.
- We intend for this to be a temporary killswitch for 60/61. We'll remove it in 62.
- The extra pref check only happens for sites that use SameSite and we don't believe there are many of those yet.
(Reporter)

Comment 4

a year ago
mozreview-review
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

https://reviewboard.mozilla.org/r/235144/#review240850

just adding a drive by nit.

::: netwerk/cookie/nsCookieService.cpp:3979
(Diff revision 1)
> +      bool sameSiteEnabled = false;
> +      nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
> +      if (prefs &&
> +          NS_SUCCEEDED(prefs->GetBoolPref("network.cookie.same-site.enabled",
> +                                          &sameSiteEnabled)) &&
> +          sameSiteEnabled) {

cookies are quite a hot code path and querying a pref is an expensive operation. I suggest we use some sort of singleton pattern to only query the pref once, something like:

static bool sameSiteEnabled = false;
static bool prefInitialized = false;
if (!prefInitialized) {
  nsCOMPtr<nsIPrefBranch> prefs =
    do_GetService(NS_PREFSERVICE_CONTRACTID);
  prefs->GetBoolPref("network.cookie.same-site.enabled", &sameSiteEnabled);
  prefInitialized = true;
}

if (sameSiteEnabled) {
  bla
}
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4)
> > +          NS_SUCCEEDED(prefs->GetBoolPref("network.cookie.same-site.enabled",
> > +                                          &sameSiteEnabled)) &&
> > +          sameSiteEnabled) {
> 
> cookies are quite a hot code path and querying a pref is an expensive
> operation. I suggest we use some sort of singleton pattern to only query the
> pref once, something like:
> 
> static bool sameSiteEnabled = false;
> static bool prefInitialized = false;
> if (!prefInitialized) {
>   nsCOMPtr<nsIPrefBranch> prefs =
>     do_GetService(NS_PREFSERVICE_CONTRACTID);
>   prefs->GetBoolPref("network.cookie.same-site.enabled", &sameSiteEnabled);
>   prefInitialized = true;
> }

Preferences::AddBoolVarCache is even better, as you don't have to do all the dirty work yourself :)
(In reply to Valentin Gosu [:valentin] from comment #5)
> Preferences::AddBoolVarCache is even better, as you don't have to do all the
> dirty work yourself :)

Indeed. In fact it needs to be AddBoolVarCache which gets notified in case the pref gets updated!
You've put the check in the attribute parsing code, but that code landed in 58 and is already on the 60 branch. If we're worried about uplifting then shouldn't the pref control the new behavior of whether we _send_ the cookie?
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
(In reply to Daniel Veditz [:dveditz] from comment #7)
> You've put the check in the attribute parsing code, but that code landed in
> 58 and is already on the 60 branch. If we're worried about uplifting then
> shouldn't the pref control the new behavior of whether we _send_ the cookie?

You're right, we don't really need to disable the parsing code which has already been in the platform for a while. I chose to go with Christoph's approach because it seemed like the simplest way to ensure we don't treat SameSite cookies differently in any way. I also don't see any downsides in disabling the parsing of the cookie attribute.
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

https://reviewboard.mozilla.org/r/235144/#review241722

r=me with a couple minor nits.

::: netwerk/cookie/nsCookieService.cpp:79
(Diff revision 2)
>   * nsCookieService impl:
>   * useful types & constants
>   ******************************************************************************/
>  
>  static StaticRefPtr<nsCookieService> gCookieService;
> +bool nsCookieService::gSameSiteEnabled = false;

nit: I think the naming convention is sSameSiteEnabled if it's a static on the cookieService, or gSameSiteEnabled if it's a "global" variable.

::: netwerk/cookie/nsCookieService.cpp:3977
(Diff revision 2)
>      // just set the boolean
>      else if (tokenString.LowerCaseEqualsLiteral(kHttpOnly))
>        aCookieAttributes.isHttpOnly = true;
>  
>      else if (tokenString.LowerCaseEqualsLiteral(kSameSite)) {
> +      if (gSameSiteEnabled) {

Instead of the indentation level, you could put gSameSiteEnabled in the condition above:
if (tokenString.Equals(..) && gSameSiteEnabled)
Attachment #8966442 - Flags: review?(valentin.gosu) → review+
(In reply to Daniel Veditz [:dveditz] from comment #7)
> You've put the check in the attribute parsing code, but that code landed in
> 58 and is already on the 60 branch. If we're worried about uplifting then
> shouldn't the pref control the new behavior of whether we _send_ the cookie?

I guess Dan is right. Think about the following scenario, someone visits a page which sets a same site cookie using FF59. The cookie gets set, but there is no enforcement of same-site cookies within FF59. Now that user updates to Firefox 60 and disables the pref.

Disabling the pref now means the no *new* same-site cookies can bet set, right? But the once that are already set are enforced in the backend code, right? Given that I think it might make more sense to go with Dan's approach and actually disable the the 'enforcement' code.

Probably worth rebasing after Bug 1452496 has landed.

Updated

a year ago
Whiteboard: [necko-triaged]
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
(In reply to François Marier [:francois] from comment #9)
> I also don't see any downsides in disabling the parsing of the cookie attribute.

There is one downside, which I suppose is what Dan had in mind: since the parsing has been enabled in many releases, existing profiles likely already have cookies with the SameSite attribute stored. If we only disable the parsing of attributes, we prevent new cookies from being treated in the same-site way, but we let existing cookies go through the same-site restrictions. 

This latest revision follows the approach suggested by Dan in comment 7.

It turns out we can't rely on nsCookieService::Init() being called before nsCookieService::ParseAttributes(), therefore we can't stash the AddBoolVarCache() call there and need to resort to the local static variable trick that Christoph originally suggested in comment 4.
(Assignee)

Comment 14

a year ago
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

Flagging Valentin again since the patch has changed quite a bit since the last r+.
Attachment #8966442 - Flags: review+ → review?(valentin.gosu)
(Reporter)

Comment 15

a year ago
mozreview-review
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

https://reviewboard.mozilla.org/r/235144/#review242134

Yeah, having the pref there correctly guards the enforcement of same-site cookies. r=me

::: netwerk/cookie/nsCookieService.cpp:3080
(Diff revision 3)
>        (aCookie->IsDomain() && StringEndsWith(aHost, aCookie->Host()));
>  }
>  
>  bool
> +nsCookieService::IsSameSiteEnabled()
> +{

I personally prefer the static variable also to be in the static function, but that's just my personal preference. I leave that up to you!
Attachment #8966442 - Flags: review?(ckerschb) → review+
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

https://reviewboard.mozilla.org/r/235144/#review242162
Attachment #8966442 - Flags: review?(valentin.gosu) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

a year ago
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

After going through some manual tests, I found that I also needed to add a check here: https://searchfox.org/mozilla-central/rev/fca4426325624fecbd493c31389721513fc49fef/netwerk/cookie/nsCookieService.cpp#3472-3481

I extended our automated tests to cover both the same-site.enabled = true and same-site.enabled = false cases.
Attachment #8966442 - Flags: review?(valentin.gosu)
Attachment #8966442 - Flags: review?(ckerschb)
Attachment #8966442 - Flags: review+
(Assignee)

Comment 20

a year ago
The first patch is a work-around for some caching issues I discovered while extending the tests. I noticed that the following would fail in test_same_site_cross_origin_context.html:

var tests = [
  {
    description: "regular cookie in cross origin context",
    imgSRC: CROSS_ORIGIN + PATH + "?setRegularCookie",
    frameSRC: CROSS_ORIGIN + PATH + "?loadFrame",
    result: "myKey=regularCookie",
  },
  {
    description: "regular cookie in cross origin context",
    imgSRC: CROSS_ORIGIN + PATH + "?setRegularCookie",
    frameSRC: CROSS_ORIGIN + PATH + "?loadFrame",
    result: "myKey=regularCookie",
  },
];

(i.e. running the same test twice)

As far as I can tell, the cookie does get cleared properly between runs, but the second attempt to set a cookie will always fail. This means that the test would pass regardless of whether or not the implementation works.

I spent several hours trying to get to the bottom of this, but in the end I gave up and found a work-around which works reliably.
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

https://reviewboard.mozilla.org/r/235144/#review242420

Looks good. Thanks for the tests!
Attachment #8966442 - Flags: review?(valentin.gosu) → review+
(Reporter)

Comment 22

a year ago
mozreview-review
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

https://reviewboard.mozilla.org/r/235144/#review242472

Ok, I am fine with this!
Attachment #8966442 - Flags: review?(ckerschb) → review+
(Reporter)

Comment 23

a year ago
mozreview-review
Comment on attachment 8967917 [details]
Bug 1452699 - Work-around caching issues in test_same_site_cookies_*.

https://reviewboard.mozilla.org/r/236614/#review242474

Same here, I am fine with this. Thanks!
Attachment #8967917 - Flags: review?(ckerschb) → review+

Comment 24

a year ago
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a22a88728d52
Work-around caching issues in test_same_site_cookies_*. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/38a419def0a7
Add a temporary pref to disable same-site cookies. r=ckerschb,valentin
https://hg.mozilla.org/mozilla-central/rev/a22a88728d52
https://hg.mozilla.org/mozilla-central/rev/38a419def0a7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Assignee)

Comment 26

a year ago
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

Approval Request Comment
[Feature/Bug causing the regression]: bug 795346
[User impact if declined]: No way to disable same-site cookies if our implementation causes problems.
[Is this code covered by automated tests?]: Yes, see second patch.
[Has the fix been verified in Nightly?]: Yes, 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?]: It's actually reducing risk by giving us a way to turn off the feature on release.
[String changes made/needed]: None
Attachment #8966442 - Flags: approval-mozilla-beta?
(Assignee)

Comment 27

a year ago
Comment on attachment 8967917 [details]
Bug 1452699 - Work-around caching issues in test_same_site_cookies_*.

Approval Request Comment

See comment 26. This needs to be applied first.
Attachment #8967917 - Flags: approval-mozilla-beta?
Comment on attachment 8966442 [details]
Bug 1452699 - Add a temporary pref to disable same-site cookies.

sounds good, approved for 60.0b14
Attachment #8966442 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8967917 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment hidden (obsolete)
Comment hidden (obsolete)
Depends on: 1468912
You need to log in before you can comment on or make changes to this bug.