Doc #54374 [NEW]: Insufficient validating of upload name leading to corrupted $_FILES indices

From: Operating system: All PHP version: 5.3.6 Package: Variables related Bug Type: Documentation Problem Bug description:Insufficient validating of upload name leading to corrupted $_FILES indices 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 ) ) -- Edit bug report at http://bugs.php.net/bug.php?id=54374&edit=1 -- Try a snapshot (PHP 5.2): http://bugs.php.net/fix.php?id=54374&r=trysnapshot52 Try a snapshot (PHP 5.3): http://bugs.php.net/fix.php?id=54374&r=trysnapshot53 Try a snapshot (trunk): http://bugs.php.net/fix.php?id=54374&r=trysnapshottrunk Fixed in SVN: http://bugs.php.net/fix.php?id=54374&r=fixed Fixed in SVN and need be documented: http://bugs.php.net/fix.php?id=54374&r=needdocs Fixed in release: http://bugs.php.net/fix.php?id=54374&r=alreadyfixed Need backtrace: http://bugs.php.net/fix.php?id=54374&r=needtrace Need Reproduce Script: http://bugs.php.net/fix.php?id=54374&r=needscript Try newer version: http://bugs.php.net/fix.php?id=54374&r=oldversion Not developer issue: http://bugs.php.net/fix.php?id=54374&r=support Expected behavior: http://bugs.php.net/fix.php?id=54374&r=notwrong Not enough info: http://bugs.php.net/fix.php?id=54374&r=notenoughinfo Submitted twice: http://bugs.php.net/fix.php?id=54374&r=submittedtwice register_globals: http://bugs.php.net/fix.php?id=54374&r=globals PHP 4 support discontinued: http://bugs.php.net/fix.php?id=54374&r=php4 Daylight Savings: http://bugs.php.net/fix.php?id=54374&r=dst IIS Stability: http://bugs.php.net/fix.php?id=54374&r=isapi Install GNU Sed: http://bugs.php.net/fix.php?id=54374&r=gnused Floating point limitations: http://bugs.php.net/fix.php?id=54374&r=float No Zend Extensions: http://bugs.php.net/fix.php?id=54374&r=nozend MySQL Configuration Error: http://bugs.php.net/fix.php?id=54374&r=mysqlcfg