php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #54374 Insufficient validating of upload name leading to corrupted $_FILES indices
Submitted: 2011-03-24 20:07 UTC Modified: 2012-03-21 06:03 UTC
Votes:2
Avg. Score:3.0 ± 2.0
Reproduced:0 of 0 (0.0%)
From: lekensteyn at gmail dot com Assigned: pajoye (profile)
Status: Closed Package: Variables related
PHP Version: 5.3.8 OS: All
Private report: No CVE-ID: 2012-1172
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: lekensteyn at gmail dot com
New email:
PHP Version: OS:

 

 [2011-03-24 20:07 UTC] lekensteyn at gmail dot com
Description:
------------
SAPI: Apache 2 module (it should apply to other SAPI's which accepts uploads as well)
OS: Debian 6 (it should apply to other OSes as well)
PHP: 5.3.6 (from source, test compile: ./configure --prefix=/tmp/diebug --disable-all --with-apxs2=/tmp/diebug/bin/apxs --disable-cli)

Upload names with brackets ([ and ]) are created for creating arrays of files.
Any array index or variable name containing a bracket should be invalid.

The current implementation only checks whether more closing brackets are detected than opening brackets.
Related files
http://lxr.php.net/opengrok/xref/PHP_5_3/main/rfc1867.c#990
http://lxr.php.net/opengrok/xref/PHP_TRUNK/main/rfc1867.c#920

Relevant code:
--snip--
/* New Rule: never repair potential malicious user input */
if (!skip_upload) {
	long c = 0;
	tmp = param;

	while (*tmp) {
		if (*tmp == '[') {
			c++;
		} else if (*tmp == ']') {
			c--;
			if (tmp[1] && tmp[1] != '[') {
				skip_upload = 1;
				break;
			}
		}
		if (c < 0) {
			skip_upload = 1;
			break;
		}
		tmp++;
	}
}
--snip--

So names like `test]` and `test[]]` are invalid, but names like `test[` pass this test.

Now it gets worse, the upload is accepted and without checking the name, and registered:
--snip--
if (is_arr_upload) {
	snprintf(lbuf, llen, "%s[name][%s]", abuf, array_index);
} else {
	snprintf(lbuf, llen, "%s[name]", param);
}
if (s && s > filename) {
	register_http_post_files_variable(lbuf, s+1, http_post_files, 0 TSRMLS_CC);
} else {
	register_http_post_files_variable(lbuf, filename, http_post_files, 0 TSRMLS_CC);
}
--snip--

register_http_post_files_variable calls safe_php_register_variable:
--snip
if (override_protection || !is_protected_variable(var TSRMLS_CC)) {
	php_register_variable_safe(var, strval, val_len, track_vars_array TSRMLS_CC);
}
--snip--

override_protection is false, the only condition that checks whether the variable name is accepted is the is_protected_variable call, passing the upload name. The variable name is normalized using normalize_protected_variable() and then checked for existence in the $_FILES array.
The normalization function normalize_protected_variable checks whether a closing bracket is found, and otherwise uses the following string as index:
--snip--
	indexend = strchr(index, ']');
	indexend = indexend ? indexend + 1 : index + strlen(index);

--snip--
This implies that the index name can contain a opening bracket as well, which will be accepted and passed directly to php_register_variable_safe.

The suggested patch adds a check to ensure that the leftover open brackets is always zero. If not, it simply drops the upload (better safe than sorry).

Test script:
---------------
<?php
if (isset($_POST['submitted'])) {
	if (isset($_FILES['test'])) {
		if (isset($_FILES['test']['error'])) {
			echo 'OK expected result';
		} else if(isset($_FILES['test']['[error'])) {
			echo 'Unexpected result';
		}
	} else {
		echo 'OK expected result';
	}
	printf('<pre>%s</pre>', htmlspecialchars(print_r($_FILES, true)));
}
?>
<form enctype="multipart/form-data" method="post">
<input type="file" name="test[">
<input type="submit" name="submitted">
</form>


Expected result:
----------------
I expected to see "OK expected result" and an empty array dump because the name is invalid.

Actual result:
--------------
The test script produces "Unexpected result".
The upload is accepted but the $_FILES array is corrupted:
Array
(
    [test] => Array
        (
            [[name] => 
            [[type] => 
            [[tmp_name] => 
            [[error] => 4
            [[size] => 0
        )

)

Patches

check-for-unclosed-opening-brackets (last revision 2011-03-24 19:09 UTC by lekensteyn at gmail dot com)

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2011-11-18 10:40 UTC] lekensteyn at gmail dot com
This is not a documentation bug and still exists in PHP 5.3.8.
 [2011-11-18 10:40 UTC] lekensteyn at gmail dot com
-Type: Documentation Problem +Type: Bug -PHP Version: 5.3.6 +PHP Version: 5.3.8
 [2012-01-01 23:53 UTC] stas@php.net
Automatic comment from SVN on behalf of stas
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=321664
Log: fix bug #54374, bug #55500 - filter file names better, no dangling [s
 [2012-01-01 23:55 UTC] stas@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: stas
 [2012-01-01 23:55 UTC] stas@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.


 [2012-03-13 12:52 UTC] pajoye@php.net
-CVE-ID: +CVE-ID: 2012-1172
 [2012-03-21 06:03 UTC] pajoye@php.net
-Assigned To: stas +Assigned To: pajoye
 [2012-03-21 06:03 UTC] pajoye@php.net
Merged into 5.3
 [2012-04-18 09:46 UTC] laruence@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7c3177e5ab4129f9b9e9764ed0e4faa4f87e443b
Log: fix bug #54374, bug #55500 - filter file names better, no dangling [s
 [2012-07-24 23:37 UTC] rasmus@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7c3177e5ab4129f9b9e9764ed0e4faa4f87e443b
Log: fix bug #54374, bug #55500 - filter file names better, no dangling [s
 [2013-11-17 09:34 UTC] laruence@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7c3177e5ab4129f9b9e9764ed0e4faa4f87e443b
Log: fix bug #54374, bug #55500 - filter file names better, no dangling [s
 [2014-10-07 23:28 UTC] stas@php.net
Automatic comment on behalf of pierre.php@gmail.com
Revision: http://git.php.net/?p=php-src-security.git;a=commit;h=95dcd799fb6fdccbc60d3bba3cd759f6b421ee69
Log: - merge fix bug #54374, bug #55500 - filter file names better, no dangling [s, svn revision 321664
 [2014-10-07 23:39 UTC] stas@php.net
Automatic comment on behalf of pierre.php@gmail.com
Revision: http://git.php.net/?p=php-src-security.git;a=commit;h=95dcd799fb6fdccbc60d3bba3cd759f6b421ee69
Log: - merge fix bug #54374, bug #55500 - filter file names better, no dangling [s, svn revision 321664
 
PHP Copyright © 2001-2025 The PHP Group
All rights reserved.
Last updated: Mon Mar 31 23:01:30 2025 UTC