[SOLVED] if anyone has time...

[justAnoob]

Is this secure enough for a registration script??? Or should I do more to it??? I'm new to the whole security thing and not really sure what I should be looking for.

<?php
session_start();
include ("connection.php");

$username = mysql_real_escape_string($_POST["username"]);
$sql="SELECT * FROM $tbl_name WHERE username = '$username'";
$result=mysql_query($sql);
$count=mysql_num_rows($result);

if($count > 0)
{
$_SESSION['duplicate'] = "<font color=red> This username is already registered. Please try again.";
header("location: http://www.--------.com/----------.php");
exit();
}
else
{
$username = mysql_real_escape_string($_POST["username"]);
$password = md5($_POST["password"]);
$email = mysql_real_escape_string($_POST["email"]);
$fname = mysql_real_escape_string($_POST["fname"]);
$lname = mysql_real_escape_string($_POST["lname"]);
$address = mysql_real_escape_string($_POST["address"]);
$city = mysql_real_escape_string($_POST["city"]);
$state = mysql_real_escape_string($_POST["state"]);
$zipcode = mysql_real_escape_string($_POST["zipcode"]);
$country = mysql_real_escape_string($_POST["country"]);

    $sql = "INSERT INTO $tbl_name(username, password, email)VALUES('$username','$password','$email')";
mysql_query($sql) or die(mysql_error());

$_SESSION['goodreg'] = "<font color=white>Thank you for registering $username. You may now log in.";
sleep(3);
header("location: http://www.--------.com/--------.php");
exit();

mysql_close();
}
?>

[Ken2k7]

This is just a code review:

1. Run checks on valid characters in a username.

2. Use LIMIT 1 in your SQL.

3. Don't use mysql_real_escape_string on username twice.

4. You should have password confirm field for registration.

5. Run checks on valid email, etc fields. Make sure password is of good length.

6. Use mysql_real_escape_string on the password before hashing.

7. Utilize salt + password.

8. <font> tag is deprecated. Don't use it. And if you do, at least close the tag.

[GingerRobot]

6. Use mysql_real_escape_string() on the password before hashing.

 

Why? What's the point?

[roopurt18]

$username = mysql_real_escape_string($_POST["username"]);
$sql="SELECT * FROM $tbl_name WHERE username = '$username'";
$result=mysql_query($sql);
$count=mysql_num_rows($result);

 

If all you're doing here is checking if the name is available, then change the query to:

$username = mysql_real_escape_string( $_POST["username"] );
$sql = "select count(*) as n from `$tbl_name` where username='{$username}'";
$result = mysql_query( $sql );
if( !$result ) {
  throw new Exception( "Count username." );
}
$row = mysql_fetch_object( $result );
if( !$row ) {
  throw new Exception( "No row." );
}
if( $row->n > 0 ) {
  // user exists
}else{
  // add user
}

[Ken2k7]

6. Use mysql_real_escape_string() on the password before hashing.

 

Why? What's the point?

I never had to until a I did a Freelance a few weeks back. The client had a bunch of SQLs that needed values to go through mysql_real_escape_string so the SQL errors would stop. I did all except the password, but apparently it still didn't pass. So I tried using the same function for the password and it worked. :-\

 

So from then on, I just went with it.

[roopurt18]

I did all except the password, but apparently it still didn't pass. So I tried using the same function for the password and it worked. :-\

 

Does not compute!

[Ken2k7]

I did all except the password, but apparently it still didn't pass. So I tried using the same function for the password and it worked. :-\

 

Does not compute!

http://www.phpfreaks.com/forums/index.php/topic,252603.0.html

 

That's the topic for it. For some reason, it just didn't work. But I already took care of the job.

[roopurt18]

If you are hashing the password, then it's irrelevant to escape_string() the password before hashing it as the hashing function will likely remove the harmful characters anyways.  All that matters is the data as it is going to be entered into the database is escaped(), which means:

 

$password = mysql_real_escape_string( md5( $salt . $password ) );

 

as opposed to:

 

$password = md5( mysql_real_escape_string( $salt . $password ) );

 

Note that in the second example, it will work as I don't think md5() will create values that need to be escaped for the database.  But if you are using a hash function which may output: ' \ or other harmful characters, then you still need to:

 

$password = mysql_real_escape_string( some_hash( mysql_real_escape_string( $salt . $password ) ) );

 

Long story short is data needs to be escaped when it's in the form or state that it will be entered into the database, escaping it at any other point in time is irrelevant.

[Ken2k7]

Ah got it. Thanks for the explanation roopurt18.

[GingerRobot]

$password = mysql_real_escape_string( some_hash( mysql_real_escape_string( $salt . $password ) ) );

 

Why would you escape it twice? You'd only need to escape it after running it through the hash function that produces special chars as it's output.

 

If you're using MD5, it's unnecessary to escape the string at all. I.e. this:

 

$password = md5($salt . $password );

 

Is no more safe than this:

 

$password = md5( mysql_real_escape_string( $salt . $password ) );

[roopurt18]

I guess I wasn't clear, but my point is you wouldn't.

 

You only escape the data once it's in the form that it will be going into the database, which is after the hash function.  You should still escape the output from hashing functions, even if at this point in time it doesn't seem necessary.

 

So just to be ultra clear, this is the correct way:

$password = mysql_real_escape_string( md5( $salt . $password ) );