Registration script

xukster

Hello folks. I'm new to php - and programming overall - and this is my first script. I'm not too confident that I did things in an efficient manner or if I'm missing a thing or two that would result in a rock solid piece of php. Any tips, tricks, insight or constructive criticism would be greatly appreciated... thanks in advance!

<?php require_once("includes/connection.php");?>
<?php require_once("includes/functions.php");?>

<?php
/*
 * Using the hidden form field to check if the form has been submitted
 * According to O'reilly: "Testing for the presence of a hidden element
 * avoids problems that can result from browsers' varying behaviors..."
 */
if (array_key_exists('data_submitted', $_POST)) {
    // We convert the post data into variables to work with them easier
    // using htmlentities to allow funky characters for better passwords
    $email = htmlentities($_POST['email'], ENT_QUOTES);
    $password = htmlentities($_POST['password'], ENT_QUOTES);
    $verify = htmlentities($_POST['verify'], ENT_QUOTES);

// Set up an empty variable to hold errors, if we get any
// Using this instead of an array so it can be echoed prettily if desired
$errors = "";

// We use a regEx to see if the $email given is legit
// Using this method we wont need to run a length check on the address
if(!preg_match("/^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,4}$/", "$email")){
    // add a string to the $errors variable to echo
    $errors = $errors."The system cannot recognize the given email address.<br>";
}

// Checking if the $email is already registered
$checkMailInUse = mysql_query("SELECT * FROM users WHERE email='$email'");
if(mysql_num_rows($checkMailInUse) != 0){
    $errors = $errors."Email address is already registered.<br>";
}

// Still have to check the length of the $password
if(strlen($password)<6 || strlen($password)>28){
    $errors = $errors."Passwords must be between 6 and 28 characters long.<br>";
}

// And see that it has been entered correctly
if($verify != $password){
    $errors = $errors."Passwords don't match.<br>";
}

// Now lets check if there were any errors
if($errors != ""){
    // dont bother doing anything, the page will reload and errors will be displayed
}else{
    // encrypt the password
    $md5 = md5($password);
    // and insert the information into the database
    mysql_query("INSERT INTO users (email, hash, joined)
    VALUES ('{$email}', '{$md5}', CURDATE());")
    or die(mysql_error());
}

}else{
    //do nothing
}

?>












<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
    <head>
        <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
        <title></title>
    </head>
    <body>
                <form action="register.php" method="post">
                    Email Address:<br>
                    <input type="text" name="email" id="email"><br><br>
                    Desired Password:<br>
                    <input type="password" name="password" id="password"><br><br>
                    Verify Password:<br>
                    <input type="password" name="verify" id="verify"><br><br>
                    <input type="submit" name="submit" id="submit" value="Register">
                    <input type="hidden" name="data_submitted" value="1"/>
                </form>

    <p><?php echo($errors); ?></p>
</body>
</html>

halojoy

Looks quite alright.
After the INSERT your script goes to display form again.
Maybe finish by some message 'you were registered + link + exit();

Weedpacket

    // using htmlentities to allow funky characters for better passwords

Since these passwords are never being displayed - let alone displayed in HTML, then a simple escaping operation would be sufficient (since you're using MySQL, that would be [man]mysql_real_escape_string[/man]). Using the correct function would also be more robust and wouldn't mean garbling the data you store.

In fact, once you hash it, then no matter what characters were in the password to begin with, the result will always be exactly thirty-two characters selected from [0123456789abcdef] - none of which need any sort of escaping to be embedded in an INSERT query.

On the other hand, using html entities means that "<password>" will hash as "cb1ebc435675187bdcfb539b370c2e37" instead of "34b339799d540a72bf1c408c0e68afdd". If there might be any passwords with "funky characters" already stored then unless you're prepared to replace them you're stuck with using htmlentities.

"Passwords must be between 6 and 28 characters long."

Why the maximum? If I want [font=monospace]PoEyapTiPVJotGHPCAywndbatpfdotorotGrtboahertyssarypiootsfdTpwtslttoyEmTy[/font] (and I can reproduce that from memory when needed) then why can't I have it? It doesn't cost you anything extra to store.

xukster

thanks for the feedback, good points on all !

bradgrafelman

Did you accept any of the suggestions above and are you still looking for more? If so, you might want to post a fresh copy of your code so that others can base their suggestions on the latest version.

DexterMorgan

I have been told that using md5 for hashing passwords is no longer good enough and you should use sha1(); instead, not entirely sure though.

Personally I use hash('sha512',$value); which creates a crazy long string from the password.

bradgrafelman

DexterMorgan;10953163 wrote:
I have been told that using md5 for hashing passwords is no longer good enough

While MD5 is getting relatively weaker (heck, even a PlayStation 3 can hack it), it probably doesn't help that people don't utilize a strong salt (or any salt at all) when hashing passwords either.

DexterMorgan

bradgrafelman;10953206 wrote:
While MD5 is getting relatively weaker (heck, even a PlayStation 3 can hack it), it probably doesn't help that people don't utilize a strong salt (or any salt at all) when hashing passwords either.

Hi, what do you mean by a salt on a password? like a random set of characters stuck to the end or beginning? I would not mind making my scripts more secure.

Thanks

laserlight

DexterMorgan wrote:
Hi, what do you mean by a salt on a password? like a random set of characters stuck to the end or beginning?

Yes, or otherwise combined with the password, or a hash thereof.

bradgrafelman

It's quite useful to use a salt when hashing passwords, since you're bound to have some users that don't pick strong passwords. If an attacker were to somehow compromise your application and gain access to a user's hashed password stored in the DB, they could then take that hash and use a rainbow table (or try to brute force it, but they might not want to wait around that long) to find the user's password OR find a collision (e.g. a different string that results in the same hash value).

With a salt, it becomes more unlikely that a rainbow table will be very useful (even if you have a weak password).

laserlight

You should use a user specific salt so that even if such an attacker does want to use brute force, it would have to be done for each password, rather than across the board.

DexterMorgan

If the attacker has access to these hashed passwords would he not also have access to the script that is adding these salts and just reverse engineer them?

laserlight

DexterMorgan wrote:
If the attacker has access to these hashed passwords would he not also have access to the script that is adding these salts and just reverse engineer them?

In fact, the salt values can be stored as plaintext with the hashed passwords. It is assumed that the attacker knows the method by which the salt is combined with the password, but this is no help in "reverse engineering", assuming that the cryptographic hash algorithm has preimage resistance.

DexterMorgan

laserlight;10953264 wrote:
In fact, the salt values can be stored as plaintext with the hashed passwords. It is assumed that the attacker knows the method by which the salt is combined with the password, but this is no help in "reverse engineering", assuming that the cryptographic hash algorithm has preimage resistance.

ahhh I did not think of that, I was thinking of $password.$salt then hashing them. Very clever, thanks for clearing that up.

laserlight

DexterMorgan wrote:
I was thinking of $password.$salt then hashing them

hmm... just in case there is a misunderstanding: hashing $password.$salt is indeed one way. $password is discarded, but $salt is also stored. Personally, I prefer to compute the hash of the password, then concatenate the salt, and then compute and store the hash of the result.

DexterMorgan

laserlight;10953268 wrote:
hmm... just in case there is a misunderstanding: hashing $password.$salt is indeed one way. $password is discarded, but $salt is also stored. Personally, I prefer to compute the hash of the password, then concatenate the salt, and then compute and store the hash of the result.

Ahh right I see, yeah I understand, I will try this technique on one of my projects, thanks a lot.