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
 [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-2024 The PHP Group
All rights reserved.
Last updated: Tue Dec 03 17:01:29 2024 UTC