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
        )

)