Posted by Steve on 04/09/07 01:53

ken...

have you thought of using array_walk?

function escapeData(&$value)
{
if (ini_get('magic_quotes_gpc'))
{
$value = stripslashes($value);
}
$value = mysql_real_escape_string(strip_tags(trim(urldecode($value))));
}

array_walk($_POST, escapeData);


since the function is compiled into php, it will run natively and is
therefore much faster than using your own 'for' loop to iterate the post
array. that may be something for you to think about.

as for escapeData, you can cause yourself headaches by assuming all data is
passed as urlencoded. it is not needed as far as i'm concerned...i only
urlencode when i'm dynamically creating a hyperlink for html. php typically
presents you with the decoded version of data (so to speak). you would be
forcing user input that had a space character to be encoded as a plus sign
(+) from the client to the server, and then force the server to translate it
back to a space. i'm not sure why you'd do this.

oh, and also, you can run into problems casting escaped data into things
like dates and currency. better to allow a less broad stroke to handle
things...that means, be more delicate.

another issue would be stripping tags as some sort of default (with no
option to override it). i may be asked on your form to enter my email
address. if i put:

Joe Blow <jblow@example.com>

my address would be valid, but you're code would be potentially stripping
out the portion of it that is the actual valid piece of data...and again,
you provide no means to override that functionality.

as for doing $$key as a global from the post array...BAD IDEA !!! most
obvious is the introduction of an opportunity for a hacker to overright
predicted variables (userName, isValid, isSecure, etc.). the not-so-obvious
downfall is that your variables are not explicitly set, transparent to other
developers...including yourself when you come back to edit later. they'd be
saying, 'where does this variable come from? from an included file, from a
post, get, or other means? has it been set already, or is this where it is
being defined?' that list goes on...and really pisses people off. be
explicit at all times so that no one has to guess an how things work.

your last idea is probably a good compromise. again though, you may want to
simplify your code and speed it up using array_walk. also reuse namelist to
help build your forms. namelist should not only define the variable name but
what data-type it is supposed to be, what validation applies to it,
max-length, input-type (text, hidden, select, etc.), and value. what you
have now is the criterion for a class object. your namelist is an array of
those objects and instead of just escaping data, you can set the value
either to a default when empty or to the actual posted data. further, you
can apply the validation and generate errors for the end-user. even more
than that, you can walk the namelist array to generate your form with less
lines of code...which means changes (enhancements, bug-fixes, etc.) are
isolated to one section of code. AND, you've now been *very* explicit as to
what the data means, where it comes from, and how it fits in to the
application as a whole. no one being left confused. and, do you also see
that this would allow you to now catch that the data is a date, currency,
float, or other where you could now not just escape the string but use
set_type() on the value to avoid problems later?

one final remark...NEVER, EVER use $global. until you've learned the
condition(s) under which it is a good idea, 'never' is literal...otherwise,
you'll know that i really mean, 'only when it is absolutely
necessary...which is almost never'.

;^)

hth,

me


"kenoli" <kenoli.p@gmail.com> wrote in message
news:1176078404.793386.307530@w1g2000hsg.googlegroups.com...
| In addition to validating post input to see if it has been entered or
| confroms to a regular expression, I am interested in secure ways to
| efficiently process incomiing form data.
|
| I use a function to process this data, namely:
|
| function escape_data ($data) {
|
| if (ini_get('magic_quotes_gpc')) {
| $data = stripslashes($data);
| }
| return mysql_real_escape_string(strip_tags(trim(urldecode($data))));
| }
|
| It's tempting to apply it to all incoming data at once, like this:
|
| foreach($_POST as $key => $value) {
| global $$key;
| $$key = escape_data($value);
|
| }
|
| Of course this introduces the well known vulnerability of putting the
| data into variables that can be modified.
|
| Another way to do this is to put the data in an array, like this:
|
| foreach($_POST as $key => $value) {
| global $input;
| $input[$key] = escape_data($value);
|
| }
|
| This has the advantage that I can use implode() to create strings that
| can be applied to a database query.
|
| However, it includes all post variables, including data I dont' intend
| to insert in my database so I can be selective about what form
| elements I include like this:
|
| function ($namelist) {
|
| for each ($namelist as $formdata) {
| if (!empty($_POST[$formdata]) {
| global $formdata;
| $formdata[] = escape_data($_POST[$formdata]);
| }
| }
| }
|
| $namelist being an array of the names of the form elements I want to
| extract for my query.
|
| I'm interested on people's reflections on the security of these
| methods as well as other comments and suggestions for other
| approaches.
|
| Thanks.
|
| --Kenoli
|

[Back to original message]