8
\$\begingroup\$

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;
\$\endgroup\$
  • 3
    \$\begingroup\$ Anyone who down votes or uses VTC please leave a comment as to why you down voted. \$\endgroup\$ – pacmaninbw Aug 2 at 19:57
  • \$\begingroup\$ I don't really see anything unsecure to point out in your code. \$\endgroup\$ – LIL LOFIE Aug 2 at 20:46
  • \$\begingroup\$ $activation_key = md5(uniqid(mt_rand(), true)); - stop, just do bin2hex(random_bytes(15)); or something like that, it's much safer than uniqid() \$\endgroup\$ – hanshenrik Aug 3 at 9:47
9
\$\begingroup\$

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 &amp;. 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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I definitely agree with not sanitizing the password string. I don't know that uniqid()'s "uniqueness" is a concern worth stressing over. There are two factors in the activate requirements, so an attacker would need to know a new user's fname and blow through a lot of attempts to activate the account ...and to what end? They activate your account for you? I will hope/assume that the activation receiving script will force the user to login with their password, otherwise there could be a vulnerability there. \$\endgroup\$ – mickmackusa Aug 3 at 1:59
  • 1
    \$\begingroup\$ and these can cope with arbitrary input i wish that was true. but it's not true, see: 3v4l.org/SaRef - they will choke on any strings containing null bytes. they think that these 2 strings are equivalent: X\x00Roland and X\x00ASUFHUWEGFHSEJUHFW - do those look the same to you? no? well, they look the same to password_hash() and password_verify() <.< \$\endgroup\$ – hanshenrik Aug 3 at 10:00
  • \$\begingroup\$ @hanshenrik That may be a bug, as it assumes "X" as equal as well. But the function doesn't say that it is binary-safe. But on the other hand, strlen isn't binary-safe either but var_dump(strlen("X\x00ASUFHUWEGFHSEJUHFW")); returns 20. \$\endgroup\$ – Ismael Miguel Aug 3 at 10:21
  • 1
    \$\begingroup\$ @IsmaelMiguel actually PHP's strlen() is binary-safe (horrifyingly, it's possible to configure strlen to be non-binary-safe, but that's deprecated as of 7.2: php.net/manual/en/mbstring.overload.php ) \$\endgroup\$ – hanshenrik Aug 3 at 18:16
  • 1
    \$\begingroup\$ @IsmaelMiguel oh like that, yeah you're right, it's a shame the strlen() docs doesn't mention the binary safety properties of strlen() \$\endgroup\$ – hanshenrik Aug 3 at 23:24
5
\$\begingroup\$
  • 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.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I'm torn between downvoting and not downvoting this answer. Reading the documentation, you can see that the intended use is to use PASSWORD_DEFAULT for easiness, and then more advanced algorithms if supported. Then, you use password_needs_rehash() to see if you need to update the hash on login. Also, PASSWORD_BCRYPT cuts the password to 72 characters. That's TERRIBLE! Also, "[...] which means that it will stop passing in one update." is just wrong as the default will change, the hash verifies anyway and then updated. ... \$\endgroup\$ – Ismael Miguel Aug 3 at 9:46
  • \$\begingroup\$ ... However, increasing the rounds is a good mitigation and a good way to slowdown brute-force. But other methods could be added, for login, but it's outside the scope of the review. But then, you say you would store those rounds with the password. Which makes me believe that you have a wrong understanding of how these functions work. All the information (algorithm, rounds, salt and hash) is stored in the result of password_hash(). All you ned to do is to check the result with password_verify(). ... \$\endgroup\$ – Ismael Miguel Aug 3 at 10:00
  • \$\begingroup\$ ... You should check how these work on here: sandbox.onlinephpfunctions.com/code/…. You can try to change the algorithm, the password, the options. Just see what happens. The password hash was taken from the 1st example. You can see that you don't need to store anything else to see if the password matches or not. Just set whatever you want and it works fine. \$\endgroup\$ – Ismael Miguel Aug 3 at 10:03
  • \$\begingroup\$ (In short, the algo is stored in the hash. It won't stop passing if the default algo changes.) \$\endgroup\$ – Progrock Aug 19 at 14:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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