UBB.Developers Forums Community Development Programming PHP and Server-side Scripting Register globals off, want input

First some background: I'm working on a non-threads related article system that I use on my own site and am doing quite extensive extensions at the moment. It can be found on sourceforge.

The current released version doesn't work with register globals set to off, but while making the next version I thought I'd try to make it work with that option since I'm doing a lot of changes anyway.

Now, to my question. Since the scripts work with register globals on I want to go the easy way and use a function to read the values that are used in the script. But I don't want to read all values, since it wouldn't increase security. So I wrote the following function:

code:

function getCGIVars($vars) {
	$cgivars = Array();
	while (list($key, $var) = each($vars)) {
		$cgivars[$var] = $_GET[$var];
		if ($_POST[$var]) $cgivars[$var] = $_POST[$var];
	}

	return $cgivars;
}

---

What it does is take an array with the names of all values that should be read and returns an array with those GET or POST values. If there are both GET and POST values of the same name, the value of the POST will be used. After calling this function with the wanted values, I only have to use extract() on the returned array to get those values in the script.

Can anyone think of any security issues with this function? Using it, I can decide which values to read in each script without having to add a line for each.

Hmmmm .... it makes sense to me.

Ideally, I would prefer to emulate Perl's taint checking and not only limit the names of variables that can be imported, but also test that the values are "safe" for whatever the variables are being used for. But your approach is better than blindly importing all variables.

One suggestion that's not really security-related. Replace the body of the loop with:

code:
if (isset($_POST[$var])) {
   $cgivars[$var] = $_POST[$var];
} elseif (isset($_GET[$var])) {
   $cgivars[$var] = $_GET[$var];
}

---

That will allow the code to work without warnings if you happen to set the error reporting level higher, e.g., error_reporting(E_ALL), and will also allow you to distinguish between unset variables and zero/empty-string variables.

Thanks, that's a good idea, I'll use isset() instead.

About the checking that the values are safe, do you mean that they should be of the correct datatype? That is, the array sent to the function could include an extra value for each name that says if it should be a string, a number or such?

Checking the type might be a good idea.

I was thinking more of cases where an user's input might be used to construct a directory path or filename. In that case, I would clean non-alphanumeric characters from the variable, to prevent something like "../../etc/passwd".

Well, I'd say that's something for the script that's going to use the value to take care of.