Secured registration script

Question

I created code for my registration form on my social network. I want to know if it is secured enough to release on the web as a public social network. Also, all of the code works. I just want to make sure it's all secure.

registration.php:

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);

//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register'])) 
{
// looks like a hack, send to index.php
header('Location: index.php');
die();
}

include("connect.php");

            if(isset($_POST['Register'])) {

               if (empty($_POST["username"])) {
                echo"Fill in username to sign up";
                } else {

                if (empty($_POST["pw"])) {
                 echo"Fill in password to sign up";
                } else {

                if (empty($_POST["pw2"])) {
                echo"Confirm password to sign up";
                 } else {

                if (empty($_POST["email"])) {
                     echo"Fill in email to sign up";
                 } else {

                if ($_POST['pw'] == $_POST['pw2']) {
                 $username = mysqli_real_escape_string($conn, $_POST["username"]);
                 $pw= mysqli_real_escape_string($conn, $_POST["pw"]);
                 $pw2= mysqli_real_escape_string($conn, $_POST["pw2"]);
                 $email = mysqli_real_escape_string($conn, $_POST["email"]);

                 $result = mysqli_query($conn ,"SELECT * FROM users WHERE username='" . $username . "'");

                    if(mysqli_num_rows($result) > 0)
                    {
                    echo "Username exists . <a href= index.php>Try again</a><br /> ";
                    } else {

                       $result2 = mysqli_query($conn ,"SELECT * FROM users WHERE email='" . $email. "'");

                       if(mysqli_num_rows($result2) > 0)
                       {
                       echo "Email exist.  <a href= index.php>Try again</a><br /> ";
                       } else {

                       $pw = password_hash($pw, PASSWORD_BCRYPT, array('cost' => 14));          

                       $stmt = $conn->prepare("INSERT INTO users (username, pw, email) VALUES(?, ?, ?)");
                       $stmt->bind_param("sss", $username, $pw, $email);

                      $username = $username;
                      $pw = $pw;
                      $email = $email;
                      $stmt->execute();

                      echo"Registration successful <a href= index.php>Login here</a><br />";


                    } } } else{
            echo "The passwords do not match.";  // and send them back to registration page
            }
}
}}}}

login.php:

error_reporting(E_ALL);
ini_set('display_errors', 1);

include("connect.php");

$_SESSION['username'] = $_POST['username'];

//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['signin'])) 
{
// looks like a hack, send to index.php
header('Location: index.php');
die();
}

if (empty($_POST["username"])) {
echo 'Fill in username to sign in. <a href= index.php>Try again</a><br />';
die();
}

if (empty($_POST["pw"])) {
echo 'Fill in password to sign in. <a href= index.php>Try again</a><br />';
die();
}

$sql = "SELECT pw FROM users WHERE username = ?";
$stmt = mysqli_prepare($conn, $sql);
if ( !$stmt ) {
echo mysqli_error($conn);
die();
}

$stmt->bind_param('s', $_POST['username']);
if ( !$stmt->execute() ) {
echo mysqli_error($conn);
die();
}
// we found a row with that username, 
// now we need to check the password is correct

// get the password from the row
$stmt->bind_result($hashed_pwd);
$stmt->fetch();

if ( password_verify($_POST['pw'], $hashed_pwd) ) {
// password verified
$_SESSION["username"] = $_POST['username'];
header('Location: profile.php');
} else {
echo 'Incorrect username or Password. <a href= index.php>Try again</a><br />';
}

Answer 1

Correctness

Also, all of the code works.

No, it doesn't, so let's start with that.

                 $username = mysqli_real_escape_string($conn, $_POST["username"]);

...

                 $result = mysqli_query($conn ,"SELECT * FROM users WHERE username='" . $username . "'");

                    if(mysqli_num_rows($result) > 0)
                    {
                    echo "Username exists . <a href= index.php>Try again</a><br /> ";
                    } else {

...

                       $stmt = $conn->prepare("INSERT INTO users (username, pw, email) VALUES(?, ?, ?)");
                       $stmt->bind_param("sss", $username, $pw, $email);

                      $username = $username;
                      $pw = $pw;
                      $email = $email;
                      $stmt->execute();

If the supplied username is such that mysqli_real_escape_string is not a no-op, the check for an existing user and the insertion of the user use different usernames. This is an important bug. Quite how important depends on things you haven't shown us, in particular on the database schema.

Given the current mix of database access techniques, you should ditch the one which requires you to escape things yourself (dangerous, because it's easy to forget when you change the code) and use prepared statements for everything.

---

I also struggle to see how this can possibly work:

//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register'])) 
{
// looks like a hack, send to index.php
header('Location: index.php');
die();
}

...

                    } } } else{
            echo "The passwords do not match.";  // and send them back to registration page

If registration.php is the registration page then (a) it seems rather lacking in content; and (b) there seems to be a catch-22 with rendering it for the first time in order to submit data.

---

Maintainability

Maintainability is important in its own right, but also particularly relevant to security because unmaintainable code is more likely to acquire bugs over time.

Fix the indentation. It currently appears to be at random, and is completely unreadable.

---

Avoid massive chains of if statements. The various checks here would be better refactored into a function which can take advantage of early return.

---

Avoid pointless code such as

                      $username = $username;
                      $pw = $pw;
                      $email = $email;

---

Security

This code leaks far too much information to a malicious caller.

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);

is absolutely not something you want to do on a production system unless you're also using set_error_handler to log it to somewhere private and ensure that the end user never sees the error messages.

Similarly

echo mysqli_error($conn);

---

The registration form asks for an e-mail address. Why? If you don't use it, don't ask for it. If you do use it, does your security model not require it to be correct? So shouldn't there be an e-mail validation step in the registration process?

Answer 2

Your code is secure but hard to read, unclear and inconsistent. Poor Scarlett O'Hara will be saved in the database as Scarlett O\'Hara which won't make her any happier. There are also minor security issues as well.

Hard to read

You must always indent your code, indenting subordinate code lines, but your login.php lacks indentation at all.

At the same time, you shouldn't abuse indentation as well,

• by making random indents (like you did after include("connect.php");)
• by adding unnecessary conditions (like if(isset($_POST['Register'])))
• by introducing unnecessary indentation levels with all these nested conditions.

Instead of making a lot of nested conditions, run them in order, and collect errors in the array, i.e

$errors = [];
if (empty($_POST["username"])) {
    $errors[] = "Fill in username to sign up";
}
...
if ($_POST['pw'] !== $_POST['pw2']) {
    $errors[] = "The passwords do not match";
}
...

and finally test the $errors variable before insert, running a query it it's empty or echoing all errors in a loop otherwise.

Inconsistency

You are using two contradicting approaches in your code, adding strings directly to SQL and adding them through prepared statements. You must stick with the latter. Which makes all mysqli_real_escape_string stuff obsolete and harmful. Convert all your old style queries to prepared statements and get rid of mysqli_real_escape_string calls.

Also, a code like this

                  $username = $username;
                  $pw = $pw;
                  $email = $email;

makes no sense. you can remove it as well.

Security

echo mysqli_error($conn);

is the worst way to report errors ever. On a live site it will reveal some important details to a potential hacker. I've got a dedicated article that explains PHP Error reporting that you are welcome to read. In short, just add the following line before mysqli_connect

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

and forget about testing the individual queries' results.

Refactored registration

In the end your registration code could be like this

$errors = [];
if (empty($_POST["username"])) {
    $errors[] = "Fill in username to sign up";
}
// and so on...
if ($_POST['pw'] !== $_POST['pw2']) {
    $errors[] = "The passwords do not match.";
}
if (!empty($_POST["username"]) || !empty($_POST["pw"])) {
    $stmt = $conn->prepare("SELECT * FROM users WHERE username=? or email=?");
    $stmt->bind_param("ss", $_POST['username'], $_POST['email']);
    $stmt->execute();
    $row = $stmt->get_result()->fetch_assoc();

    if ($row && $row['username'] == $_POST['username']) {
        $errors[] = "Username exists";
    }
    if ($row && $row['email'] == $_POST['email']) {
        $errors[] = "Email exists";
    }
}
if (!$errors) {
    $pw = password_hash($_POST['pw'], PASSWORD_BCRYPT, array('cost' => 14));

    $stmt = $conn->prepare("INSERT INTO users (username, pw, email) VALUES(?, ?, ?)");
    $stmt->bind_param("sss", $_POST['username'], $pw, $_POST['email']);
    $stmt->execute();

    echo "Registration successful <a href= index.php>Login here</a><br />";
} else {
    foreach ($errors as $error) {
        echo "$error <br /> \n";
    }
    echo '<a href="index.php">Try again</a><br />';
}

Usability

You should think of the user's convenience. It's boresome to enter all values again in case of error. Consider posting on the same page and showing already entered information in the fields in case of error.

Answer 3

I fully agree with the existing answers, and only have two small additions.

This looks very odd and possible insecure:

// login.php:
$_SESSION['username'] = $_POST['username'];

[...]

if ( password_verify($_POST['pw'], $hashed_pwd) ) {
// password verified
$_SESSION["username"] = $_POST['username'];

You are assigning the username to the session if the user is authenticated and if they are not authenticated.

My guess is that profile.php has a check such as if (isset($_SESSION['username']) { $user = $_SESSION['username']; handleRequestForUser($user); } else { header('Location: login.php'); }?

Have you tried logging in with a correct username and incorrect password, followed by simply visiting profile.php? My guess is that you might then be logged in.

Even if not though, the assignment seems inconsistent, insecure, and just begging for future bugs.

The other issue I have is that you use prepared statements (good!), but not always. Why fall back on the less secure mysqli_real_escape_string? Just use prepared statements everywhere.