2
\$\begingroup\$

I have coded a script that I run when I need to verify the authentication for a user, I use it as a php script and host it on api.mydomain.com. It just simply checks to see if the user details are correct, the same as the parameters passed and to check if the account has a subscription.

The end result is checking if the response contains congratulations.

<?php
$connection = mysqli_connect(
    "localhost",
    "root",
    "",
    "mydb"
);

if (mysqli_connect_errno()) {
    exit('failed to connect to the database server.');
}

if (!isset($_GET['auth_username']) || !isset($_GET['auth_password'])) {
    exit('you failed to provide the correct parameters.');
}

$username = isset($_GET['auth_username']) ? $_GET['auth_username'] : '';
$password = isset($_GET['auth_password']) ? $_GET['auth_password'] : '';

$commandText = "SELECT `password`,`has_subscription` FROM `a__client_accounts`";
$whereClause = "WHERE `username` = '" . $username . "'";
$result = $connection->query($commandText . " " . $whereClause);

if (!$result) {
    exit('Invalid query: ' . $connection->error);
}
else if ($result->num_rows > 0) {
    $array =  $result->fetch_array();

    if (!password_verify($password, $array['password'])) {
        exit('we found your account, but you entered the wrong password.');
    }
    else if ($array['has_subscription'] == '0') {
        exit('you authenticated, but you don\'t have a subscription');
    }
    else {
        exit('congratulations');
    }
}
else {
    echo 'no account was found with that name.';
}
\$\endgroup\$
  • 1
    \$\begingroup\$ It always bugs me when people do things like " . $username . ". PHP will parse variables inside quotes. There's no need to use the catenation operator. Minor note: my preference (others may disagree) is to not use if B then X else Y structure when X has an exit. At that point, you can just run Y since you know X can't be true or the program would've exited. \$\endgroup\$ – Barry Carter Oct 29 '17 at 16:50
1
\$\begingroup\$

I would make my review mostly on the security, with some other improvements.

  • The main issue with this code, obviously, is SQL injection. It can easily allow anyone to browse the whole contents of your database. You should be really using prepared statements for the every SQL query you run, if at least one variable is going to be used in it.
  • Worse yet, by echoing the error out, you ease the task for the attacker significantly. By getting the feedback for their actions through the error message, an attacker could save themselves a lot of time. In case of error, only a generalized error message should be ever sent outside.
  • a somewhat similar issue: never give a potential hacker any certain feedback regarding the correctness of login and password - it's none of their business. Whereas a honest user could be just informed that their credentials didn't match. So it should be the only response in case of the wrong credentials.
  • Another issue is HTTP method used. It is considered a good practice to use POST method for the credentials, as it has a much lower footprint. For example, when using GET method, you will have all user passwords stored in the server's access log in the raw form. Why bother with hashing them in the database, if raw passwords can be easily accessed anyway?

I also removed redundant isset() calls. Given you already tested your input variables with isset, a condition $username = isset($_GET['auth_username']) will never be evaluated to false. So I just removed it altogether.

A note on the error reporting. Like it is said above, a system error message should be never echoed outside. At the same time, it should be logged carefully for the site admin's reference. Seemingly contradicting at first, there rules are perfectly go together. I wrote an article on the PHP error reporting in which I summarized all my experience and which I highly recommend to read.

So the code would be

<?php

error_reporting(E_ALL);
ini_set('display_errors', 0);
ini_set('log_errors', 1);
mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

try {
    $connection = mysqli_connect("localhost","root","","mydb");

    if (!isset($_POST['auth_username'],$_POST['auth_password'])) {
        exit('you failed to provide the correct parameters.');
    }

    $sql = "SELECT password, has_subscription FROM a__client_accounts WHERE username=?";
    $stmt = $connection->prepare($sql);
    $stmt->bind_param("s", $_POST['auth_username'])
    $stmt->execute();
    $stmt->store_result();
    $stmt->bind_result($db_password, $has_subscription);

    if ($stmt->fetch() && password_verify($_POST['auth_password'], $db_password)
    {
        if (!$has_subscription)
        {
            exit('you authenticated, but you don\'t have a subscription');
        } else {
            exit('congratulations');
        }
    } else {
        echo 'wrong login or password';
    }
} catch (\Exception $e) {
    error_log($e);
    exit("server error");
}
\$\endgroup\$
  • \$\begingroup\$ This is probably a nitpick, but in PHP I prefer to avoid updates-by-reference, especially the bind_result and populating variables by reference that you have here. It creates a disconnect between where variables are "attached" to a result and when the update actually happens. In this case, it was very strange to see a $stmt->fetch() inside a conditional. It took a second to sink in that $db_password was being updated by reference as a result of the $stmt->fetch(). Better to just use fetch_assoc() IMO (less lines of code too). \$\endgroup\$ – Conor Mancone Oct 30 '17 at 14:05
  • \$\begingroup\$ @ConorMancone I'd prefer it too but it's just less portable. \$\endgroup\$ – Your Common Sense Oct 30 '17 at 14:09

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.