Critique My Form Validation/Security Code

[sp0om]

Over the past couple of days, I've been adding security features to the forms on a site I'm building. Below is the validation process for $username. Does it look complete? Are there any holes? Should I add anything else?

//make_safe function defined to prevent SQL injection
    function make_safe($variable) {
     $variable = addslashes(trim(mysql_escape_string($variable)));
      return $variable;
    }

//make_safe function applied to $username
    $username =  make_safe($_POST['username']);

//runs checks to ensure form is filled with only letters and numbers
    if((!$username) || (!eregi("^[a-z0-9]+$", $username)) {

        if(!$username){
               echo "Username is a required field <br>";
        }

        if(!eregi("^[a-z0-9]+$", $username)){
        echo "Only letters and numbers are allowed in the username field<br>";
        }

    } else { 

//validation complete
    LOGIN CODE 

Feedback of any kind would be very much appreciated. Thanks.

[F4nat1c]

$variable = addslashes(trim(mysql_escape_string($variable))); 

Is that really nesessary?

Maybe you could limit the amount of characters that the user is allowed to put in. Maybe minimum of 6, and maximum of 15/20.

But yeah that's a very good piece of validation.

[zeeshanhashmi]

Hi
Seems good. U can further improve it by telling REGEXP the Maximum and Minimum numbers of Letters/Digits. like if u add ^[a-z0-9]{8}+$ this will only accept 8 letters/digits ^[a-z0-9]{8,20}+$ this will accept anything between 8 to 20 letter/digits. Hope this will add some more flexibility.

[devil cat]

 $variable = addslashes(trim(mysql_escape_string($variable))); 

This is a bit redundant. addslashes is the less protective version of mysql_escape_string. If you drop addslashes, you will still be okay.

[F4nat1c]

 $variable = addslashes(trim(mysql_escape_string($variable))); 

This is a bit redundant. addslashes is the less protective version of mysql_escape_string. If you drop addslashes, you will still be okay.

That's the point i tried to make in my original post .

$variable = addslashes(trim(mysql_escape_string($variable))); 

Is that really nesessary?

[devil cat]

I have a habit of not being very specific, and I'm trying to break it. That's all.

[zeeshanhashmi]

I think when u have DEFINED a REGEX Pattern for Alphabets and Digits only then there is no need to use Add slashes.

[paulyG]

I think his point was :
to use make_safe() to frisk the variable - no matter what you were going to do with it next.

Then, at the script level, make sure it fits the pattern you expect.

But wont addslashes screw the regex though?

If the make_safe() code is sql specific shouldn't it be used after the regex?

[F4nat1c]

I think his point was :
to use make_safe() to frisk the variable - no matter what you were going to do with it next.

Then, at the script level, make sure it fits the pattern you expect.

But wont addslashes screw the regex though?

If the make_safe() code is sql specific shouldn't it be used after the regex?

Good point. The code may do a better purpose if written as:

//make_safe function defined to prevent SQL injection
    function make_safe($variable) {
    $variable = trim(mysql_escape_string($variable));
      return $variable;
    }

//runs checks to ensure form is filled with only letters and numbers
    if((!$username) || (!eregi("^[a-z0-9]+$", $username)) {

        if(!$username){
               echo "Username is a required field <br>";
        }

        if(!eregi("^[a-z0-9]+$", $username)){
        echo "Only letters and numbers are allowed in the username field<br>";
        }

    } else {

//make_safe function applied to $username
    $username =  make_safe($_POST['username']); {

//validation complete
    LOGIN CODE 

Or something along those lines. Besides, there's no point applying the function to the variable if it hasn't been set yet.

[lvismer]

I would rather use the PCRE functions as I feel they are more efficient. You can also use something like the following approach to avoid doing the eregi twice,

$clean = array();
$errors = array();

if ( isset($_POST['username']) ) {
    if ( preg_match('/^[a-z0-9]+$/i', $username) ) {
        $clean['username'] = $username;
    } else {
        array_push($error, 'Username must be alpha numberic');
    }
} else {
    array_push($error, 'Username is a required field');
}

if ( isset($clean['username']) {
    // Do login stuff
} else {
    foreach ( $errors as $error ) {
        echo "$error";
    }
} 

I would move/avoid the make_safe function until you get to the database abstraction layer.

--
lv