User register script PHP

Question

So, I needed to make me a register script for my website so pepole can register, and just wanted to know if It's okey. So, is there any vulns in this script i made?It works perfectly, just that i wanted to have someone to look over it here. Or if there could be some inprovments that could make it better.

 <?php
        $Username = $_POST['Username'];
        $Password = $_POST['Password'];
        $Email    = $_POST['Email'];
        $IP       = $_SERVER['REMOTE_ADDR'];
        if ($IP != "84.***.***.**") { die(); } // Beta, Restrict users from using it until it's done
        /* Random Activate code generator */
        function GenerateCode() {
            $R1 = rand(1,9);
            $R2 = rand(1,9);
            $R3 = rand(1,9);
            $R4 = rand(1,9);
            $R5 = rand(1,9);
            $R6 = rand(1,9);
            $C1 = "$R1$R2$R3$R4$R5$R6"; // Simple complie method
            return $C1;
        }
        function Encrypt($Pass) {
            $B64 = base64_encode($Pass);
            $MD5 = md5($B64);
            $Hash = base64_encode($MD5);
            return $Hash;
        }
        /* Mysql data */
        $MysqlUsername = "root";
        $MysqlPassword = "**********";
        $MysqlHostname = "localhost";
        $MysqlDatabase = "teamgamersnet";

        $Sql = new mysqli($MysqlHostname, $MysqlUsername, $MysqlPassword, $MysqlDatabase);
        if ($Sql->connect_error){ echo $Sql->connect_error; }
        $sUser = $Sql->real_escape_string($Username);
        $sPass = $Sql->real_escape_string($Password);
        $sMail = $Sql->real_escape_string($Email);
        $xPass = $Sql->real_escape_string(Encrypt($Password));
        if (!$Username | !$Password | $Email) { die("Please fill in all the fields"); }
        $CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
        $CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
        if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
        if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }
        $xCode = GenerateCode();
        $Insert = "
        INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`) 
        VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');
        ";
        $Create = $Sql->query($Insert);
        if (!$Create) { echo "S**t, a error happened D:";}
        $to = $Email;
        $subject = 'User account activation | Team Gamers Net';
        $message = '
        <h3>Thank you for registering at Team Gamers Net</h3>
        <p>Just one more step and that is to activate your account</p>
        <p>You registred with username '.$Username.'<p>
        <p>And your password you selected (not shown)</p>
        <p>To activate and contiune, please click <a href="http://TeamGamers.net/Activate.php"><b>HERE</b></a></p>
        ';
        $headers = "From: [email protected] \r\n";
        $headers .= "Reply-To: [email protected] \r\n";
        $headers .= "MIME-Version: 1.0\r\n";
        $headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
        mail($to, $subject, $message, $headers);
        header("Location: /?register=true");
    ?>

mseancole · Accepted Answer · 2012-10-09 21:52:52Z

This answer is partially an extension of jsanc623's. He did a good job, but I thought I would clarify a few things he missed.

First, There is still a lot of repetition here that can easily be solved by adding a loop or function. For instance, in jsan's answer he is using an unwound loop as a return for the GenerateCode() function. Unwinding loops is really unnecessary in PHP as its performance returns are negligible. Instead, if you used a for loop you could define the amount of random numbers you wished to generate, that way this would be reusable and extensible.

function GenerateCode( $length = 6 ) {
    $return = '';

    for( $i = 0; $i < $length; $i++ ) {
        $return .= rand( 1, 9 );
    }

    return $return;
}

Also, be wary of your line lengths. For instance, the following line is too long and can cause difficulties in legibility. Add a new line after the opening bracket, and if that isn't enough, don't be afraid to add whitespace and newlines inside the statement too.

if (empty($Username) || empty($Password) || empty($Email)) { die("Please fill in all the fields"); }
//better
if (empty($Username) || empty($Password) || empty($Email)) {
    die("Please fill in all the fields");
}
//also good
if (
    empty($Username)
    || empty($Password)
    || empty($Email)
) {
    die("Please fill in all the fields");
}

Finally, when you have large strings, you can either use heredoc or concatenation. The advantage of heredoc is that you don't have to worry about escaping entities, such as quotation marks, or even simple variables. When using straight concatenation, especially if you are not doing anything between the concatenations, then there is no need to redeclare the variable again and again.

//heredoc
$message = <<<HTML
<h3>Thank you for registering at Team Gamers Net</h3>
<p>Just one more step and that is to activate your account</p>
<p>You registred with username $Username<p>
<p>And your password you selected (not shown)</p>
<p>To activate and contiune, please click <a href="http://TeamGamers.net/Activate.php"><b>HERE</b></a></p>
HTML;//note that this MUST be unindented

//straight concatenation
$headers = "From: [email protected] \r\n"
    . "Reply-To: [email protected] \r\n"
    . "MIME-Version: 1.0\r\n"
    . "Content-Type: text/html; charset=ISO-8859-1\r\n"
;

Thanks mseancole! Great points you made - especially the repetition and unwound loop (I've been stuck in opt mode lately it seems). I want to reiterate that the ending of the heredoc must NOT be indented - I don't believe you've put enough emphasis there, and god knows how much time I spent scratching my head when first encountering heredocs when I started coding. — jsanc623, Oct 16, 2012 at 16:40

jsanc623 · Accepted Answer · 2012-10-06 01:08:35Z

Ok, there are a couple of things wrong with your code and your method of organizing the code. I've pointed them out to you in the code below within comments. The comments preceded with # are my comments.

<?php

/* Mysql data */
$MysqlUsername = "root";
$MysqlPassword = "**********";
$MysqlHostname = "localhost";
$MysqlDatabase = "teamgamersnet";

$Username = $_POST['Username'];
$Password = $_POST['Password'];
$Email    = $_POST['Email'];

# Check for empty variables 
if (empty($Username) || empty($Password) || empty($Email)) { die("Please fill in all the fields"); }

# We're only using $IP once, so why set it above? Let's do a direct comparison here
// Beta, Restrict users from using it until it's done
#OLD:: 
#if ($IP != "84.***.***.***") { die(); }

#NEW::
if ($_SERVER['REMOTE_ADDR'] != "84.***.***.**") { die(); } 

/* Random Activate code generator */
function GenerateCode() {
    # You're setting 7 variables with like variables, and returning them
    #OLD::
    #$R1 = rand(1,9);
    #$R2 = rand(1,9);
    #$R3 = rand(1,9);
    #$R4 = rand(1,9);
    #$R5 = rand(1,9);
    #$R6 = rand(1,9);
    #$C1 = "$R1$R2$R3$R4$R5$R6";
    #return $C1;

    #NEW::
    return rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9);
}

/* Encrypt the password */
function Encrypt($Pass) {
    # Again, setting multiple variables in the name of readability over performance
    #OLD::
    #$B64 = base64_encode($Pass);
    #$MD5 = md5($B64);
    #$Hash = base64_encode($MD5);
    #return $Hash;

    #NEW::
    return base64_encode( md5( base64_encode($Pass) ) );
}


# Let's move the Mysql Connection Data to the top, so its easier to modify in the future
#OLD::
/* Mysql data */
#$MysqlUsername = "root";
#$MysqlPassword = "**********";
#$MysqlHostname = "localhost";
#$MysqlDatabase = "teamgamersnet";

#NEW::
# Moved to top

$Sql = new mysqli($MysqlHostname, $MysqlUsername, $MysqlPassword, $MysqlDatabase);
if ($Sql->connect_error){ echo $Sql->connect_error; }

/* Let's add a comment here: Escape the user data */
$sUser = $Sql->real_escape_string($Username);
$sPass = $Sql->real_escape_string($Password);
$sMail = $Sql->real_escape_string($Email);
$xPass = $Sql->real_escape_string(Encrypt($Password));

# Let's move this to the top, right after these are set from _POST, to avoid all the 
# prior computation before running into this. Also, use empty() or isset() instead of ! checking
# See: http://www.phpbench.com/
#OLD::
#if (!$Username | !$Password | $Email) { die("Please fill in all the fields"); }

#NEW::
# Moved to top, also - syntax errors fixed: it's || not | 

/* Let's add a comment here: Check if the user exists */
$CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
$CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }

/* Let's add a comment here: Generate an activation code */
$xCode = GenerateCode();

/* Let's add a comment here: Write out the Insert statement  */ 
$Insert = "INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`) 
VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');";

/* Let's add a comment here: Insert the SQL statement */
$Create = $Sql->query($Insert);
# Why are we echoing? Shouldn't we be dying?
#OLD::
#if (!$Create) { echo "S**t, a error happened D:";}

#NEW::
if ( empty($Create) || !isset($Create) ) { die("Sorry, an error occurred during Create."); }

/* Let's add a comment here: Build the message, and send it to the user. */
$to = $Email;
$subject = 'User account activation | Team Gamers Net';
$message = '
<h3>Thank you for registering at Team Gamers Net</h3>
<p>Just one more step and that is to activate your account</p>
<p>You registred with username '.$Username.'<p>
<p>And your password you selected (not shown)</p>
<p>To activate and contiune, please click <a href="http://TeamGamers.net/Activate.php"><b>HERE</b></a></p>
';
$headers = "From: [email protected] \r\n";
$headers .= "Reply-To: [email protected] \r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
mail($to, $subject, $message, $headers);

header("Location: /?register=true");    
?>

Here's the same code, with my comments removed:

<?php

/* Mysql connection data */
$MysqlUsername = "root";
$MysqlPassword = "**********";
$MysqlHostname = "localhost";
$MysqlDatabase = "teamgamersnet";

$Username = $_POST['Username'];
$Password = $_POST['Password'];
$Email    = $_POST['Email'];

/* Check for empty variables */ 
if (empty($Username) || empty($Password) || empty($Email)) { die("Please fill in all the fields"); }

/* Beta, Restrict users from using it until it's done */
if ($_SERVER['REMOTE_ADDR'] != "84.***.***.**") { die(); } 

/* Random Activate code generator */
function GenerateCode() {
    return rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9);
}

/* Encrypt the password */
function Encrypt($Pass) { return base64_encode( md5( base64_encode($Pass) ) ); }

$Sql = new mysqli($MysqlHostname, $MysqlUsername, $MysqlPassword, $MysqlDatabase);
if ($Sql->connect_error){ echo $Sql->connect_error; }

/* Escape the user data */
$sUser = $Sql->real_escape_string($Username);
$sPass = $Sql->real_escape_string($Password);
$sMail = $Sql->real_escape_string($Email);
$xPass = $Sql->real_escape_string(Encrypt($Password));

/* Check if the user exists */
$CheckUser = $Sql->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
$CheckMail = $Sql->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
if ($CheckUser->num_rows == 1) { die("Username allready exist"); }
if ($CheckMail->num_rows == 1) { die("E-Mail allready exist"); }

/* Generate an activation code */
$xCode = GenerateCode();

/* Write out the Insert statement  */   
$Insert = "INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`) 
VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');";

/* Insert the SQL statement */
$Create = $Sql->query($Insert);

/* Check if SQL insert went well, otherwise die */
if ( empty($Create) || !isset($Create) ) { die("Sorry, an error occurred during Create."); }

/* Build the message, and send it to the user. */
$to = $Email;
$subject = 'User account activation | Team Gamers Net';
$message = '
<h3>Thank you for registering at Team Gamers Net</h3>
<p>Just one more step and that is to activate your account</p>
<p>You registred with username '.$Username.'<p>
<p>And your password you selected (not shown)</p>
<p>To activate and contiune, please click <a href="http://TeamGamers.net/Activate.php"><b>HERE</b></a></p>
';
$headers = "From: [email protected] \r\n";
$headers .= "Reply-To: [email protected] \r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
mail($to, $subject, $message, $headers);

header("Location: /?register=true");    
?>

Sunderi Mehta · Accepted Answer · 2012-10-06 02:52:37Z

You have bad security. First of all, base64->md5->base64 is in no way more secure than md5. In addition, md5 is not a cryptographic hash function to be used for passwords, and whatever hash you use, you need to salt. Also, following coding conventions will help people critique your code in the future.

I recommend you use BCrypt or SCrypt, but I'll demonstrate Sha256 here.

// X= function Encrypt($Pass) =X Hashing does NOT equal encrypting
function hash_pass($pass, $salt) {
    return hash("sha256", $pass . $salt);
}

function simple_salt() {
    mt_srand(microtime(true) * 100000 + memory_get_usage(true)); // Don't be scared, all this is doing is adding more entropy to the random seed
    return md5(uniqid(mt_rand(), true));
}

$salt = simple_salt(); // Store this as well
$hash = hash_pass($pass, $salt);

This, times a million. I completely forgot about salting the hash. — jsanc623, Oct 6, 2012 at 5:34

Dirk McQuickly · Accepted Answer · 2012-10-12 15:05:58Z

function GenerateCode() {
    $R1 = rand(1,9);
    $R2 = rand(1,9);
    $R3 = rand(1,9);
    $R4 = rand(1,9);
    $R5 = rand(1,9);
    $R6 = rand(1,9);
    $C1 = "$R1$R2$R3$R4$R5$R6"; // Simple complie method
    return $C1;
}

Can also be written as:

function GenerateCode()
{
    return rand(100000, 999999);
}

Since the random number generator is seeded automatically this will be just as random as concatenating 6 random generated digits. You can optionally cast the number to a string like

return (string)rand(100000, 999999);

Samson · Accepted Answer · 2013-02-07 09:14:54Z

Just adding my 2 cents in here, as I see nobody did this remark. Your code definitely lacks structure and you should get familiar with MVC pattern.

Do not do that mixup of database logic and HTML output in a single file. Use objects to organise your data.

/* Mysql connection data */
$mysqlUsername = "root";
include 'UserModel.php'
$mysqlPassword = "**********";
$mysqlHostname = "localhost";
$mysqlDatabase = "teamgamersnet";

$username = $_POST['Username'];
$password = $_POST['Password'];
$email    = $_POST['Email'];

$userModel=new UserModel();
$userModel->connect($mysqlHostname, $mysqlUsername, $mysqlPassword, $mysqlDatabase);
$userModel->addUser($username, $password, $email)

/* Build the message, and send it to the user. */
$to = $Email;
$subject = 'User account activation | Team Gamers Net';
$message = '
<h3>Thank you for registering at Team Gamers Net</h3>
<p>Just one more step and that is to activate your account</p>
<p>You registred with username '.$Username.'<p>
<p>And your password you selected (not shown)</p>
<p>To activate and contiune, please click <a href="http://TeamGamers.net/Activate.php"><b>HERE</b></a></p>
';
$headers = "From: [email protected] \r\n";
$headers .= "Reply-To: [email protected] \r\n";
$headers .= "MIME-Version: 1.0\r\n";
$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
mail($to, $subject, $message, $headers);
header("Location: /?register=true");    


/* UserModel.php 
 * Wraps all your logic (no HTML here)
*/
<?php
class UserModel {

    /* holds a mysql connection */
    private $sqlConnection;

    public function connect($mysqlHostname, $mysqlUsername, $mysqlPassword, $mysqlDatabase){
        $this->sqlConnection= new mysqli($mysqlHostname, $mysqlUsername, $mysqlPassword, $mysqlDatabase);
        if ($this->sqlConnection->connect_error){ 
            die $this->sqlConnection->connect_error; 
        }
    }
    public function generateCode() {
        return rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9) . rand(1,9);
    }

    public function encrypt($pass) { 
        return base64_encode( md5( base64_encode($pass) ) ); 
    }

    public function addUser($username, $password, $email){
        if (empty($username) || empty($password) || empty($email)) { 
            die("Please fill in all the fields"); 
        }
        /* Escape the user data */
        $sUser = $sqlConnection->real_escape_string($username);
        $sPass = $sqlConnection->real_escape_string($password);
        $sMail = $sqlConnection->real_escape_string($email);
        $xPass = $sqlConnection->real_escape_string(Encrypt($password));

        /* Check if the user exists */
        $checkUser = $sqlConnection->query("SELECT Username FROM `users` WHERE `Username` = '".$sUser."'");
        $checkMail = $sqlConnection->query("SELECT `E-Mail` FROM `users` WHERE `E-Mail` = '".$sMail."'");
        if ($checkUser->num_rows == 1) { 
            die("Username allready exist"); 
        }
        if ($checkMail->num_rows == 1) { 
            die("E-Mail allready exist"); 
        }

        /* Generate an activation code */
        $xCode = GenerateCode();

        /* Write out the Insert statement  */   
        $insert = "INSERT INTO `teamgamersnet`.`users` (`ID`, `Username`, `Password`, `E-Mail`, `IP`, `Activated`, `ActivateId`) 
        VALUES (NULL, '".$sUser."', '".$xPass."', '".$sMail."', '".$IP."', 'false', '".$xCode."');";

        /* Insert the SQL statement */
        $result= $sql->query($insert);
        if ( empty($result) || !isset($result) ) { 
            die("Sorry, an error occurred during Create."); 
    }
}

Here is an idea on how to clean-up a bit of that code, make it easier to understand and mantain. This is only half finished, you can finish separating the concerns after you go through some MVC tutorials. Also there are some coding standards on which you should take a look. You could start here

Stack Exchange Network

User register script PHP

5 Answers 5

Your Answer

Not the answer you're looking for? Browse other questions tagged
php
or ask your own question.

Hot Network Questions

User register script PHP

5 Answers 5

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged php or ask your own question.

Related

Hot Network Questions

Not the answer you're looking for? Browse other questions tagged
php
or ask your own question.