Function review

[bkeep]

Hello all I have created a basic login script that I want to release as opensource software and was wondering if I could get some comment on form security I have a registration function and need to know if what I have done is enough or if I need to do more.

function registerUser($user, $pass, $email, $first_name, $last_name, $phone, $alt_phone, $fax, $agree) {
 
 $user = mysql_real_escape_string(stripslashes($user));
 $pass = mysql_real_escape_string(stripslashes($pass));
 $email = mysql_real_escape_string(stripslashes($email));
 $first_name = mysql_real_escape_string(stripslashes($first_name));
 $last_name = mysql_real_escape_string(stripslashes($last_name));
 $phone = mysql_real_escape_string(stripslashes($phone));
 $alt_phone = mysql_real_escape_string(stripslashes($alt_phone));
 $fax = mysql_real_escape_string(stripslashes($fax));
if (checkIfUser($user)){
return 1;
}elseif (!validateEmail($email)){
return 2;
}elseif (!validateUsername($user)) {
return 3;
}elseif (checkIfEmail($email)) {
return 4;
}elseif ($agree!= "agree") {
return 5;
}elseif ($pass == "") {
return 6;
}else {
 // Get remote IP
$ipaddress = getenv('REMOTE_ADDR');
$reg_date = date("Y-m-d H:i:s");
 
 //Encrypt password for database
 $salt = 's+(_a*';
$pass = md5($pass.$salt);
 
$sql = "INSERT INTO users (ipaddress,user,pass,email,first_name,last_name,phone,alt_phone,fax,reg_date) VALUES ('" . $ipaddress . "', '" . $user . "','" . $pass . "', '" . $email . "', '" . $first_name . "', '" . $last_name . "', '" . $phone . "', '" . $alt_phone . "', '" . $fax . "', '" . $reg_date . "')"; 
$result = mysql_query($sql);
return 99;
}
}

I would also like to let some of you look at it for an external code review if you would be willing this will be a project hosted on sourceforge and I appreciate any comments.

Best Regards,
Brandon

[eelixduppy]

The only thing I would say is that you shouldn't just stripslashes without knowing anything about the data. Since you want to distribute this as open source code, it needs to run on different servers no matter what their configuration is, in which case each escape statement should check for magic quotes being enabled:

$user = mysql_real_escape_string(([url=http://us2.php.net/get-magic-quotes-gpc]get_magic_quotes_gpc[/url]())? stripslashes($user): $user); 

Try that.

[bkeep]

Thanks for replying. I think I got it sorted. I should not have been using stripslashes instead I should have been using addslashes. I think because by changing that it works as expected.

$user = mysql_real_escape_string((get_magic_quotes_gpc())? $user: addslashes($user));

than to pull data that has been addslashed I just do this

function getUserRecords($user) {
$sql = "SELECT * FROM users WHERE user = '" . $user . "'"; 
$res = mysql_query($sql);
$c=0;
while ($a_row = mysql_fetch_array($res)) {
 $records[$c]["user"] = (get_magic_quotes_gpc())? $a_row["user"]: stripslashes($a_row["user"]);
$c++;
 }
return $records;
}

Would the above code be correct?
Thanks for all of your help pointing me in the right direction.

Best Regards,
Brandon

[eelixduppy]

No, you wouldn't want to apply addslashes to the string. You would only want to keep it how I have shown you above, no other string manipulation. mysql_real_escape_string takes care of everything that needs to be taken care of.

[bkeep]

Ok so when I pull the data nothing needs to be done to it either? in the database the data o'reily should look like o\'reily or should it just be shown as o'reily. This is where I show my ignorance.

Thanks for helping me out I appreciate it

Best Regards,
Brandon

[bkeep]

For whatever reason I feel dumb when reading about this this post shed a little light on it for me.

excerpted from about halfway down the page

4> You should never need to use stripslashes on output from a database. Aside from security, escaping quotes for instance, is solely for the benefit of the SQL parser. This is no different than escaping quotes in PHP strings, in order to not confuse the PHP parser and get an error. If you echo the string, it will not contain the escape characters, they are there solely to tell the parser what language characters are intended to be part of the string, and which ones are to be interpreted as actual code. The SQL parser in a database is no different.

5> Magic quotes and addslashes are vulnerable to character set differences and should not be used. If you can't turn it off, run code to defeat it, which is the proper use of stripslashes.

referenced from this post [webmasterworld.com...]

Regards,
Brandon