[RFC] SameSite cookie option #3398

pmmaga · 2018-07-19T19:39:11Z

This PR is an implementation of the winning vote in the samesite cookie RFC: https://wiki.php.net/rfc/same-site-cookie

This PR supersedes #2613.

Any feedback on the implementation is welcome.

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/

nikic · 2018-07-19T19:51:21Z

ext/session/session.c

+					samesite = Z_STR_P(value);
+					found++;
+				}
+			}


Should maybe error if key not string?

nikic · 2018-07-19T19:52:07Z

ext/session/session.c

+					convert_to_string_ex(lifetime);
+					found++;
+				} else if(!strcasecmp("path", ZSTR_VAL(key))) {
+					path = Z_STR_P(value);


You're assuming here that the path is a string. Either you need to explicitly check this, or use something like zval_get_string.

I've tried to use zval_get_string for all of these, however, I'm missing something as it is currently leaking on ext/session/tests/session_set_cookie_params_error.phpt.

AFAIK, you have to zend_string_release all strings obtained by using zval_get_string.

I think that they are all covered at the moment. The test uses the same variable for all the parameters in sequence. And I think that that's the cause of the leak (as it doesn't leak on any other test where the parameters are constants).

nikic · 2018-07-19T19:52:45Z

ext/session/session.c

+				} else if(!strcasecmp("samesite", ZSTR_VAL(key))) {
+					samesite = Z_STR_P(value);
+					found++;
+				}


Should an unknown option generate a warning?

I've added the warning and the error. Thanks

nikic · 2018-07-19T19:54:07Z

ext/standard/head.c

+	} ZEND_HASH_FOREACH_END();
+
+	/* Array is not empty but no valid keys were found */
+	if (found == 0 && Z_ARRVAL_P(options)->nNumUsed > 0) {


This should use zend_hash_num_elements() instead of nNumUsed.

nikic · 2018-07-19T19:55:28Z

ext/session/session.c

+				ZVAL_DEREF(value);
+				if(!strcasecmp("lifetime", ZSTR_VAL(key))) {
+					lifetime = value;
+					convert_to_string_ex(lifetime);


This is going to modify the array in-place. Either this should have a strict type check, or use zval_get_string (with appropriate refcount management for the other case).

pmmaga · 2018-07-20T22:10:55Z

I've fixed the pending issues and implemented the suggestions. Thanks for the feedback

nikic · 2018-07-22T20:22:19Z

ext/session/session.c

+					php_error_docref(NULL, E_WARNING, "Unrecognized key '%s' found in the options array", ZSTR_VAL(key));
+				}
+			} else {
+				php_error_docref(NULL, E_ERROR, "The options array must only use string keys");


This should also be just E_WARNING. E_ERROR is a fatal error.

Agreed, downgraded to warning.

nikic · 2018-07-22T20:23:21Z

ext/session/session.c

+			RETURN_FALSE;
+		}
+	} else {
+		convert_to_string_ex(lifetime_or_options);


The convert_to_string_ex call can be dropped if this uses zval_get_string.

Although unnecessary, I left it behind as the old code was modifying the variable in place.

The in-place modification is not externally visible though, so it's safe to drop it.

nikic · 2018-07-22T20:24:11Z

ext/session/session.c

 	}
+    if (samesite) {
+        ini_name = zend_string_init("session.cookie_samesite", sizeof("session.cookie_samesite") - 1, 0);


nit: Indentation

nikic · 2018-07-22T20:27:08Z

ext/standard/head.c

 		RETVAL_TRUE;
 	} else {
 		RETVAL_FALSE;
 	}
+
+cleanup:
+	if (path) {


Won't this cause double releases for the non-array codepath?

It would indeed, thanks

jvoisin · 2018-07-22T20:32:40Z

ext/session/session.c

+		Z_PARAM_STR(path)
+		Z_PARAM_STR(domain)
+		Z_PARAM_BOOL_EX(secure, secure_null, 1, 0)
+		Z_PARAM_BOOL_EX(httponly, httponly_null, 1, 0)


Don't we want to add SameSite here too?

Nope, samesite will only be supported in the new array forms of these functions, not the (now legacy) multi-argument variant.

jvoisin · 2018-07-22T20:39:49Z

ext/session/session.c

+					httponly_null = 0;
+					found++;
+				} else if(!strcasecmp("samesite", ZSTR_VAL(key))) {
+					samesite = zval_get_string(value);


Shouldn't we want to check that value is either Strict or Lax?

We could, but that falls a bit out of the scope here. No other values are sanitized at this moment. It would probably be a good proposal to validate all values (ie. check if the domain is actually a domain) but I'd keep that a separate discussion.

nikic · 2018-07-29T12:41:40Z

ext/standard/head.c

+	if (found == 0 && zend_hash_num_elements(Z_ARRVAL_P(options)) > 0) {
+		php_error_docref(NULL, E_WARNING, "No valid options were found in the given array");
+		return 0;
+	}


This warning seems a bit redundant. Isn't it only thrown if one of the above warnings already triggered?

While it's true that it will only trigger when one of the others also triggered, I think it has some usefulness to distinguish between some keys being invalid and all keys being invalid. OTOH, it does feel a bit redundant given that it will only show paired with one of the above. If we remove the warning, should we also remove the return 0;? As it stands, the cookie won't be set when this warning is shown.

nikic

Based on the internals discussion, I believe we have enough consensus to land this with the implementation proposed here.

Allows using an alternative array argument with support for the samesite option on the following functions: setcookie setrawcookie session_set_cookie_params

cmb69 · 2018-07-31T10:37:38Z

Okay, I'm going to merge this PR. However, we'll likely have to work on this as soon as possible, since currently it seems to be possible to pass additional parameters after the options array, e.g.:

<?php
session_set_cookie_params(array('path'=>'/foo/'), 'bar', 'www.example.com');
var_dump(session_get_cookie_params());

outputs:

array(6) {
  ["lifetime"]=>
  int(0)
  ["path"]=>
  string(5) "/foo/"
  ["domain"]=>
  string(15) "www.example.com"
  ["secure"]=>
  bool(false)
  ["httponly"]=>
  bool(false)
  ["samesite"]=>
  string(0) ""
}

php-pulls · 2018-07-31T10:51:27Z

Comment on behalf of cmb at php.net:

Thanks! Applied via 08b9310 and 2b58ab2.

frederikbosch · 2018-10-30T13:14:21Z

@pmmaga @cmb69 @nikic @jvoisin Thank you all for taking over the implementation of the RFC. I am happy that it landed after all in PHP 7.3. Great job!

jvoisin · 2018-10-30T13:44:23Z

For the record, the Snuffleupagus module provides support for the SameSite attribute for PHP7.0, 7.1 and 7.2 :)

Thank you for pushing this feature upstream ♥

implement same site cookie

487e1e1

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/

nikic reviewed Jul 19, 2018

View reviewed changes

ext/session/session.c Outdated

samesite = Z_STR_P(value);

found++;

}

}

Copy link

Member

nikic Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should maybe error if key not string?

nikic reviewed Jul 19, 2018

View reviewed changes

nikic added the PHP 7.3 Nominated label Jul 19, 2018

pmmaga force-pushed the samesite branch from 4d5c5b3 to bf440f9 Compare July 20, 2018 21:20

pmmaga changed the title ~~[WIP] [RFC] SameSite cookie option~~ [RFC] SameSite cookie option Jul 20, 2018

pmmaga force-pushed the samesite branch from bf440f9 to 425771b Compare July 21, 2018 20:58

nikic mentioned this pull request Jul 22, 2018

implement same site cookie #2613

Closed

nikic reviewed Jul 22, 2018

View reviewed changes

jvoisin reviewed Jul 22, 2018

View reviewed changes

pmmaga force-pushed the samesite branch from 425771b to a3fa4aa Compare July 23, 2018 16:44

nikic reviewed Jul 29, 2018

View reviewed changes

nikic approved these changes Jul 29, 2018

View reviewed changes

Support for samesite cookies with array syntax

f8b6c8a

Allows using an alternative array argument with support for the samesite option on the following functions: setcookie setrawcookie session_set_cookie_params

pmmaga force-pushed the samesite branch from a3fa4aa to f8b6c8a Compare July 29, 2018 13:59

php-pulls closed this Jul 31, 2018

NicolasCARPi mentioned this pull request Sep 20, 2018

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

Closed

[RFC] SameSite cookie option #3398

[RFC] SameSite cookie option #3398

Uh oh!

Conversation

pmmaga commented Jul 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmmaga commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

cmb69 commented Jul 31, 2018

Uh oh!

php-pulls commented Jul 31, 2018

Uh oh!

frederikbosch commented Oct 30, 2018

Uh oh!

jvoisin commented Oct 30, 2018

Uh oh!

Uh oh!