implement same site cookie #2613

frederikbosch · 2017-07-07T17:09:55Z

This is a PR of the original patch made in bug 72230.

See also

staabm · 2017-07-10T07:16:43Z

@krakjoe this looks more like a enhancement/feature then a bugfix to me.

frederikbosch · 2017-07-10T07:21:31Z

@staabm It definitely is. Would it require a RFC?

krakjoe · 2017-07-10T08:31:54Z

Okay, I read the patch ...

I think it would be best to get the RFC process started, if a clear consensus emerges from the discussion alone, then the voting may not be necessary. But we should at least attempt to have the discussion, and the best way to inform the discussion is a well written RFC ...

see https://bugs.php.net/bug.php?id=72230 see https://tools.ietf.org/html/draft-west-first-party-cookies-07 see https://scotthelme.co.uk/csrf-is-dead/

frederikbosch · 2017-07-10T08:42:56Z

I squashed the commits, since the lights should be green by now.
@krakjoe I will write the RFC.

paragonie-scott · 2017-07-10T16:08:01Z

This gets a big 👍 from me, although I can't vote on RFCs. :)

pmmaga · 2017-07-11T12:58:48Z

This sounds like a good idea, but probably we should wait for the proposal to be formally accepted before bringing it into the language?

frederikbosch · 2017-07-11T13:01:22Z

@pmmaga I received RFC access yesterday. I will prioritize the writing of the document. Hope to finish this or next week.

pmmaga · 2017-07-11T13:13:12Z

@frederikbosch sorry, I was unclear, I meant the actual document that proposes the standard: http://httpwg.org/http-extensions/draft-ietf-httpbis-cookie-same-site.html

frederikbosch · 2017-07-11T13:15:20Z

@pmmaga I will include that opinion in the cons of the RFC. If we implement before it is an actual standard, there is a risk we might end-up with an API that becomes useless in the future.

frederikbosch · 2017-07-16T19:45:15Z

Link to the RFC: https://wiki.php.net/rfc/same-site-cookie. Still in draft, will finish in upcoming days and send to mailing list.

blar · 2017-07-23T21:37:46Z

Pull request for http_cookie_set is available at #2643. I wait for access to create a RFC for that. This function allows you to add your own option and add the required code in the source for:

http_cookie_set('foo', 'bar', [
    'SameSite' => 'Strict'
]);

frederikbosch · 2017-07-24T08:59:55Z

RFC linked to this PR is updated. It now contains two possible solutions.

As in this PR: additional argument to setcookie, setrawcookie and session_set_cookie_params
Allow an array of options for setcookie, setrawcookie and session_set_cookie_params

When both solutions will be rejected, the floor will be completely open for the proposal of http_cookie_set/remove by @blar since we then investigated all the possible solutions to the current set of functions.

Voting will be opened at August 1st.
RFC: https://wiki.php.net/rfc/same-site-cookie

sgolemon · 2017-08-25T23:02:37Z

ext/session/session.c

@@ -1724,6 +1730,12 @@ static PHP_FUNCTION(session_set_cookie_params)
 		zend_string_release(ini_name);
 	}

+    if (argc > 5) {


if (samesite) {

Testing the var is more readable and less likely to get screwed up because of some silly off-by-one error later on.

sgolemon · 2017-08-25T23:03:46Z

ext/session/session.c

@@ -1724,6 +1730,12 @@ static PHP_FUNCTION(session_set_cookie_params)
 		zend_string_release(ini_name);
 	}

+    if (argc > 5) {
+        ini_name = zend_string_init("session.cookie_samesite", sizeof("session.cookie_samesite") - 1, 0);
+        zend_alter_ini_entry(ini_name, samesite, PHP_INI_USER, PHP_INI_STAGE_RUNTIME);


In fact... ugh. Are these parameters really all just wrappers for ini_set()? Let's stop that madness and just let people use ini_set(). This is a silly function.

Too bad that we decided on 2017-10-10 to alternatively allow to pass an option array to this function. Anyhow, I think session_set_cookie_params() and maybe session_get_cookie_params() might be worthwhile additions to https://wiki.php.net/rfc/deprecations_php_7_4.

@cmb69 feel free to add anything you please to the RFC, its an open WIP

staabm · 2017-09-01T09:09:27Z

php.ini-production

+; Add SameSite attribute to cookie to help mitigate Cross-Site Request Forgery (CSRF/XSRF)
+; Current valid values are "Lax" or "Strict"
+; https://tools.ietf.org/html/draft-west-first-party-cookies-07
+session.cookie_samesite =


should we be more strict per default with the default cookie settings, to be more secure per default (e.g. with php8)?

I think that would be a good idea. At least it deserves an RFC.

kelunik · 2018-01-10T16:54:08Z

What's the status here? I just set the RFC's status to Accepted instead of Voting.

frederikbosch · 2018-01-10T16:57:39Z

@kelunik I planned to write an update to the PR this month.

halaei · 2018-01-12T14:17:12Z

Regarding spectre attack and suggested mitigations should this be considered as a critical security fix?

frederikbosch · 2018-01-12T14:23:28Z

@halaei I was not aware that same-site cookie was suggested as spectre fix. If the language would decide this should be considered a critical fix for spectre, then I would be willing to devote time to it really soon. If not, I will take time necessary to make sure it is there (as currently planned) when PHP 7.3 will be released.

halaei · 2018-01-12T14:32:02Z

@frederikbosch I don't know how serious this is. I also asked the same question in an open PR for symfony/symfony to use header() function instead of setcookie() and setrawcookie().

jabcreations · 2018-05-02T21:57:46Z

What is the current status for implementing samesite flag support?

frederikbosch · 2018-05-02T21:58:58Z

Still being worked on.

frederikbosch · 2018-05-10T14:55:46Z

Can someone help me compiling/building PHP SRC? I keep getting the same errors. I am using the following Dockerfile. It is based on the official Docker for PHP7.2 CLI with Alpine 7.3.

I keep getting the following error, no matter what I try.

Error relocating /usr/src/php/sapi/cli/php: unsupported relocation type 37
Error relocating /usr/src/php/sapi/cli/php: unsupported relocation type 37
Error relocating /usr/src/php/sapi/cli/php: unsupported relocation type 37
Error relocating /usr/src/php/sapi/cli/php: unsupported relocation type 37

I have no idea where to start.

krakjoe · 2018-06-20T09:20:22Z

Can we get an update on the status here please ?

frederikbosch · 2018-06-20T09:28:35Z

@krakjoe I am not getting tests working as I indicated earlier. I would love to work on this, especially now PHP 7.3 is coming closer, but I need some help with running tests.

Is there some container that can be used to get tests running? I don't care if it Alpine, Stretch, Ubuntu etc.

There is also a bug report for this by @petk. See https://bugs.php.net/bug.php?id=76392.

remicollet · 2018-06-29T07:00:24Z

With PHP < 7.3 some people use a hack to add the samesite option (path="xxx; samesite=..."), but this doesn't work anymore in 7.3 ad the values are filtered. So we really need this to be implemented.

cmb69 · 2018-07-03T16:06:24Z

@frederikbosch Wrt. the Alpine build errors, try with PR #3349.

nikic · 2018-07-07T10:02:22Z

PR #3349 is merged now, so hopefully you should no longer be seeing issues with musl-based docker images.

KalleZ · 2018-07-07T10:14:23Z

I think we are getting to a point where the number of parameters for setcookie() is getting massive, perhaps we could add something an array of options or similar, as it is already fairlying annoying to pass a long list of arguments to this function.

nikic · 2018-07-07T10:15:55Z

@KalleZ That's exactly what has been accepted as part of RFC https://wiki.php.net/rfc/same-site-cookie :)

KalleZ · 2018-07-07T10:17:45Z

@nikic ah thanks for the link! I must have missed that part, so its great.

jvoisin · 2018-07-13T14:06:22Z

ext/standard/head.c

@@ -121,6 +120,9 @@ PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires,
 	if (domain) {
 		len += ZSTR_LEN(domain);
 	}
+	if (samesite) {


Shouldn't you check that the samesite value is either Strict or Lax?

Note that samesite may also be NULL, here.

nikic · 2018-07-17T09:21:21Z

If @frederikbosch doesn't have time it would be great if someone else picks this up, due to the issue that @remicollet mentioned. (I don't have time myself.)

frederikbosch · 2018-07-17T09:29:51Z

I think @nikic is right. I am really disappointed that I still haven't found time to implement it. But at the moment I simply cannot find the time to spend days on it. For me that is what it takes, because my experience with C is very limited.

My original PR was based on a patch made by someone else (acknowledged in the RFC) and that was done in limited time. Now that the outcome of the RFC is that the methods should accept both array and multiple arguments, I will have to do some research first to find out how to do it. This will take me a lot of time, which I don't have at the moment. I am really sorry. I hope someone is able to do it. If not, I am still hoping to find time to do it before feature freeze of PHP 7.3.

cmb69 · 2018-07-17T22:38:22Z

ext/standard/head.c

@@ -121,6 +120,9 @@ PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires,
 	if (domain) {
 		len += ZSTR_LEN(domain);
 	}
+	if (samesite) {
+		len += ZSTR_LEN(samesite);
+	}

 	cookie = emalloc(len + 100);


This is smelly! Apparently, we're not calculating the correct length. If 100 additional bytes (why 100?) won't suffice, we'd have problem. Unless we're able to calculate the exact length (or to show that 100 is always sufficient), refactoring to the smart_str or smart_string API might be sensible.

Yeah this seems odd indeed, I think it should be handled outside the scope of this PR tho

nikic · 2018-07-22T15:44:37Z

Closing this PR in favor of #3398, which implements the new format using arrays. There is still some uncertainty around implementation details, ref https://externals.io/message/100304#102909 and following.

krakjoe added the Bug label Jul 8, 2017

krakjoe removed the Bug label Jul 10, 2017

krakjoe added the RFC label Jul 10, 2017

implement same site cookie

39baff4

see https://bugs.php.net/bug.php?id=72230 see https://tools.ietf.org/html/draft-west-first-party-cookies-07 see https://scotthelme.co.uk/csrf-is-dead/

frederikbosch force-pushed the cookie-samesite branch from ff0e7c4 to 39baff4 Compare July 10, 2017 08:41

sgolemon reviewed Aug 25, 2017

View reviewed changes

staabm reviewed Sep 1, 2017

View reviewed changes

staabm mentioned this pull request Sep 18, 2017

SameSite cookies im Backend verwenden redaxo/redaxo#1209

Closed

NicolasCARPi mentioned this pull request Dec 7, 2017

Add SameSite to cookies to prevent CSRF elabftw/elabftw#394

Closed

nikic added the PHP 7.3 Nominated label Jul 5, 2018

jvoisin reviewed Jul 13, 2018

View reviewed changes

cmb69 reviewed Jul 17, 2018

View reviewed changes

pmmaga mentioned this pull request Jul 19, 2018

[RFC] SameSite cookie option #3398

Closed

nikic closed this Jul 22, 2018

implement same site cookie #2613

implement same site cookie #2613

Conversation

frederikbosch commented Jul 7, 2017

staabm commented Jul 10, 2017

frederikbosch commented Jul 10, 2017

krakjoe commented Jul 10, 2017 • edited Loading

frederikbosch commented Jul 10, 2017

paragonie-scott commented Jul 10, 2017

pmmaga commented Jul 11, 2017

frederikbosch commented Jul 11, 2017

pmmaga commented Jul 11, 2017

frederikbosch commented Jul 11, 2017

frederikbosch commented Jul 16, 2017

blar commented Jul 23, 2017

frederikbosch commented Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelunik commented Jan 10, 2018

frederikbosch commented Jan 10, 2018

halaei commented Jan 12, 2018

frederikbosch commented Jan 12, 2018

halaei commented Jan 12, 2018

jabcreations commented May 2, 2018

frederikbosch commented May 2, 2018

frederikbosch commented May 10, 2018

krakjoe commented Jun 20, 2018

frederikbosch commented Jun 20, 2018

remicollet commented Jun 29, 2018

cmb69 commented Jul 3, 2018

nikic commented Jul 7, 2018

KalleZ commented Jul 7, 2018

nikic commented Jul 7, 2018

KalleZ commented Jul 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Jul 17, 2018

frederikbosch commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Jul 22, 2018

krakjoe commented Jul 10, 2017 •

edited

Loading

frederikbosch commented Jul 24, 2017 •

edited

Loading