I was checking my script for vulnerabilities and was shocked the way i used to do in the past which is extremely insecure:
foreach ($_GET as $key => $value){
$$key = $value;
}
or shorter
extract( $_GET );
I altered with firebug some POST/GET variables to match a name i used in my script. they can be overwritten if the name would be guessed correctly.
So i thought i had to do it individually naming like this: $allowed_vars =
$allowed_vars = array("time","hotfile","netload","megaupload","user","pfda","xyz","sara","amount_needed");
foreach ($_GET as $key => $value)
{
if (in_array($key,$allowed_vars))
{
$$key = $value;
}
}
This way saves some time than naming them individually.
What kind of automation have to be used for this?
I don't use any automatism of the kind.
ReplyDeleteI see no point in assigning request variables to global variables automatically.
If it's one or two variables, I could deal with them manually.
If there are more, I'd rather keep them as array members for the convenient handling.
Yet I am using some sort of whitelisting approach similar to yours.
but not to create global variables out of POST data but to add that data into SQL query.
Like in this simple helper function to produce SET statement:
function dbSet($fields) {
$set='';
foreach ($fields as $field) {
if (isset($_POST[$field])) {
$set.="`$field`='".mysql_real_escape_string($_POST[$field])."', ";
}
}
return substr($set, 0, -2);
}
$id = intval($_POST['id']);
$fields = explode(" ","name surname lastname address zip fax phone");
$query = "UPDATE $table SET ".dbSet($fields)." stamp=NOW() WHERE id=$id";
You can save even more time by not extrating them at all. Just use them from the $_GET array. The advantages of this are not just avoiding collision with script variables (or worse) but also that you don't have to update that "automatism" when you add request parameters.
ReplyDeleteWhen I am working with POST data, as from a form, I often process each explicitly:
$data = array();
$data['field1'] = someSaniFunction($_POST['field1']);
$data['field2'] = someOtherFunction($_POST['field2']);
...
In this way I ensure that each field is properly handled, and only the fields I expect are touched.
In my experience, you shouldn't transform data in the $_REQUEST array into variables using $$ as it gives the possibility of overwriting variables held within the current scope.
ReplyDeleteInstead, you should consider having a request object or array in which you filter the data and only access named variables that you require. This way, you do not have to keep extending your allowed variable names and still maintain security.
The ZF for example has a request object, and they recommend using a input filter when working on this data:
http://framework.zend.com/manual/en/zend.filter.input.html
You can use the extract function in a more secure way:
ReplyDeleteextract($_REQUEST, EXTR_SKIP);
This will not overwrite variables that already exist in your code. See here for other parameters you can use