php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #48597 Unclosed array keys break space escaping in $_GET/POST/REQUEST
Submitted: 2009-06-18 16:17 UTC Modified: 2015-07-29 20:12 UTC
Votes:4
Avg. Score:3.5 ± 1.7
Reproduced:3 of 3 (100.0%)
Same Version:0 (0.0%)
Same OS:0 (0.0%)
From: crmalibu at gmail dot com Assigned:
Status: Open Package: *General Issues
PHP Version: 5.*, 6CVS (2009-07-01) OS: *
Private report: No CVE-ID: None
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: crmalibu at gmail dot com
New email:
PHP Version: OS:

 

 [2009-06-18 16:17 UTC] crmalibu at gmail dot com
Description:
------------
I marked the version as 5.2.9 but it looks like the relevant code is the same for 5.3 and php 6 as well.

I don't know c, so I struggle to read the source code, but I think I found something unexpected. In main/php_variables.c in php_register_variable_ex I think the parsing behaves inconsistent. 

After reading the comments in the source code, I would think a gpc variable name should not make it through which has ' ' or '.' or '[' character in the name. But I've found a way to do it. It seems the routine for recognizing and parsing the array syntax is at fault.

In particular, characters after the first occurrence of a '[' char will be left alone because it thinks it needs to parse it as the special array syntax. But while it does later recognize that it's not proper array syntax, it doesn't properly convert the remaining character to underscore.

I don't know if this is a bug, or if it's serious or what. But the source code comment about removing those chars  due to not being binary safe made me think someone needs to look at this.



Reproduce code:
---------------
<form action=>
<input name="goodvar .[">
<input name="goodarray[foo]">
<input name="badvar[ . [">
<input type=submit>
</form>

<?php
print_r($_GET);
?>

Expected result:
----------------
Array
(
    [goodvar___] => 
    [goodarray] => Array
        (
            [foo] => 
        )

    [badvar_____] => 
)

Actual result:
--------------
Array
(
    [goodvar___] => 
    [goodarray] => Array
        (
            [foo] => 
        )

    [badvar_ . [] => 
)

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2009-07-04 22:22 UTC] jani@php.net
See also bug #48794
 [2009-09-27 02:52 UTC] jani@php.net
See also bug #49683
 [2009-10-13 22:36 UTC] chrisstocktonaz at gmail dot com
Here is a fix.

Index: main/php_variables.c
===================================================================
--- main/php_variables.c	(revision 289602)
+++ main/php_variables.c	(working copy)
@@ -61,6 +61,7 @@
 {
 	char *p = NULL;
 	char *ip;		/* index pointer */
+	char *pmarker;		/* marker to index before */
 	char *index, *escaped_index = NULL;
 	char *var, *var_orig;
 	int var_len, index_len;
@@ -100,12 +101,19 @@
 		if (*p == ' ' || *p == '.') {
 			*p='_';
 		} else if (*p == '[') {
-			is_array = 1;
-			ip = p;
-			*p = 0;
-			break;
+			for(pmarker = p; *pmarker; pmarker++) {
+				if(*pmarker == ']') {
+					is_array = 1;
+					ip = p;
+					*p = 0;
+					goto var_continue;
+				}
+                        }
+			*p='_';
 		}
 	}
+
+	var_continue:
 	var_len = p - var;
 
 	if (var_len==0) { /* empty variable name, or variable name with a space in it */
 [2009-10-14 02:42 UTC] chrisstocktonaz at gmail dot com
Sorry for extra noise.. it seems my patch mixed the case of something like: <input name="badvar[[[. [ . . .]>

So updated:
Index: main/php_variables.c
===================================================================
--- main/php_variables.c	(revision 289602)
+++ main/php_variables.c	(working copy)
@@ -61,6 +61,7 @@
 {
 	char *p = NULL;
 	char *ip;		/* index pointer */
+	char *pmarker;
 	char *index, *escaped_index = NULL;
 	char *var, *var_orig;
 	int var_len, index_len;
@@ -100,12 +101,18 @@
 		if (*p == ' ' || *p == '.') {
 			*p='_';
 		} else if (*p == '[') {
-			is_array = 1;
-			ip = p;
-			*p = 0;
-			break;
+			for(pmarker = p; *pmarker; pmarker++) {
+				if(*pmarker == ']') {
+					is_array = 1;
+					ip = p;
+					*p = 0;
+					goto var_continue;
+				}
+                        }
+			*p='_';
 		}
 	}
+	var_continue:
 	var_len = p - var;
 
 	if (var_len==0) { /* empty variable name, or variable name with a space in it */
@@ -225,6 +232,13 @@
 			} else {
 				escaped_index = index;
 			}
+			/* clean up the array index */
+			for (pmarker = escaped_index; *pmarker; pmarker++) {
+				if (*pmarker == '[' || *pmarker == ']'
+					|| *pmarker == '.' || isspace(*pmarker)) {
+					*pmarker = '_';
+				}
+			}
 			/* 
 			 * According to rfc2965, more specific paths are listed above the less specific ones.
 			 * If we encounter a duplicate cookie name, we should skip it, since it is not possible
 [2009-10-23 21:28 UTC] e dot ehritt at web dot de
I left a report too. http://bugs.php.net/bug.php?id=49975

If I follow the documentation, a key of an array is a string. That means, a key "abc[des]" is possible. (e. g. $a=array("abc[des]"=>'d'); )

<form action="s.php" method="post">
                <input name="a[b[c]]" type="text"/>
                <input type="submit">
</form>

$_POST=array('a'=>array('b[c]'=>'d'));

Both should result in even structure, but it does not. There are no reason, why incoming datas could not follow the same rules as data in a script.
 [2015-07-29 20:12 UTC] cmb@php.net
IMO this is not a bug (at least I won't fix it). The mangling of
some special characters may have made sense when variables were
automatically created from GPC parameters (register_globals), but
nowadays I'd rather get rid of the mangling at all.
 [2024-07-16 08:58 UTC] chris1058wright at outlook dot com
This article is amazing I hope we will see again this type of article in the future. (https://github.com)(https://www.adpvantages.com)
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Sat Nov 23 12:01:29 2024 UTC