WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 3 weeks ago

#37000 new enhancement

Support for the SameSite cookie attribute

Reported by: johnbillion Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Security Keywords: has-patch dev-feedback needs-dev-note has-unit-tests
Focuses: administration Cc:

Description

IETF's Same-site Cookies draft was shipped in Chrome 51 and Opera 39.

The SameSite cookie attribute instructs a browser not to send that cookie with cross-origin third-party requests (such as iframes, embedded images, and Ajax requests). This effectively mitigates CSRF attacks as, for example, the user will not be authenticated for a given third party URL that's being used in a CSRF attack.

More information on the SameSite attribute can be found here: http://www.sjoerdlangkemper.nl/2016/04/14/preventing-csrf-with-samesite-cookie-attribute/

We should investigate whether setting the SameSite=lax attribute is of benefit to the auth and/or logged_in cookies in WordPress, and if so consider implementing it once the draft becomes an RFC.

PHP uses the setcookie() wrapper for setting cookies, which means that setting the SameSite attribute is not possible using that function, until such point that support for the attribute gets added. If WordPress were to implement the SameSite attribute, we'd need our own cookie handling function which constructs and sets the Set-Cookie header itself, and use it in place of setcookie() (side note: this may also be beneficial to unit testing).

Attachments (2)

samesite.diff (20.7 KB) - added by tomdxw 3 years ago.
pluggable.diff (6.0 KB) - added by mikhailroot 10 months ago.
Introduced filter wp_auth_cookie_same_site to be able to manage SameSite Auth cookies parameter.

Download all attachments as: .zip

Change History (38)

#1 @mwaclawek
4 years ago

Just a quick note: A compatible and testable replacement for PHP's "setcookie" is available here: https://github.com/delight-im/PHP-Cookie It has support for same-site cookies as well.

Do we really need to wait until the spec is finalized? Chrome has shipped an implementation based on the current draft already, so implementing the current spec would have a real impact on user security immediately (though only for Chrome users).

#2 @noam@…
4 years ago

Related: PHP #72230: Add SameSite Cookies to setcookie() https://bugs.php.net/bug.php?id=72230

#4 @demetris
3 years ago

I just came upon a hack that makes it possible to set the SameSite attribute using the default setcookie() function:

<?php
setcookie('a-cookie', '1', 0, '/; samesite=strict');

I am trying it out on WordPress by passing a modified COOKIEPATH to setcookie():

<?php
$path_plus_samesite = COOKIEPATH . '; samesite=strict';

It seems to work OK on Chrome and on Firefox 59 (the first Firefox version to implement SameSite, currently in the beta channel).

I don’t know whether it would be acceptable to use such a hack in WordPress core, but I thought it would be good to know that it exists.

Link: https://stackoverflow.com/questions/39750906/php-setcookie-samesite-strict

Cheers!

#5 @tomdxw
3 years ago

I’ve written a proof of concept plugin which adds SameSite=Lax for all auth cookies: https://gist.github.com/tomdxw/9d7eced5f951680d6eeb52fe6a7a48dc

I tested with a vulnerable plugin, and it works to prevent the CSRF.

The change is tiny - when being merged into core it would require adding one function and modifying 4 lines (the 4 setcookie() lines in wp_set_auth_cookie).

In the very rare situations where a site needs to receive authenticated POST requests, the SameSite cookie can be disabled via filter.

I agree with @mwaclawek. As it’s already been implemented by three of the major browsers (shipped in Chrome, coming to Firefox in May, implemented in WebKit but not released in Safari yet), there’s little chance of substantial change. But it will do a great deal of good by fixing the majority of CSRF vulnerabilities in WordPress plugins (of which there are a lot: https://wpvulndb.com/search?utf8=%E2%9C%93&text=csrf ).

#6 @johnbillion
3 years ago

  • Keywords dev-feedback removed
  • Milestone changed from Awaiting Review to 5.0

#7 @tomdxw
3 years ago

  • Keywords has-patch added; needs-patch removed

I’ve written a patch for this. I forked the https://github.com/delight-im/PHP-Cookie library suggested by @mwaclawek, and added support for older versions of PHP. It’s here: https://github.com/dxw/PHP-Cookie-Legacy

Only one change was made to src/Cookie.php, and that was to remove the namespace: https://github.com/dxw/PHP-Cookie-Legacy/commit/a5516d70826a56075eb6d452ae5e2028d61cce7c#diff-6c1c5ff819fd1e7697d48e0098012117

The tests are passing with 5.3, 5.4, 5.5, 5.6, 7.0, 7.1, and 7.2: https://travis-ci.org/dxw/PHP-Cookie-Legacy

Due to the tests using php-fpm, and php-fpm being introduced in PHP 5.3, I wasn’t able to get the tests to work with PHP 5.2 (pull requests welcome).

I copied src/Cookie.php (without modification) into WordPress, and modified four lines in wp-includes/pluggable.php. See attached patch.

@tomdxw
3 years ago

#8 @tomdxw
3 years ago

  • Keywords dev-feedback added

#9 @pento
2 years ago

  • Milestone changed from 5.0 to 5.1

#10 @pento
23 months ago

  • Milestone changed from 5.1 to 5.2

#11 @desrosj
21 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.2 to 5.3

Patch needs to be refreshed to apply cleanly to trunk. 5.2 beta is in less than 2 days. Punting to 5.3.

#12 @ayeshrajans
19 months ago

I'd love to see this move forward too. Even though we commit to a more recent WordPress version, it wouldn't be PHP 7.3 right away, so for BC, we will need a polyfill.

I worked on a quite small scale polyfill (https://github.com/Ayesh/WordPress-SameSite/blob/50705de58a598c46f1ddd70faf6ccce59877b0cc/samesite.php#L172-L188) at just 16 lines and a single function. This function doesn't try to be a complete poyfill, but is a rather auth-cookie exclusive one.

I'd be happy to roll a patch with this smaller polyfill or the delight-im/PHP-Cookie package.

#13 @ayeshrajans
19 months ago

If anyone is interested, I created a plugin to do just that: https://wordpress.org/plugins/samesite

#14 @johnbillion
18 months ago

  • Milestone changed from 5.3 to Future Release

#15 @johnbillion
14 months ago

#48183 was marked as a duplicate.

#16 @mikeschroder
14 months ago

Wanted to be sure the information from #48183 made its way to this ticket.

Chrome is going to set SameSite=lax by default, targeted for Chrome 80:
https://www.chromestatus.com/feature/5088147346030592
https://www.chromium.org/updates/same-site

Looks like that will likely be first quarter next year:
https://www.chromestatus.com/features/schedule

Last edited 14 months ago by mikeschroder (previous) (diff)

#17 @johnbillion
14 months ago

  • Milestone changed from Future Release to 5.4

Moving to 5.4 based on the above.

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


13 months ago

#19 @Toru
11 months ago

Looks like that will likely be first quarter next year:

It is now scheduled for 4 Feb, 2020, which is way before the planned 5.4 date.

https://www.chromestatus.com/features/schedule
https://make.wordpress.org/core/2020/01/14/wordpress-5-4-planning-roundup/

@mikhailroot
10 months ago

Introduced filter wp_auth_cookie_same_site to be able to manage SameSite Auth cookies parameter.

#20 follow-up: @mikhailroot
10 months ago

By default Chrome will treat missing SameSite param as Lax, so most of users will be treated well out of the box. (https://web.dev/samesite-cookies-explained/)

If certain amount of users require to manage this part they will update to PHP 7.3.0 + (e.g. they might require to embed wp-admin or authenticated state of their site into some other one via iframe - they need to set it to None).

There's no big need to have polyfill code to run for everyone to try to support older php versions, which don't support new setcookie syntax which supports SameSite param.

That's why i came up with this simpler solution https://core.trac.wordpress.org/attachment/ticket/37000/pluggable.diff

#21 @davidbaumwald
10 months ago

  • Keywords needs-dev-note added
  • Milestone changed from 5.4 to Future Release

This ticket still needs a decision, and with 5.4 Beta 1 landing tomorrow, this is being moved to Future Release. If any maintainer or committer feels this can be included in 5.4 or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

Also marking this as needs-dev-note as the new filter needs at least a small call-out on the Misc Dev Note.

#22 follow-up: @adam320
8 months ago

Will the addition of the wp_auth_cookie_same_site filter solve the issue I am having with multisite domain mapping? Chrome no longer logs you into the front end of a subdomain network because of the SameSite issue. What is odd is that in firefox, the cookies currently are set flagged as SameSite=None; Secure, but in chrome they show as blocked and the flags are not set.

#23 in reply to: ↑ 22 @mikhailroot
8 months ago

The thing is that PHP only since 7.3 has ability to set SameSite, so first check which PHP version you are using. Chrome defaults to SameSite=Lax if it's not set. I've decided to add this filter because I consider it's better to give other developers more control of this setting if it gets set explicitly.
I personally have tested it with WordPress+Shopify sites which are on subdomains like wp.example.com - running WordPress and example.com - hosted by Shopify, and another site is example2.com running WP and shopify.example2.com - running shopify. All works smooth with SameSite=None - I needed to allow access to WordPress authenticated admin side to be used inside Shopify's App Iframe, without SameSite=None it would get blocked in Chrome. I consider if you have php 7.3+ and you've applied patch I've proposed it should work for you.

Replying to adam320:

Will the addition of the wp_auth_cookie_same_site filter solve the issue I am having with multisite domain mapping? Chrome no longer logs you into the front end of a subdomain network because of the SameSite issue. What is odd is that in firefox, the cookies currently are set flagged as SameSite=None; Secure, but in chrome they show as blocked and the flags are not set.

This ticket was mentioned in Slack in #core by steelwagstaff. View the logs.


8 months ago

#25 in reply to: ↑ 20 ; follow-up: @SteelWagstaff
8 months ago

Hi -- I'm the product manager for Pressbooks, an open source publishing tool built on WordPress multisite. We're in that small subset of users who need

to embed wp-admin or authenticated state of their site into some other one via iframe

as described by @mikhailroot. We do this when our tool is securely embedded within a learning management system using the LTI specification. In order for the LTI connection to work as expected in Chrome, we need to be able to set WP authorization cookies to SameSite=None. We're using PHP 7.3, which added an options array supporting the setting of SameSite attributes, but I don't know how we can set this value without forking core WordPress, which we're very reticent to do. Ideas/suggestions welcomed!

#26 in reply to: ↑ 25 ; follow-up: @ayeshrajans
8 months ago

I have released v1.4 of the plugin https://wordpress.org/plugins/samesite/ , that supports configurable SameSite values. You can add the following to wp-config.php file to override the browser-default (what browsers assume when SameSite flag is not set) and php.ini default:

define( 'WP_SAMESITE_COOKIE', 'Lax' );

Change "Lax" to "None" if you want to forcefully disable Chrome 80+ behavior of automatically assuming SameSite=Lax on its A/B test users.

#27 in reply to: ↑ 26 @adam320
8 months ago

Is there a way to set 'Secure' as well. All my research said that you need to set SameSite=None; Secure

#28 follow-up: @ayeshrajans
8 months ago

WordPress should pass the "secure" flag if the page is served with HTTPS. Setting "secure" flag over an HTTP connection will make browsers downright reject the cookie. Is your site is being served over HTTPS?

#29 in reply to: ↑ 28 @adam320
8 months ago

Ah, sweet, thanks for the explanation.

#30 @mikhailroot
8 months ago

I've created this plugin (works with PHP 7.3+ - for older versions it just informs you need to update PHP) which allows to set SameSite parameter in WP Admin https://github.com/MikhailRoot/samesite-cookie-manager
I use it till SameSite param management will be added to Core.
Hope it will help someone.

#31 @MadtownLems
3 months ago

I would like to add my support for hoping we can get a Core filter for the auth/logged-in cookies' samesite attribute. We are one of many networks out there that serve some front-ends from a different domain than the dashboard is served through, and our users are no longer treated as being logged in on the front-end.

This ticket was mentioned in Slack in #core by jonshipman. View the logs.


3 weeks ago

#33 @jonshipman
3 weeks ago

I suggest that there be a WordPress function that acts as a wrapper around setcookie that provides an action hook users can remove/add to. Something like a wp_setcookie that performs a 'wp_setcookie' do_action.

This ticket was mentioned in PR #687 on WordPress/wordpress-develop by jonshipman.


3 weeks ago

  • Keywords has-unit-tests added; needs-refresh removed

Created a function for wp_setcookie acting as a wrapper for an action called wp_setcookie which is hooked to a default action function named... _wp_setcookie. Lays a foundation to implement 37000. Doesn't outright fix the issue, but should retain existing functionality with little fuss while letting devs implement their own solutions in the interim.

https://core.trac.wordpress.org/ticket/37000

#35 @jonshipman
3 weeks ago

Just noticed that there is in fact a deprecated function called wp_setcookie. We could call it wp_setbiscuit.

This ticket was mentioned in Slack in #core-restapi by jonshipman. View the logs.


3 weeks ago

Note: See TracTickets for help on using tickets.