Registration code [closed]

Question

I was following a tutorial for a social network. I want to know how secure this code is because I plan on launching it to the public web.

<?php

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

// declaring vars to prevent errors

$fname = ""; //First name
$lname = ""; //Last name
$em = ""; //Email
$em2 = ""; // Email 2
$password = ""; //password
$password2 = ""; //password 2
$date = ""; // sign up date
$error_array = array(); //holds error messages

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

// registration form values

// first name
$uname = strip_tags($_POST['reg_uname']); // remove html tags
$uname = str_replace(' ', '', $uname); //remove spaces
$_SESSION['reg_uname'] = $uname; // stores first name in session variable

// first name
$fname = strip_tags($_POST['reg_fname']); // remove html tags
$fname = str_replace(' ', '', $fname); //remove spaces
$fname = ucfirst(strtolower($fname)); //uppercase first letter
$_SESSION['reg_fname'] = $fname; // stores first name in session variable

// Last name
$lname = strip_tags($_POST['reg_lname']); // remove html tags
$lname = str_replace(' ', '', $lname); //remove spaces
$lname = ucfirst(strtolower($lname)); //uppercase first letter
$_SESSION['reg_lname'] = $lname; // stores last name in session variable

// E-mail
$em = strip_tags($_POST['reg_email']); // remove html tags
$em = str_replace(' ', '', $em); //remove spaces
$em = ucfirst(strtolower($em)); //uppercase first letter
$_SESSION['reg_email'] = $em; // stores email in session variable

// E-mail 2
$em2 = strip_tags($_POST['reg_email2']); // remove html tags
$em2 = str_replace(' ', '', $em2); //remove spaces
$em2 = ucfirst(strtolower($em2)); //uppercase first letter
$_SESSION['reg_email2'] = $em2; // stores email2 in session variable

// Password
$password = strip_tags($_POST['reg_password']); // remove html tags
$password2 = strip_tags($_POST['reg_password2']); // remove html tags

$date = date("Y-m-d"); //current date

if($em == $em2) {

    //check if email is in valid format
    if(filter_var($em, FILTER_VALIDATE_EMAIL)){

        $em = filter_var($em, FILTER_VALIDATE_EMAIL);

        // check if email already exists
        $e_check = mysqli_query($con, "SELECT email FROM users WHERE email='$em'");

        // Count the number of rows returned
        $num_rows = mysqli_num_rows($e_check);

        if($num_rows > 0){
            array_push($error_array, "E-mail already in use<br>");
        }

    } else{

        array_push($error_array, "Invalid email format<br>");
    }

} else {
    array_push($error_array, "E-mails don't match<br>");
}

if(strlen($uname) > 25 || strlen($uname) < 1) {

    array_push($error_array, "Your username must be between 2 and 25 characters<br>");
}

if(strlen($fname) > 25 || strlen($fname) < 1) {

    array_push($error_array, "Your first name must be between 2 and 25 characters<br>");
}

if(strlen($lname) > 25 || strlen($lname) < 1) {

    array_push($error_array, "Your last name must be between 2 and 25 characters<br>");
}

if($password != $password2){

    array_push($error_array, "Your passwords do not match<br>");

} else {

    if(preg_match('/[^A-Za-z0-9]/', $password)) {

    array_push($error_array, "Your password can only contain english characters or numbers.<br>");

    }

}    

if(strlen($password > 30 || strlen($password) < 5)){

    array_push($error_array, "Your password must be between 5 and 30 characters<br>");
}

if(empty($error_array)) {

    // $password = password_hash($password); // encrypt password
    $password = password_hash($_POST['reg_password'], PASSWORD_BCRYPT, array('cost' => 14));

    // profile picture assignment
    $profile_pic = "assets/images/profile_pics/defaults/default.jpg";

    $query = mysqli_query($con, "INSERT INTO users VALUES(NULL, '$fname', '$lname', '$uname', '$em', 
    '$password', '$date', '$profile_pic', '0', '0', 'no', ',')");

    array_push($error_array, "<span style='color: #669999' >You can now login</span><br>");

    //clear session variables

    $_SESSION['reg_fname'] = "";
    $_SESSION['reg_uname'] = "";
    $_SESSION['reg_lname'] = "";
    $_SESSION['reg_email'] = "";
    $_SESSION['reg_email2'] = "";

}
}

?>

Answer 1

The code is horrible.

It probably contains all of the top 10 vulnerabilities listed on the OWASP web site.

It uses a programming language that is well-known for its bad security history and its awful API that makes it difficult to write secure, solid code.

Forget about that tutorial, warn others about it, find a better tutorial with focus on good code and security, and start over again. A good starting point is the OWASP site. If you really want to stay with PHP, at least use the latest version.

You also have an off-by-one error: the condition < 1 doesn't match the error message at least 2. This means you didn't even test your code properly. Especially testing the edge cases is important.

Answer 2

I am bit puzzled by strip_tags:

$em = strip_tags($_POST['reg_email']); // remove html tags
$em = str_replace(' ', '', $em); //remove spaces
$em = ucfirst(strtolower($em)); //uppercase first letter

I have the impression that you stumbled upon incredibly old tutorials. You don't use that function to check user names or E-mails, because you are not expecting that people will actually put HTML tags in those fields, and besides you should have a more comprehensive routine to check the values entered.

The purpose of strip_tags is to remove html from text messages (and thus filter out some spam, like clickable hyperlinks). Javascript too.

Since you are checking the E-mail address with filter_var you can rid of those lines, they are pointless.

Why capitalize the first letter of the E-mail ? Call the variable $email, it's more intuitive. em reminds me of the HTML tag.

To remove whitespace around the string you can just use the trim function.

This code introduces a potential vulnerability (SQL injection):

$e_check = mysqli_query($con, "SELECT email FROM users WHERE email='$em'");

I am wondering why you are not using parameterized queries (or prepared statements) systematically (at least whenever user input is involved). I am all the more surprised since your previous submission did not have that kind of vulnerability.

In this particular case, you should be spared the vulnerability because you are relying on filter_var above to ensure that the E-mail is valid but this your only line of defense. What if you forget to check the E-mail in another part of your code ? Also, filter_var may have shortcomings that we don't know about.

In general websites will only allow alphanumeric characters in usernames plus a few signs like space, hyphen, underscore etc. The most straightforward way is to use a regular expression.

There is no need to remove spaces everywhere:

$fname = str_replace(' ', '', $fname); //remove spaces

First names may very well contain spaces or be compound names, nothing wrong with that. Trim: yes. Remove all whitespace: no.

In this code, you are checking for minimum password length but not for complexity. 12345 does pass your test but you should not accept that kind of password. But you've done the exact opposite, users are not allowed to choose strong passwords...

if(preg_match('/[^A-Za-z0-9]/', $password)) {

array_push($error_array, "Your password can only contain english characters or numbers.<br>");

}

Devise a better password policy like: 12 characters minimum with at least one digit, one special character, or a long password like a passphrase that is easier to remember for humans. Each site make up their own rules, which are often counter-productive and nonsensical. The point is to find a balance between security and convenience. People tend to reuse the same passwords, and part of the reason is that they are forced to choose passwords that are not intuitive.

I don't know why you have $_SESSION variables, you are not doing anything with them. In fact they are useless, unless you want to initiate a user session (that is log the user in) right after registration. But you usually you will want to validate their E-mail address first (by sending a registration code).

I don't know large is the scope of your project but it may be a good idea to use a development framework because:

• it will bring you up to speed with the 21th century, and thus increase your value on the job market if you pursue a career as a developer...
• it will relieve you of that tedious form building and validation exercise - do not reinvent the wheel, use the modern tools that other coders use. If you want to reinvent the wheel do it right. But this will take time without guarantee of good results.

This kind of code is 15 years old. At least. And it's not good.

Unfortunately the Web is littered with outdated tutorials, many of which perpetuate bad practices and security vulnerabilities. In fact it seems that the best-ranking tutorials are the old stuff that nobody should read anymore. I am still looking for a decent tutorial as we speak.

Answer 3

Do not use this code and php5, if you want to use mysqli + "procedural style", use mysqli_real_escape_string. The best way out - use "prepared statement"

My example (This is INPUT)

  function insert_user($users) {
    global $db;

    $hashed_password = password_hash($users['password'], PASSWORD_BCRYPT);

    $sql = "INSERT INTO users ";
    $sql .= "(first_name, last_name, email, username, hashed_password) ";
    $sql .= "VALUES (";
    $sql .= "'" . mysqli_real_escape_string($db, $users['first_name']) . "',";
    $sql .= "'" . mysqli_real_escape_string($db, $users['last_name']) . "',";
    $sql .= "'" . mysqli_real_escape_string($db, $users['email']) . "',";
    $sql .= "'" . mysqli_real_escape_string($db, $users['username']) . "',";
    $sql .= "'" . mysqli_real_escape_string($db, $hashed_password) . "'";
    $sql .= ")";
    $result = mysqli_query($db, $sql);

    // For INSERT statements, $result is true/false
    if($result) {
      return true;
    } else {
      // INSERT failed
      echo mysqli_error($db);
      exit;
    }
  }

How to use this function?

$result = insert_user($users);

Use strip_tags for OUTPUT