2
\$\begingroup\$

Is this authentication mechanism vulnerable to SQL injection? If so, how could one exploit it, and what is the most effective way to mitigate this?

if (isset($_POST['userid']) && isset($_POST['password'])){

$username = $_POST['userid'];
$password = crypt($_POST['password'],'34l5kj');


    $query = "SELECT * FROM users WHERE name =:username and pass=:password";
$db->query($query);
$db->bind(':username', $username);
$db->bind(':password', $password);
$db->execute();
$data = $db->single();

            $_SESSION['valid_user'] = $username;
            $_SESSION['user_id'] = $data['id'];
            $_SESSION['pwd']=$_POST['password'];
\$\endgroup\$

closed as off-topic by Phrancis, Mast, Vogel612, t3chb0t, Toby Speight Oct 23 '17 at 13:09

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Welcome to Code Review. Is this your real, working code? If so, you should be able to test it for SQL injection fairly easily, just call it setting $username to"'foo'; SELECT * FROM users;--". Regardless, you should look into prepared statements if you really want to protect against injection. \$\endgroup\$ – Phrancis Oct 22 '17 at 19:07
  • \$\begingroup\$ Just read this question and it's answers.. \$\endgroup\$ – Mast Oct 22 '17 at 19:47
  • \$\begingroup\$ Relevant \$\endgroup\$ – Mast Oct 22 '17 at 19:48
  • \$\begingroup\$ Sorry you're having some trouble today Judy! If you edit your post to be a little less specific it may go better. Here at code review people will automatically review all aspects of your code, so you could just edit it to be "PHP: basic login system" or something along those lines \$\endgroup\$ – Conor Mancone Oct 22 '17 at 22:12
  • 2
    \$\begingroup\$ I'm reasonably sure that this code is not all there is to it. It'd be preferrable if you posted your actual full code, such that we can review it within it's context \$\endgroup\$ – Vogel612 Oct 22 '17 at 22:18
3
\$\begingroup\$

First, regarding your initial question: the code you have right here is definitely safe against SQL injection. Using prepared queries (which you are doing) will protect you from the majority of SQL injection vulnerabilities. It's worth doing some reading on SQL injection in general, but otherwise the biggest concern you have is for those queries that cannot use prepared queries. This is typically the case if you need a column name to change based on user input, in which case you need to apply a white-list. However, that isn't an issue here, so no need to over complicate things. So while you are immediately protected against SQL injection, there are still some potential concerns here, and some general room for improvement:

  • You store the raw user data in the session. In particular, the username is stored without any filtering. This can result in a second-order SQL injection vulnerability. In effect, you are using prepared queries here, but unsanitized data is being stored in the session. Later on when that data is retrieved from the session and used (for instance, to verify the user when they return to the site), you can't see that it is dirty data and may forget to use proper prepared queries. As a result, you may accidentally introduce an SQL injection vulnerability that is hard to see. So remember to abide by two very important rules: 1) Always clean user input before storing and 2) Always use prepared queries. The latter could very well be true already, but make sure and do #1 as well.
  • In this particular case though it is better not to worry about cleaning the username so much as halting all processing if you find invalid input. In an ideal situation this would be part of a check_login function and you would return false as soon as you determined that the login was invalid. So before you even do anything with the username you first check if it is valid (probably by making sure it doesn't contain any special characters) and return false if you find any problems. This way there is no chance of getting dangerous input into the session in the first place. It is much easier if people login with an email instead of a username, because you can use filter_var to make sure an actual email address was given, and return false if not. Email addresses, of course, cannot be used for SQL injection.
  • Don't use crypt. Use password_hash, and let it pick the salt for you.
  • Don't compare the password in the database. Password hash comparisons are actually difficult to do right, and (while difficult to execute), a direct comparison like you are doing can leave your application vulnerable to timing attacks and other subtle weaknesses. Select the hashed password out of the database and use password_verify to check if it matches the user input.
  • It is probably a bit to soon for you yet, but if you are just learning then now is a good time to start reading about and learning about dependency injection and test automation. Those two will substantially improve the quality of your code in the long run, but obviously aren't being used here.
  • Your spacing is very inconsistent. Following good coding conventions is a surprisingly important way of minimizing bugs in the long run and making maintenance easier. Pick a coding convention and stick to it. PSR is the standard in the PHP world, but sticking to any standard is better than being inconsistent.

Code organization is also very important to keep bugs at a minimum in the long run, but since only part of your code is posted here, there isn't much I can say about that at the moment.

Edit to add more about input filtering

I think it is worth a few more minutes to expand upon my discussion about input filtering. The issue is that there are more vulnerabilities to be concerned about than just SQL Injection. When you store user input directly in the application (i.e. $_SESSION['valid_user'] = $_POST['userid']) you leave a lot of room for security holes to leak in. In this case, because you are checking that the username exists in the database, you are effectively checking the username against a whitelist. Validating against a white-list is the typically the best approach to security, and in this particular case it is unlikely to cause you any specific trouble, but most security vulnerabilities are the more subtle kind that come from failures to properly validate user input at every step of the process.

The concept you want to go for is defense-in-depth. In this case your login code is implicitly relying upon the fact that the registration form performs proper user-input validation. As a result, a simple failure in validation logic on the registration page can potentially open up additional avenues for attack when the login page doesn't do any validation of its own. When each area of the system practices proper security for all data relevant to itself, it becomes much more difficult for an attacker to find an exploitable security bug. As a result, the most secure way to handle user input is:

  1. Validate user input as strictly as you can, and reject any requests with invalid data
  2. Always use prepared queries
\$\endgroup\$
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Mathieu Guindon Oct 23 '17 at 13:31
  • \$\begingroup\$ The "the username is stored without any filtering. This can result in a second-order SQL injection vulnerability" statement is wrong. What is leading to (any order) SQL injection vulnerability is negligence towards prepared statements. \$\endgroup\$ – Your Common Sense Oct 23 '17 at 14:21
  • \$\begingroup\$ the "checking that the username exists in the database, you are effectively checking the username against a whitelist" statement is wrong. A white list is an explicit list of allowed entities. it could be applied to an ACL list, where some users have certain privileges. But simply checking whether an entity exists in the database is a database lookup, not a white list \$\endgroup\$ – Your Common Sense Oct 23 '17 at 14:24
  • 1
    \$\begingroup\$ @YourCommonSense Our conversation was moved to chat for a reason. You and I obviously disagree about how best to approach security. Continuing that discussion here just adds to the noise, and doesn't help anyone \$\endgroup\$ – Conor Mancone Oct 23 '17 at 14:28
3
\$\begingroup\$

Strictly speaking, we cannot tell whether your code is safe or not, because you are using some homemade solution for the database interaction. From the way it is used, we could assume that it's safe, but nobody can tell it for sure, unless we could see the code for this class.

Another thing about your database class is it's stateful. Means some day you will have an internal variable assigned to a particular query rewritten, which will lead to unwanted results.

Like it is said above, you should use password_verify() instead of your own implementation. Given all that, here is your reviewed code, using vanilla PDO and password_hash()

if (isset($_POST['userid'],$_POST['password'])) {
    $stmt = $pdo->prepare("SELECT * FROM users WHERE name = ?");
    $stmt->execute([$_POST['userid']]);
    $user = $stmt->fetch();

    if ($user && password_verify($_POST['password'], $user['pass']))
    {
        $_SESSION['valid_user'] = $user['name'];
        $_SESSION['user_id'] = $user['id'];
        header("Location: /");
        exit;
    }
}
// whatever you want to do for the failed login
exit;

this code is based on the canonical example I wrote for the case. As you can see, with vanilla PDO this code is easier than yours, so I encourage you to use PDO instead.

\$\endgroup\$
  • \$\begingroup\$ Good catch on the stateful DB class: there are all manner of subtle bugs that can creep in with that one. \$\endgroup\$ – Conor Mancone Oct 23 '17 at 9:35

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