From 86941db164456bd1e9cea1159884c3c107605dbe Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Wed, 22 Jan 2025 17:27:59 +0000 Subject: [PATCH 1/2] ext/session: Fix GH-17541 (ext/session NULL pointer dereferencement during ID reset) --- ext/session/session.c | 20 ++++++++----- ext/session/tests/bug66481.phpt | 2 +- ext/session/tests/gh17541.phpt | 30 +++++++++++++++++++ .../tests/session_name_variation1.phpt | 20 ++++++------- 4 files changed, 52 insertions(+), 20 deletions(-) create mode 100644 ext/session/tests/gh17541.phpt diff --git a/ext/session/session.c b/ext/session/session.c index b838b132b16d0..bf1454687e9b9 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -94,6 +94,7 @@ zend_class_entry *php_session_update_timestamp_iface_entry; } #define SESSION_FORBIDDEN_CHARS "=,;.[ \t\r\n\013\014" +#define SESSION_FORBIDDEN_CHARS_FOR_ERROR_MSG "=,;.[ \\t\\r\\n\\013\\014" #define APPLY_TRANS_SID (PS(use_trans_sid) && !PS(use_only_cookies)) @@ -683,7 +684,11 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */ SESSION_CHECK_OUTPUT_STATE; /* Numeric session.name won't work at all */ - if ((!ZSTR_LEN(new_value) || is_numeric_string(ZSTR_VAL(new_value), ZSTR_LEN(new_value), NULL, NULL, 0))) { + if ( + ZSTR_LEN(new_value) == 0 + || is_numeric_str_function(new_value, NULL, NULL) + || strpbrk(ZSTR_VAL(new_value), SESSION_FORBIDDEN_CHARS) != NULL + ) { int err_type; if (stage == ZEND_INI_STAGE_RUNTIME || stage == ZEND_INI_STAGE_ACTIVATE || stage == ZEND_INI_STAGE_STARTUP) { @@ -694,7 +699,7 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */ /* Do not output error when restoring ini options. */ if (stage != ZEND_INI_STAGE_DEACTIVATE) { - php_error_docref(NULL, err_type, "session.name \"%s\" cannot be numeric or empty", ZSTR_VAL(new_value)); + php_error_docref(NULL, err_type, "session.name \"%s\" must not be numeric, empty, or contain any of the following characters \"" SESSION_FORBIDDEN_CHARS_FOR_ERROR_MSG "\"", ZSTR_VAL(new_value)); } return FAILURE; } @@ -1338,11 +1343,7 @@ static zend_result php_session_send_cookie(void) /* {{{ */ return FAILURE; } - /* Prevent broken Set-Cookie header, because the session_name might be user supplied */ - if (strpbrk(PS(session_name), SESSION_FORBIDDEN_CHARS) != NULL) { /* man isspace for \013 and \014 */ - php_error_docref(NULL, E_WARNING, "session.name cannot contain any of the following '=,;.[ \\t\\r\\n\\013\\014'"); - return FAILURE; - } + ZEND_ASSERT(strpbrk(PS(session_name), SESSION_FORBIDDEN_CHARS) == NULL); /* URL encode id because it might be user supplied */ e_id = php_url_encode(ZSTR_VAL(PS(id)), ZSTR_LEN(PS(id))); @@ -1462,7 +1463,10 @@ PHPAPI zend_result php_session_reset_id(void) /* {{{ */ } if (PS(use_cookies) && PS(send_cookie)) { - php_session_send_cookie(); + zend_result cookies_sent = php_session_send_cookie(); + if (UNEXPECTED(cookies_sent == FAILURE)) { + return FAILURE; + } PS(send_cookie) = 0; } diff --git a/ext/session/tests/bug66481.phpt b/ext/session/tests/bug66481.phpt index 88c2e48ed7999..fb8ca6cfa6f46 100644 --- a/ext/session/tests/bug66481.phpt +++ b/ext/session/tests/bug66481.phpt @@ -15,6 +15,6 @@ var_dump(session_name("foo")); var_dump(session_name("bar")); ?> --EXPECT-- -Warning: PHP Startup: session.name "" cannot be numeric or empty in Unknown on line 0 +Warning: PHP Startup: session.name "" must not be numeric, empty, or contain any of the following characters "=,;.[ \t\r\n\013\014" in Unknown on line 0 string(9) "PHPSESSID" string(3) "foo" diff --git a/ext/session/tests/gh17541.phpt b/ext/session/tests/gh17541.phpt new file mode 100644 index 0000000000000..867cc12835606 --- /dev/null +++ b/ext/session/tests/gh17541.phpt @@ -0,0 +1,30 @@ +--TEST-- +GH-17541 (ext/session NULL pointer dereferencement during ID reset) +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +Warning: session_name(): session.name " " must not be numeric, empty, or contain any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d +string(9) "PHPSESSID" +bool(true) +bool(true) +string(9) "PHPSESSID" +bool(true) +bool(true) diff --git a/ext/session/tests/session_name_variation1.phpt b/ext/session/tests/session_name_variation1.phpt index 07fb7f10ca89b..a4f2fde1f7199 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -38,25 +38,23 @@ ob_end_flush(); ?> --EXPECTF-- *** Testing session_name() : variation *** + +Warning: session_name(): session.name " " must not be numeric, empty, or contain any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" -string(9) "PHPSESSID" - -Warning: session_start(): session.name cannot contain any of the following '=,;.[ \t\r\n\013\014' in %s on line %d bool(true) -string(1) " " +string(9) "PHPSESSID" bool(true) -string(1) " " - -Warning: session_name(): session.name "" cannot be numeric or empty in %s on line %d -string(1) " " +string(9) "PHPSESSID" +string(9) "PHPSESSID" -Warning: session_start(): session.name cannot contain any of the following '=,;.[ \t\r\n\013\014' in %s on line %d +Warning: session_name(): session.name "" must not be numeric, empty, or contain any of the following characters "=,;.[ \t\r\n\013\014" in %s on line 1%d +string(9) "PHPSESSID" bool(true) -string(1) " " +string(9) "PHPSESSID" bool(true) -string(1) " " +string(9) "PHPSESSID" Done From b53551dccfd1b3c1081832d26b439dcf12001347 Mon Sep 17 00:00:00 2001 From: Gina Peter Banyard Date: Thu, 23 Jan 2025 16:49:39 +0000 Subject: [PATCH 2/2] Check for null bytes and simplify test --- ext/session/session.c | 3 ++- ext/session/tests/bug66481.phpt | 2 +- ext/session/tests/gh17541.phpt | 16 +++++----------- ext/session/tests/session_name_variation1.phpt | 8 +++++--- 4 files changed, 13 insertions(+), 16 deletions(-) diff --git a/ext/session/session.c b/ext/session/session.c index bf1454687e9b9..62a1db5ac8edf 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -686,6 +686,7 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */ /* Numeric session.name won't work at all */ if ( ZSTR_LEN(new_value) == 0 + || zend_str_has_nul_byte(new_value) || is_numeric_str_function(new_value, NULL, NULL) || strpbrk(ZSTR_VAL(new_value), SESSION_FORBIDDEN_CHARS) != NULL ) { @@ -699,7 +700,7 @@ static PHP_INI_MH(OnUpdateName) /* {{{ */ /* Do not output error when restoring ini options. */ if (stage != ZEND_INI_STAGE_DEACTIVATE) { - php_error_docref(NULL, err_type, "session.name \"%s\" must not be numeric, empty, or contain any of the following characters \"" SESSION_FORBIDDEN_CHARS_FOR_ERROR_MSG "\"", ZSTR_VAL(new_value)); + php_error_docref(NULL, err_type, "session.name \"%s\" must not be numeric, empty, contain null bytes or any of the following characters \"" SESSION_FORBIDDEN_CHARS_FOR_ERROR_MSG "\"", ZSTR_VAL(new_value)); } return FAILURE; } diff --git a/ext/session/tests/bug66481.phpt b/ext/session/tests/bug66481.phpt index fb8ca6cfa6f46..26a31279fbed2 100644 --- a/ext/session/tests/bug66481.phpt +++ b/ext/session/tests/bug66481.phpt @@ -15,6 +15,6 @@ var_dump(session_name("foo")); var_dump(session_name("bar")); ?> --EXPECT-- -Warning: PHP Startup: session.name "" must not be numeric, empty, or contain any of the following characters "=,;.[ \t\r\n\013\014" in Unknown on line 0 +Warning: PHP Startup: session.name "" must not be numeric, empty, contain null bytes or any of the following characters "=,;.[ \t\r\n\013\014" in Unknown on line 0 string(9) "PHPSESSID" string(3) "foo" diff --git a/ext/session/tests/gh17541.phpt b/ext/session/tests/gh17541.phpt index 867cc12835606..bbf6a50393b5e 100644 --- a/ext/session/tests/gh17541.phpt +++ b/ext/session/tests/gh17541.phpt @@ -6,25 +6,19 @@ session --FILE-- --EXPECTF-- -Warning: session_name(): session.name " " must not be numeric, empty, or contain any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d +Warning: session_destroy(): Trying to destroy uninitialized session in %s on line %d string(9) "PHPSESSID" bool(true) -bool(true) -string(9) "PHPSESSID" -bool(true) -bool(true) diff --git a/ext/session/tests/session_name_variation1.phpt b/ext/session/tests/session_name_variation1.phpt index a4f2fde1f7199..a298e643c0490 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -39,19 +39,21 @@ ob_end_flush(); --EXPECTF-- *** Testing session_name() : variation *** -Warning: session_name(): session.name " " must not be numeric, empty, or contain any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d +Warning: session_name(): session.name "" must not be numeric, empty, contain null bytes or any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" -bool(true) + +Warning: session_name(): session.name " " must not be numeric, empty, contain null bytes or any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID" +bool(true) string(9) "PHPSESSID" -Warning: session_name(): session.name "" must not be numeric, empty, or contain any of the following characters "=,;.[ \t\r\n\013\014" in %s on line 1%d +Warning: session_name(): session.name "" must not be numeric, empty, contain null bytes or any of the following characters "=,;.[ \t\r\n\013\014" in %s on line %d string(9) "PHPSESSID" bool(true) string(9) "PHPSESSID"