php://temp bug fixed in 8.1.6 is not backported to 8.1.2 release

Bug #1990302 reported by Martin Kucej
12
This bug affects 1 person
Affects Status Importance Assigned to Milestone
php-defaults (Ubuntu)
Invalid
Undecided
Unassigned
php8.1 (Ubuntu)
Invalid
Undecided
Unassigned
Jammy
Fix Released
Undecided
Athos Ribeiro

Bug Description

[ Impact ]

PHP gives users access to the temporary data stream php://temp (see https://www.php.net/manual/en/wrappers.php.php for further reference). These data streams are stored in memory until it reaches a predefined size limit. When it happens, PHP internally moves the data from memory to a temporary file.

In jammy, when thise move from memory to disk happens, the file position is not preserved. Instead, PHP will set the file position to the end of the file.

This will result in corrupted/unwanted data being generated when users are manipulating the files in a lower level.

The proposed patch fixes the issue by preserving the file position when the data stream is moved from memory to a file.

[ Test Plan ]

The following PHP snippet reproduces the bug. Run it after applying the fix and verify the output is as expected (listed after the script).

<?php

$f = fopen('php://temp/maxmemory:1024', 'r+');
fwrite($f, str_repeat("1", 738));
fseek($f, 0, SEEK_SET);
fwrite($f, str_repeat("2", 512));

fseek($f, 0, SEEK_SET);
var_dump(fread($f, 16));

fseek($f, 0, SEEK_END);
var_dump(ftell($f));

?>

Buggy PHP output:
$ php reproduce.php
string(16) "1111111111111111"
int(1250)

Fixed PHP output:
$ php reproduce.php
string(16) "2222222222222222"
int(738)

[ Where problems could occur ]

If the proposed patch (originated from and already released by the upstream project) contains any flaws, they would likely be manifested through a different bug when manipulating file positions for the php temporary data streams (instead of having a full regression). We would then need to work with upstream to find the best solution for the new issue.

[ Other Info ]

The proposed patch was released in PHP 8.1.6. Therefore, it only affects jammy.

[ Original message ]

Ubuntu 22.04 default PHP package (2:8.1+92ubuntu1) contains the pre-8.1.6 bug:

https://www.php.net/ChangeLog-8.php#8.1.6

Streams:

    Fixed php://temp does not preserve file-position when switched to temporary file.

Fixed in:

https://github.com/php/php-src/commit/84c18f9f04cb9852d992194e613927154f765192

Current workaround:

When opening php://temp, prevent its conversion to a file by increasing maxmemory to whatever number your application needs:

$fp = fopen("php://temp/maxmemory:5242880", 'w+');

Related branches

Changed in php-defaults (Ubuntu):
status: New → Invalid
no longer affects: php-defaults (Ubuntu Jammy)
Changed in php8.1 (Ubuntu):
status: New → Invalid
Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Thanks for taking the time to report this bug.

This is a minimal reproducer, available in the linked upstream patch:

<?php

$f = fopen('php://temp/maxmemory:1024', 'r+');
fwrite($f, str_repeat("1", 738));
fseek($f, 0, SEEK_SET);
fwrite($f, str_repeat("2", 512));

fseek($f, 0, SEEK_SET);
var_dump(fread($f, 16));

fseek($f, 0, SEEK_END);
var_dump(ftell($f));

?>

Buggy (jammy):
$ php reproduce.php
string(16) "1111111111111111"
int(1250)

Fixed (kinetic):
$ php reproduce.php
string(16) "2222222222222222"
int(738)

As one can see, the file position is indeed not being preserved in the affected run (jammy).

Changed in php8.1 (Ubuntu Jammy):
status: New → Triaged
assignee: nobody → Athos Ribeiro (athos-ribeiro)
tags: added: server-todo
description: updated
Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Hi, Martin.

I just uploaded a fix proposal to the following PPA: https://launchpad.net/~athos-ribeiro/+archive/ubuntu/lp1990302-tmp-stream-file-pos

Would you mind testing and verifying it fixes the issue for you?

Thank you!

Changed in php8.1 (Ubuntu Jammy):
status: Triaged → In Progress
Revision history for this message
Robie Basak (racb) wrote : Please test proposed package

Hello Martin, or anyone else affected,

Accepted php8.1 into jammy-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/php8.1/8.1.2-1ubuntu2.9 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-jammy to verification-done-jammy. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-jammy. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Changed in php8.1 (Ubuntu Jammy):
status: In Progress → Fix Committed
tags: added: verification-needed verification-needed-jammy
Revision history for this message
Robie Basak (racb) wrote :

(I rebased onto jammy-security for you)

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

Verifying on a clean jammy installation:

# php reproduce.php
string(16) "1111111111111111"
int(1250)

Now with the package in proposed:

# php reproduce.php
string(16) "2222222222222222"
int(738)

As it is expected (per the test plan).

LGTM.

tags: added: verification-done verification-done-jammy
removed: verification-needed verification-needed-jammy
Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

And BTW, thanks, Robie! For taking care of the rebase for us :)

Revision history for this message
Ubuntu SRU Bot (ubuntu-sru-bot) wrote : Autopkgtest regression report (php8.1/8.1.2-1ubuntu2.9)

All autopkgtests for the newly accepted php8.1 (8.1.2-1ubuntu2.9) for jammy have finished running.
The following regressions have been reported in tests triggered by the package:

php-pda-pheanstalk/4.0.4-1 (amd64)

Please visit the excuses page listed below and investigate the failures, proceeding afterwards as per the StableReleaseUpdates policy regarding autopkgtest regressions [1].

https://people.canonical.com/~ubuntu-archive/proposed-migration/jammy/update_excuses.html#php8.1

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Autopkgtest_Regressions

Thank you!

Revision history for this message
Athos Ribeiro (athos-ribeiro) wrote :

The regression above was a flaky test. Re-triggering it fixed the issue.

Revision history for this message
Andreas Hasenack (ahasenack) wrote : Update Released

The verification of the Stable Release Update for php8.1 has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package php8.1 - 8.1.2-1ubuntu2.9

---------------
php8.1 (8.1.2-1ubuntu2.9) jammy; urgency=medium

  * d/p/0049-Preserve-file-position-when-php-temp-switches.patch: PHP provides
    a temporary data stream, php://temp, whose contents are moved to a
    temporary file when a predefined size limit is hit. In jammy, the file
    position is set to the end of the file, which results in corrupted/unwanted
    data. Fix this by preserving the file position in this situation.
    (LP: #1990302)

 -- Athos Ribeiro <email address hidden> Wed, 19 Oct 2022 11:58:09 -0300

Changed in php8.1 (Ubuntu Jammy):
status: Fix Committed → Fix Released
Revision history for this message
Martin Kucej (mkucej) wrote :

Our servers were updated and this bug is fully mitigated. Thank you so much for working on this!!! Although not a CVE, this bug affects such a commonly used functionality that it needed a backport.

BTW, I would argue this bug should have received a CVE. It's easy to imagine than string(16) "1111111111111111" is some kind of secret.

Thanks again.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.