From 790e3fec9df153a1dd722e0ce74300b23d2bf74d Mon Sep 17 00:00:00 2001 From: Brett Gardner Date: Thu, 9 Mar 2023 09:37:54 +1100 Subject: [PATCH 1/3] BUG-10807: session_regenerate_id fails to check results of validateId correctly --- ext/session/session.c | 2 +- ext/session/tests/bug10807.phpt | 69 +++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 ext/session/tests/bug10807.phpt diff --git a/ext/session/session.c b/ext/session/session.c index 5c8cf470b33f6..698fdf03c1b8f 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -2295,7 +2295,7 @@ PHP_FUNCTION(session_regenerate_id) if ((!PS(mod_user_implemented) && PS(mod)->s_validate_sid) || !Z_ISUNDEF(PS(mod_user_names).ps_validate_sid)) { int limit = 3; /* Try to generate non-existing ID */ - while (limit-- && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == SUCCESS) { + while (limit-- && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == FAILURE) { zend_string_release_ex(PS(id), 0); PS(id) = PS(mod)->s_create_sid(&PS(mod_data)); if (!PS(id)) { diff --git a/ext/session/tests/bug10807.phpt b/ext/session/tests/bug10807.phpt new file mode 100644 index 0000000000000..2571cd3721074 --- /dev/null +++ b/ext/session/tests/bug10807.phpt @@ -0,0 +1,69 @@ +--TEST-- +Bug #10807 (session_regenerate_id with custom handler and use_strict_mode generates three new session ids) +--EXTENSIONS-- +session +--INI-- +html_errors=0 +session.save_handler=files +session.use_strict_mode=1 +--FILE-- +fail_validation = FALSE; + $this->sid_counter = 1; + + session_set_save_handler($this, FALSE); + } + + public function setFailValidation() { + $this->fail_validation = TRUE; + } + + public function create_sid() : string { + return strval($this->sid_counter++); + } + + public function validateId(string $id) : bool { + + if ($this->fail_validation && intval($id)< 4) { + return FALSE; + } + + return TRUE; + + } + + public function close(): bool { return TRUE; } + public function destroy(string $id): bool { return TRUE; } + public function gc(int $max_lifetime): int|false { return TRUE; } + public function open(string $path, string $name): bool { return TRUE; } + public function read(string $id): string|false { return ''; } + public function write(string $id, string $data): bool { return TRUE; } + +} + +ob_start(); + +$save_handler = new Bug10807SessionHandler(); +session_start(); //session id = 1 +session_regenerate_id(); //session id = 2 +var_dump(session_id()); + +$save_handler->setFailValidation(); +session_regenerate_id(); //should invoke create_sid twice as session id 3 will be invalid +var_dump(session_id()); + +?> +--EXPECT-- +string(1) "2" +string(1) "4" From 281ec2d1c997fd23ed1b1342dcc71febc1384355 Mon Sep 17 00:00:00 2001 From: Brett Gardner Date: Thu, 9 Mar 2023 10:38:10 +1100 Subject: [PATCH 2/3] Renamed files as per request --- ext/session/tests/bug10807.phpt | 69 --------------------------------- 1 file changed, 69 deletions(-) delete mode 100644 ext/session/tests/bug10807.phpt diff --git a/ext/session/tests/bug10807.phpt b/ext/session/tests/bug10807.phpt deleted file mode 100644 index 2571cd3721074..0000000000000 --- a/ext/session/tests/bug10807.phpt +++ /dev/null @@ -1,69 +0,0 @@ ---TEST-- -Bug #10807 (session_regenerate_id with custom handler and use_strict_mode generates three new session ids) ---EXTENSIONS-- -session ---INI-- -html_errors=0 -session.save_handler=files -session.use_strict_mode=1 ---FILE-- -fail_validation = FALSE; - $this->sid_counter = 1; - - session_set_save_handler($this, FALSE); - } - - public function setFailValidation() { - $this->fail_validation = TRUE; - } - - public function create_sid() : string { - return strval($this->sid_counter++); - } - - public function validateId(string $id) : bool { - - if ($this->fail_validation && intval($id)< 4) { - return FALSE; - } - - return TRUE; - - } - - public function close(): bool { return TRUE; } - public function destroy(string $id): bool { return TRUE; } - public function gc(int $max_lifetime): int|false { return TRUE; } - public function open(string $path, string $name): bool { return TRUE; } - public function read(string $id): string|false { return ''; } - public function write(string $id, string $data): bool { return TRUE; } - -} - -ob_start(); - -$save_handler = new Bug10807SessionHandler(); -session_start(); //session id = 1 -session_regenerate_id(); //session id = 2 -var_dump(session_id()); - -$save_handler->setFailValidation(); -session_regenerate_id(); //should invoke create_sid twice as session id 3 will be invalid -var_dump(session_id()); - -?> ---EXPECT-- -string(1) "2" -string(1) "4" From d4a8a024ab1124c604cadda9b34780669995d974 Mon Sep 17 00:00:00 2001 From: Brett Gardner Date: Thu, 9 Mar 2023 11:52:00 +1100 Subject: [PATCH 3/3] Added further test for session_create_id during active session with failing and non failing validation --- ext/session/session.c | 2 +- ext/session/tests/gh10807.phpt | 83 ++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 ext/session/tests/gh10807.phpt diff --git a/ext/session/session.c b/ext/session/session.c index 698fdf03c1b8f..43bc5081442ca 100644 --- a/ext/session/session.c +++ b/ext/session/session.c @@ -2362,7 +2362,7 @@ PHP_FUNCTION(session_create_id) break; } else { /* Detect collision and retry */ - if (PS(mod)->s_validate_sid(&PS(mod_data), new_id) == SUCCESS) { + if (PS(mod)->s_validate_sid(&PS(mod_data), new_id) == FAILURE) { zend_string_release_ex(new_id, 0); new_id = NULL; continue; diff --git a/ext/session/tests/gh10807.phpt b/ext/session/tests/gh10807.phpt new file mode 100644 index 0000000000000..6466370dbe190 --- /dev/null +++ b/ext/session/tests/gh10807.phpt @@ -0,0 +1,83 @@ +--TEST-- +GH-10807 (session_regenerate_id with custom handler and use_strict_mode generates three new session ids) +--EXTENSIONS-- +session +--INI-- +html_errors=0 +session.save_handler=files +session.use_strict_mode=1 +--FILE-- +fail_validation = 0; + $this->sid_counter = 1; + + session_set_save_handler($this, FALSE); + } + + public function setFailValidation(int $fail) { + $this->fail_validation = $fail; + } + + public function create_sid() : string { + return strval($this->sid_counter++); + } + + public function validateId(string $id) : bool { + + if ($this->fail_validation > 0 && intval($id)< $this->fail_validation) { + return FALSE; + } + + return TRUE; + + } + + public function close(): bool { return TRUE; } + public function destroy(string $id): bool { return TRUE; } + public function gc(int $max_lifetime): int|false { return TRUE; } + public function open(string $path, string $name): bool { return TRUE; } + public function read(string $id): string|false { return ''; } + public function write(string $id, string $data): bool { return TRUE; } + +} + +ob_start(); + +$save_handler = new Bug10807SessionHandler(); +session_start(); //session id = 1 +session_regenerate_id(); //session id = 2 +var_dump(session_id()); + +$save_handler->setFailValidation(4); +session_regenerate_id(); //should invoke create_sid twice as session id 3 will be invalid +var_dump(session_id()); + +$save_handler->setFailValidation(8); +$new_id = session_create_id(); //Should cause an error due to failing validation 3 times +print("\n"); //This is required to get the EXPECTF to work below +var_dump($new_id); + +$save_handler->setFailValidation(0); +$new_id = session_create_id(); //Should succeed +var_dump($new_id); + +?> +--EXPECTF-- +string(1) "2" +string(1) "4" + +Warning: session_create_id(): Failed to create new ID in %s on line %d + +bool(false) +string(1) "8"