...

View Full Version : foreach through POST data - is this secure?



quadrant6
02-08-2006, 08:58 PM
Very often, I'm working with forms that have alot of fields. In order to access them/make them into PHP variables, I could do this:


$name = $_POST['name'];

This is fine for a few but soon becomes messy and bloated when you get a lot of form variables. So I've been doing this:


# get form input
foreach($_POST as $key=>$value){
$$key = TRIM($value);
}

However, I suspect there may be security issues with this (if someone manages to POST another wierd variable in, it will be processed also).

What you do you think? is there a better way to do it?

ralph l mayo
02-08-2006, 09:17 PM
it's not secure, here's a better way:

$_POST = array_map('trim', $_POST);

The security issue arises if you have something like


if ($admin)
{
// protected functionality
}


The method you posted allows the user to set $admin to true by submitting a post with admin=1

Kid Charming
02-08-2006, 09:20 PM
Well, even if you create an unintended variable, if you never use it for anything, it's not a security risk. This doesn't mean you shouldn't clean and validate everything you're pulling with something more than a TRIM().

I wouldn't say the "$name = $_POST['foo']" method is "messy and bloated". If anything, it's more efficient than a foreach/TRIM() construct, and it's also a very clear, easy to refer to list of the variables you'll be working with, which can be invaluable to someone else who has to work on your code (or to yourself when you have to revisit the script after a few months). Less typing is not necessarily better.

raf
02-08-2006, 09:55 PM
Less typing is not necessarily better.
Quote of the day ...

You always need to be extremely carefull with input from the client, so don't go automatically turn all the variables from the post-collecction into internal variables. If you wanna do that, thes just set register_globals = on ...

Nothing wrong with it if you are an extremely good coder that has all his other bases covered, but maybe some wannabe like me later needs to work on your super-code and creates security-risks...

Element
02-08-2006, 10:35 PM
it's not secure, here's a better way:

$_POST = array_map('trim', $_POST);

The security issue arises if you have something like


if ($admin)
{
// protected functionality
}


The method you posted allows the user to set $admin to true by submitting a post with admin=1

Now who in their right mind would determine the admin based on form inputs? You should always go from a ID or username, and some sort of validation and do the work in PHP to see if they are an admin. the foreach() method is just fine, depending on what you do with it. For example:



<?php

mysql_connect('localhost', 'root', '') or die(mysql_error());
mysql_select_db('chinese_pizza') or die(mysql_error());

foreach($_POST as $key => $value) {
${'post_' . $key} = mysql_real_escape_string($value);
}

$query = sprintf("INSERT INTO `orders` VALUES(null, '%s', '%s')", $post_name, $post_order);
$result = mysql_result($query);

if($result) {
echo 'Success ' . $post_order . ' has been added to our delivery list!';
}

?>

raf
02-08-2006, 10:48 PM
Now who in their right mind would determine the admin based on form inputs? You should always go from a ID or username, and some sort of validation and do the work in PHP to see if they are an admin.
i think you're missing his point.

Imagine you determined that he is an admin (by doing a select or some sessionvariable or IPchecking or whatever), and you store this like
$admin=True;
and then go on in your script to process the form and do some actions based on his user-profile.
If i changed your form and added a formfield with name 'admin' and value '1' then i the script would treat me as if i am a validated admin.

felgall
02-09-2006, 10:52 PM
You could use the following code where all of the expected post variables are listed in the first line so no others get copied.


$allowed = "parmone,parmtwo";
while (list ($key, $val) = each ($_POST))
{
if (strstr($allowed,$key) && $val)
$$key = $val;
}

Element
02-09-2006, 11:08 PM
You could use the following code where all of the expected post variables are listed in the first line so no others get copied.


$allowed = "parmone,parmtwo";
while (list ($key, $val) = each ($_POST))
{
if (strstr($allowed,$key) && $val)
$$key = $val;
}
Yeah, I used this method with my old member's script. I had the set form names in the config, so it is not only easy to alter for whever the form names may be used but also allows a client to furtherr customize it with ease.

And as far as not getting it, I think I do because you also should define $admin after you process post data to be used where ever in the file. At least this is how I code, but I know it would be easy for someone knew to do, which is why you should know how to handlee them. Thats why I also add $'post_'name before my variables to seperate them from the set pre-set variables an script variables.

GJay
02-09-2006, 11:09 PM
using in_array would seem a more logical way of doing that to me, do you use strstr for a particular reason?

Element
02-10-2006, 01:21 AM
Yeah thats what I would use, cause its much simpler to contruct an array for the names, like how I did it, in a configuration file.

Something like:

Config file


<?php

// Mumbo Jumbo here...

$config['formnames'] = array('username', 'password', 'menu_tree', 'submit', 'friends_email', 'senders_email');

?>


example


<?php

require_once('/home/root/public_html/includes/config.ini');

foreach($_POST as $key => $value) {
if(in_array($key, $config['formnames']) {
${'p_' . $key} = trim(strip_functions(strip_tags($value)));
}
}

?>


nevermind strip_functoins() its a custom function.



EZ Archive Ads Plugin for vBulletin Copyright 2006 Computer Help Forum