-1

I have problem with my login form sql injection is working on it so how to stop it.

I am using mysql_real_escape_string but nothing changed

if(isset($_POST['submit-login'])) { 

    $user = $_POST['username'];
    $pass = $_POST['password'];

    $username = mysql_real_escape_string($user);
    $password = mysql_real_escape_string($pass);

    $usertool = new Usertool();
    if($usertool->login($username, $password)){
        //successful login, redirect them to a page
        header("Location: index.php");
    }else{
        $error = "Incorrect username or password. Please try again.";
    }
}

Here is usertool

class usertool {
    public function login($username, $password) {
        $hashedPassword = md5($password);
        $result = mysql_query("SELECT * FROM tbl_user WHERE uname = '$username' OR eemail = '$username' AND password = '$hashedPassword'");
        if (mysql_num_rows($result) == 1) {
            $_SESSION["user"] = serialize(new User(mysql_fetch_assoc($result)));
            $_SESSION["login_time"] = time();
            $_SESSION["logged_in"] = 1;     
            return true;
        } else {
            return false;
        }
}
deerox
  • 1,045
  • 3
  • 14
  • 28
  • 2
    follow this http://stackoverflow.com/questions/60174/how-to-prevent-sql-injection-in-php?rq=1 – Rahul11 May 20 '13 at 08:02
  • 1
    @ØHankyPankyØ It is not true. Neither devotion nor avoidance of mysql ext can affect sql injection vulnerability – Your Common Sense May 20 '13 at 08:09
  • @YourCommonSense Advice taken and comment removed – Hanky Panky May 20 '13 at 08:10
  • This code fragment does not seem to be vulnerable. – Gumbo May 20 '13 at 08:16
  • it is i try it my self :( @Gumbo – deerox May 20 '13 at 08:16
  • @deerox Ok, now I see what’s wrong: Your `WHERE` clause is flawed due to [operator precedence](http://dev.mysql.com/doc/refman/5.6/en/operator-precedence.html). `uname = '$username' OR eemail = '$username' AND password = '$hashedPassword'` is equivalent to `uname = '$username' OR (eemail = '$username' AND password = '$hashedPassword')`; use `(uname = '$username' OR eemail = '$username') AND password = '$hashedPassword'` instead. – Gumbo May 20 '13 at 08:20
  • [Please, don't use mysql_* functions in new code.](http://bit.ly/phpmsql) They are no longer maintained and [are officially deprecated.](https://wiki.php.net/rfc/mysql_deprecation) See the [red box?](http://j.mp/Te9zIL) Learn about prepared statements instead, and use [PDO](http://php.net/pdo) or [MySQLi](http://php.net/mysqli) - [this article](http://j.mp/QEx8IB) will help you decide which. If you choose PDO, here is a [good tutorial](http://j.mp/PoWehJ). – eldblz May 20 '13 at 08:21

1 Answers1

4

it is not a classic SQL injection in your case but rather wrong SQL logic.

You need to add braces to your query:

SELECT * FROM tbl_user 
   WHERE (uname = '$username' OR eemail = '$username') 
      AND password = '$hashedPassword'"

In your original query the whole statement evaluates to true if entered username or email matches, and password not even being checked

And regarding SQL injections in general, to make your queries safe, you have to format your query parts according to these rules

  1. Formatting have to be complete. mysql_real_escape_string does incomplete formatting alone: you ought to add apostrophes around whatever data you escaped using this function.
  2. Formatting have to be adequate, means you can't format a number or an identifier with string formatting. EVery SQL literal require it's own distinct formatting.
  3. Formatting have to be done as close to the query execution as possible.

Following these rules you'll be pretty safe from injection. And using prepared statements is the easiest way to follow them.

One don't need neither mysqli not PDO to use native prepared statements though - you can create your own variant. But nevertheless, you have to move mysql_real_escape_string as closer to the query execution as possible and always add apostrophes around the result.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345