CODEWITHSUNDEEP
phpsqlcode-injection
Austin
Austin

Reputation: 4929

Is this vulnerable to SQL injection

I realize there are a lot of questions already about this. But my method isn't the same as theirs, so I wanted to know. I think I understand SQL, but I don't want to risk making a mistake in the future, so thanks for any help. (This is just a project I'm doing, not homework or anything important). 

function checkLogin($username, $password) {
    $username = strtolower($username);
    connectToDatabase();
    $result = mysql_query("SELECT * FROM `users` WHERE username='$username'");
    $dbpassword = "";
while($row = mysql_fetch_array($result))
  {
    $rowuser = $row['username'];
  if($username != $row['username']) continue;
  $dbpassword = $row['password'];
  }
    if($dbpassword == "") {
        return false;
    }
    $genpass = generatePassword($password);
   return $genpass == $dbpassword;
}

So hit me with your best shot :) And I don't think my method is as efficient as it could be. I don't understand php enough to understand what $row = mysql_fetch_array($result) is doing unfortunately.

Upvotes: 1

Views: 3298

Answers (4)

Ray Toal
Ray Toal

Reputation: 88468

Because you are taking an arbitrary string and placing it directly into an SQL statement, you are vulnerable to SQL injection.

( EDITED based on a comment below. )

The classic example of SQL injection is making a username such as the following:

Robert'); DROP TABLE users;--

Obligatory XKCD link

Explanation:

Given the "username" above, interpolation into your string results in:

SELECT * FROM `users` WHERE username='Robert'); DROP TABLE users;--'

The comment symbol -- at the end is required to "get rid" of your closing quote, because I just substituted one of mine to end your select statement so that I could inject a DROP TABLE statement.

As @sarnold pointed out, PHP's mysql_query only executes a the first query in the string, so the above example (known as query stacking) does not apply. The function is explained here: http://php.net/manual/en/function.mysql-query.php.

A better example can be found here. Here they use a username of

' OR 1 OR username = '

which interpolated becomes

SELECT * FROM `users` WHERE username='' OR 1 OR username = ''

and which would cause your application to retrieve all users.

Upvotes: 6

sarnold
sarnold

Reputation: 104110

The other answers are an excellent description of your problem, however, I think they both overlook the best solution: use PHP's PDO Prepared Statements for your queries.

$stmt = $dbh->prepare("SELECT * FROM users where username = ?");
if ($stmt->execute(array($username))) {
  while ($row = $stmt->fetch()) {
    print_r($row);
  }
}

This is a small, simple example. There are more sophisticated ways of using PDO that might fit your application better.

When you use PDO prepared statements you never need to manually escape anything and so long as you use this slightly different style, you will never write an SQL injection vulnerability and you don't have to maintain two variables per underlying "data" -- one sanitized, one as the user supplied it -- because only one is ever required.

Upvotes: 4

Phil Lello
Phil Lello

Reputation: 8639

The short answer is yes.

A perhaps more helpful answer is that you should never trust user input; prepared statements are the easiest way to protect against this, if you have PDO available. See PDO Prepared Statements

<?php
$stmt = $dbh->prepare("SELECT * FROM `users` WHERE username=?");
if ($stmt->execute($username)) {
  while ($row = $stmt->fetch()) {
    print_r($row);
  }
}
?>

Upvotes: 4

Lucas
Lucas

Reputation: 10646

I would say yes it is open to SQL injection.

This is because you are taking user input in the form of $username and putting it into your SQL statement without making sure it is clean.

This is a function that I like to use in my applications for the purpose of cleaning strings:

function escape($data) {
    $magicQuotes = get_magic_quotes_gpc();

    if(function_exists('mysql_real_escape_string')) {
        if($magicQuotes) {
            $data = stripslashes($data);
        }

        $data = mysql_real_escape_string($data);
    }
    else {
        if(!$magicQuotes) {
            $data = addslashes($data);
        }
    }

    return $data;
}

Then you can use it like this:

$username = escape(strtolower($username));
connectToDatabase();
$result = mysql_query("SELECT * FROM `users` WHERE username='$username'");

Upvotes: 2

Related Questions