Skip to content

Preserve session records and release buffers after upload-progress decode failures #24679

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
programsurf wants to merge 1 commit into
base: PHP-8.4
Choose a base branch
from
+301 −10
Open

Preserve session records and release buffers after upload-progress decode failures #24679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ext/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,17 @@ static zend_string *php_session_encode(void) /* {{{ */

static ZEND_COLD void php_session_cancel_decode(void)
{
if (PS(in_rfc1867_update)) {
/* The upload progress decoder is deliberately more restrictive than
* session_start(). Leave the stored record for the application to read. */
php_session_abort();
/* An aborted update will not use the lazy-write comparison buffer. */
if (PS(session_vars)) {
zend_string_release_ex(PS(session_vars), 0);
PS(session_vars) = NULL;
}
return;
}
php_session_destroy();
php_session_track_init();
php_error_docref(NULL, E_WARNING, "Failed to decode session object. Session has been destroyed");
Expand Down
130 changes: 130 additions & 0 deletions ext/session/tests/gh24493_upload_progress_memory.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
--TEST--
GH-24493: aborted upload progress updates release the lazy-write buffer
--EXTENSIONS--
session
--SKIPIF--
<?php
include('skipif.inc');
if (!getenv('TEST_PHP_CGI_EXECUTABLE')) {
die('skip CGI not available');
}
if (!function_exists('proc_open')) {
die('skip proc_open not available');
}
?>
--FILE--
<?php
$dir = __DIR__ . '/gh24493_upload_progress_memory';
@mkdir($dir);
$worker = "$dir/worker.php";
file_put_contents($worker, <<<'CODE'
<?php
$action = getenv('TEST_ACTION');
if ($action === 'upload') {
echo 'UPLOAD:', $_FILES['file']['error'], ':',
file_get_contents($_FILES['file']['tmp_name']), ':',
isset($_SESSION) ? 'exposed' : 'clean', "\n";
return;
}
session_start();
if ($action === 'seed') {
$object = new stdClass;
$object->__PHP_Incomplete_Class_Name = 'application value';
/* The restricted decoder rejects the object before reading the payload. */
$_SESSION = ['object' => $object, 'payload' => str_repeat('A', 5 * 1024 * 1024)];
session_write_close();
echo "SEEDED\n";
return;
}
$ok = ($_SESSION['object']->__PHP_Incomplete_Class_Name ?? null) === 'application value'
&& ($_SESSION['payload'] ?? null) === str_repeat('A', 5 * 1024 * 1024);
session_write_close();
echo $ok ? "PRESERVED\n" : "LOST\n";
CODE);
$boundary = 'sessionmemoryboundary';
$body = "--$boundary\r\nContent-Disposition: form-data; name=\"PHP_SESSION_UPLOAD_PROGRESS\"\r\n\r\nkey\r\n"
. "--$boundary\r\nContent-Disposition: form-data; name=\"file\"; filename=\"file.txt\"\r\n\r\nabc\r\n"
. "--$boundary--\r\n";
$ini = ['session.save_path' => $dir, 'session.save_handler' => 'files',
'session.gc_probability' => '0', 'session.use_strict_mode' => '0',
'session.use_cookies' => '1', 'session.use_only_cookies' => '1',
'session.cache_limiter' => '', 'session.name' => 'PHPSESSID',
'session.lazy_write' => '1', 'session.upload_progress.enabled' => '1',
'session.upload_progress.name' => 'PHP_SESSION_UPLOAD_PROGRESS',
'session.upload_progress.freq' => '1', 'session.upload_progress.min_freq' => '0',
'file_uploads' => '1', 'display_errors' => '1', 'html_errors' => '0'];
$extensionDir = ini_get('extension_dir');
foreach (['session.' . PHP_SHLIB_SUFFIX, 'php_session.' . PHP_SHLIB_SUFFIX] as $module) {
if ($extensionDir !== false && file_exists("$extensionDir/$module")) {
$ini['extension_dir'] = $extensionDir;
$ini['extension'] = $module;
break;
}
}
foreach (['php', 'php_binary', 'php_serialize'] as $serializer) {
foreach ([0, 1] as $cleanup) {
$sid = 'memory' . $cleanup;
@unlink("$dir/sess_$sid");
$ok = true;
foreach (['seed', 'upload', 'read'] as $action) {
$cmd = [getenv('TEST_PHP_CGI_EXECUTABLE'), '-n', '-q'];
$settings = $ini + ['session.serialize_handler' => $serializer,
'session.upload_progress.cleanup' => (string) $cleanup,
'memory_limit' => $action === 'upload' ? '8M' : '64M'];
foreach ($settings as $name => $value) {
$cmd[] = '-d';
$cmd[] = "$name=$value";
}
$cmd[] = '-f';
$cmd[] = $worker;
$env = getenv();
$env['REDIRECT_STATUS'] = '1';
$env['REQUEST_METHOD'] = $action === 'upload' ? 'POST' : 'GET';
$env['SCRIPT_FILENAME'] = $worker;
$env['PATH_TRANSLATED'] = $worker;
$env['QUERY_STRING'] = '';
$env['HTTP_COOKIE'] = "PHPSESSID=$sid";
$env['CONTENT_TYPE'] = "multipart/form-data; boundary=$boundary";
$env['CONTENT_LENGTH'] = $action === 'upload' ? (string) strlen($body) : '0';
$env['TEST_ACTION'] = $action;
$proc = proc_open($cmd, [0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w']], $pipes, $dir, $env);
if ($action === 'upload') {
fwrite($pipes[0], $body);
}
fclose($pipes[0]);
$output = stream_get_contents($pipes[1]);
$error = stream_get_contents($pipes[2]);
fclose($pipes[1]);
fclose($pipes[2]);
$exit = proc_close($proc);
$expected = match ($action) {
'seed' => 'SEEDED', 'upload' => 'UPLOAD:0:abc:clean', 'read' => 'PRESERVED'
};
$ok = $ok && $exit === 0 && $error === '' && str_contains($output, $expected)
&& !str_contains($output, 'Warning:') && !str_contains($output, 'Fatal error:');
if ($action === 'seed') {
$recordHash = hash_file('sha256', "$dir/sess_$sid");
} elseif ($action === 'upload') {
$ok = $ok && file_exists("$dir/sess_$sid")
&& hash_file('sha256', "$dir/sess_$sid") === $recordHash;
}
}
printf("%s cleanup=%d %s\n", $serializer, $cleanup, $ok ? 'OK' : 'FAIL');
}
}
?>
--CLEAN--
<?php
$dir = __DIR__ . '/gh24493_upload_progress_memory';
foreach (glob("$dir/*") as $file) {
unlink($file);
}
rmdir($dir);
?>
--EXPECT--
php cleanup=0 OK
php cleanup=1 OK
php_binary cleanup=0 OK
php_binary cleanup=1 OK
php_serialize cleanup=0 OK
php_serialize cleanup=1 OK
151 changes: 151 additions & 0 deletions ext/session/tests/gh24493_upload_progress_session_roundtrip.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
--TEST--
GH-24493: upload progress preserves application-generated session records across serializers
--EXTENSIONS--
session
--SKIPIF--
<?php
include('skipif.inc');
if (!getenv('TEST_PHP_CGI_EXECUTABLE')) {
die('skip CGI not available');
}
?>
--FILE--
<?php
$dir = __DIR__ . '/gh24493_upload_progress_session_roundtrip';
@mkdir($dir);
$worker = "$dir/worker.php";
file_put_contents($worker, <<<'CODE'
<?php
$property = getenv('TEST_PROPERTY');
$action = getenv('TEST_ACTION');
if ($action === 'upload') {
echo 'UPLOAD:', $_FILES['file']['error'], ':',
isset($_SESSION) ? 'exposed' : 'clean', "\n";
return;
}
session_start();
if ($action === 'seed') {
$object = new stdClass;
$object->{$property} = 'BUSINESS_LITERAL';
$_SESSION = ['object' => $object, 'alias' => $object, 'sentinel' => 'KEEP_LITERAL'];
session_write_close();
echo "SEEDED\n";
return;
}
$object = $_SESSION['object'] ?? null;
$ok = $object instanceof stdClass
&& ($object->{$property} ?? null) === 'BUSINESS_LITERAL'
&& ($_SESSION['sentinel'] ?? null) === 'KEEP_LITERAL'
&& $object === ($_SESSION['alias'] ?? null);
session_write_close();
echo $ok ? "PRESERVED\n" : "LOST\n";
CODE);
$properties = ['normal', '__PHP_Incomplete_Class_Name',
'__PHP_Incomplete_Class_Serialized', '__PHP_Incomplete_Enum_Case'];
$boundary = 'gh24493boundary';
$body = "--$boundary\r\nContent-Disposition: form-data; name=\"PHP_SESSION_UPLOAD_PROGRESS\"\r\n\r\nkey\r\n"
. "--$boundary\r\nContent-Disposition: form-data; name=\"file\"; filename=\"normal.txt\"\r\n\r\nabc\r\n"
. "--$boundary--\r\n";
$ini = ['session.save_path' => $dir, 'session.save_handler' => 'files',
'session.use_strict_mode' => '0', 'session.use_cookies' => '1',
'session.use_only_cookies' => '1', 'session.cache_limiter' => '',
'session.name' => 'PHPSESSID', 'session.upload_progress.enabled' => '1',
'session.upload_progress.name' => 'PHP_SESSION_UPLOAD_PROGRESS',
'session.upload_progress.min_freq' => '0', 'session.upload_progress.freq' => '1',
'file_uploads' => '1', 'display_errors' => '1', 'html_errors' => '0'];
$extensionDir = ini_get('extension_dir');
foreach (['session.' . PHP_SHLIB_SUFFIX, 'php_session.' . PHP_SHLIB_SUFFIX] as $module) {
if ($extensionDir !== false && file_exists("$extensionDir/$module")) {
$ini['extension_dir'] = $extensionDir;
$ini['extension'] = $module;
break;
}
}
foreach (['php', 'php_binary', 'php_serialize'] as $serializer) {
foreach ([0, 1] as $cleanup) {
foreach ($properties as $index => $property) {
$sid = 'roundtrip' . $cleanup . $index;
@unlink("$dir/sess_$sid");
$cmd = [getenv('TEST_PHP_CGI_EXECUTABLE'), '-n', '-q'];
$settings = $ini + ['session.serialize_handler' => $serializer,
'session.upload_progress.cleanup' => (string) $cleanup];
foreach ($settings as $name => $value) {
$cmd[] = '-d';
$cmd[] = "$name=$value";
}
$cmd[] = '-f';
$cmd[] = $worker;
$ok = true;
foreach (['seed', 'upload', 'read'] as $action) {
$env = getenv();
$env['REDIRECT_STATUS'] = '1';
$env['REQUEST_METHOD'] = $action === 'upload' ? 'POST' : 'GET';
$env['SCRIPT_FILENAME'] = $worker;
$env['PATH_TRANSLATED'] = $worker;
$env['QUERY_STRING'] = '';
$env['HTTP_COOKIE'] = "PHPSESSID=$sid";
$env['CONTENT_TYPE'] = "multipart/form-data; boundary=$boundary";
$env['CONTENT_LENGTH'] = $action === 'upload' ? (string) strlen($body) : '0';
$env['TEST_PROPERTY'] = $property;
$env['TEST_ACTION'] = $action;
$proc = proc_open($cmd, [0 => ['pipe','r'], 1 => ['pipe','w'], 2 => ['pipe','w']], $pipes, $dir, $env);
if ($action === 'upload') {
fwrite($pipes[0], $body);
}
fclose($pipes[0]);
$output = stream_get_contents($pipes[1]);
$error = stream_get_contents($pipes[2]);
fclose($pipes[1]);
fclose($pipes[2]);
$exit = proc_close($proc);
$expected = match ($action) {
'seed' => 'SEEDED', 'upload' => 'UPLOAD:0:clean', 'read' => 'PRESERVED'
};
$ok = $ok && $exit === 0 && $error === '' && str_contains($output, $expected)
&& !str_contains($output, 'Warning:');
if ($action === 'seed') {
$record = file_get_contents("$dir/sess_$sid");
} elseif ($action === 'upload' && $property !== 'normal') {
/* A rejected progress update must not rewrite even a partial record. */
$ok = $ok && file_exists("$dir/sess_$sid")
&& file_get_contents("$dir/sess_$sid") === $record;
}
}
printf("%s cleanup=%d property=%d %s\n", $serializer, $cleanup, $index, $ok ? 'OK' : 'FAIL');
}
}
}
?>
--CLEAN--
<?php
$dir = __DIR__ . '/gh24493_upload_progress_session_roundtrip';
foreach (glob("$dir/*") as $file) {
unlink($file);
}
rmdir($dir);
?>
--EXPECT--
php cleanup=0 property=0 OK
php cleanup=0 property=1 OK
php cleanup=0 property=2 OK
php cleanup=0 property=3 OK
php cleanup=1 property=0 OK
php cleanup=1 property=1 OK
php cleanup=1 property=2 OK
php cleanup=1 property=3 OK
php_binary cleanup=0 property=0 OK
php_binary cleanup=0 property=1 OK
php_binary cleanup=0 property=2 OK
php_binary cleanup=0 property=3 OK
php_binary cleanup=1 property=0 OK
php_binary cleanup=1 property=1 OK
php_binary cleanup=1 property=2 OK
php_binary cleanup=1 property=3 OK
php_serialize cleanup=0 property=0 OK
php_serialize cleanup=0 property=1 OK
php_serialize cleanup=0 property=2 OK
php_serialize cleanup=0 property=3 OK
php_serialize cleanup=1 property=0 OK
php_serialize cleanup=1 property=1 OK
php_serialize cleanup=1 property=2 OK
php_serialize cleanup=1 property=3 OK
19 changes: 9 additions & 10 deletions ext/session/tests/rfc1867_incomplete_class_reserved_props.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
--TEST--
session rfc1867: the upload progress update rejects session data whose objects carry the incomplete class bookkeeping property names
session rfc1867: the upload progress update preserves records it cannot decode without user code
--EXTENSIONS--
session
--SKIPIF--
Expand All @@ -23,7 +23,8 @@ file_put_contents($worker, '<?php');
* __PHP_Incomplete_Class, which stores the class name of each object and the
* payload of C:/E: entries under these three property names. Data that already
* carries one of them cannot be told apart from that bookkeeping, so the decode
* has to fail rather than let the two meanings be confused. */
* has to fail rather than let the two meanings be confused. The progress
* update must abort without deleting or rewriting the stored record. */
$cases = [
'control' => 'x|O:8:"stdClass":1:{s:1:"a";i:1;}',
'Class_Name' => 'x|O:8:"stdClass":1:{s:27:"__PHP_Incomplete_Class_Name";s:3:"foo";}',
Expand Down Expand Up @@ -109,9 +110,7 @@ foreach ($cases as $label => $record) {
fclose($pipes[2]);
proc_close($proc);

/* A refused decode must leave exactly one warning: the handler stops there
* instead of writing the progress into the empty session it was left with,
* which would add a second warning about the save path. */
/* A progress-only decode failure must leave the stored session intact. */
$kept = file_exists($file) && str_starts_with(file_get_contents($file), $record);
printf("%-16s warnings: %d, decode refused: %-5s record kept: %s\n", $label,
substr_count($out, 'Warning:'),
Expand All @@ -131,9 +130,9 @@ foreach (glob("$dir/sess_*") as $f) {
?>
--EXPECT--
control warnings: 0, decode refused: false record kept: true
Class_Name warnings: 1, decode refused: true record kept: false
Class_Serialized warnings: 1, decode refused: true record kept: false
Enum_Case warnings: 1, decode refused: true record kept: false
nested warnings: 1, decode refused: true record kept: false
Class_Name warnings: 0, decode refused: false record kept: true
Class_Serialized warnings: 0, decode refused: false record kept: true
Enum_Case warnings: 0, decode refused: false record kept: true
nested warnings: 0, decode refused: false record kept: true
mangled warnings: 0, decode refused: false record kept: true
corrupt warnings: 1, decode refused: true record kept: false
corrupt warnings: 0, decode refused: false record kept: true
Loading