Repository navigation
Fix GH-24493: preserve session data when upload-progress decoding fails - #24677
Closed
programsurf wants to merge 1 commit into
Closed
programsurf wants to merge 1 commit into
programsurf wants to merge 1 commit into
Conversation
…failures Keep the restricted pre-RINIT decoder intact, but abort its progress update without deleting the stored session. Add public-API round-trip coverage for three serializers, reserved properties, cleanup modes and object aliases. Acknowledges the abort-update approach in lazerg/php-src commit a45ffe7.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix GH-24493: preserve session data when upload-progress decoding fails
A valid session written with
session_start()/session_write_close()may contain an ordinary object property named__PHP_Incomplete_Class_Name,__PHP_Incomplete_Class_Serialized, or__PHP_Incomplete_Enum_Case. During a multipart upload, PHP 8.5.12's restricted pre-RINIT decoder rejects those names. The shared decode-failure path then destroys the entire stored session, including unrelated application data.This change aborts only the upload-progress session update when decoding fails in
in_rfc1867_update. It closes the session handler without writing or deleting the stored record. The normal application request can subsequently decode its session with the regular decoder.The reserved-member checks and
incomplete_classes_onlyrestriction remain in place. Ordinarysession_start()decode failures keep their existing behavior. For a record the restricted decoder cannot handle, progress updates are skipped; the upload itself and the stored application data are preserved.Changes
php_session_cancel_decode().gh24493_upload_progress_session_roundtrip.phpt: sessions are produced through the public PHP API, then uploaded with the same cookie and read back. The test covers all three serializers, three reserved names plus a normal-name control, and both cleanup settings (24 combinations).$_SESSIONexposure, independent stored literals, object alias identity, and byte-identical stored records immediately after rejected progress updates.Validation
PR base:
PHP-8.4, commit055e11e0c7e6fe5a9649afc2b79391608933ee79. On that branch, the same frozen normal suites pass 40/40 twice after repair (official build: Blue 18/20, Red 10/20). The full session suite has 267 passes, one existing expected failure, one skip requiring unavailable posix/pcntl extensions, and no unexpected failures. The reserved-property and no-user-code regression tests pass.Additional validation against PHP 8.5.12, commit
136e049ac10b032c83018e65b63b6df858943750, using Clang/AddressSanitizer and real PHP-FPM/CGI execution:ext/session/tests: 269 passed, one pre-existing expected failure (bug71162.phpt), zero unexpected failures, zero skips.rfc1867_no_user_code.phptpasses, including raw C:/E: payload preservation and callback timing checks.Prior work and attribution
This is the known regression in php/php-src#24493, not a new vulnerability discovery. The abort-update approach also appears in Lazizbek Ergashev (lazerg)'s commit a45ffe71. This local proposal acknowledges that implementation and adds a public-API round-trip matrix and independent validation evidence. The additional public-API regression matrix and execution results can also be incorporated into that existing fix if preferred.
The attached results concern this source version and these executions. The skipped progress update is an intentional fallback for session records unsupported by the restricted startup decoder.