From b9f64f7afa4ee592b24ea7ec91fd1f72c1f5b344 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Sat, 2 May 2026 15:37:10 +0100 Subject: [PATCH 1/2] Improve ZPP weak parsing by returning value directly when possible These changes are based on a previous PR by @ndossche to reduce codebloat: https://github.com/php/php-src/pull/18436 For zend_parse_arg_str_weak() we can return a `zend_string*` directly, as errors can be indicated by a NULL pointer return. For zend_parse_arg_double_weak() we can return a `double` directly, as we can represent ZPP errors via `NAN` as booleans and integers never coerce to NAN. One might think that the string `'NAN'` can be coerced to double `NAN`, however PHP doesn't allow this coercion. And an explicit cast to double via `(float)` of such a string results in 0. For zend_parse_arg_bool_weak() we instead create a new enum zpp_parse_bool_status that represents a tri-state which we return. Allowing us to return the boolean value via the return type instead of using an out pointer. --- Zend/zend_API.c | 82 +++++++++++++++++----------------- Zend/zend_API.h | 44 +++++++++++------- Zend/zend_execute.c | 27 ++++++----- Zend/zend_frameless_function.h | 2 +- Zend/zend_vm_def.h | 2 +- Zend/zend_vm_execute.h | 12 ++--- 6 files changed, 93 insertions(+), 76 deletions(-) diff --git a/Zend/zend_API.c b/Zend/zend_API.c index c97d9308e208..c04217983b5d 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -509,35 +509,33 @@ static ZEND_COLD bool zend_null_arg_deprecated(const char *fallback_type, uint32 return !EG(exception); } -ZEND_API bool ZEND_FASTCALL zend_parse_arg_bool_weak(const zval *arg, bool *dest, uint32_t arg_num) /* {{{ */ +ZEND_API zpp_parse_bool_status ZEND_FASTCALL zend_parse_arg_bool_weak(const zval *arg, uint32_t arg_num) /* {{{ */ { if (EXPECTED(Z_TYPE_P(arg) <= IS_STRING)) { if (UNEXPECTED(Z_TYPE_P(arg) == IS_NULL) && !zend_null_arg_deprecated("bool", arg_num)) { - return 0; + return ZPP_PARSE_BOOL_STATUS_ERROR; } - *dest = zend_is_true(arg); - } else { - return 0; + return zend_is_true(arg); } - return 1; + return ZPP_PARSE_BOOL_STATUS_ERROR; } /* }}} */ -ZEND_API bool ZEND_FASTCALL zend_parse_arg_bool_slow(const zval *arg, bool *dest, uint32_t arg_num) /* {{{ */ +ZEND_API zpp_parse_bool_status ZEND_FASTCALL zend_parse_arg_bool_slow(const zval *arg, uint32_t arg_num) /* {{{ */ { if (UNEXPECTED(ZEND_ARG_USES_STRICT_TYPES())) { - return 0; + return ZPP_PARSE_BOOL_STATUS_ERROR; } - return zend_parse_arg_bool_weak(arg, dest, arg_num); + return zend_parse_arg_bool_weak(arg, arg_num); } /* }}} */ -ZEND_API bool ZEND_FASTCALL zend_flf_parse_arg_bool_slow(const zval *arg, bool *dest, uint32_t arg_num) +ZEND_API zpp_parse_bool_status ZEND_FASTCALL zend_flf_parse_arg_bool_slow(const zval *arg, uint32_t arg_num) { if (UNEXPECTED(ZEND_FLF_ARG_USES_STRICT_TYPES())) { - return 0; + return ZPP_PARSE_BOOL_STATUS_ERROR; } - return zend_parse_arg_bool_weak(arg, dest, arg_num); + return zend_parse_arg_bool_weak(arg, arg_num); } ZEND_API bool ZEND_FASTCALL zend_parse_arg_long_weak(const zval *arg, zend_long *dest, uint32_t arg_num) /* {{{ */ @@ -624,44 +622,46 @@ ZEND_API bool ZEND_FASTCALL zend_flf_parse_arg_long_slow(const zval *arg, zend_l return zend_parse_arg_long_weak(arg, dest, arg_num); } -ZEND_API bool ZEND_FASTCALL zend_parse_arg_double_weak(const zval *arg, double *dest, uint32_t arg_num) /* {{{ */ +ZEND_API double ZEND_FASTCALL zend_parse_arg_double_weak(const zval *arg, uint32_t arg_num) /* {{{ */ { if (EXPECTED(Z_TYPE_P(arg) == IS_LONG)) { - *dest = (double)Z_LVAL_P(arg); + return (double)Z_LVAL_P(arg); } else if (EXPECTED(Z_TYPE_P(arg) == IS_STRING)) { zend_long l; + double dval; uint8_t type; - if (UNEXPECTED((type = is_numeric_str_function(Z_STR_P(arg), &l, dest)) != IS_DOUBLE)) { + if (UNEXPECTED((type = is_numeric_str_function(Z_STR_P(arg), &l, &dval)) != IS_DOUBLE)) { if (EXPECTED(type != 0)) { - *dest = (double)(l); + return (double)(l); } else { - return 0; + return NAN; } + } else { + return dval; } } else if (EXPECTED(Z_TYPE_P(arg) < IS_TRUE)) { if (UNEXPECTED(Z_TYPE_P(arg) == IS_NULL) && !zend_null_arg_deprecated("float", arg_num)) { - return 0; + return NAN; } - *dest = 0.0; + return 0.0; } else if (EXPECTED(Z_TYPE_P(arg) == IS_TRUE)) { - *dest = 1.0; + return 1.0; } else { - return 0; + return NAN; } - return 1; } /* }}} */ -ZEND_API bool ZEND_FASTCALL zend_parse_arg_double_slow(const zval *arg, double *dest, uint32_t arg_num) /* {{{ */ +ZEND_API double ZEND_FASTCALL zend_parse_arg_double_slow(const zval *arg, uint32_t arg_num) /* {{{ */ { if (EXPECTED(Z_TYPE_P(arg) == IS_LONG)) { /* SSTH Exception: IS_LONG may be accepted instead as IS_DOUBLE */ - *dest = (double)Z_LVAL_P(arg); + return (double)Z_LVAL_P(arg); } else if (UNEXPECTED(ZEND_ARG_USES_STRICT_TYPES())) { - return 0; + return NAN; } - return zend_parse_arg_double_weak(arg, dest, arg_num); + return zend_parse_arg_double_weak(arg, arg_num); } /* }}} */ @@ -728,58 +728,58 @@ ZEND_API bool ZEND_FASTCALL zend_parse_arg_number_or_str_slow(zval *arg, zval ** return true; } -ZEND_API bool ZEND_FASTCALL zend_parse_arg_str_weak(zval *arg, zend_string **dest, uint32_t arg_num) /* {{{ */ +ZEND_API zend_string* ZEND_FASTCALL zend_parse_arg_str_weak(zval *arg, uint32_t arg_num) /* {{{ */ { if (EXPECTED(Z_TYPE_P(arg) < IS_STRING)) { if (UNEXPECTED(Z_TYPE_P(arg) == IS_NULL) && !zend_null_arg_deprecated("string", arg_num)) { - return 0; + return NULL; } convert_to_string(arg); - *dest = Z_STR_P(arg); + return Z_STR_P(arg); } else if (UNEXPECTED(Z_TYPE_P(arg) == IS_OBJECT)) { zend_object *zobj = Z_OBJ_P(arg); zval obj; if (zobj->handlers->cast_object(zobj, &obj, IS_STRING) == SUCCESS) { OBJ_RELEASE(zobj); ZVAL_COPY_VALUE(arg, &obj); - *dest = Z_STR_P(arg); - return 1; + return Z_STR_P(arg); } - return 0; + return NULL; } else { - return 0; + return NULL; } - return 1; } /* }}} */ -ZEND_API bool ZEND_FASTCALL zend_parse_arg_str_slow(zval *arg, zend_string **dest, uint32_t arg_num) /* {{{ */ +ZEND_API zend_string* ZEND_FASTCALL zend_parse_arg_str_slow(zval *arg, uint32_t arg_num) /* {{{ */ { if (UNEXPECTED(ZEND_ARG_USES_STRICT_TYPES())) { - return 0; + return NULL; } - return zend_parse_arg_str_weak(arg, dest, arg_num); + return zend_parse_arg_str_weak(arg, arg_num); } /* }}} */ -ZEND_API bool ZEND_FASTCALL zend_flf_parse_arg_str_slow(zval *arg, zend_string **dest, uint32_t arg_num) +ZEND_API zend_string* ZEND_FASTCALL zend_flf_parse_arg_str_slow(zval *arg, uint32_t arg_num) { if (UNEXPECTED(ZEND_FLF_ARG_USES_STRICT_TYPES())) { - return 0; + return NULL; } - return zend_parse_arg_str_weak(arg, dest, arg_num); + return zend_parse_arg_str_weak(arg, arg_num); } ZEND_API bool ZEND_FASTCALL zend_parse_arg_str_or_long_slow(zval *arg, zend_string **dest_str, zend_long *dest_long, uint32_t arg_num) /* {{{ */ { + zend_string *str; if (UNEXPECTED(ZEND_ARG_USES_STRICT_TYPES())) { return 0; } if (zend_parse_arg_long_weak(arg, dest_long, arg_num)) { *dest_str = NULL; return 1; - } else if (zend_parse_arg_str_weak(arg, dest_str, arg_num)) { + } else if ((str = zend_parse_arg_str_weak(arg, arg_num)) != NULL) { *dest_long = 0; + *dest_str = str; return 1; } else { return 0; diff --git a/Zend/zend_API.h b/Zend/zend_API.h index 7cc9b6ff38dd..01a9202be1c3 100644 --- a/Zend/zend_API.h +++ b/Zend/zend_API.h @@ -2179,21 +2179,27 @@ ZEND_API ZEND_COLD void zend_class_redeclaration_error_ex(int type, zend_string /* Inlined implementations shared by new and old parameter parsing APIs */ +typedef enum zpp_parse_bool_status { + ZPP_PARSE_BOOL_STATUS_FALSE = 0, + ZPP_PARSE_BOOL_STATUS_TRUE = 1, + ZPP_PARSE_BOOL_STATUS_ERROR = 2, +} zpp_parse_bool_status; + ZEND_API bool ZEND_FASTCALL zend_parse_arg_class(zval *arg, zend_class_entry **pce, uint32_t num, bool check_null); -ZEND_API bool ZEND_FASTCALL zend_parse_arg_bool_slow(const zval *arg, bool *dest, uint32_t arg_num); -ZEND_API bool ZEND_FASTCALL zend_parse_arg_bool_weak(const zval *arg, bool *dest, uint32_t arg_num); +ZEND_API zpp_parse_bool_status ZEND_FASTCALL zend_parse_arg_bool_slow(const zval *arg, uint32_t arg_num); +ZEND_API zpp_parse_bool_status ZEND_FASTCALL zend_parse_arg_bool_weak(const zval *arg, uint32_t arg_num); ZEND_API bool ZEND_FASTCALL zend_parse_arg_long_slow(const zval *arg, zend_long *dest, uint32_t arg_num); ZEND_API bool ZEND_FASTCALL zend_parse_arg_long_weak(const zval *arg, zend_long *dest, uint32_t arg_num); -ZEND_API bool ZEND_FASTCALL zend_parse_arg_double_slow(const zval *arg, double *dest, uint32_t arg_num); -ZEND_API bool ZEND_FASTCALL zend_parse_arg_double_weak(const zval *arg, double *dest, uint32_t arg_num); -ZEND_API bool ZEND_FASTCALL zend_parse_arg_str_slow(zval *arg, zend_string **dest, uint32_t arg_num); -ZEND_API bool ZEND_FASTCALL zend_parse_arg_str_weak(zval *arg, zend_string **dest, uint32_t arg_num); +ZEND_API double ZEND_FASTCALL zend_parse_arg_double_slow(const zval *arg, uint32_t arg_num); +ZEND_API double ZEND_FASTCALL zend_parse_arg_double_weak(const zval *arg, uint32_t arg_num); +ZEND_API zend_string* ZEND_FASTCALL zend_parse_arg_str_slow(zval *arg, uint32_t arg_num); +ZEND_API zend_string* ZEND_FASTCALL zend_parse_arg_str_weak(zval *arg, uint32_t arg_num); ZEND_API bool ZEND_FASTCALL zend_parse_arg_number_slow(zval *arg, zval **dest, uint32_t arg_num); ZEND_API bool ZEND_FASTCALL zend_parse_arg_number_or_str_slow(zval *arg, zval **dest, uint32_t arg_num); ZEND_API bool ZEND_FASTCALL zend_parse_arg_str_or_long_slow(zval *arg, zend_string **dest_str, zend_long *dest_long, uint32_t arg_num); -ZEND_API bool ZEND_FASTCALL zend_flf_parse_arg_bool_slow(const zval *arg, bool *dest, uint32_t arg_num); -ZEND_API bool ZEND_FASTCALL zend_flf_parse_arg_str_slow(zval *arg, zend_string **dest, uint32_t arg_num); +ZEND_API zpp_parse_bool_status ZEND_FASTCALL zend_flf_parse_arg_bool_slow(const zval *arg, uint32_t arg_num); +ZEND_API zend_string* ZEND_FASTCALL zend_flf_parse_arg_str_slow(zval *arg, uint32_t arg_num); ZEND_API bool ZEND_FASTCALL zend_flf_parse_arg_long_slow(const zval *arg, zend_long *dest, uint32_t arg_num); static zend_always_inline bool zend_parse_arg_bool_ex(const zval *arg, bool *dest, bool *is_null, bool check_null, uint32_t arg_num, bool frameless) @@ -2209,11 +2215,16 @@ static zend_always_inline bool zend_parse_arg_bool_ex(const zval *arg, bool *des *is_null = 1; *dest = 0; } else { + zpp_parse_bool_status result; if (frameless) { - return zend_flf_parse_arg_bool_slow(arg, dest, arg_num); + result = zend_flf_parse_arg_bool_slow(arg, arg_num); } else { - return zend_parse_arg_bool_slow(arg, dest, arg_num); + result = zend_parse_arg_bool_slow(arg, arg_num); + } + if (UNEXPECTED(result == ZPP_PARSE_BOOL_STATUS_ERROR)) { + return false; } + *dest = result; } return 1; } @@ -2259,7 +2270,8 @@ static zend_always_inline bool zend_parse_arg_double(const zval *arg, double *de *is_null = 1; *dest = 0.0; } else { - return zend_parse_arg_double_slow(arg, dest, arg_num); + *dest = zend_parse_arg_double_slow(arg, arg_num); + return !zend_isnan(*dest); } return 1; } @@ -2296,12 +2308,13 @@ static zend_always_inline bool zend_parse_arg_str_ex(zval *arg, zend_string **de *dest = NULL; } else { if (frameless) { - return zend_flf_parse_arg_str_slow(arg, dest, arg_num); + *dest = zend_flf_parse_arg_str_slow(arg, arg_num); } else { - return zend_parse_arg_str_slow(arg, dest, arg_num); + *dest = zend_parse_arg_str_slow(arg, arg_num); } + return *dest != NULL; } - return 1; + return true; } static zend_always_inline bool zend_parse_arg_str(zval *arg, zend_string **dest, bool check_null, uint32_t arg_num) @@ -2532,7 +2545,8 @@ static zend_always_inline bool zend_parse_arg_array_ht_or_str( *dest_str = NULL; } else { *dest_ht = NULL; - return zend_parse_arg_str_slow(arg, dest_str, arg_num); + *dest_str = zend_parse_arg_str_slow(arg, arg_num); + return *dest_str != NULL; } return 1; } diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 85461eaa1569..4253037fda52 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -733,8 +733,6 @@ static bool zend_verify_weak_scalar_type_hint(uint32_t type_mask, zval *arg) { zend_long lval; double dval; - zend_string *str; - bool bval; /* Type preference order: int -> float -> string -> bool */ if (type_mask & MAY_BE_LONG) { @@ -760,16 +758,23 @@ static bool zend_verify_weak_scalar_type_hint(uint32_t type_mask, zval *arg) return false; } } - if ((type_mask & MAY_BE_DOUBLE) && zend_parse_arg_double_weak(arg, &dval, 0)) { - zval_ptr_dtor(arg); - ZVAL_DOUBLE(arg, dval); - return true; + if (type_mask & MAY_BE_DOUBLE) { + dval = zend_parse_arg_double_weak(arg, 0); + if (EXPECTED(!zend_isnan(dval))) { + zval_ptr_dtor(arg); + ZVAL_DOUBLE(arg, dval); + return true; + } } - if ((type_mask & MAY_BE_STRING) && zend_parse_arg_str_weak(arg, &str, 0)) { + if ((type_mask & MAY_BE_STRING) && zend_parse_arg_str_weak(arg, 0)) { /* on success "arg" is converted to IS_STRING */ return true; } - if ((type_mask & MAY_BE_BOOL) == MAY_BE_BOOL && zend_parse_arg_bool_weak(arg, &bval, 0)) { + if ((type_mask & MAY_BE_BOOL) == MAY_BE_BOOL) { + zpp_parse_bool_status bval = zend_parse_arg_bool_weak(arg, 0); + if (UNEXPECTED(bval == ZPP_PARSE_BOOL_STATUS_ERROR)) { + return false; + } zval_ptr_dtor(arg); ZVAL_BOOL(arg, bval); return true; @@ -793,21 +798,19 @@ static bool can_convert_to_string(const zval *zv) { static bool zend_verify_weak_scalar_type_hint_no_sideeffect(uint32_t type_mask, const zval *arg) { zend_long lval; - double dval; - bool bval; /* Pass (uint32_t)-1 as arg_num to indicate to ZPP not to emit any deprecation notice, * this is needed because the version with side effects also uses 0 (e.g. for typed properties) */ if ((type_mask & MAY_BE_LONG) && zend_parse_arg_long_weak(arg, &lval, (uint32_t)-1)) { return true; } - if ((type_mask & MAY_BE_DOUBLE) && zend_parse_arg_double_weak(arg, &dval, (uint32_t)-1)) { + if ((type_mask & MAY_BE_DOUBLE) && !zend_isnan(zend_parse_arg_double_weak(arg, (uint32_t)-1))) { return true; } if ((type_mask & MAY_BE_STRING) && can_convert_to_string(arg)) { return true; } - if ((type_mask & MAY_BE_BOOL) == MAY_BE_BOOL && zend_parse_arg_bool_weak(arg, &bval, (uint32_t)-1)) { + if ((type_mask & MAY_BE_BOOL) == MAY_BE_BOOL && zend_parse_arg_bool_weak(arg, (uint32_t)-1) != ZPP_PARSE_BOOL_STATUS_ERROR) { return true; } return false; diff --git a/Zend/zend_frameless_function.h b/Zend/zend_frameless_function.h index 241507aa99e7..b6f361f104b0 100644 --- a/Zend/zend_frameless_function.h +++ b/Zend/zend_frameless_function.h @@ -64,7 +64,7 @@ dest_ht = NULL; \ ZVAL_COPY(&str_tmp, arg ## arg_num); \ arg ## arg_num = &str_tmp; \ - if (!zend_flf_parse_arg_str_slow(arg ## arg_num, &dest_str, arg_num)) { \ + if (!(dest_str = zend_flf_parse_arg_str_slow(arg ## arg_num, arg_num))) { \ zend_wrong_parameter_type_error(arg_num, Z_EXPECTED_ARRAY_OR_STRING, arg ## arg_num); \ goto flf_clean; \ } \ diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h index 1f7e09d1be35..05923bbc2b61 100644 --- a/Zend/zend_vm_def.h +++ b/Zend/zend_vm_def.h @@ -8843,7 +8843,7 @@ ZEND_VM_COLD_CONST_HANDLER(121, ZEND_STRLEN, CONST|TMP|CV, ANY) } ZVAL_COPY(&tmp, value); - if (zend_parse_arg_str_weak(&tmp, &str, 1)) { + if ((str = zend_parse_arg_str_weak(&tmp, 1)) != NULL) { ZVAL_LONG(EX_VAR(opline->result.var), ZSTR_LEN(str)); zval_ptr_dtor(&tmp); break; diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h index d5860da23b4c..2b5b9e5fcd47 100644 --- a/Zend/zend_vm_execute.h +++ b/Zend/zend_vm_execute.h @@ -6083,7 +6083,7 @@ static ZEND_VM_COLD ZEND_OPCODE_HANDLER_RET ZEND_OPCODE_HANDLER_FUNC_CCONV ZEND_ } ZVAL_COPY(&tmp, value); - if (zend_parse_arg_str_weak(&tmp, &str, 1)) { + if ((str = zend_parse_arg_str_weak(&tmp, 1)) != NULL) { ZVAL_LONG(EX_VAR(opline->result.var), ZSTR_LEN(str)); zval_ptr_dtor(&tmp); break; @@ -18167,7 +18167,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_OPCODE_HANDLER_FUNC_CCONV ZEND_STRLEN_SPEC_T } ZVAL_COPY(&tmp, value); - if (zend_parse_arg_str_weak(&tmp, &str, 1)) { + if ((str = zend_parse_arg_str_weak(&tmp, 1)) != NULL) { ZVAL_LONG(EX_VAR(opline->result.var), ZSTR_LEN(str)); zval_ptr_dtor(&tmp); break; @@ -40758,7 +40758,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_OPCODE_HANDLER_FUNC_CCONV ZEND_STRLEN_SPEC_C } ZVAL_COPY(&tmp, value); - if (zend_parse_arg_str_weak(&tmp, &str, 1)) { + if ((str = zend_parse_arg_str_weak(&tmp, 1)) != NULL) { ZVAL_LONG(EX_VAR(opline->result.var), ZSTR_LEN(str)); zval_ptr_dtor(&tmp); break; @@ -58741,7 +58741,7 @@ static ZEND_VM_COLD ZEND_OPCODE_HANDLER_RET ZEND_OPCODE_HANDLER_CCONV ZEND_STRLE } ZVAL_COPY(&tmp, value); - if (zend_parse_arg_str_weak(&tmp, &str, 1)) { + if ((str = zend_parse_arg_str_weak(&tmp, 1)) != NULL) { ZVAL_LONG(EX_VAR(opline->result.var), ZSTR_LEN(str)); zval_ptr_dtor(&tmp); break; @@ -70723,7 +70723,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_OPCODE_HANDLER_CCONV ZEND_STRLEN_SPEC_TMP_TA } ZVAL_COPY(&tmp, value); - if (zend_parse_arg_str_weak(&tmp, &str, 1)) { + if ((str = zend_parse_arg_str_weak(&tmp, 1)) != NULL) { ZVAL_LONG(EX_VAR(opline->result.var), ZSTR_LEN(str)); zval_ptr_dtor(&tmp); break; @@ -93214,7 +93214,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_OPCODE_HANDLER_CCONV ZEND_STRLEN_SPEC_CV_TAI } ZVAL_COPY(&tmp, value); - if (zend_parse_arg_str_weak(&tmp, &str, 1)) { + if ((str = zend_parse_arg_str_weak(&tmp, 1)) != NULL) { ZVAL_LONG(EX_VAR(opline->result.var), ZSTR_LEN(str)); zval_ptr_dtor(&tmp); break; From c130d42be4ede3e2c530e43d4212a8b26c3e0533 Mon Sep 17 00:00:00 2001 From: Jorg Adam Sowa Date: Sat, 2 May 2026 16:51:02 +0200 Subject: [PATCH 2/2] Validate SameSite cookie attribute against allowed values (#21670) Extract php_is_valid_samesite_value() in ext/standard/head.c as a shared validation function that enforces the SameSite whitelist (Strict, Lax, None, or empty string) with case-insensitive matching. Apply validation in both setcookie()/setrawcookie() (replacing the existing TODO comment) and the session.cookie_samesite INI handler. Previously arbitrary strings including CRLF sequences were accepted and appended verbatim into the Set-Cookie header. --- ext/session/session.c | 16 +++++- .../session_get_cookie_params_basic.phpt | 6 +-- .../session_get_cookie_params_variation1.phpt | 6 +-- ...session_set_cookie_params_invalid_ini.phpt | 22 ++++++++ .../session_set_cookie_params_variation6.phpt | 54 +++++++++++++------ .../session_set_cookie_params_variation7.phpt | 4 +- ext/standard/head.c | 13 ++++- ext/standard/head.h | 2 + .../tests/setcookie_samesite_validation.phpt | 52 ++++++++++++++++++ 9 files changed, 148 insertions(+), 27 deletions(-) create mode 100644 ext/session/tests/session_set_cookie_params_invalid_ini.phpt create mode 100644 ext/standard/tests/setcookie_samesite_validation.phpt diff --git a/ext/session/session.c b/ext/session/session.c index 96e32ea7043f..0a6ad38e5581 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -739,6 +739,20 @@ static PHP_INI_MH(OnUpdateSessionStr) return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); } +static PHP_INI_MH(OnUpdateSessionSameSite) +{ + SESSION_CHECK_ACTIVE_STATE; + SESSION_CHECK_OUTPUT_STATE; + + if (new_value && ZSTR_LEN(new_value) > 0 && !php_is_valid_samesite_value(new_value)) { + php_error_docref(NULL, E_WARNING, + "session.cookie_samesite must be \"Strict\", \"Lax\", \"None\", or \"\""); + return FAILURE; + } + + return OnUpdateStr(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage); +} + static PHP_INI_MH(OnUpdateSessionBool) { SESSION_CHECK_ACTIVE_STATE; @@ -910,7 +924,7 @@ PHP_INI_BEGIN() STD_PHP_INI_BOOLEAN("session.cookie_secure", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_secure, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.cookie_partitioned", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_partitioned, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.cookie_httponly", "0", PHP_INI_ALL, OnUpdateSessionBool, cookie_httponly, php_ps_globals, ps_globals) - STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionStr, cookie_samesite, php_ps_globals, ps_globals) + STD_PHP_INI_ENTRY("session.cookie_samesite", "", PHP_INI_ALL, OnUpdateSessionSameSite, cookie_samesite, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_cookies", "1", PHP_INI_ALL, OnUpdateSessionBool, use_cookies, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_only_cookies", "1", PHP_INI_ALL, OnUpdateUseOnlyCookies, use_only_cookies, php_ps_globals, ps_globals) STD_PHP_INI_BOOLEAN("session.use_strict_mode", "0", PHP_INI_ALL, OnUpdateSessionBool, use_strict_mode, php_ps_globals, ps_globals) diff --git a/ext/session/tests/session_get_cookie_params_basic.phpt b/ext/session/tests/session_get_cookie_params_basic.phpt index 73ffd4c94d1e..8e7f8da4e3fb 100644 --- a/ext/session/tests/session_get_cookie_params_basic.phpt +++ b/ext/session/tests/session_get_cookie_params_basic.phpt @@ -30,7 +30,7 @@ var_dump(session_set_cookie_params([ "domain" => "baz", "secure" => FALSE, "httponly" => FALSE, - "samesite" => "please"])); + "samesite" => "Strict"])); var_dump(session_get_cookie_params()); var_dump(session_set_cookie_params([ "secure" => TRUE, @@ -107,7 +107,7 @@ array(7) { ["httponly"]=> bool(false) ["samesite"]=> - string(6) "please" + string(6) "Strict" } bool(true) array(7) { @@ -124,6 +124,6 @@ array(7) { ["httponly"]=> bool(false) ["samesite"]=> - string(6) "please" + string(6) "Strict" } Done diff --git a/ext/session/tests/session_get_cookie_params_variation1.phpt b/ext/session/tests/session_get_cookie_params_variation1.phpt index 7ce112c9b94d..0ab0f233530b 100644 --- a/ext/session/tests/session_get_cookie_params_variation1.phpt +++ b/ext/session/tests/session_get_cookie_params_variation1.phpt @@ -30,7 +30,7 @@ ini_set("session.cookie_secure", TRUE); var_dump(session_get_cookie_params()); ini_set("session.cookie_httponly", TRUE); var_dump(session_get_cookie_params()); -ini_set("session.cookie_samesite", "foo"); +ini_set("session.cookie_samesite", "Lax"); var_dump(session_get_cookie_params()); ini_set("session.cookie_partitioned", TRUE); var_dump(session_get_cookie_params()); @@ -150,7 +150,7 @@ array(7) { ["httponly"]=> bool(true) ["samesite"]=> - string(3) "foo" + string(3) "Lax" } array(7) { ["lifetime"]=> @@ -166,6 +166,6 @@ array(7) { ["httponly"]=> bool(true) ["samesite"]=> - string(3) "foo" + string(3) "Lax" } Done diff --git a/ext/session/tests/session_set_cookie_params_invalid_ini.phpt b/ext/session/tests/session_set_cookie_params_invalid_ini.phpt new file mode 100644 index 000000000000..61728c342ea2 --- /dev/null +++ b/ext/session/tests/session_set_cookie_params_invalid_ini.phpt @@ -0,0 +1,22 @@ +--TEST-- +Test session.cookie_samesite with invalid INI value +--INI-- +session.cookie_samesite=Invalid +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +Warning: PHP Startup: session.cookie_samesite must be "Strict", "Lax", "None", or "" in Unknown on line 0 +string(0) "" +Done diff --git a/ext/session/tests/session_set_cookie_params_variation6.phpt b/ext/session/tests/session_set_cookie_params_variation6.phpt index 61243f82751e..fbf958a0be07 100644 --- a/ext/session/tests/session_set_cookie_params_variation6.phpt +++ b/ext/session/tests/session_set_cookie_params_variation6.phpt @@ -1,7 +1,7 @@ --TEST-- -Test session_set_cookie_params() function : variation +Test session_set_cookie_params() samesite validation --INI-- -session.cookie_samesite=test +session.cookie_samesite=Lax --EXTENSIONS-- session --SKIPIF-- @@ -11,36 +11,56 @@ session ob_start(); -echo "*** Testing session_set_cookie_params() : variation ***\n"; - +echo "-- Valid values --\n"; var_dump(ini_get("session.cookie_samesite")); -var_dump(session_set_cookie_params(["samesite" => "nothing"])); +var_dump(session_set_cookie_params(["samesite" => "Strict"])); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_start()); +var_dump(session_set_cookie_params(["samesite" => "None"])); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_set_cookie_params(["samesite" => "test"])); +var_dump(session_set_cookie_params(["samesite" => ""])); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_destroy()); + +echo "-- Invalid value via session_set_cookie_params --\n"; +var_dump(session_set_cookie_params(["samesite" => "Invalid"])); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_set_cookie_params(["samesite" => "other"])); + +echo "-- Invalid value via ini_set --\n"; +var_dump(ini_set("session.cookie_samesite", "Invalid")); var_dump(ini_get("session.cookie_samesite")); +echo "-- Cannot change while session is active --\n"; +var_dump(session_set_cookie_params(["samesite" => "Lax"])); +var_dump(session_start()); +var_dump(session_set_cookie_params(["samesite" => "Strict"])); +var_dump(session_destroy()); + echo "Done"; ob_end_flush(); ?> --EXPECTF-- -*** Testing session_set_cookie_params() : variation *** -string(4) "test" +-- Valid values -- +string(3) "Lax" bool(true) -string(7) "nothing" +string(6) "Strict" bool(true) -string(7) "nothing" +string(4) "None" +bool(true) +string(0) "" +-- Invalid value via session_set_cookie_params -- -Warning: session_set_cookie_params(): Session cookie parameters cannot be changed when a session is active (started from %s on line %d) in %s on line %d +Warning: session_set_cookie_params(): session.cookie_samesite must be "Strict", "Lax", "None", or "" in %s on line %d +bool(false) +string(0) "" +-- Invalid value via ini_set -- + +Warning: ini_set(): session.cookie_samesite must be "Strict", "Lax", "None", or "" in %s on line %d bool(false) -string(7) "nothing" +string(0) "" +-- Cannot change while session is active -- bool(true) -string(7) "nothing" bool(true) -string(5) "other" + +Warning: session_set_cookie_params(): Session cookie parameters cannot be changed when a session is active (started from %s on line %d) in %s on line %d +bool(false) +bool(true) Done diff --git a/ext/session/tests/session_set_cookie_params_variation7.phpt b/ext/session/tests/session_set_cookie_params_variation7.phpt index 430c6efc36e9..3780fc0222f1 100644 --- a/ext/session/tests/session_set_cookie_params_variation7.phpt +++ b/ext/session/tests/session_set_cookie_params_variation7.phpt @@ -33,7 +33,7 @@ try { var_dump(ini_get("session.cookie_secure")); var_dump(ini_get("session.cookie_samesite")); -var_dump(session_set_cookie_params(["secure" => true, "samesite" => "please"])); +var_dump(session_set_cookie_params(["secure" => true, "samesite" => "Strict"])); var_dump(ini_get("session.cookie_secure")); var_dump(ini_get("session.cookie_samesite")); @@ -66,7 +66,7 @@ string(1) "0" string(0) "" bool(true) string(1) "1" -string(6) "please" +string(6) "Strict" string(1) "0" bool(true) string(2) "42" diff --git a/ext/standard/head.c b/ext/standard/head.c index 69e8d1f794ff..03f2e6189ee5 100644 --- a/ext/standard/head.c +++ b/ext/standard/head.c @@ -74,6 +74,13 @@ PHPAPI bool php_header(void) } } +PHPAPI bool php_is_valid_samesite_value(zend_string *value) +{ + return zend_string_equals_literal_ci(value, "Strict") + || zend_string_equals_literal_ci(value, "Lax") + || zend_string_equals_literal_ci(value, "None"); +} + #define ILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", or \"\\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, @@ -121,7 +128,11 @@ PHPAPI zend_result php_setcookie(zend_string *name, zend_string *value, time_t e return FAILURE; } - /* Should check value of SameSite? */ + if (samesite && ZSTR_LEN(samesite) > 0 && !php_is_valid_samesite_value(samesite)) { + zend_value_error("%s(): \"samesite\" option must be \"Strict\", \"Lax\", \"None\", or \"\"", + get_active_function_name()); + return FAILURE; + } if (value == NULL || ZSTR_LEN(value) == 0) { /* diff --git a/ext/standard/head.h b/ext/standard/head.h index 07c947f8ad0b..8b91371a46e2 100644 --- a/ext/standard/head.h +++ b/ext/standard/head.h @@ -24,6 +24,8 @@ #define COOKIE_SAMESITE "; SameSite=" #define COOKIE_PARTITIONED "; Partitioned" +PHPAPI bool php_is_valid_samesite_value(zend_string *value); + extern PHP_RINIT_FUNCTION(head); PHPAPI bool php_header(void); diff --git a/ext/standard/tests/setcookie_samesite_validation.phpt b/ext/standard/tests/setcookie_samesite_validation.phpt new file mode 100644 index 000000000000..3827f04fe8d4 --- /dev/null +++ b/ext/standard/tests/setcookie_samesite_validation.phpt @@ -0,0 +1,52 @@ +--TEST-- +setcookie() and setrawcookie() validate samesite option +--FILE-- + 'Strict'])); +var_dump(setcookie('test', 'value', ['samesite' => 'Lax'])); +var_dump(setcookie('test', 'value', ['samesite' => 'None'])); +var_dump(setcookie('test', 'value', ['samesite' => ''])); + +// Case-insensitive +var_dump(setcookie('test', 'value', ['samesite' => 'strict'])); +var_dump(setcookie('test', 'value', ['samesite' => 'LAX'])); +var_dump(setcookie('test', 'value', ['samesite' => 'NONE'])); + +// setrawcookie uses the same validation +var_dump(setrawcookie('test', 'value', ['samesite' => 'Lax'])); + +// Invalid values +try { + setcookie('test', 'value', ['samesite' => 'Invalid']); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} + +try { + setcookie('test', 'value', ['samesite' => "Strict\r\nX-Injected: evil"]); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} + +try { + setrawcookie('test', 'value', ['samesite' => 'Invalid']); +} catch (ValueError $e) { + echo $e->getMessage() . "\n"; +} + +?> +--EXPECTF-- +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +bool(true) +setcookie(): "samesite" option must be "Strict", "Lax", "None", or "" +setcookie(): "samesite" option must be "Strict", "Lax", "None", or "" +setrawcookie(): "samesite" option must be "Strict", "Lax", "None", or ""