6
\$\begingroup\$

I am new to web development and am working on a portfolio piece, so I would like to make sure I'm not creating obvious vulnerabilities. I am wondering whether there are any clear flaws in the following account creation and login code. Also, the relevant query wrapper is included as the last code snippet, and I am wondering whether there are SQL injection possibilities.

All of the files for this LAMP site are at: https://github.com/ian-luttrell/lamp-site

Create Account (controller):

<?php

require_once('../App/Models/CreateAccount.php');

class CreateAccount
{
    public function index()
    {
        $view = '../App/Views/CreateAccount/index.php';
        View::render($view, []);
    }

    public function submit()
    {
        // pass registration credentials to model for processing
        $cred = ['username' => $_POST['username'],
                    'password' => $_POST['password']];
        $processed_cred = CreateAccountModel::processCredentials($cred);

        // pass processed credentials to model for account creation     
        CreateAccountModel::register($processed_cred); 

        $username = $processed_cred['username'];
        $data = ['username' => $username];
        $view = '../App/Views/CreateAccount/submit.php';
        View::render($view, $data);
    }
}

?>

Create Account (model):

<?php

require_once('../Core/Model.php');

class CreateAccountModel extends Model
{
    public static function processCredentials($cred)
    {
        $username = htmlspecialchars($cred['username']);
        $hashed_password = password_hash($cred['password'], PASSWORD_DEFAULT);
        $processed_cred = ['username' => $username,
                            'hashed_pass' => $hashed_password];

        return $processed_cred;
    }

    public static function register($processed_cred)
    {
        $username = $processed_cred['username'];
        $hashed_password = $processed_cred['hashed_pass'];
        $record = ['id' => NULL, 
                    'username' => $username, 
                    'hashed_password' => $hashed_password, 
                    'created_at' => NULL];

        $conn = static::getConn();
        $db = new QueryBuilder($conn);
        $db->insert('users', $record);
    }

}

?>

Login (controller):

<?php

require_once('../App/Models/Login.php');
require_once('../Core/View.php');

class Login
{
    public function index()
    {
        $view = '../App/Views/Login/index.php';
        View::render($view, []);
    }

    public function submit()
    {
        $cred = ['username' => $_POST['username'],
                    'password' => $_POST['password']];

        // pass supplied username to model for escaping
        $username = $cred['username'];
        $escapedUsername = LoginModel::escapeUsername($username);
        $password = $_POST['password'];
        $processedCred = ['username' => $escapedUsername,
                            'password' => $password];       

        // pass processed credentials to model for authentication
        $validLogin = LoginModel::authenticate($processedCred); 
        if ($validLogin) {
            session_start();
            $_SESSION['user'] = $escapedUsername;

            $data = ['username' => $escapedUsername];
            $view = '../App/Views/Login/successful_login.php';
            View::render($view, $data); 
        } else {
            $view = '../App/Views/Login/failed_login.php';
            View::render($view, []);
        }   
    }

    public function logOut()
    {
        session_start();
        $_SESSION = [];
        session_destroy();

        $view = '../App/Views/Login/index.php';
        View::render($view, []);
    }
}

?>

Login (model):

<?php

require_once('../Core/Model.php');

class LoginModel extends Model
{
    public static function escapeUsername($username)
    {
        return(htmlspecialchars($username));
    }

    public static function authenticate($processedCred)
    {
        $username = $processedCred['username'];
        $password = $processedCred['password'];

        $conn = static::getConn();

        $sql = 'SELECT * FROM users WHERE username=:username';      
        $stmt = $conn->prepare($sql);
        $stmt->bindParam('username', $username);
        $user_exists = $stmt->execute();
        if ($user_exists) {
            $results = $stmt->fetchAll(PDO::FETCH_ASSOC);       
            foreach ($results as $row) {
                $hashed_password = $row['hashed_password'];
                if (password_verify($password, $hashed_password)) {
                    return True;
                }
            }
        }
        return False;
    }

}

?>

Query wrapper:

<?php

// Config.php is a configuration file in the App directory containing
//   sensitive database login information, so it is NOT posted on GitHub
require_once('../App/Config.php');

class QueryBuilder
{
    protected $pdo;

    public function __construct($dbConn)
    {
        $this->pdo = $dbConn;
    }

    public function selectAll($table)
    {
        $statement = $this->pdo->prepare("SELECT * FROM {$table}");
        $statement->execute();

        return $statement->fetchAll(PDO::FETCH_ASSOC);
    }

    public function insert($table, $record) 
    {
            $col_names = implode(', ', array_keys($record));
            $parameters = ':' . implode(',:', array_keys($record));
            $sql = sprintf("INSERT INTO %s (%s) VALUES (%s)", $table, $col_names, $parameters);

            $parameters = explode(',', $parameters);            
            $arr = array_combine($parameters, $record);

            try {
                $stmt = $this->pdo->prepare($sql);
                foreach ($arr as $param => $val) {
                    // cleaner to use bindValue() instead of bindParam()
                    //   when values may be NULL
                    $stmt->bindValue($param, $val);
                }
                $stmt->execute();
            } catch (PDOException $e) {
                die('Database insert error.');
            }
    }
}

?>
\$\endgroup\$
0

1 Answer 1

Reset to default
1
\$\begingroup\$

There are no direct vulnerabilities in the code present. However, there is a potential vulnerability in the query builder. By itself, it is not safe, and relies for the safety on the calling code, which stakes the safety on the programmer's skill which is always a slippery ground. An inexperienced programmer could be tempted to convey $table or $record from the user input, assuming the method is safe. So I would at least make table and field names escaped.

There are other areas of improvement, of course.

Error handling.

Let me be frank, it is outright wrong. die('Database insert error.'); is wrong on many levels. What a user is supposed to do after getting such a message? Why should a user face such a blunt message where a beautiful site design just have been? Where to click, what to do? Why not to show a more friendly error page instead?

What is much more important, how a programmer is supposed to be notified of the problem's cause in order to fix the error? There is a detailed error message that tells a programmer what the problem is and you are just ditching it away.

After all, why a query builder should decide how to handle an application error? Every module should be responsible for its job only. In case there is an error, an error handler should be employed, whereas it's none of the query builder's business how an error message should be processed. In short, it should just leave the error message alone.

If you're interested in the topic, I wrote an article on the PHP error reporting basics.

Query builder.

selectAll() is just useless. Trust me, such a function will never see any use. Or at least it doesn't worth to have a distinct method for sure.

cleaner to use bindValue() instead of bindParam() when values may be NULL

is just not true. bindParam() is all right with nulls, its drawback is completely different (it just cannot be used in a loop like this).

Either way, manual binding would be pointless anyway as you can always just send $record right into execute() And yes, no offense but $arr = array_combine($parameters, $record); is just funny, as it actually recreates $arr back from $record. Why?

The most important part. Even if you are always using hardcoded values for $table and keys in the $record, any of them could cause an error anyway, for example if it happen to be a database keyword or contain a forbidden symbol like a space or a hyphen. Of course the latter is less likely to happen but still. There are strange databases to work with. So all table and column names have to be properly formatted, as well as placeholder names.

So in the first place there should a method to format identifiers. For mysql it could be like

public function escapeIdent($name) {
    return "`".str_replace("`", "``", $name)."`";
}

which should be used for the every identifier added to the query. And insert method would become like this

public function insert($table, $record) 
{
        $table = $this->escapeIdent($table);

        $col_names = array_keys($record);
        $col_names = array_map([$this, 'escapeIdent'), $col_names);
        $col_names = implode(", ", $col_names);

        $values = array_values($record);

        // let's use positional placeholders for simplicity
        $parameters = str_repeat('?,', count($values) - 1) . '?';

        $sql = "INSERT INTO $table ($col_names) VALUES ($parameters)";
        $this->pdo->prepare($sql)->execute($values);
}

Model

Models are not usually organized like that. You will end up with thousands of "models" for the average-sized application this way. A model should be linked to an entity that roughly represents a database table (such as user), and provide methods to work with. In your case there should be just a single model User that should contain both authenticate() and register() in it.

Whereas whatever function escapeUsername() by no means belong to the Model. A model is a database. How does HTML escaping belong to a database? All calls to htmlspecialchars() should be done in the View.

As a result, whatever processCredentials() (if any) method should be internal to the model an never called in the Controller.

Autoload

there should be not a single require call in a modern application. Implement a PSR-4 autoload that will load your classes automatically.

Misc

The code in LoginModel is just untidy. First, your model should have a connection as a class variable instead of calling connection in every method.

Also, $user_exists = $stmt->execute(); is simply not true. execute()'s return value has nothing to do with the query result, it will never tell you whether a query found anything or not. You need to fetch the actual row to do so.

But I have no idea why you decided to use fetchAll() here and process the result in a loop. Given there should be just a single user with a given name, why not to just fetch a single record and use it away. I wrote a canonical example for authenticating a user using PDO and password_verify() so we can adapt it for your method:

public static function authenticate($username, $password)
{
    $sql = 'SELECT * FROM users WHERE username=?';      
    $stmt = $this->conn->prepare($sql);
    $stmt->execute([$username]);
    $user = $stmt->fetch();
    if ($user && password_verify($password, $user['hashed_password'])) {
        return $user;
    }
}

so it will make your controller not that messy as well (honestly, I got dizzy from all these creds, processedCreds, escapedCreds and such):

public function submit()
{
    $user = LoginModel::authenticate($_POST['username'], $_POST['password']); 
    if ($user) {
        session_start();
        $_SESSION['user'] = $user['id'];
        ...
    }
}

Note that after the POST request it is obligatory to do a GET redirect, so in your login method you should call a redirect instead of rendering a user account page right away.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.