0

recently revealed a problem in my login handler. The thing is, that even though the entered password is correct and matches the one in the database, script still sends me to the mistake page.

session_start();
include ("db.php");

 if (isset($_POST['login'])) {                                     
        $login = $_POST['login']; 
        $login = stripslashes($login);
        $login = htmlspecialchars($login);
        $login = trim($login);
        if ($login == '') {
            unset($login);
            } 
            } 

        if (isset($_POST['password'])) {
        $password=$_POST['password']; 
        $password = stripslashes($password);
        $password = htmlspecialchars($password);
        $password = trim($password);
        $password = hash("md5",$password); 
        if ($password =='') {
            unset($password);
            }
            }


if (empty($login) or empty($password)) 
    {
    exit (header('location:index.php'));
    }

$result = mysql_query("SELECT * FROM users_data WHERE login='$login'");

    $row = mysql_fetch_array($result);

    if (empty($row['password']))
    {
    exit (header('location:mistake.php'));
    }
    else {

    if ($row['password']==$password) {

    $_SESSION['login']=$row['login']; 
    $_SESSION['users_id']=$row['users_id'];
    header('location:first.php');

    }
    else {

        header('location:mistake.php');



    }
    }

The HTML form:

<form action="login.php" method="post" class="login">



<label><span>Login:</span>
<input name="login" type="text" size="20" maxlength="100">
</label>




<label><span>Password:</span>
<input name="password" type="password" size="20" maxlength="100">
</label>



<p>
<input type="submit" name="submit" class ="submit" value="Login">
</p>

UPD: Thank you for your answers, finally I've got where the problem was - I just specified not enough length of password values in the database.

iacm
  • 29
  • 6
  • what is the password and what do you have stored in the database for the user you are trying? – Alex Andrei Dec 09 '15 at 13:37
  • echo $row['password'] and echo $password and post here please – Sugumar Venkatesan Dec 09 '15 at 13:39
  • Why does the sql not also use the password? – Professor Abronsius Dec 09 '15 at 13:39
  • 1
    Also use `mysqli` instead of `mysql`. Mysql is deprecated. – Inurosen Dec 09 '15 at 13:41
  • You really shouldn't use [MD5 password hashes](http://security.stackexchange.com/questions/19906/is-md5-considered-insecure). Please use PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security. If you're using a PHP version less than 5.5 you can use the `password_hash()` [compatibility pack](https://github.com/ircmaxell/password_compat). – Jay Blanchard Dec 09 '15 at 13:41
  • @AlexAndrei I dont understand the question. Password is the users password. In the database is stored hashed users password. – iacm Dec 09 '15 at 13:47
  • @SugumarVenkatesan It just prints the same hashed password value, both of these. – iacm Dec 09 '15 at 13:47
  • @RamRaider Is it necessary? Login and password are stored in the same table. – iacm Dec 09 '15 at 13:47
  • if you post it we can try, how to make the if condition return true – Sugumar Venkatesan Dec 09 '15 at 13:48
  • 1
    Please [stop using `mysql_*` functions](http://stackoverflow.com/questions/12859942/why-shouldnt-i-use-mysql-functions-in-php). [These extensions](http://php.net/manual/en/migration70.removed-exts-sapis.php) have been removed in PHP 7. Learn about [prepared](http://en.wikipedia.org/wiki/Prepared_statement) statements for [PDO](http://php.net/manual/en/pdo.prepared-statements.php) and [MySQLi](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) and consider using PDO, [it's really pretty easy](http://jayblanchard.net/demystifying_php_pdo.html). – Jay Blanchard Dec 09 '15 at 13:54
  • 2
    there are way too many conditional statements here that will probably mess up somewhere. I wouldn't waste anymore time with this if I were you, but use something that's already proven to work and ***worthy of this century***. http://stackoverflow.com/a/34158109/ – Funk Forty Niner Dec 09 '15 at 13:58
  • @SugumarVenkatesan Now value of it is just "test" and in database it stored hashed as "098f6bcd4621d373cade4e832627b4" – iacm Dec 09 '15 at 14:07
  • If you change to PHP's [built-in functions](http://jayblanchard.net/proper_password_hashing_with_PHP.html) to handle password security you will not have the problems you're having. If you're using a PHP version less than 5.5 you can use the `password_hash()` [compatibility pack](https://github.com/ircmaxell/password_compat). – Jay Blanchard Dec 09 '15 at 14:12
  • for test md5 hash value is 098f6bcd4621d373cade4e832627b4f6 and your stored hash is different "098f6bcd4621d373cade4e832627b4" that's why you are getting to the mistake.php (my opinion) – Sugumar Venkatesan Dec 09 '15 at 14:15

3 Answers3

0

First of all why would you store the password in the database without hashing them(e.g. md5).

If you would do that, then there would be no need to process the password and you could just compare the stored md5(password) with the md5 hash of the password posted by the user.

Also w.r.t it is most likely that you are being redirected to the mistake.php page instead of the success.php page because of the encoding.

It would help if you provide us with the password you are using to test the code (assuming you are testing it. ;) ).

Cheers!

EDIT: Please look at better encryption techniques, as suggested by @jayblancard in the comments below.

bIgBoY
  • 417
  • 2
  • 12
  • You really shouldn't use [MD5 password hashes](http://security.stackexchange.com/questions/19906/is-md5-considered-insecure). – Jay Blanchard Dec 09 '15 at 13:44
  • Thanks @JayBlanchard, it was an insightful read, but I made the suggestion to use md5 purely for simplicity and better understanding of the "asker". – bIgBoY Dec 09 '15 at 13:48
  • 3
    While it may be simple it is wrong in this day and age. We should encourage good coding habits and good security management. If we're not going to do that we aren't doing our jobs. – Jay Blanchard Dec 09 '15 at 13:50
  • @Aman concerning the password. Now value of it is just "test" and in database it stored hashed as "098f6bcd4621d373cade4e832627b4". – iacm Dec 09 '15 at 14:04
  • By passing `test` through md5 [here](http://www.md5.cz/) one gets the hashed value as "098f6bcd4621d373cade4e832627b4f6" which is 2 characters more than the value you provided. – bIgBoY Dec 09 '15 at 14:07
  • @Aman Thats really weird, because I stored it with pretty the same handler of 'password'. – iacm Dec 09 '15 at 14:12
0

try to use isset() instead of empty

if (isset($row['password']))
Marc El Bichon
  • 397
  • 1
  • 7
  • 24
  • 3
    Why should the OP "try this"? A ***good answer*** will always have an explanation of what was done and why it was done that way, not only for the OP but for future visitors to SO. – Jay Blanchard Dec 09 '15 at 13:43
0
  1. I will just advice you to try to debug your code, mistake DOT php is called in multiple places so use a die("die message") to see which one is being fired.

  2. Since you don't have tests to your code debug output of valid and invalid input and check output.

  3. Once you are satisfied with the inputs and outputs, check if conditions if they are behaving as expected like previously using die condition maybe.

NB: your code is messy look at this to lean basics

Also look at OO programming