Making a secure login System.

[thara]

Why I start this tread is, Just I need to create a secure user registration system in php including login functionality with remember me option.

 

In the starting point I needed to create user registration script. Here I have include my code so far, and just I need to know , is my script secure to prevent any possible attack? my password hashing method is correct? If not can any profession guys tell me what are the things that I need to do to make this script more secure? 

 

This is my PHP so far from user registration script:

<?php
// Error Flag
$error_msg = "";

if (isset($_POST['username'], $_POST['email'], $_POST['password'])) {
    // Sanitize and validate the data passed in
    $username = filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING);
    $email = filter_input(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL);
    $email = filter_var($email, FILTER_VALIDATE_EMAIL);
    if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
        $error_msg .= 'The email address you entered is not valid';
    }
    
    $password = filter_input(INPUT_POST, 'ppassword', FILTER_SANITIZE_STRING);
        
    $prep_stmt = "SELECT member_id FROM members WHERE email = ? LIMIT 1";
    $stmt = $mysqli->prepare($prep_stmt);
    
    if ($stmt) {
        $stmt->bind_param('s', $email);
        $stmt->execute();
        $stmt->store_result();
        
        if ($stmt->num_rows == 1) {
          // A user with this email address already exists
          $error_msg .= 'A user with this email address already exists.';
        }
    } else {
        $error_msg .= 'Database error';
    }
    
        
    if (empty($error_msg)) {
    
                // Create a random salt
        //$random_salt = hash('sha512', uniqid(openssl_random_pseudo_bytes(16), TRUE)); // Did not work
        $random_salt = hash('sha512', uniqid(mt_rand(1, mt_getrandmax()), true));

        // Create salted password
        $password = hash('sha512', $password . $random_salt);

        // Insert the new user into the database
        if ($insert_stmt = $mysqli->prepare("INSERT INTO members (username, email, password, salt) VALUES (?, ?, ?, ?)")) {
            $insert_stmt->bind_param('ssss', $username, $email, $password, $random_salt);
            // Execute the prepared query.
            if (!$insert_stmt->execute()) {
                header('Location: ../error.php?err=Registration failure: INSERT');
                exit();
            }
        }
                
                // Success massege
                $success = "The account has been created successfully.";
                // Store success massege in SESSION:
                $_SESSION['success'] = $success;
                // Redirect page to same page
                $url = BASE_URL.BASE_URI.'index.php?p=admin-dashboard';        
                             // Define the URL.
                             header("Location: $url");
                             exit(); // Quit the script.
    }
}

?>

Any comments would be greatly appreciate.

 

Thank you.

Edited November 6, 2015 by thara

[Barand]

Use password_hash() and password_verify().

 

http://php.net/manual/en/function.password-hash.php

(You may find the notes on that page useful if you are not yet on PHP5.5+)

[Jacques1]

No, this is not secure, and appearently it's not even your own code. You seem to have randomly copied and pasted all kinds of nonsense you found somewhere on the Internet (without attribution).

 

Ironically, you've even copied comments like this:

// Did not work

So the original author clearly states that he has no idea what he's doing and needs help himself, yet you want to use his code in production?

 

I think this kind of Frankenstein code is pointless. You should make a clear decision: Do you want to write your own code, or do you want to use existing code? If you want to use code, it's important to follow some basic rules:

• Make sure the author actually knows what he's doing.
• The code needs to be organized in an actively maintained library so that you get bugfixes and updates.
• Respect the author. I understand that copy-and-paste is extremely common in the PHP universe, but not everybody is OK with that. Some authors want to be attributed, others use a specific license. Make sure you don't violate any “terms of use”.

If you want to write your own code, forget about copy-and-paste. Learn the basics of security, learn how to hash passwords, and then actually write code. This won't be an easy task, so this may take weeks or even months.

[thara]

@Jacques1, Thank for your post. In the above code most of thing I have coded except the password hashing part. Yes its found me somewhere on internet and he has used a different method to hash passwords other than using "hash_password()".

 

As you have mentioned that code is not secure, Then I used hash_password() and recreated the registration script:

 

This is how looks it now:

if ($_SERVER['REQUEST_METHOD'] == "POST") {
    
    //echo '<pre>', print_r($_POST, true).'</pre>';
    
    // Sanitize the data passed in
    $name = filter_input(INPUT_POST, 'name', FILTER_SANITIZE_STRING);
    $username = filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING);
    $email = filter_input(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL);
    $email = filter_var($email, FILTER_VALIDATE_EMAIL);
    $password = filter_input(INPUT_POST, 'password', FILTER_SANITIZE_STRING);
    
    // check username and email already exist:
    $prep_stmt = "SELECT user_id FROM users WHERE email = ? AND username = ? LIMIT 1";
    $stmt = $mysqli->prepare($prep_stmt);
    
    if ($stmt) {
            $stmt->bind_param('ss', $email, $username);
            $stmt->execute();
            $stmt->store_result();
            
            if ($stmt->num_rows == 1) {
                // A user with this email address already exists
                $error[] = 'A user with this email address or username already exists.';
            }
    } else {
            $error[] = 'Database error';
    }
    
    if (empty($error)) {   
        // Create a hashed password
        $options = [
                'cost' => 12,
        ];
        
        $hash_password = password_hash($password, PASSWORD_BCRYPT, $options);
        //echo $hash_password;
        
        // Insert the new user into the database
        $query = "INSERT INTO users ( name
                                                                , username
                                                                , email
                                                                , password
                                                                ) VALUES (?, ?, ?, ?)";
        $insert_stmt = $mysqli->prepare($query);
        
        if ($insert_stmt){
            // Bind variable for placeholder:
            $insert_stmt->bind_param('ssss', $name, $username, $email, $hash_password);
            // Execute the prepared query.
            $insert_stmt->execute();
                        
            if ($insert_stmt->affected_rows == 1) {            
                // Success massege
                $success = "The account has been created successfully.";
                // Store success massege in SESSION:
                $_SESSION['success'] = $success;
                // Redirect page to same page
                $url = 'register.php';        
              // Define the URL.
                header("Location: $url");
                exit(); // Quit the script.                 
                        
                        
            } else {
                // if registration fail:
                header("Location: error.php?err=Registration failure: INSERT");
                exit();
            }            
        } else {
            echo $mysqli->error;
        }
  }        
}

Now Can you kindly tell me, is there any security issues in my updated code?

 

Thanks in Advance.

[maxxd]

You're creating a race condition by checking the supplied username and email before inserting the data. Just make sure your database has a unique constraint that covers the username and email columns, and try to insert the record. If the insert fails because of the constraint, you know that username/email combo already exists.

 

And obviously you're not going to want to display the mysqli error directly to the user if the prepare() fails for whatever reason.

[Jacques1]

Look, I completely understand that you want to get this stuff done as quickly as possible, but critical code simply cannot be written in the usual trial-and-error fashion. You need to actually understand what you're doing.

 

At least half of the code is downright harmful and will, in the worst case, leave you with empty passwords and duplicate usernames. That's pretty serious. Sure, you can keep adding bugfixes, but in the end, it's still just trial-and-error.

 

So again: If you want to write this yourself, you need time, and you have to do a lot of research before you write a single line of code. Do you have that time and patience? If not, forget about it. You're better off using a premade solution.

[thara]

 

At least half of the code is downright harmful and will, in the worst case, leave you with empty passwords and duplicate usernames.

 

@Jacques1, can you kindly elaborate it for better understand?

 

Thank you.

[Jacques1]

Yes, but please note that the two defects are just examples. I'm not saying that your code will be OK if only you add two new bugfixes. There may be plenty of other problems which we simply haven't seen yet.

 

With that being said:

 

When you use filter_input() with the FILTER_SANITZE_STRING flag, you damage the user input. The function will scan the string for anything that looks like HTML and remove it. This is nonsensical (the password has nothing to do with HTML), ill-conceived (by the PHP core developers) and downright harmful, because removing characters from the password will weaken it. In the worst case, you end up with no password at all:

<?php

// before: an excellent password
$password = '<"(]AxQZ/~Sc@4ImnaXs';

// mangle it
$password =  filter_var($password, FILTER_SANITIZE_STRING);

// after: an *empty* password
var_dump($password);

It's generally a very bad idea to silently change user input. You can validate and reject the input, but never turn it into something different.

 

Using SELECT to check if the username already exists doesn't work either, because it creates a time-of-check-to-time-of-use defect: If two clients simultaneously request the same unused name, they're both allowed to have it, because at this time, the name indeed doesn't exist in your database. However, the actual insertion happens later, and at that time, the condition is no longer true, because one of the users will already have inserted the name when the second user executes the INSERT query. In the worst case (there's no UNIQUE constraint attached to the username column), you now have a duplicate name -- despite your check.

 

The only way to prevent this situation from happening is to encapsulate the check and the insertion in a single atomic operation. This must be done by the database system itself: Set up a UNIQUE constraint on the username column, run your INSERT queries with no prior checks and then catch all constraint violations. What's nice about this is that it also makes the code much shorter and a bit more efficient, because you only need a single query. See this example implementation.

 

Like I said: Implementing an authentication system is very difficult, because defects like this cannot be found with standard tests. There's no PHP error message to tell you that your username check will fail in certain scenarios. You have to know it in advance.

[thara]

@Jacques1, Thank you for your previous details comment. It greatly helped me. Still I am learning and doing things to make this registration system more secure.

 

Again I updated my script according to your previous post and this is what I do so far:

// MySQL error codes
define("MYSQL_ER_DUP_ENTRY", 23000);

if ($_SERVER['REQUEST_METHOD'] == "POST") {
    
    //echo '<pre>', print_r($_POST, true).'</pre>';
    
    // Sanitize and validate the data passed in
    
    // Check for the full name:
    if (!empty($_POST['name'])) {    
        $name  = filter_input(INPUT_POST, 'name', FILTER_SANITIZE_STRING);
        if (!preg_match ('/^[A-Z \'.-]{2,80}$/i', $name)) {
            // Not a valid name
            $error[] = 'First Name does not match the required format.';
        }
    }    
    
    // Check for the username:
    if (!empty($_POST['username'])) {    
        $username  = filter_input(INPUT_POST, 'username', FILTER_SANITIZE_STRING);
        if (!preg_match ('/^[a-z0-9_-]{3,15}$/', $username)) {
            // Not a valid username
            $error[] = 'Username does not match the required format.';
        }
    }
    
    // Check for the email:
    $email = filter_input(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL);
    $email = filter_var($email, FILTER_VALIDATE_EMAIL);
    if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
            // Not a valid email
            $error[] = 'The email address you entered is not valid';
    }
    
    // Check for a password and match against the confirmed password:
    if ( !empty( $_POST['password'])) {    
        if (preg_match ('/^(\w*(?=\w*\d)(?=\w*[a-z])(?=\w*[A-Z])\w*){6,20}$/', $_POST['password']) ) {
                                
            if ($_POST['password'] == $_POST['password_conform']) {
                $password = $_POST['password'];
            } else {
                $error[] = 'Your password did not match the confirmed password!';
            }
        } else {
            $error[] = 'You are NOT entered a valid password! (use atleast 1 number & 1 capital letter)';
        }    
    } else {
        $error[] = 'Your password field can not be empty!';
    }
        
    
    if (empty($error)) {   
        // Create a hashed password
        $options = [
                'cost' => 12,
        ];
        
        $hash_password = password_hash($password, PASSWORD_BCRYPT, $options);
        //echo $hash_password;
        
        // Insert the new user into the database
        $query = "INSERT INTO users ( name
                                    , username
                                    , email
                                    , password
                                    ) VALUES (?, ?, ?, ?)";
        $insert_stmt = $mysqli->prepare($query);
        
        if ($insert_stmt){
            // Bind variable for placeholder:
            $insert_stmt->bind_param('ssss', $name, $username, $email, $hash_password);
            // Execute the prepared query.
            $insert_stmt->execute();
                        
            if ($insert_stmt->affected_rows == 1) {            
                // Success massege
                $success = "The account has been created successfully.";
                // Store success massege in SESSION:
                $_SESSION['success'] = $success;
                // Redirect page to same page
                $url = 'register.php';        
              // Define the URL.
                header("Location: $url");
                exit(); // Quit the script.                 
                        
            } else {
                // if registration fail:
                if (MYSQL_ER_DUP_ENTRY == $mysqli->sqlstate) {
                    $error[] = 'You or somebody else has tried to register at our website with this username and e-mail address, but these details already exists in our database.';
                } else {
                    header("Location: error.php?err=Registration failure: INSERT");
                    exit();
                }
            }            
        } else {
            echo $mysqli->error;
        }
  }        
}

I am greatly appreciating yours valuable suggestion and comments.

 

Thank you.

Edited November 8, 2015 by thara

[Jacques1]

thara, you're missing the point.

 

I explicitly told you that fixing the two example defects does not fix your code. I even put a “disclaimer” before my reply, but you just ignore it and do the exact opposite. As I already said multiple times, critical code cannot be written in a trial-and-error fashion and requires a period of learning and preparation. But you, again, ignore all advice and continue your trial-and-error loop.

 

An authentication system is not a friggin' HTML layout where you can just keep moving elements around and pressing F5 until things look OK.

 

Sorry, I can't help you. Maybe somebody else is willing to assemble the 10,000th half-assed PHP log-in code. I'm not.