Authentication system (no browser) with mail verification

Question

I made an authentication system for my software (no browser is used) that works like this:

• User receives a Key on it's mail after buying the software
• User registers account using that key and inputs username, mail, password.
• registration.php checks if the format is correct then sends an email with a confirmation code using a template and inserts the values into DB (verify table).
• The HTML template contains username (from user input), 2 images from the same directory as the registration.php and the template2.html and a button with a get like this: www.mysite.com/activate.php?id=$serial&conf=$confirmation_code.
• activate.php checks if everything is valid and then inserts the verify table's row into accounts table. (after it is inserted into accounts table, it becomes a valid account).

Everything works as expected (I'm still making the login.php).

Why I'm posting this here:

As a beginner (this is the first php, mysql and html thing I do) I know there is a lot of things that can be improved. My goal is to make it as secure as possible, I followed OWASP as much as I could and at the same time I read a lot on Stack Overflow and Security.Stackexchange.

Here is my code:

registration.php

<?php
$CurrentVersionHash="asd123";
$CurrentVersion="alpha1";
$CurrentHash="asd123";
$serial_length="10";
$memory_cost="31250";
$time_cost="10";
$threads="1";
require 'mail.php';

if ( ! empty( $_POST ) ) // Check if Post is not empty
{
    $hash = $_POST['exe'];
    $pw = $_POST['pass'];
    $user = $_POST['name'];
    $version = $_POST['ver'];
    $email = $_POST['mail'];
    $confirm = $_POST['pass2'];
    $serial = $_POST['key'];
    if ( empty($hash) or empty($version))
        return;
    if ( $hash<>$CurrentHash) // Check if file Hash is valid
    {
        if ( $version<>$CurrentVersionHash ) // Check if it's because it's outdated
        {
            echo "Old exe";
        }
        else
        {
            echo "Exe not valid";
        }
        return;
    }
    // Checking if there is any problem in the format
    if ( (empty( $user )) or ( strlen( $user ) < 6 ) or ( strlen( $user ) > 254 ) or ( ! ctype_alnum ( $user )) )
    {
        echo "Error"; 
        return;
    }
    if ( (empty( $pw)) or (strlen($pw) < 10) or (strlen($pw) > 254) or (! preg_match("#[0-9]+#", $pw)) or ( !preg_match("#[a-z]+#", $pw )) or ( !preg_match("#[A-Z]+#", $pw )) )
    { 
        echo "Error"; 
        return;
    }
    if ( $pw != $confirm  or empty( $confirm ) )
    { 
        echo "Error"; 
        return;
    }
    if (( empty( $email )) or ( ! filter_var($email, FILTER_VALIDATE_EMAIL)) or (strlen($email) > 254) )
    { 
        echo "Error"; 
        return;
    }
    if ( ( empty( $serial ) ) or ( strlen( $serial ) <> $serial_length ) )
    { 
        echo "Error"; 
        return;
    }
    // Check if Serial exists and is not already taken
    $con = new mysqli($host, $username, $password, $database);
    $stmt = $con->prepare("SELECT * FROM acc WHERE serial=?");
    $stmt->bind_param('s', $serial);
    $stmt->execute();
    $result = $stmt->get_result();
    $num_of_rows = $result->num_rows;
    $result = $result->fetch_array();
    // If Serial doesn't exist or is already taken
    if ( ($num_of_rows==0) or ( ! empty( $result['username'] )) ) 
    {
        echo "Invalid Serial";
        $error=1;
    }
    $stmt = $con->prepare("SELECT * FROM verify WHERE serial=?");
    $stmt->bind_param('s', $serial);
    $stmt->execute();
    $result = $stmt->get_result();
    $num_of_rows = $result->num_rows;
    $result = $result->fetch_array();
    // If Serial is on verify DB (means that someone already registered using it)
    if ( ($num_of_rows<>0) and ($error<>1) )
    {
        echo "Invalid Serial";
        $error=1;
    }
    if ($error<>1)
    {
        // Check if Username or E-mail is not already taken
        $stmt = $con->prepare("SELECT * FROM acc WHERE username= ? OR email= ? LIMIT 1");
        $stmt->bind_param('ss', $user, $email);
        $stmt->execute();
        $result = $stmt->get_result();
        $num_of_rows = $result->num_rows;
        $result = $result->fetch_array();
        $stmt = $con->prepare("SELECT * FROM verify WHERE username= ? OR email= ? LIMIT 1");
        $stmt->bind_param('ss', $user, $email);
        $stmt->execute();
        $result2 = $stmt->get_result();
        $num_of_rows2 = $result2->num_rows;
        $result2 = $result2->fetch_array();
        if ( ($num_of_rows) or ($num_of_rows2) ) // If user or email already exists on verify and acc DB
        {
            if ( (strcasecmp($result['username'], $user) == 0) or (strcasecmp($result2['username'], $user) == 0) ) // If user already exists
            {
                echo "Username in use";
            }
            else if ( (strcasecmp($result['email'], $email) == 0) or (strcasecmp($result2['email'], $email) == 0) )// If e-mail is already taken
            {
                echo "Email in use";
            }
        }
        else
        {
            // Do registration
            $password = password_hash( $pw, PASSWORD_ARGON2ID, [
                    'memory_cost' => $memory_cost,
                    'time_cost'   => $time_cost,
                    'threads'     => $threads,
            ]);
            $rand_id = random_int(-10000, 10000);
            $rand_secret = random_str(32);
            $stmt = $con->prepare("INSERT INTO verify (username, password, email, serial, rand_id, rand_secret) VALUES (?,?,?,?,?,?)");
            $stmt->bind_param('ssssss', $user, $password, $email, $serial, $rand_id, $rand_secret);
            $stmt->execute();
            $confirmation_code = hash_hmac('sha256', $rand_id, $rand_secret);
            $variables = array();
            $variables['User'] = $user;
            $variables['Serial'] = $serial;
            $variables['Code'] = $confirmation_code;
            $template = file_get_contents("template2.html");
            foreach($variables as $key => $value)
            {
                $template = str_replace('{{ '.$key.' }}', $value, $template);
            }
            $mail->addAddress($email, $user);
            $mail->msgHTML($template, __DIR__);
            $mail->AltBody = "You can activate your account here: www.mysite.com/activate.php?id=$serial&conf=$confirmation_code";
            if (!$mail->send()) 
            {
                echo 'error, mail not delivered';
            } 
            else 
            {
                echo "Registration OK! mail sent";
            }
        }
    }
    $stmt->close();
    $con->close();
    return;
}

function random_str($length, $keyspace = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')
{
    $pieces = [];
    $max = mb_strlen($keyspace, '8bit') - 1;
    for ($i = 0; $i < $length; ++$i) {
        $pieces []= $keyspace[random_int(0, $max)];
    }
    return implode('', $pieces);
}
?>

activate.php

<?php
if ( ! empty( $_GET ) )
{
    if ( isset($_GET['id']) and isset($_GET['conf']) )
    {
        $key=$_GET['id'];
        $code=$_GET['conf'];
        $con = new mysqli($host, $username, $password, $database);
        $stmt = $con->prepare("SELECT * FROM verify WHERE serial=?");
        $stmt->bind_param('s', $key);
        $stmt->execute();
        $result = $stmt->get_result();
        $num_of_rows = $result->num_rows;
        $result = $result->fetch_array();
        // If Serial doesn't exist or is already taken
        if ( ($num_of_rows==0) or ( empty( $result['username'] )) or ( empty( $result['rand_id'] )) or ( empty( $result['rand_secret'] )) )
        {
            $error=1;
            echo "Invalid code";
        }
        if ( $error<>1 )
        {
            $rand_id = $result['rand_id'];
            $rand_secret = $result['rand_secret'];
            $confirmation_code = hash_hmac('sha256', $rand_id, $rand_secret);
            if ($confirmation_code == $code)
            {
                $user=$result['username'];
                $password=$result['password'];
                $email=$result['email'];
                $stmt = $con->prepare("UPDATE accounts SET username = ?, password = ?, email = ? WHERE Serial = ?");
                $stmt->bind_param('ssss', $user, $password, $email, $key);
                $stmt->execute();
                $stmt = $con->prepare("DELETE from verify WHERE serial = ?");
                $stmt->bind_param('s', $key);
                $stmt->execute();
                echo "Account activated";
            }
            else
            {
                $error=1;
                echo "Invalid code";
            }
        }
        $stmt->close();
        $con->close();
        return;
    }
}

function random_str($length, $keyspace = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')
{
    $pieces = [];
    $max = mb_strlen($keyspace, '8bit') - 1;
    for ($i = 0; $i < $length; ++$i) {
        $pieces []= $keyspace[random_int(0, $max)];
    }
    return implode('', $pieces);
}
?>

mail.php

<?php
use PHPMailer\PHPMailer\PHPMailer;
require 'vendor/autoload.php';
$mail = new PHPMailer;
$mail->isSMTP();
$mail->Host = 'smtphost';
$mail->Port = port;
$mail->SMTPAuth = true;
$mail->Username = 'mymail';
$mail->Password = 'mypassword';
$mail->setFrom('mymail', 'myname');
$mail->addReplyTo('mysupportmail', 'myname');
$mail->AddEmbeddedImage('img/logo.png', 'mylogo');
$mail->AddEmbeddedImage('img/gif.gif', 'mygif');
$mail->Subject = "Activation for account";
?>

Thanks in advance.

Answer 1

Overall this is a lot of procedural code to process. That is fine to use but I would suggest you look into an MVC structure, or at least OOP - abstracting bits of code into controller methods and other functions or static methods that can be called by these pages, as well as tested by unit tests. I know that might be a lot to expect from a beginner but it is worth learning about and utilizing.

The code already uses require for mail.php. I would suggest abstracting the repeated function random_str() into a single file, along with any other common functions, and then using require to include it wherever necessary, which would be inline with the Don't Repeat Yourself principle. That way any updates to the function can be done in one spot instead of multiple. If you use OOP/MVC techniques as recommended above, that function could be static method of a class - e.g. Authentication, AuthenticationController, Registration, RegistrationController, etc.

Also, those variables listed at the top of registration.php which do not change can be stored as constants - either with define() or the const keyword - especially the latter if there is an appropriate class created to associate those with. And a common convention for constants is for them to be named using all capitalized letters. While this is not a requirement, many believe it helps when reading the code to distinguish constants from other values.

The sequential queries in registration.php i.e. "SELECT * FROM acc WHERE serial=?" and "SELECT verify FROM acc WHERE serial=?" make me wonder if serial is a primary/foreign key of one or both of those tables, and if those two queries could be combined into a single query where the tables are JOINed one those fields.

It would be a good habit to use the Identical comparison operator (i.e. ===) where appropriate instead of the Equal comparison operator (i.e. ==) unless you are sure that type-juggling is fine for your use case. And the same is true for the Not Identical Operator (i.e. !==) vs Not equal (i.e. != or <>).