Simple Function To Check Any and All User Input

[Paul]

You should add the following function to your script to elimnate the possibility of people passing binary data through your forms which is usually used by hackers to gain access to your entire web site and possibly the entire server you are on.


<?
function secure($var) {
$var = strip_tags($var);
$var = trim(htmlentities($var, ENT_QUOTES));
return $var;
}
?>


usage


$form_variable = secure($_POST['form_variable']);


You should apply this function to every variable that a user passes to your script using a form.

Thanks to another member in another forum really giving me crap for not doing this in a script. ;)

[dimeric]

You don't need to have the htmlentities in there. Especially if you want to allow html input. htmlentities should be applied to unsafe data before displaying it to users, it shouldnt be applied to input.

[Paul]

You don't need to have the htmlentities in there. Especially if you want to allow html input. htmlentities should be applied to unsafe data before displaying it to users, it shouldnt be applied to input.

Thanks for your input, if I don't need that in there I'll go a head and take it out in my script.

But isnt that the only real way to protect your script from binary input in php 4.x.x ? I know strip_tags does it but I think that only works properly in PHP 5, no?

[erisco]

A lot of functions are binary safe... this is an interesting security issue that I have not looked a lot into. I want to do some research tonight... that function looks fine but I wonder if there isn't already something else (built-in) to safe-guard scripts.. you'd think so given that this is a known security problem.

[Paul]

If you find anything please let me know. I've been attacked using this method on several different occasions, usually when dealing with email/contact forms.

[erisco]

Well I have failed to find anything on "binary attacks". If such a thing did exist, then I still fail to see why safe-guarding against HTML injection has anything to do with it.

Binary-safe on functions means they can read binary, as I have learned. However, be it binary or not it doesn't matter.. the security issues are caused by not validating user input to what it needs to be. Htmlentities converts items found in the html table (exact name I am not sure of.. functions to work with it though), trim knocks off whitespace before and after a string.. and strip_tags removes all html tags from a string. Again, I do not see how this translates into anything but avoiding HTML injection. This is absolutely no threat to PHP or your server.. just to your site on the client-end..

That said, although perhaps wrong, a custom function may not be needed. After all, if we do not want to have html we simply use strip_tags.. and if we don't want to be stripping out intentional brackets then htmlspecialchars is just as viable. If anything, this is a good convenience function.
<?php
funciton h($string) {
return htmlspecialchars($string);
}
?>
Borrowed from the cakePHP framework.

[Paul]

I will need to look at this a little more but here is a good article about hex injection:

http://www.securephpwiki.com/index.php/Email_Injection

[erisco]

Header injection is not necessarily a security issue, but rather allows someone to change where your emails are going and/or say where they came from. It certainly shouldn't be a risk of PHP or server security... there isn't any way you should be giving out any credentials over form email is there?

The hex injection was to insert a line feed (newline "\n") which again resulted in header injection. This time it was used to abuse Bcc and Cc (and even multiple to addresses). Stripping html, removing whitespace, it does not prevent this kind of trouble whatsoever.

What will protect this kind of injection is taking appropriate caution.. which as the article has stated many projects have already done. At the bottom it lists different packages you could safely use to avoid this type of email nuisance. It also does a little explanation of how you could protect your emails for yourself. But, unfortunately Paul I do not believe the function you have provided will work against these exploits.

[dimeric]

I think your (Paul not ericso) confusing injection attacks (which are attacking either the browser or datastore) with attacks against php, such things like buffer overflow etc are quite different and arn't really defended against by parsing user input. Instead you just ensure that your code has sufficient exception handling and doesn't call unsafe functions (i'm not so good when it comes to php stuff as far as security goes) you should of course also parse user input, but this is just a first line of defense (it is pretty much the only line of defence for injection).

Also while php function may read binary data, the input is handled as a string, and you can force this with typecasting. Which would avoid any chance of it being handled as binary data (unfotunately this is a problem with loose typing, the solution is of course to use a strongly typed language!).

Either way do update if you find anything else about these binary attacks

[Paul]

Thanks to both of you for all the input. I've been hacked too many damn times so at this point I dont want to run anything that could be exploited in anyway.

I added this function to my code today and deleted the htmlentities part. What's strange is that with this function if you check it with isset() it always returns true :confused:. For example.

I changed it to the following and still same problem:


#Check User Input
function secure($var) {
if($var) {
$var = strip_tags($var);
$var = trim($var);
return $var;
}
else {
return false;
}
}


So if I run the following I //code here always gets executed even if $submitted is not set:


$submitted = secure($_POST["submit"]);
if(isset($submitted) {
//code here
}


yet when I do this it works fine:


if($submitted) {
//code here
}


Strange, no?

[dimeric]

Um of course it always evaluates true.

$submitted = secure($_POST["submit"]);
if(isset($submitted) {
//code here
}

This either sets $submetted to the value off $_POST["submit"] or it sets it the boolean value false (a nice example of the weak typing). isset() then checks if this variable has a value associated with it (There would be a subtlety here if your variable is a reference type but thats slightly beyond this).

$submitted always has a value once secure() has been run, so isset() always returns true.

You could change it like this:


#Check User Input
function secure($var, $outVar)
{
if(isset($var))
{
$var = strip_tags($var);
$var = trim($var);
$outVar = $var;
return true;
}
else
{
return false;
}
}


//then use it like so
$submitted = "";
if(secure($_POST["submit"], $submitted))
{
//code here
//You can now use $submitted
}


or you could do this
#Check User Input
function secure($var)
{
if(isset($var))
{
$var = strip_tags($var);
$var = trim($var);
return $var;
}
else
{
return; //here this effectively returns void
}
}

But its better practice to have functions return only 1 type but with different values (as the first example does).

[erisco]

or why not...

function secure($string) {
if(!empty($string)) {
return trim(strip_tags($string));
}
return false;
}

$submitted = secure($_POST['unsafe']);

if ($submitted !== false) {
echo $submitted;
}
I think that is better yet.

dimeric, I do not see how that is "better practice" to only return one type. IMHO, it depends on what the function is. I'd say quite a few built-in functions have multiple return values. Returning false on function failure is not that bad of an idea.

[Paul]

I was under the impression that if you return false it becomes null and in return isset would return false. Thanks both for the help.

[dimeric]

The reason i say better practice is because it provides a feedback as to whether a function has worked or not, regardless of input. Also its good practice as thats how most other languages work. Look at professional API's and you will find they like single return types (for all sort of reasons, like exposing to webservices etc) but its just nicer. Also PHPs type comparisons are notoriously "iffy"!

Either way glad you have it sorted, you just have to remeber that false is a value.

[erisco]

dimeric, fair argument. I propose this:
function secure($string) {
return trim(strip_tags($string));
}

if (!empty($_POST['unsafe'])) {
$submitted = secure($_POST['unsafe']);
}

null, true, false, are all three entirely different values. However, take this example:

$foo = null;
if (isset($foo)) echo 'foo exists';
The variable is equal to null, the variable exists, therefore isset() indeed proves true.

[Paul]

That's really great information. So even if you set something to false it is still set when checking with isset. I hate to drag this on but since we are having a good discussion here about it let me throw this question out there. So is there really any good reason to check a variable with isset() as opposed to just doing the following:


if($somevar) {
echo "Somevar is set";
}


My code doesn't execute unless submit has been pressed. So when I do the secure funtion above I can not longer use isset() as I posted above. I don't see any problems with this but I'm curious if you guys do?

[dimeric]

yes, now we see a problem with softly typed language!


//first we do this
if($varString)
{
//this isn't reached
}

if($varBool)
{
//this isn't reached
}

//But we now define our variables (well we create an instance then define a value to be correct)
$varString = "hello"
$varBool = false

if($varString)
{
//this is reached because $varString exists
}

if($varBool) //this doesn't perform a "psuedo-isset()" function but instead evaluates the bool.
{
//this isn't reached because if(false) always fails.
}


And thus we see a problem! You have to know what variable type you are placing inside the if() statement. Now this kind of defies the point of soft typing, so really you should be using the Isset() function.

If you want to be super correct its quite useful to use if($varBool == true) as then you only get true if its value is true and boolean! (i think).

Hope that helps

Oh and interestingly enough if you have a function that either sets a var to a string value or boolean false then it will work as you wanted because if it sets it to a string value then if($var) returns true as $var has a value, but on the other hand if its the boolean false then if($var) returns false as $var has the boolean value of false.

[erisco]

If you want to be super correct its quite useful to use if($varBool == true)
if ($varBool === true) // actually :D
// if $varBool is equal to value and equal to type (===)

Paul, I highly suggest you stick this at the top of all of your scripts during any development:
error_reporting(E_ALL);
It will clearly show why
if ($foo)
is stupid. It doesn't run isset(), instead it tries to make a boolean decision. false, null, 0, empty string, or an empty array will all return false. I am not sure if it is only with empty() but a string containing only containing the digit zero may return false as well. Everything else will return true. However, take this complete script into consideration.
<?php
if ($foo) echo 'Foo evaluated to true!';
?>
ERROR! $foo has not been set! This is something many people do not see because they have no idea about errors that go as notices (strict errors are some of them). This is why I suggested the error reporting change. You cannot reference a variable that does not exist. So how do you test if the variable exists yet? Ah ha! isset()!
<?php
if (isset($foo)) echo 'Foo was set, and if it is not, I do not get an error message!';
?>
isset() does not check if a value is set, it rather checks if the variable is set.

Another misconception is forms. If someone does not fill out a field, the variable is still set.. however, the value is an empty string. See that word "empty"? empty is another language construct (like isset) that will return true or false. When does it return false? When the variable is not set (like isset), when the variable is one of; null, false, 0, empty string, empty array, or a string with the digit 0, and everything else returns true.
<?php
if (isset($_POST['name'])) {
// If the form was submitted this is ALWAYS reached
}
if (!empty($_POST['name'])) {
// If the form was submitted but the "name" field was empty, this will be reached
}
?>
Keep in mind that using empty() and isset() on unset variables (like my if($foo) test) will not produce any errors. Why? Because these are not functions, they are language constructs, and they can be special in that way.

So the difference between
if ($foo)
// and
if (!empty($foo))

is simply that one could produce an error if $foo does not exist while the other will not.

I hope this clears some things up...