Skip to content

Fix GH-24493: preserve session data when upload-progress decoding fails - #24677

Closed
programsurf wants to merge 1 commit into
php:PHP-8.4from
programsurf:fix/gh24493-upload-progress-record-preservation
Closed

programsurf wants to merge 1 commit into
php:PHP-8.4from
programsurf:fix/gh24493-upload-progress-record-preservation

Conversation

@programsurf

@programsurf programsurf commented Oct 25, 2026 •

Copy link
Copy Markdown

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_only restriction remain in place. Ordinary session_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

  • Add the upload-progress-specific abort path in php_session_cancel_decode().
  • Update the existing reserved-property regression test to require stored-record preservation.
  • Add 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).
  • Check upload success, no premature $_SESSION exposure, independent stored literals, object alias identity, and byte-identical stored records immediately after rejected progress updates.

Validation

PR base: PHP-8.4, commit 055e11e0c7e6fe5a9649afc2b79391608933ee79. 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:

  • Both suites repeated twice with unchanged inputs and expected results.
  • Original stale-fatal-backtrace PoC: vulnerable control reproduces UAF twice; official and repaired builds do not reproduce it in the same replay.
  • Retained session-loss witness: official build loses the stored values twice; repaired build retains them twice.
  • Full ext/session/tests: 269 passed, one pre-existing expected failure (bug71162.phpt), zero unexpected failures, zero skips.
  • New and updated regression tests both fail on the unmodified official build (negative control).
  • Existing rfc1867_no_user_code.phpt passes, 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.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant