Edit report at https://bugs.php.net/bug.php?id=54374&edit=1
ID: 54374
Updated by: stas@php.net
Reported by: lekensteyn at gmail dot com
Summary: Insufficient validating of upload name leading to
corrupted $_FILES indices
-Status: Open
+Status: Closed
Type: Bug
Package: Variables related
Operating System: All
PHP Version: 5.3.8
-Assigned To:
+Assigned To: stas
Block user comment: N
Private report: N
New Comment:
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.
Previous Comments:
------------------------------------------------------------------------
[2012-01-01 23:53:58] stas@php.net
Automatic comment from SVN on behalf of stas
Revision: http://svn.php.net/viewvc/?view=revision&revision=321664
Log: fix bug #54374, bug #55500 - filter file names better, no dangling [s
------------------------------------------------------------------------
[2011-11-18 10:40:22] lekensteyn at gmail dot com
This is not a documentation bug and still exists in PHP 5.3.8.
------------------------------------------------------------------------
[2011-03-24 20:07:31] 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#990http://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
)
)
------------------------------------------------------------------------
--
Edit this bug report at https://bugs.php.net/bug.php?id=54374&edit=1