unsecure login script - how to protect

Question

I am investigating a project that could be sql injected (one user reported it).

Here is part of login script:

  $loginemail = $_POST['loginemail'];                 
            $loginemail_sql = mysql_real_escape_string($loginemail);                

            $password = $_POST['loginpass'];                 
            $password_sql = mysql_real_escape_string($password);                

             //get user data
            $q = 'SELECT uid, full_name, score, status FROM users WHERE email="'.$loginemail.'" AND password="'.$password_sql.'" LIMIT 1';

I would like to now if this is part of code that could be injected? Is there a problem that $loginemail and $password are treated incorrectly and could contain some dangerouse "SQL parts"?

`mysql_real_escape_string` is deprecated, and you should be hashing your passwords. — Waleed Khan, Mar 20 '13 at 19:56
[More info](http://www.php.net/manual/en/mysqlinfo.api.choosing.php) on the deprecation of `mysql*` functions. Switch to `mysqli` or PDO. — ajp15243, Mar 20 '13 at 19:59

score 6 · Accepted Answer · edited May 25 '19 at 11:45

Let's see:

Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

For starters.

Second, you're escaping $loginemail, but using the unescaped version (instead of $loginmail_sql).

Third, it's implied by your code that you store your passwords in the database as-is. You shouldn't. If an attacker gets his hands on the database, your passwords would be compromised. You should hash the password and store the hash there.

score 2 · Answer 2 · edited May 23 '17 at 12:28

2

Yes, you are using $loginemail in your query that is unchanged. Use $loginemail_sql instead.

For further reading, have a look at How to prevent SQL injection in PHP?

edited May 23 '17 at 12:28

Community

1
1

answered Mar 20 '13 at 19:56

Gumbo

643,351
109
780
844

score 2 · Answer 3 · answered Mar 20 '13 at 20:20

Well as @Madara Uchiha said it's good to stop using mysql_* functions. But he gave no example. So i'll. Sorry for my english, i'm brazilian :)

PDO:

<?php
$loginemail = $_POST['loginemail'];
$password = $_POST['loginpass'];

// open a PDO connection within mysql driver
// PDO uses DSN string for connections they look like: "driver:options"
$dsn = 'mysql:dbname=testdb;host=127.0.0.1';
// DB username
$dbUser = 'dbuser';
// DB password
$dbPassword = 'dbpass';
// PDO is instantiable, so you need to create an object for each connection
$dbh = new PDO($dsn, $dbUser, $dbPassword);
// You must prepare your query, PDO uses a placeholder system for sanitize data and avoid injection, params can be passed using "?" or ":varname"
$stmt = $dbh->prepare('SELECT uid, full_name, score, status FROM users WHERE email= :email AND password= :password LIMIT 1');
// Binding a param called :email
$stmt->bindValue(':email', $loginemail);
// Binding a param called :password
$stmt->bindValue(':password', $password);
// Execute the PDOStatement
$stmt->execute();
// Then fetch result
$result = $stmt->fetchAll(PDO::FETCH_ASSOC);
// ALSO, you can fetch only one row using
$firstRow = $stmt->fetch(PDO::FETCH_ASSOC);

score 1 · Answer 4 · answered Mar 20 '13 at 19:56

1

For one thing, you're escaping $loginemail, but then using the unescaped version in the query.

One thing you might want to consider, if possible, is using PDO and prepared statements to simplify escaping and parameter substitution.

answered Mar 20 '13 at 19:56

Telgin

1,614
10
10

score 1 · Answer 5 · answered Mar 20 '13 at 19:57

You use mysql_real_escape_string on $loginemail to arrive at $loginemail_sql, but you then stop short of using it in your query:

$q = 'SELECT uid, full_name, score, status FROM users WHERE email="'.$loginemail.'" AND password="'.$password_sql.'" LIMIT 1';

score 1 · Answer 6 · edited May 23 '17 at 12:12

I'm no expert on sql injection but escaping the strings is a great first step which should be enough although there is probably more that can be done.

My biggest issue with your code is that you seem to store the passwords in plan text in the database. You should use a salted hash to store them. Check this out for more information: Secure hash and salt for PHP passwords

Also you made a mistake in this code:

$q = 'SELECT uid, full_name, score, status FROM users WHERE email="'.$loginemail.'" AND password="'.$password_sql.'" LIMIT 1';

Should be:

$q = 'SELECT uid, full_name, score, status FROM users WHERE email="'.$loginemail_sql.'" AND password="'.$password_sql.'" LIMIT 1';

unsecure login script - how to protect

6 Answers6