0

Okay so I have this in my index.php

 <form action="login.php" method="post">
<font >Username</font><br />
<input type="text" class="form-control" name="name" value="" style="width: 140px" />
<br />
<font >Password</font><br />
<input type="password" class="form-control" name="passwd" value="" style="width:140px"/>
<br /><br />
<input type="submit" value="Login" class="btn btn-primary"" style="width: 140px"/>         </form>

The problem I'm having is, when the user logs in they're able to use any password and still proceed to the usercp.php so it's only checking the username, what part am I missing or have I done wrong? Much appreciation to any replies.

Additional info: My database has saved passwords in md5

Then this is in my login.php

*UPDAATE

I changed my code to;

<?php
require("common.php");
$submitted_name = '';
if(!empty($_POST))
{
$query = "
    SELECT
        name,
        passwd
    FROM users
    WHERE
        name = :name
";

$query_params = array(
    ':name' => $_POST['name']
);
try
{
    $stmt = $db->prepare($query);
    $result = $stmt->execute($query_params);
}
catch(PDOException $ex)
{
    die("Failed to run query: " . $ex->getMessage());
}

$login_ok = false;

$row = $stmt->fetch();
if($row)
{
    $check_passwd = md5( $_POST['passwd']);

    if($check_passwd === md5($row['passwd']))
    {
        $login_ok = true;
    }
}    if($login_ok)
{
    unset($row['passwd']);            
    $_SESSION['user'] = $row;
    header("Location: usercp.php");
    die("Redirecting to: usercp.php");
}
else
{
    print("Login Failed.");
    $submitted_name = htmlentities($_POST['name'], ENT_QUOTES, 'UTF-8');
}
}
?>

By changing 

if($check_passwd = md5($row['passwd']))

to this 

if($check_passwd === md5($row['passwd']))

I can't log in at all with the right or wrong password, i've also tried with x2 =

All help is very appreciated, thankyou!!

Here2Learn
  • 3
  • 3
  • 1
    What is `for($round = 0; $round < 65536; $round++)` supposed to be doing? – Mark Baker Mar 01 '14 at 18:44
  • You should not be using md5 to hash passwords. This is highly insecure. Instead use [password_hash()](http://php.net/password_hash). – Mike Mar 01 '14 at 18:45
  • I'm not sure @Mark, it was from what a friend setup for me a while back, I'm not so good with PHP. and the game server running off of this login is apparently set to read md5 – Here2Learn Mar 01 '14 at 18:51
  • @MarkBaker What it does is it rehashes the password 65536 times. It's better than nothing, but still should not be used. – Mike Mar 01 '14 at 18:54
  • @mike - In reality, it's just reducing the entropy of an already low-entropy hashing algorithm still further, the first hash at least works off a (potentially) full 256-bit character set; every rehash after that first is working off a limited character set (A-Z,0-9) – Mark Baker Mar 01 '14 at 19:23
  • @MarkBaker using md5 in most cases would actually increase entropy. The average password has far less entropy than the average md5 digest. Entering an empty string with zero entropy into md5 will create a 128 bit string, so entropy had to be added to create that. Since all we are interested in is the initial password and the final hash produced, not how we got there, stretching the password 65536 times far outweighs any potential loss in entropy, if any. – Mike Mar 01 '14 at 22:22
  • OK, I stand corrected, I'm not a professional cryptographer; unsalted MD5 is perfectly good for passwords as long as you repeat it a few thousand times... but I trust you'll forgive me for not using it with any of my own systems – Mark Baker Mar 01 '14 at 23:10
  • @MarkBaker Perhaps I worded it wrong, but I would never recommend md5 to be used for password hashing, even if it is stretched. See my first comment above. – Mike Mar 02 '14 at 01:19

4 Answers4

1

The problem is a simple syntax error:

if($check_passwd = md5($row['passwd']))

Should be

if($check_passwd === md5($row['passwd']))

However, as I mentioned in the comment above, md5 is not suitable for hashing passwords.

Edit:

This is just a shot in the dark, but I believe this is what you need to do to get it working with md5:

if($row)
{
    $check_passwd = md5( $_POST['passwd']);
    for($round = 0; $round < 65536; $round++)
    {
        // I took out $row['passwd'] from the following line because it makes no sense
        $check_passwd = md5( $check_passwd); // <-- This is probably what you want
    }

    if($check_passwd === md5($row['passwd']))
    {
        $login_ok = true;
    }
}

If you would instead heed my warning and use password_hash(), this is what you can do (note, if you have PHP < 5.5 there is a compatibility function from github in the previous link that is forward compatible):

First, add a new field to your database to store the bcrypt hash and adjust all logic in your program to use that instead of md5:

if($row) {
    $login_ok = password_verify($_POST['passwd'], $row['bcrypt_passwd'])
}

Then just email all of your users that you have updated your script and you require everyone to reset their password and provide a link to your password reset link (you do have one, right?).

To calculate a hash of a password, all you need to do is:

$hash = password_hash($_POST['passwd']);

This hash is what you store in the database.

Community
  • 1
  • 1
Mike
  • 23,542
  • 14
  • 76
  • 87
  • Actually, that should be `if($check_passwd === md5($row['passwd']))` – OSborn Mar 01 '14 at 18:53
  • Even with adding that it's allow me to proceed with the wrong password >. – Here2Learn Mar 02 '14 at 01:16
  • You have changed your logic in your original question. That's why it's not working. You used to loop through and hash 65536 times. Now you only do it once. You can't expect it to be the same. – Mike Mar 02 '14 at 01:22
  • I see, sorry for all the questions! I have added back the loop to try the edited method above, but I can't get in with any right/wrong password now – Here2Learn Mar 02 '14 at 01:44
1

You said your DB is having passwords in hash, but then why are you applying MD5 again?

    if($check_passwd === md5($row['passwd'])) //This is wrong
    {
       $login_ok = true;
    }

    if($check_passwd === $row['passwd']) //This should do the trick
    {
       $login_ok = true;
    }
Jagjot
  • 5,816
  • 2
  • 24
  • 41
  • It seems to be what works with the game login system, however for the website login system which is where i'm having the trouble. I applied your change and I receive "Login Failed" on every attempt, I've double checked i'm 100% accurate with the info, but it will wont let me in. if($check_passwd === $row['passwd']) // if i remove 2 of the = it allows me to login but with any password again. I apologize for such inexperience, thank you very much for you help – Here2Learn Mar 02 '14 at 01:54
  • Can I see a screen-shot of your DB Table and the plain-text password that you are trying with? – Jagjot Mar 02 '14 at 02:19
  • And what is the plain-text password that you are trying to log-in with? – Jagjot Mar 02 '14 at 02:52
  • The password is "testacc" without quotations – Here2Learn Mar 02 '14 at 02:59
  • The md5 for "testacc"(with quotes) is 33c0fc48dfec3ddae7e3398e30e89a61 not what you have in the DB. Check your registration php-script. It is having some errors I guess. – Jagjot Mar 02 '14 at 03:02
  • Hmm.. These are the only parts in the register.php that contain pass. `$Pass = StrToLower(Trim($Pass));` `$Salt = "0x" . md5($Login.$Pass); MySQL_Query("call adduser('{$Login}', {$Salt},` – Here2Learn Mar 02 '14 at 03:07
  • `$Login = mysql_real_escape_string($_POST['login'], $Link); $Pass = mysql_real_escape_string($_POST['password'], $Link); $Email = mysql_real_escape_string($_POST['email'], $Link); $ip = mysql_real_escape_string($_SERVER['REMOTE_ADDR'], $Link); $Login = StrToLower(Trim($Login)); $Pass = StrToLower(Trim($Pass));` – Here2Learn Mar 02 '14 at 03:09
  • Maan! This is messed up! You are using salts. Modify your login code according to your registration procedure – Jagjot Mar 02 '14 at 03:17
  • Great! :) Glad you fixed it. – Jagjot Mar 07 '14 at 09:51
0

SELECT name, passwd, FROM users WHERE name = :name

Don't use ',' after passwd param.

Omegavirus
  • 277
  • 4
  • 16
  • I'm assuming that was just a typo for the question here on SO since it would have thrown an exception if there was a syntax error in the query. – Mike Mar 01 '14 at 18:47
  • From the docs of md5 and other hashes, It is seen that the function hash('md5',$string); is fast than md5($string); so use hash() function intead of md5 in the examples/answers given here. Thats it. :-) – prateekkathal Mar 02 '14 at 03:51
0

try this

if($row)
    {
        $check_passwd = md5( $_POST['passwd']);
        /*for($round = 0; $round < 65536; $round++)
        {
            $check_passwd = md5( $check_passwd . $row['passwd']);
        }*/

        if($check_passwd == $row['passwd'])
        {
            $login_ok = true;
        }
    }

i really dont see the meaning of that for loop (notihing changes if you run it once or 10 times or 65536 ), it will just make the page load slow. More over this $check_passwd = md5( $_POST['passwd'] . $row['passwd']); will never ever be equal to md5($row['passwd']). Please save your password as hashed in the database table as its not at all a standard procedure to keep the password in plain text.

Akhil Sidharth
  • 746
  • 1
  • 6
  • 16