How secure is my PHP registration script?

Question

I coded a registration form and was wondering if anyone could give me feedback on whether I'm lacking any major security mechanisms. I've done loads of reading around the topic and feel it should be secure but expert feedback would be great.

Registration script

<?php

// User/pass only used locally
$host = 'localhost';
$db   = 'new_db';
$user = 'root';
$pass = '';

$options = [
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
    PDO::ATTR_EMULATE_PREPARES   => false,
];
$dsn = "mysql:host=$host;dbname=$db";
try {
     $pdo = new PDO($dsn, $user, $pass, $options);
} catch (\PDOException $e) {
     throw new \PDOException($e->getMessage(), (int)$e->getCode());
}


$errors = [];
if ( $_POST )
{

    // Get form field values
    $fname              = filter_input(INPUT_POST, 'fname', FILTER_SANITIZE_STRING);
    $email              = filter_input(INPUT_POST, 'email', FILTER_SANITIZE_EMAIL);
    $pwd                = filter_input(INPUT_POST, 'password', FILTER_SANITIZE_STRING);
    $pwd_confirm        = filter_input(INPUT_POST, 'confirm-password', FILTER_SANITIZE_STRING);

    // Generate activation key
    $activation_key     = md5(uniqid(mt_rand(), true));
    $activation_link    = 'https://www.example.com/activate?id='.$activation_key.'&name='.$fname;

    // Check if passwords match
    if ($pwd !== $pwd_confirm) {
        $errors[] = "Passwords don't match";
    }

    // Check if password is secure
    if (strlen($pwd) < 8) {
        $errors[] = "Password not long enough! Must be at least 8 characters long";
    }

    // Check if username equals password
    if ($fname === $pwd) {
        $errors[] = "Your name cannot be your password!";
    }

    // Check if email address exists in database
    $email_query = $pdo->prepare("SELECT * FROM users WHERE email = :email");
    $email_query->bindParam(':email', $email);
    $email_query->execute();
    $email_found = $email_query->fetchColumn();
    if ($email_found) {
        $errors[] = "Your email address is associated with another account.";
    }


    // If no errors, continue with user account creation
    if (!$errors)
    {   
        // Hash password
        $hashed_password = password_hash($pwd, PASSWORD_DEFAULT);

        // Create database entry
        $create_account = $pdo->prepare("INSERT INTO users (first_name,email,password, activation_key) VALUES (:fname, :email, :password, :activation_key)");
        $create_account->bindParam(':fname', $fname);
        $create_account->bindParam(':email', $email);
        $create_account->bindParam(':password', $hashed_password);
        $create_account->bindParam(':activation_key', $activation_key);
        $create_account->execute();

        // Send out activation email
        $to=$email;
        $subject="Activate your account";
        $from = 'no-reply@example.com';
        $body='Thank you for creating your account, '.$fname.'. Please click on the following link to activate your account: <a href="'.$activation_link.'">'.$activation_link.'</a>';
        $headers = "From:".$from;
        mail($to,$subject,$body,$headers);

        // Redirect user to the dashboard
        header("Location: /dashboard.php");
        exit;
    }



}

?>

I do want to use it on a live site at one point, so the end result should not just be a registration form that's somewhat secure but could actually be used in production.

EDIT

I did some more thinking and I guess before redirecting the newly registered user, I have to start a session to detect on the dashboard page whether the user is logged in. Is the following sufficient and secure to to add after I sent out the activation email?

session_start();

$_SESSION["loggedin"] = true;
$_SESSION["email"] = $email; 

// Redirect user to the dashboard
header("Location: /dashboard.php");
exit;

Answer 1

You properly protect against SQL injection, that's good.

I worry about the filtering on the password. If I choose <script>' OR 1=1 ">>> as my password, the filtering should not reduce this to just ' OR 1=1 ", just because some part of it looks like HTML tags. A password is not supposed to be displayed anywhere, it is not supposed to be stored in plain text anywhere, the only valid use is to feed it to password_validate and password_hash, and these can cope with arbitrary input (well, except that they stop processing at the first byte that is 0, that is, the byte value 0, not the digit '0'). When I enter a password, I expect this password to be used exactly, unmodified, unnormalized, just as I entered it.

I dimly remember that uniqid is not very unique at all. You should double-check the documentation whether that function provides enough randomness to protect against guessing attacks.

Your code is sending out a mail containing <a href="...">, but nowhere in the code do I see a Content-type: text/html; charset=UTF-8 header.

Your code is sending out malformed HTML. The URL contains a plain &, which in HTML must be escaped as &. To do this, call htmlspecialchars($url). Most browsers accept this malformed HTML, but it's better do know how to correctly convert between plain text and HTML. Just to avoid cross-site scripting from the beginning.

What if the $fname contains an ampersand? Some companies have that in their name. I don't know of actual people with these names, but who knows? An ampersand would make the generated $activation_link invalid.

Answer 2

• It will be better practice to url encoding the query string of your $activation_link in case the $fname is not url-compliant. I recommend http_build_query() -- here's a post about the delimiting ampersands. I will also recommend that you not generate this string until you determine that it is actually needed down script.

• It is good that you are properly implementing prepared statements, but your unique email query is fetching more data than you intend to use. Instead of SELECT * use COUNT(1) or COUNT(*), and fetch that one particular column of the single-row result set.

  if ($email_query->fetchColumn()) {
      $errors[] = "Your email address is associated with another account.";
  }
  

• There is no benefit to re-declaring the email value to the new variable $to, just keep using $email when passing arguments to your mail() function.

• I generally advise against declaring "single-use variables". It can be a good idea when the variable name helps to describe the data, but then if your script needs this kind of meta-detail, then perhaps just use a comment. In my own project, I'd probably not declare the single-use variables like $hashed_password, $to, $from, $subject, and $body.

• You are not checking for a true response from mail(), you may like to check this instead of assuming. Then again, in all of my projects, I rely upon PHPMailer to do all my mailing functionality -- it is just a better / more robust class to work with. In the meantime, you might rewrite your mail() call like this:

  if (
      !mail(
          $email,
          "Activate your account",
          sprintf(
              'Thank you for creating your account, %1$s.'
              . ' Please click on the following link to activate your account: <a href="%2$s">%2$s</a>',
              $fname,
              'https://www.example.com/activate?' . http_build_query(['name' => $fname, 'id' => $activation_key], '', '&amp;')
          ),
          "From:no-reply@example.com"
      )
  ) {
      $errors[] = "Failed to mail activation email";
  } else {
      session_start();  // I advise that this line be unconditionally written at the start of page -- ideally in a config file which is called first by every page
  
      $_SESSION["loggedin"] = true;  // I don't think I'd bother with this element
      $_SESSION["email"] = $email; 
      header("Location: /dashboard.php");
      exit;
  }
  

  If you find this to be "uglier", then there won't be any harm in declaring the extra variables.

Answer 3

A small addition to what others have said...

password_hash() when used with PASSWORD_DEFAULT will change the algorithm in future versions of php, which means that it might stop passing in one update.

I would use PASSWORD_BCRYPT and set an appropriate number of rounds, when increasing the rounds you can update passwords on the fly after they log in.