2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Ludisposed Jan 25 at 9:27
  • \$\begingroup\$ Oh I'm very sorry, I will read that page. Thanks for changing it back to normal \$\endgroup\$ – Roberto Carlos Jan 25 at 9:29
2
\$\begingroup\$

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 <>).

\$\endgroup\$
  • \$\begingroup\$ Thanks for this answer, I will update the code with what you wrote after the first paragraph. About MVC structure, that's something new for me, I will start reading about it and will do my best to adapt my code to it. From what I read (only 1 page) it is Model View and Controllers structure where everything is separated. How would I achieve this, using different files or something else? Thanks in advance. \$\endgroup\$ – Roberto Carlos Jan 25 at 8:43
  • \$\begingroup\$ About the Serial thing being checked on both tables it's because when a user tries to register with a serial, I want to check if there is any valid account with that serial or any non-activated account with that serial. I still have to add something to clear the row in verify table if the user doesn't confirm his/her account in X amount of time (like 1 week or something). \$\endgroup\$ – Roberto Carlos Jan 25 at 9:19
  • \$\begingroup\$ I can't edit my comments so, about editing the post. Thanks to Ludisposed for telling me that it goes against Code Review. I already applied those changes in my code and everything is working fine. I'm still learning about OOP. Still I'm worried about the security, I already used prepared statements for almost everything, but I'm worried about the mail thing. Username and email are used and I don't know what to do with those. \$\endgroup\$ – Roberto Carlos Jan 25 at 9:32

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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