diff --git a/ext/session/session.c b/ext/session/session.c index b838b132b16d0..62a1db5ac8edf 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,12 @@ 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 + || zend_str_has_nul_byte(new_value) + || 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 +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\" cannot be numeric or empty", 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; } @@ -1338,11 +1344,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 +1464,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..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 "" cannot be numeric or empty 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 new file mode 100644 index 0000000000000..bbf6a50393b5e --- /dev/null +++ b/ext/session/tests/gh17541.phpt @@ -0,0 +1,24 @@ +--TEST-- +GH-17541 (ext/session NULL pointer dereferencement during ID reset) +--EXTENSIONS-- +session +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +Warning: session_destroy(): Trying to destroy uninitialized session in %s on line %d +string(9) "PHPSESSID" +bool(true) diff --git a/ext/session/tests/session_name_variation1.phpt b/ext/session/tests/session_name_variation1.phpt index 07fb7f10ca89b..a298e643c0490 100644 --- a/ext/session/tests/session_name_variation1.phpt +++ b/ext/session/tests/session_name_variation1.phpt @@ -38,25 +38,25 @@ ob_end_flush(); ?> --EXPECTF-- *** Testing session_name() : variation *** + +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" -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, 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(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" -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, 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(1) " " +string(9) "PHPSESSID" bool(true) -string(1) " " +string(9) "PHPSESSID" Done