From 6d3e4784f1133e31a8207b4d41d1061299afb611 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Tue, 7 Jul 2020 18:59:05 +0200 Subject: [PATCH 1/3] Warning to error promotion for set(raw)cookie() --- ext/standard/head.c | 128 +++++++----------- ext/standard/tests/network/bug69523.phpt | 10 +- ext/standard/tests/network/bug69948.phpt | 24 ++-- .../network/setcookie_array_option_error.phpt | 61 +++++++++ .../tests/network/setcookie_error.phpt | 58 ++++---- .../tests/network/setrawcookie_error.phpt | 48 +++++++ 6 files changed, 213 insertions(+), 116 deletions(-) create mode 100644 ext/standard/tests/network/setcookie_array_option_error.phpt create mode 100644 ext/standard/tests/network/setrawcookie_error.phpt diff --git a/ext/standard/head.c b/ext/standard/head.c index ceca0cbbefcff..c7aff4e2b3518 100644 --- a/ext/standard/head.c +++ b/ext/standard/head.c @@ -83,30 +83,6 @@ PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires, int result; smart_str buf = {0}; - if (!ZSTR_LEN(name)) { - zend_error( E_WARNING, "Cookie names must not be empty" ); - return FAILURE; - } else if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_error(E_WARNING, "Cookie names cannot contain any of the following '=,; \\t\\r\\n\\013\\014'" ); - return FAILURE; - } - - if (!url_encode && value && - strpbrk(ZSTR_VAL(value), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_error(E_WARNING, "Cookie values cannot contain any of the following ',; \\t\\r\\n\\013\\014'" ); - return FAILURE; - } - - if (path && strpbrk(ZSTR_VAL(path), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_error(E_WARNING, "Cookie paths cannot contain any of the following ',; \\t\\r\\n\\013\\014'" ); - return FAILURE; - } - - if (domain && strpbrk(ZSTR_VAL(domain), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_error(E_WARNING, "Cookie domains cannot contain any of the following ',; \\t\\r\\n\\013\\014'" ); - return FAILURE; - } - if (value == NULL || ZSTR_LEN(value) == 0) { /* * MSIE doesn't delete a cookie when you set it to a null value @@ -225,10 +201,10 @@ static void php_head_parse_cookie_options_array(zval *options, zend_long *expire } } -/* {{{ setcookie(string name [, string value [, array options]]) - Send a cookie */ -PHP_FUNCTION(setcookie) +#define ILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", and \"\\014\"" +static void php_setcookie_common(INTERNAL_FUNCTION_PARAMETERS, bool is_raw) { + /* to handle overloaded function array|int */ zval *expires_or_options = NULL; zend_string *name, *value = NULL, *path = NULL, *domain = NULL, *samesite = NULL; zend_long expires = 0; @@ -248,17 +224,53 @@ PHP_FUNCTION(setcookie) if (expires_or_options) { if (Z_TYPE_P(expires_or_options) == IS_ARRAY) { if (UNEXPECTED(ZEND_NUM_ARGS() > 3)) { - php_error_docref(NULL, E_WARNING, "Cannot pass arguments after the options array"); - RETURN_FALSE; + zend_argument_count_error("%s(): Expects exactly 3 arguments when argument #3 " + "($expires_or_options) is an array", get_active_function_name()); + RETURN_THROWS(); } php_head_parse_cookie_options_array(expires_or_options, &expires, &path, &domain, &secure, &httponly, &samesite); + if (path && strpbrk(ZSTR_VAL(path), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_value_error("%s(): Argument #3 ($expires_or_options[\"path\"]) cannot contain " + ILLEGAL_COOKIE_CHARACTER, get_active_function_name()); + goto cleanup; + RETURN_THROWS(); + } + if (domain && strpbrk(ZSTR_VAL(domain), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_value_error("%s(): Argument #3 ($expires_or_options[\"domain\"]) cannot contain " + ILLEGAL_COOKIE_CHARACTER, get_active_function_name()); + goto cleanup; + RETURN_THROWS(); + } + /* Should check value of SameSite? */ } else { expires = zval_get_long(expires_or_options); + if (path && strpbrk(ZSTR_VAL(path), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_argument_value_error(4, "cannot contain " ILLEGAL_COOKIE_CHARACTER); + RETURN_THROWS(); + } + if (domain && strpbrk(ZSTR_VAL(domain), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_argument_value_error(5, "cannot contain " ILLEGAL_COOKIE_CHARACTER); + RETURN_THROWS(); + } } } + if (!ZSTR_LEN(name)) { + zend_argument_value_error(1, "cannot be empty"); + RETURN_THROWS(); + } + if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_argument_value_error(1, "cannot contain \"=\", " ILLEGAL_COOKIE_CHARACTER); + RETURN_THROWS(); + } + if (is_raw && value && + strpbrk(ZSTR_VAL(value), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_argument_value_error(2, "cannot contain " ILLEGAL_COOKIE_CHARACTER); + RETURN_THROWS(); + } + if (!EG(exception)) { - if (php_setcookie(name, value, expires, path, domain, secure, httponly, samesite, 1) == SUCCESS) { + if (php_setcookie(name, value, expires, path, domain, secure, httponly, samesite, !is_raw) == SUCCESS) { RETVAL_TRUE; } else { RETVAL_FALSE; @@ -266,6 +278,7 @@ PHP_FUNCTION(setcookie) } if (expires_or_options && Z_TYPE_P(expires_or_options) == IS_ARRAY) { + cleanup: if (path) { zend_string_release(path); } @@ -277,59 +290,20 @@ PHP_FUNCTION(setcookie) } } } + +/* {{{ setcookie(string name [, string value [, array options]]) + Send a cookie */ +PHP_FUNCTION(setcookie) +{ + php_setcookie_common(INTERNAL_FUNCTION_PARAM_PASSTHRU, false); +} /* }}} */ /* {{{ setrawcookie(string name [, string value [, array options]]) Send a cookie with no url encoding of the value */ PHP_FUNCTION(setrawcookie) { - zval *expires_or_options = NULL; - zend_string *name, *value = NULL, *path = NULL, *domain = NULL, *samesite = NULL; - zend_long expires = 0; - zend_bool secure = 0, httponly = 0; - - ZEND_PARSE_PARAMETERS_START(1, 7) - Z_PARAM_STR(name) - Z_PARAM_OPTIONAL - Z_PARAM_STR(value) - Z_PARAM_ZVAL(expires_or_options) - Z_PARAM_STR(path) - Z_PARAM_STR(domain) - Z_PARAM_BOOL(secure) - Z_PARAM_BOOL(httponly) - ZEND_PARSE_PARAMETERS_END(); - - if (expires_or_options) { - if (Z_TYPE_P(expires_or_options) == IS_ARRAY) { - if (UNEXPECTED(ZEND_NUM_ARGS() > 3)) { - php_error_docref(NULL, E_WARNING, "Cannot pass arguments after the options array"); - RETURN_FALSE; - } - php_head_parse_cookie_options_array(expires_or_options, &expires, &path, &domain, &secure, &httponly, &samesite); - } else { - expires = zval_get_long(expires_or_options); - } - } - - if (!EG(exception)) { - if (php_setcookie(name, value, expires, path, domain, secure, httponly, samesite, 0) == SUCCESS) { - RETVAL_TRUE; - } else { - RETVAL_FALSE; - } - } - - if (expires_or_options && Z_TYPE_P(expires_or_options) == IS_ARRAY) { - if (path) { - zend_string_release(path); - } - if (domain) { - zend_string_release(domain); - } - if (samesite) { - zend_string_release(samesite); - } - } + php_setcookie_common(INTERNAL_FUNCTION_PARAM_PASSTHRU, true); } /* }}} */ diff --git a/ext/standard/tests/network/bug69523.phpt b/ext/standard/tests/network/bug69523.phpt index 979ae00d179a7..60f3643044c60 100644 --- a/ext/standard/tests/network/bug69523.phpt +++ b/ext/standard/tests/network/bug69523.phpt @@ -2,7 +2,11 @@ setcookie() allows empty cookie name --FILE-- getMessage() . \PHP_EOL; +} ?> ---EXPECTF-- -Warning: Cookie names must not be empty in %s on line %d +--EXPECT-- +setcookie(): Argument #1 ($name) cannot be empty diff --git a/ext/standard/tests/network/bug69948.phpt b/ext/standard/tests/network/bug69948.phpt index 957d72f99d23a..914b1d4286e56 100644 --- a/ext/standard/tests/network/bug69948.phpt +++ b/ext/standard/tests/network/bug69948.phpt @@ -2,17 +2,21 @@ Bug #69948 (path/domain are not sanitized for special characters in setcookie) --FILE-- getMessage() . \PHP_EOL; +} +try { + var_dump(setcookie('foo', 'bar', 0, '/', 'foobar; secure')); +} catch (\ValueError $e) { + echo $e->getMessage() . \PHP_EOL; +} + ?> ===DONE=== --EXPECTHEADERS-- ---EXPECTF-- -Warning: Cookie paths cannot contain any of the following ',; \t\r\n\013\014' in %s on line %d - -Warning: Cookie domains cannot contain any of the following ',; \t\r\n\013\014' in %s on line %d -bool(false) -bool(false) +--EXPECT-- +setcookie(): Argument #4 ($path) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): Argument #5 ($domain) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" ===DONE=== diff --git a/ext/standard/tests/network/setcookie_array_option_error.phpt b/ext/standard/tests/network/setcookie_array_option_error.phpt new file mode 100644 index 0000000000000..5b5513c9057b0 --- /dev/null +++ b/ext/standard/tests/network/setcookie_array_option_error.phpt @@ -0,0 +1,61 @@ +--TEST-- +setcookie() array variant error tests +--INI-- +date.timezone=UTC +--FILE-- + 'only']); +// Numeric key and no valid keys +setcookie('name2', 'value2', [0 => 'numeric_key']); +// Unrecognized key +setcookie('name3', 'value3', ['path' => '/path/', 'foo' => 'bar']); +// Invalid path key content +try { + setcookie('name', 'value', ['path' => '/;/']); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} +// Invalid domain key content +try { + setcookie('name', 'value', ['path' => '/path/', 'domain' => 'ba;r']); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} + +// Arguments after options array (will not be set) +try { + setcookie('name4', 'value4', [], "path", "domain.tld", true, true); +} catch (\ArgumentCountError $e) { + echo $e->getMessage() . "\n"; +} + +var_dump(headers_list()); +--EXPECTHEADERS-- + +--EXPECTF-- +Warning: setcookie(): Unrecognized key 'unknown_key' found in the options array in %s + +Warning: setcookie(): No valid options were found in the given array in %s + +Warning: setcookie(): Numeric key found in the options array in %s + +Warning: setcookie(): No valid options were found in the given array in %s + +Warning: setcookie(): Unrecognized key 'foo' found in the options array in %s +setcookie(): Argument #3 ($expires_or_options["path"]) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): Argument #3 ($expires_or_options["domain"]) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): Expects exactly 3 arguments when argument #3 ($expires_or_options) is an array +array(4) { + [0]=> + string(%d) "X-Powered-By: PHP/%s" + [1]=> + string(22) "Set-Cookie: name=value" + [2]=> + string(24) "Set-Cookie: name2=value2" + [3]=> + string(37) "Set-Cookie: name3=value3; path=/path/" +} diff --git a/ext/standard/tests/network/setcookie_error.phpt b/ext/standard/tests/network/setcookie_error.phpt index 98fb64b8512f7..0a92c3badda28 100644 --- a/ext/standard/tests/network/setcookie_error.phpt +++ b/ext/standard/tests/network/setcookie_error.phpt @@ -1,5 +1,5 @@ --TEST-- -setcookie() array variant error tests +setcookie() error tests --INI-- date.timezone=UTC --FILE-- @@ -7,37 +7,43 @@ date.timezone=UTC ob_start(); -// Unrecognized key and no valid keys -setcookie('name', 'value', ['unknown_key' => 'only']); -// Numeric key and no valid keys -setcookie('name2', 'value2', [0 => 'numeric_key']); -// Unrecognized key -setcookie('name3', 'value3', ['path' => '/path/', 'foo' => 'bar']); -// Arguments after options array (will not be set) -setcookie('name4', 'value4', [], "path", "domain.tld", true, true); +try { + setcookie(''); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} +try { + setcookie('invalid='); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} +try { + setcookie('name', 'invalid;'); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} +try { + setcookie('name', 'value', 100, 'invalid;'); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} +try { + setcookie('name', 'value', 100, 'path', 'invalid;'); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} var_dump(headers_list()); --EXPECTHEADERS-- --EXPECTF-- -Warning: setcookie(): Unrecognized key 'unknown_key' found in the options array in %s - -Warning: setcookie(): No valid options were found in the given array in %s - -Warning: setcookie(): Numeric key found in the options array in %s - -Warning: setcookie(): No valid options were found in the given array in %s - -Warning: setcookie(): Unrecognized key 'foo' found in the options array in %s - -Warning: setcookie(): Cannot pass arguments after the options array in %s -array(4) { +setcookie(): Argument #1 ($name) cannot be empty +setcookie(): Argument #1 ($name) cannot contain "=", ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): Argument #4 ($path) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): Argument #5 ($domain) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +array(2) { [0]=> string(%d) "X-Powered-By: PHP/%s" [1]=> - string(22) "Set-Cookie: name=value" - [2]=> - string(24) "Set-Cookie: name2=value2" - [3]=> - string(37) "Set-Cookie: name3=value3; path=/path/" + string(27) "Set-Cookie: name=invalid%3B" } diff --git a/ext/standard/tests/network/setrawcookie_error.phpt b/ext/standard/tests/network/setrawcookie_error.phpt new file mode 100644 index 0000000000000..387487a274e66 --- /dev/null +++ b/ext/standard/tests/network/setrawcookie_error.phpt @@ -0,0 +1,48 @@ +--TEST-- +setrawcookie() error tests +--INI-- +date.timezone=UTC +--FILE-- +getMessage() . "\n"; +} +try { + setrawcookie('invalid='); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} +try { + setrawcookie('name', 'invalid;'); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} +try { + setrawcookie('name', 'value', 100, 'invalid;'); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} +try { + setrawcookie('name', 'value', 100, 'path', 'invalid;'); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} + +var_dump(headers_list()); +--EXPECTHEADERS-- + +--EXPECTF-- +setrawcookie(): Argument #1 ($name) cannot be empty +setrawcookie(): Argument #1 ($name) cannot contain "=", ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setrawcookie(): Argument #2 ($value) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setrawcookie(): Argument #4 ($path) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setrawcookie(): Argument #5 ($domain) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +array(1) { + [0]=> + string(%d) "X-Powered-By: PHP/%s" +} From 4a214861f5a659e23b0b5d0f541a796ee31964d6 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 4 Sep 2020 16:14:28 +0200 Subject: [PATCH 2/3] Reviews --- ext/standard/head.c | 76 +++++++++---------- ext/standard/head.h | 4 +- ext/standard/tests/network/bug69948.phpt | 4 +- .../network/setcookie_array_option_error.phpt | 12 ++- .../tests/network/setcookie_error.phpt | 11 ++- .../tests/network/setrawcookie_error.phpt | 11 ++- 6 files changed, 68 insertions(+), 50 deletions(-) diff --git a/ext/standard/head.c b/ext/standard/head.c index c7aff4e2b3518..f430273d6ec4e 100644 --- a/ext/standard/head.c +++ b/ext/standard/head.c @@ -76,13 +76,42 @@ PHPAPI int php_header(void) } } -PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires, zend_string *path, zend_string *domain, int secure, int httponly, zend_string *samesite, int url_encode) +#define ILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", and \"\\014\"" +PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t expires, + zend_string *path, zend_string *domain, bool secure, bool httponly, + zend_string *samesite, bool url_encode) { zend_string *dt; sapi_header_line ctr = {0}; - int result; + zend_result result; smart_str buf = {0}; + if (!ZSTR_LEN(name)) { + zend_argument_value_error(1, "cannot be empty"); + return FAILURE; + } + if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_argument_value_error(1, "cannot contain \"=\", " ILLEGAL_COOKIE_CHARACTER); + return FAILURE; + } + if (!url_encode && value && + strpbrk(ZSTR_VAL(value), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_argument_value_error(2, "cannot contain " ILLEGAL_COOKIE_CHARACTER); + return FAILURE; + } + + if (path && strpbrk(ZSTR_VAL(path), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_value_error("%s(): \"path\" option cannot contain " ILLEGAL_COOKIE_CHARACTER, + get_active_function_name()); + return FAILURE; + } + if (domain && strpbrk(ZSTR_VAL(domain), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ + zend_value_error("%s(): \"domain\" option cannot contain " ILLEGAL_COOKIE_CHARACTER, + get_active_function_name()); + return FAILURE; + } + /* Should check value of SameSite? */ + if (value == NULL || ZSTR_LEN(value) == 0) { /* * MSIE doesn't delete a cookie when you set it to a null value @@ -118,7 +147,8 @@ PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires, if (!p || *(p + 5) != ' ') { zend_string_free(dt); smart_str_free(&buf); - zend_error(E_WARNING, "Expiry date cannot have a year greater than 9999"); + zend_value_error("%s(): \"expires\" option cannot have a year greater than 9999", + get_active_function_name()); return FAILURE; } @@ -201,7 +231,6 @@ static void php_head_parse_cookie_options_array(zval *options, zend_long *expire } } -#define ILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", and \"\\014\"" static void php_setcookie_common(INTERNAL_FUNCTION_PARAMETERS, bool is_raw) { /* to handle overloaded function array|int */ @@ -228,47 +257,13 @@ static void php_setcookie_common(INTERNAL_FUNCTION_PARAMETERS, bool is_raw) "($expires_or_options) is an array", get_active_function_name()); RETURN_THROWS(); } - php_head_parse_cookie_options_array(expires_or_options, &expires, &path, &domain, &secure, &httponly, &samesite); - if (path && strpbrk(ZSTR_VAL(path), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_value_error("%s(): Argument #3 ($expires_or_options[\"path\"]) cannot contain " - ILLEGAL_COOKIE_CHARACTER, get_active_function_name()); - goto cleanup; - RETURN_THROWS(); - } - if (domain && strpbrk(ZSTR_VAL(domain), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_value_error("%s(): Argument #3 ($expires_or_options[\"domain\"]) cannot contain " - ILLEGAL_COOKIE_CHARACTER, get_active_function_name()); - goto cleanup; - RETURN_THROWS(); - } - /* Should check value of SameSite? */ + php_head_parse_cookie_options_array(expires_or_options, &expires, &path, + &domain, &secure, &httponly, &samesite); } else { expires = zval_get_long(expires_or_options); - if (path && strpbrk(ZSTR_VAL(path), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_argument_value_error(4, "cannot contain " ILLEGAL_COOKIE_CHARACTER); - RETURN_THROWS(); - } - if (domain && strpbrk(ZSTR_VAL(domain), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_argument_value_error(5, "cannot contain " ILLEGAL_COOKIE_CHARACTER); - RETURN_THROWS(); - } } } - if (!ZSTR_LEN(name)) { - zend_argument_value_error(1, "cannot be empty"); - RETURN_THROWS(); - } - if (strpbrk(ZSTR_VAL(name), "=,; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_argument_value_error(1, "cannot contain \"=\", " ILLEGAL_COOKIE_CHARACTER); - RETURN_THROWS(); - } - if (is_raw && value && - strpbrk(ZSTR_VAL(value), ",; \t\r\n\013\014") != NULL) { /* man isspace for \013 and \014 */ - zend_argument_value_error(2, "cannot contain " ILLEGAL_COOKIE_CHARACTER); - RETURN_THROWS(); - } - if (!EG(exception)) { if (php_setcookie(name, value, expires, path, domain, secure, httponly, samesite, !is_raw) == SUCCESS) { RETVAL_TRUE; @@ -278,7 +273,6 @@ static void php_setcookie_common(INTERNAL_FUNCTION_PARAMETERS, bool is_raw) } if (expires_or_options && Z_TYPE_P(expires_or_options) == IS_ARRAY) { - cleanup: if (path) { zend_string_release(path); } diff --git a/ext/standard/head.h b/ext/standard/head.h index a972ebcf3d5df..6f44dbfe9f7b5 100644 --- a/ext/standard/head.h +++ b/ext/standard/head.h @@ -28,6 +28,8 @@ extern PHP_RINIT_FUNCTION(head); PHPAPI int php_header(void); -PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires, zend_string *path, zend_string *domain, int secure, int httponly, zend_string *samesite, int url_encode); +PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t expires, + zend_string *path, zend_string *domain, bool secure, bool httponly, + zend_string *samesite, bool url_encode); #endif diff --git a/ext/standard/tests/network/bug69948.phpt b/ext/standard/tests/network/bug69948.phpt index 914b1d4286e56..33fe2dd130649 100644 --- a/ext/standard/tests/network/bug69948.phpt +++ b/ext/standard/tests/network/bug69948.phpt @@ -17,6 +17,6 @@ try { ===DONE=== --EXPECTHEADERS-- --EXPECT-- -setcookie(): Argument #4 ($path) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" -setcookie(): Argument #5 ($domain) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): "path" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): "domain" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" ===DONE=== diff --git a/ext/standard/tests/network/setcookie_array_option_error.phpt b/ext/standard/tests/network/setcookie_array_option_error.phpt index 5b5513c9057b0..d25fd2f526ad5 100644 --- a/ext/standard/tests/network/setcookie_array_option_error.phpt +++ b/ext/standard/tests/network/setcookie_array_option_error.phpt @@ -13,6 +13,13 @@ setcookie('name', 'value', ['unknown_key' => 'only']); setcookie('name2', 'value2', [0 => 'numeric_key']); // Unrecognized key setcookie('name3', 'value3', ['path' => '/path/', 'foo' => 'bar']); +// Invalid expiration date +// To go above year 9999: 60 * 60 * 24 * 365 * 9999 +try { + setcookie('name', 'value', ['expires' => 315328464000]); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} // Invalid path key content try { setcookie('name', 'value', ['path' => '/;/']); @@ -46,8 +53,9 @@ Warning: setcookie(): Numeric key found in the options array in %s Warning: setcookie(): No valid options were found in the given array in %s Warning: setcookie(): Unrecognized key 'foo' found in the options array in %s -setcookie(): Argument #3 ($expires_or_options["path"]) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" -setcookie(): Argument #3 ($expires_or_options["domain"]) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): "expires" option cannot have a year greater than 9999 +setcookie(): "path" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): "domain" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" setcookie(): Expects exactly 3 arguments when argument #3 ($expires_or_options) is an array array(4) { [0]=> diff --git a/ext/standard/tests/network/setcookie_error.phpt b/ext/standard/tests/network/setcookie_error.phpt index 0a92c3badda28..813a373119dbf 100644 --- a/ext/standard/tests/network/setcookie_error.phpt +++ b/ext/standard/tests/network/setcookie_error.phpt @@ -22,6 +22,12 @@ try { } catch (\ValueError $e) { echo $e->getMessage() . "\n"; } +// To go above year 9999: 60 * 60 * 24 * 365 * 9999 +try { + setcookie('name', 'value', 315328464000); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} try { setcookie('name', 'value', 100, 'invalid;'); } catch (\ValueError $e) { @@ -39,8 +45,9 @@ var_dump(headers_list()); --EXPECTF-- setcookie(): Argument #1 ($name) cannot be empty setcookie(): Argument #1 ($name) cannot contain "=", ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" -setcookie(): Argument #4 ($path) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" -setcookie(): Argument #5 ($domain) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): "expires" option cannot have a year greater than 9999 +setcookie(): "path" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): "domain" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" array(2) { [0]=> string(%d) "X-Powered-By: PHP/%s" diff --git a/ext/standard/tests/network/setrawcookie_error.phpt b/ext/standard/tests/network/setrawcookie_error.phpt index 387487a274e66..044e06a934488 100644 --- a/ext/standard/tests/network/setrawcookie_error.phpt +++ b/ext/standard/tests/network/setrawcookie_error.phpt @@ -22,6 +22,12 @@ try { } catch (\ValueError $e) { echo $e->getMessage() . "\n"; } +// To go above year 9999: 60 * 60 * 24 * 365 * 9999 +try { + setcookie('name', 'value', 315328464000); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} try { setrawcookie('name', 'value', 100, 'invalid;'); } catch (\ValueError $e) { @@ -40,8 +46,9 @@ var_dump(headers_list()); setrawcookie(): Argument #1 ($name) cannot be empty setrawcookie(): Argument #1 ($name) cannot contain "=", ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" setrawcookie(): Argument #2 ($value) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" -setrawcookie(): Argument #4 ($path) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" -setrawcookie(): Argument #5 ($domain) cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setcookie(): "expires" option cannot have a year greater than 9999 +setrawcookie(): "path" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" +setrawcookie(): "domain" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" array(1) { [0]=> string(%d) "X-Powered-By: PHP/%s" From 6521060dd09dc76f7c4e74b34897484f66cd70e5 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Fri, 4 Sep 2020 16:34:11 +0200 Subject: [PATCH 3/3] Make Option array parsing stricter --- ext/standard/head.c | 69 +++++++++---------- .../network/setcookie_array_option_error.phpt | 40 +++++------ 2 files changed, 51 insertions(+), 58 deletions(-) diff --git a/ext/standard/head.c b/ext/standard/head.c index f430273d6ec4e..2bfabfadd0116 100644 --- a/ext/standard/head.c +++ b/ext/standard/head.c @@ -192,43 +192,35 @@ PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t e return result; } -static void php_head_parse_cookie_options_array(zval *options, zend_long *expires, zend_string **path, zend_string **domain, zend_bool *secure, zend_bool *httponly, zend_string **samesite) { - int found = 0; +static zend_result php_head_parse_cookie_options_array(zval *options, zend_long *expires, zend_string **path, + zend_string **domain, zend_bool *secure, zend_bool *httponly, zend_string **samesite) +{ zend_string *key; zval *value; ZEND_HASH_FOREACH_STR_KEY_VAL(Z_ARRVAL_P(options), key, value) { - if (key) { - if (zend_string_equals_literal_ci(key, "expires")) { - *expires = zval_get_long(value); - found++; - } else if (zend_string_equals_literal_ci(key, "path")) { - *path = zval_get_string(value); - found++; - } else if (zend_string_equals_literal_ci(key, "domain")) { - *domain = zval_get_string(value); - found++; - } else if (zend_string_equals_literal_ci(key, "secure")) { - *secure = zval_is_true(value); - found++; - } else if (zend_string_equals_literal_ci(key, "httponly")) { - *httponly = zval_is_true(value); - found++; - } else if (zend_string_equals_literal_ci(key, "samesite")) { - *samesite = zval_get_string(value); - found++; - } else { - php_error_docref(NULL, E_WARNING, "Unrecognized key '%s' found in the options array", ZSTR_VAL(key)); - } + if (!key) { + zend_value_error("%s(): option array cannot have numeric keys", get_active_function_name()); + return FAILURE; + } + if (zend_string_equals_literal_ci(key, "expires")) { + *expires = zval_get_long(value); + } else if (zend_string_equals_literal_ci(key, "path")) { + *path = zval_get_string(value); + } else if (zend_string_equals_literal_ci(key, "domain")) { + *domain = zval_get_string(value); + } else if (zend_string_equals_literal_ci(key, "secure")) { + *secure = zval_is_true(value); + } else if (zend_string_equals_literal_ci(key, "httponly")) { + *httponly = zval_is_true(value); + } else if (zend_string_equals_literal_ci(key, "samesite")) { + *samesite = zval_get_string(value); } else { - php_error_docref(NULL, E_WARNING, "Numeric key found in the options array"); + zend_value_error("%s(): option \"%s\" is invalid", get_active_function_name(), ZSTR_VAL(key)); + return FAILURE; } } ZEND_HASH_FOREACH_END(); - - /* Array is not empty but no valid keys were found */ - 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 SUCCESS; } static void php_setcookie_common(INTERNAL_FUNCTION_PARAMETERS, bool is_raw) @@ -257,22 +249,23 @@ static void php_setcookie_common(INTERNAL_FUNCTION_PARAMETERS, bool is_raw) "($expires_or_options) is an array", get_active_function_name()); RETURN_THROWS(); } - php_head_parse_cookie_options_array(expires_or_options, &expires, &path, - &domain, &secure, &httponly, &samesite); + if (FAILURE == php_head_parse_cookie_options_array(expires_or_options, &expires, &path, + &domain, &secure, &httponly, &samesite)) { + goto cleanup; + } } else { expires = zval_get_long(expires_or_options); } } - if (!EG(exception)) { - if (php_setcookie(name, value, expires, path, domain, secure, httponly, samesite, !is_raw) == SUCCESS) { - RETVAL_TRUE; - } else { - RETVAL_FALSE; - } + if (php_setcookie(name, value, expires, path, domain, secure, httponly, samesite, !is_raw) == SUCCESS) { + RETVAL_TRUE; + } else { + RETVAL_FALSE; } if (expires_or_options && Z_TYPE_P(expires_or_options) == IS_ARRAY) { + cleanup: if (path) { zend_string_release(path); } diff --git a/ext/standard/tests/network/setcookie_array_option_error.phpt b/ext/standard/tests/network/setcookie_array_option_error.phpt index d25fd2f526ad5..494e88fecba6d 100644 --- a/ext/standard/tests/network/setcookie_array_option_error.phpt +++ b/ext/standard/tests/network/setcookie_array_option_error.phpt @@ -8,11 +8,23 @@ date.timezone=UTC ob_start(); // Unrecognized key and no valid keys -setcookie('name', 'value', ['unknown_key' => 'only']); +try { + setcookie('name', 'value', ['unknown_key' => 'only']); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} // Numeric key and no valid keys -setcookie('name2', 'value2', [0 => 'numeric_key']); +try { + setcookie('name2', 'value2', [0 => 'numeric_key']); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} // Unrecognized key -setcookie('name3', 'value3', ['path' => '/path/', 'foo' => 'bar']); +try { + setcookie('name3', 'value3', ['path' => '/path/', 'foo' => 'bar']); +} catch (\ValueError $e) { + echo $e->getMessage() . "\n"; +} // Invalid expiration date // To go above year 9999: 60 * 60 * 24 * 365 * 9999 try { @@ -44,26 +56,14 @@ var_dump(headers_list()); --EXPECTHEADERS-- --EXPECTF-- -Warning: setcookie(): Unrecognized key 'unknown_key' found in the options array in %s - -Warning: setcookie(): No valid options were found in the given array in %s - -Warning: setcookie(): Numeric key found in the options array in %s - -Warning: setcookie(): No valid options were found in the given array in %s - -Warning: setcookie(): Unrecognized key 'foo' found in the options array in %s +setcookie(): option "unknown_key" is invalid +setcookie(): option array cannot have numeric keys +setcookie(): option "foo" is invalid setcookie(): "expires" option cannot have a year greater than 9999 setcookie(): "path" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" setcookie(): "domain" option cannot contain ",", ";", " ", "\t", "\r", "\n", "\013", and "\014" setcookie(): Expects exactly 3 arguments when argument #3 ($expires_or_options) is an array -array(4) { +array(1) { [0]=> - string(%d) "X-Powered-By: PHP/%s" - [1]=> - string(22) "Set-Cookie: name=value" - [2]=> - string(24) "Set-Cookie: name2=value2" - [3]=> - string(37) "Set-Cookie: name3=value3; path=/path/" + string(%s) "X-Powered-By: PHP/%s" }