First attempt at salting a log in

[Darkranger85]

Hey guys!

 

In my tutorials they were putting together a login system. After I watched the tutorial I decided to put one together that was my own.

 

also, the tutorial only used MD5. After I read the post on the top of this forum about MD5 I decided to give salt a go on my own to see if I could pull it off.

 

I'd like to hear what more experienced coders have to say about my code, but I'd appreciate it if you went easy on me lol. I'm quite happy with myself that I put this together all on my own and it works, I have tested it with my database lol.

 

<?php
    //Check for form values in POST array//
    if (isset($_POST['username'])&& isset($_POST['password'])){
        
        //strip tags and whitespace from user//        
        if(!empty($_POST['username'])){
            $T_user = strip_tags($_POST['username']);
            $user = str_replace(' ','',$T_user); 
        }else{
            $user = false;
        }
                
        //strip tags and spaces//
        if(!empty($_POST['password'])){
            $T_pass = strip_tags($_POST['password']);
            $T2_pass = str_replace(' ', '', $T_pass);
        
        //Generate SALT and encrypt//
        $salt = 'angelinajolie';
        $pass = md5($T2_pass.$salt);
                        
        }else{
            $pass = false;
        }
        
        //Check User and Pass for NULL then query database//
        if($pass || $user != false){
           
           $query = "SELECT id FROM users WHERE username = '$user' AND password ='$pass'";
           $query_run = mysql_query($query);
           $query_rows = mysql_num_rows($query_run);
                        
            if($query_rows == 0){
                echo 'Password and/or Username are invalid!';
                echo $query_rows;
            }else if ($query_rows != 0){
                echo 'Welcome back!';
            }
           
        }else{
            echo 'Must specify Username and Password!';            
        }
        
    }

?>


<form action="<?php echo $current_file; ?>" method="POST">
        
        Username: <input type="text" name="username" /> Password: <input type="password" name="password" />
        <input type="submit" value="Login" />
</form>

[Darkranger85]

NOTE: I have changed "false" to NULL and removed the "echo $query_rows;" from line 34.

[Darkranger85]

Anyone?

[spiderwell]

  if($pass || $user != false)

 

could be shortened

 

  if($pass || $user)

 

 

not sure why you would strip info from the inputs like spaces etc and then enter it to the DB rather than returning to user to state 'cannnot enter tags or spaces' becuase if i entered my name as joe bloggs, it would be entered as joebloggs without my knowledge and i couldnt log in!!

 

the sql statement should have the variables wrapped in mysql escape string function.

 

no need to use 2 variables when 1 can be used:

 

$T_user = strip_tags($_POST['username']);

$user = str_replace(' ','',$T_user);

 

just use $user for all of it

 

[Drongo_III]

Looks about right to me. Except you practically want to make your salt something that can't be cracked by a library. So it should really be a long string of random characters.

 

You might want to go further and hash the md5 password:

  $password = "Userpassword";
    $salt = sha1(md5($password));
    $password = md5($password.$salt);

 

[scootstah]

That's not how you use salts. A salt needs to be a randomly generated string with as much entropy as possible. And it needs to be unique to each user. If your salt is the same for every user then it is the same as not having a salt to begin with.

 

A salt has only one purpose: to make the hash of users with like passwords different. If you MD5 "qwerty" the hash would be "d8578edf8458ce06fbc5bb76a58c5ca4". So if someone got a dump of your users table, and they see two or more people with the hash "d8578edf8458ce06fbc5bb76a58c5ca4", all they have to do is crack it once and they have multiple logins. If you were to use a random salt then each hash would be different, even if the password is the same. So if the salt was "kfdgjk34639" then the hash is now "8d72631f8a2230116b735c286d8a59ba". If the salt is unique to each user, no two users will ever have the same hash.

 

Additionally, since the salt will be unique to each user you need to store the salt in the database when they create an account. Since the hash will be computed based on the salt, the salt is required to re-hash the password and get a match. So when a user tries to login you have to first retrieve the salt from the database and then hash the password with it.

 

Aside from that, another big no-no here is that you are vulnerable to SQL injection. You need to use mysql_real_escape_string() to prevent that.

 

Here is a quick concept factoring in my above points:

register.php

if (!empty($_POST)) {
$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);

if (!empty($username) && !empty($password)) {
	// create salt
	// the salt will be a random 10 character alpha-numeric string
	$salt = substr(sha1(uniqid(mt_rand(), true)), 0, 10);

	// hash the password
	$hash = md5($password . $salt);

	$query = mysql_query("INSERT INTO users
		(username, password, salt)
		VALUES
		('" . $username . "', '" . $hash . "', '" . $salt . "'");
}
} else {
echo '<form action="" method="post">
Username: <input type="text" name="username" /><br />
Password: <input type="password" name="password" /><br />
<input type="submit" name="submit" value="Register" />
</form>';
}

 

login.php

if (!empty($_POST)) {
$username = mysql_real_escape_string($_POST['username']);
$password = mysql_real_escape_string($_POST['password']);

if (!empty($username) && !empty($password)) {
	$query = mysql_query("SELECT username, password, salt
		FROM users
		WHERE username='" . $username . "'");

	if (mysql_num_rows($query) == 1) {
		$row = mysql_fetch_assoc($query);

		// re-hash the password
		$hash = md5($password . $row['salt']);

		// match them
		if ($hash == $row['password']) {
			// matched, so the user is logged in
		}
	}
}
} else {
echo '<form action="" method="post">
Username: <input type="text" name="username" /><br />
Password: <input type="password" name="password" /><br />
<input type="submit" name="submit" value="Register" />
</form>';
}

[Psycho]

Here are my comments:

 

        if(!empty($_POST['username'])){
            $T_user = strip_tags($_POST['username']);
            $user = str_replace(' ','',$T_user); 
        }else{
            $user = false;
        }

You should always trim() user input before doing any empty() validations. Otherwise an entry of nothing but spaces would pass this validation but would result in empty strings being inserted for the value in the DB!!!. Also, I agree with the above statement. trim()ing a value is a standard practice, but if you are going to do more intensive manipulations (removing inner spaces, tags, or other data) then you need to generate an error and let the user fix the problems. You *could* just do the same manipulation when the user logs in, but that's really not a good solution.

 

EDIT: A further note on manipulating input. Since this is the login form and not the page where the user "creates" there username/password you should do absolutely NO data manipulation [with the exception of trim()]. If you don't allow the character '@' (for example) in usernames then you would only have an error condition for that when the user tries to create their account. On the login page you don't need an error condition since there would be no valid username with '@'. The only person using it would be someone trying to gather information or trying to luck into a valid account./EDIT

 

Also, there is absolutely NO reason to remove/change characters in a password. The value will be converted to a hash anyway. Let the user enter whatever they want. By removing characters you are only making the ultimate value less secure. For example, let's say a user creates their password using "<b>password</b>". Your code would strip out the tags and the user would be left with "password".

 

        //Generate SALT and encrypt//
        $salt = 'angelinajolie';
        $pass = md5($T2_pass.$salt);
                        
        }else{
            $pass = false;
        }

The salt should be something unique to the user that does not change. Otherwise if a malicious user was to get your hashing method above they would have to only generate one lookup table of hash values and they could use that on every user's hashed password. If you instead used a different salt for each user then a malicious user would have to generate an entirely different lookup table to try and backward engineer each password. You can use any existing data that will not change (user ID, date joined, etc) or generate an entirely new value for the salt and store it in the database for each user.

 

Your successful login needs to do something more such as setting a session value to use on subsequent page loads so you know the user is logged in. Otherwise, the login accomplished nothing. Instead, you should create this script to first check if the user is logged in. If not, then display the form or authenticate. You could then call this script on every page load. If the user is logged in then the original page would display. If not, then the user will go to the login process.

 

EDIT#2

Also, you should create a function for your salting process because you need to use it both on the account creation process and the login process. You don't want to have two sets of code for that as it would be too easy to accidentally make a small difference that could cause problems later.

[Psycho]

Here is my take on a rewrite. I've added some comments to hopefully explain the logic flow

 

<?php

//Include this function on BOTH the registration and login scripts
function saltedPassword($password, $salt)
{
    return md5($password . $salt);
}

session_start();

//Check if user is already logged in
if(isset($_SESSION['logged_in']))
{
    //Return control back to calling script
    return true;
}
else
{
    $error_msg = '';
    //Check if login form was submitted
    if (isset($_POST['username'])&& isset($_POST['password']))
    {
        //Preprocess the input
        $username = trim($_POST['username']);
        $password = trim($_POST['username']);

        //Validate data
        $errors = array();
        if(empty($username))
        (
            $errors[] = "Username is required.";
        )
        if(empty($password))
        {
            $errors[] = "Password is required.";
        }

        //If validations passed, attempt authentication
        if(!count($errors))
        {
            $query = sprintf("SELECT salt, password FROM users WHERE username = '%s'",
                             mysql_real_esacpe_string($username));
            $result = mysql_query($query);

            if(mysql_num_rows($result))
            {
                $user_data = mysql_fetch_assoc($result);
                if(saltedPassword($password, $user_data['salt']) == $user_data['password'])
                {
                    //Authentication passed
                    $_SESSION['logged_in'] = 1;
                    return true;
                }
            }

            //Authentication did not pass
            $errors[] = "Password and/or Username are invalid!";
        }
    }

    if(count($errors))
    {
        $error_msg = "The following errors occured:<ul>\n";
        foreach($errors as $err)
        {
            $error_msg .= "<li>{$err}</li>\n";
        }
        $error_msg .= "</ul>\n";
    }
}
?>
<html>
<body>

<?php echo $error_msg; ?>

<form action="<?php echo $current_file; ?>" method="POST">
    Username: <input type="text" name="username" /><br>
    Password: <input type="password" name="password" />
    <input type="submit" value="Login" />
</form>

</body>
</html>

[Darkranger85]

Thank you guys for your suggestions!

 

Again, keeping in mind that I put this together with only a rudimentary knowledge of the concept lol. But I will certainly take your suggestions into account and alter my code. It's a good learning experience cause a lot of the commands and functions you guys have shown I didn't know before this, so my php "vocabulary" is increasing. 

 

 

not sure why you would strip info from the inputs like spaces etc and then enter it to the DB rather than returning to user to state 'cannnot enter tags or spaces' becuase if i entered my name as joe bloggs, it would be entered as joebloggs without my knowledge and i couldnt log in!!

 

 

Please keep in mind that I'm a noob so, being incorrect is always a possibility lol,  but I would assume that as long as the input is striped of space in both the registration and the login it wouldn't matter if you knew it or not because I would match anyway.

 

As for the "why," I was told that you always strip spaces from input in order to prevent mysql commands from being entered, but I supposed sending back a message that those things are not allowed would probably serve that function better. Perhaps I was told wrong? lol

 

But again, thank you guys! And if anyone else has any input for me please post it!

[scootstah]

As for the "why," I was told that you always strip spaces from input in order to prevent mysql commands from being entered, but I supposed sending back a message that those things are not allowed would probably serve that function better. Perhaps I was told wrong? lol

 

You can't hurt a database with spaces. Furthermore, simply removing spaces is not going to prevent SQL injection.

 

To do that, you need to use mysql_real_escape_string() which will escape harmful characters (like quotes) before they interact with the database.

[Psycho]

As for the "why," I was told that you always strip spaces from input in order to prevent mysql commands from being entered, but I supposed sending back a message that those things are not allowed would probably serve that function better. Perhaps I was told wrong? lol

 

You can't hurt a database with spaces. Furthermore, simply removing spaces is not going to prevent SQL injection.

 

To do that, you need to use mysql_real_escape_string() which will escape harmful characters (like quotes) before they interact with the database.

 

To add to that. As I stated above "trimming" spaces IS a standard practice. But, trimming (using the trim() command) only removes white-space characters before and after non-white-space characters. White-space characters are things such as spaces, tabs, line breaks etc. It is NOT standard practice to remove spaces within the value.

 

Plus, the purpose of trimming has nothing to do with protecting the database. It is characters such as quote marks, '*' and some others that could cause a problem. but as scootstah stated using mysql_real_escape_string() will prevent any problems (or using prepared statements).

 

When dealing with user input you need to know exactly why you are doing what you are doing. If you need to restrict certain input know what you are doing that and provide appropriate feedback to the user. It is a bad idea to automatically change the user input without the user knowing.

 

There is no general reason to restrict input for normal "text" values (names, addresses, etc.). So, analyze what the input is for to determine if there is a need. For example, if the input should be numeric, then you obviously want to ensure it doesn't contain letters - same goes for dates. However, if you are allowing the user to enter text that would be displayed in a web-page then you also need to prevent problems if the data has HTML code. It is true that you could strip_tags() on the input before saving it - but that is again modifying the input without the user's knowledge. So, if you don't want to allow them you can validate for it first then not allow the input. Or, you can simply allow the input and instead transform the data so it will still display the original user input but not be interpreted as HTML code using htmlspecialchars().

 

[Darkranger85]

Noted!