DragonPrime - LoGD Resource Community
Welcome Guest
  • Good evening, Guest.
    Please log in, or register.
  • May 24, 2019, 05:54:14 PM
Home Forums News Downloads Login Register Advanced Search
* * *
DragonPrime Menu
Login
 
 
Resource Pages
Search

Pages: [1]   Go Down
  Print  
Author Topic: Going from md5 to password_hash function  (Read 770 times)
0 Members and 1 Guest are viewing this topic.
artti
Member
Captain of the Guard
***
Offline Offline

Posts: 176



View Profile
« on: February 19, 2019, 02:34:44 AM »

Hello,

I want to go from md5() that we use in crypting passwords to new password_hash() that is more secure.

http://php.net/manual/en/function.password-hash.php

On the login.php page around line 33 is

Code:

$name = httppost("name");
$password = httppost("password");
$password = md5(md5($password));

$sql = "SELECT * FROM " . db_prefix("accounts") . " WHERE login = '$name' AND password='$password' AND locked=0";
$result = db_query($sql);

if (db_num_rows($result)==1) $session['user']=db_fetch_assoc($result);

would it make sense to go next route?

Code:
$name = httppost("name");
$password = httppost("password");

$sql = "SELECT * FROM " . db_prefix("accounts") . " WHERE login = '$name' AND locked=0";
$result = db_query($sql);
$SomeNameVariable = db_fetch_assoc($result);

$allCorrect = password_verify($password,$SomeNameVariable['password']);

if ($allCorrect) $session['user']=$SomeNameVariable;

or should I have first query just to get password hash from database where login = $name and then select again.

After writing this all out, it makes sense to have just one query, but what are your opinions?
Logged
pharis
Militia
**
Offline Offline

Posts: 71


Take this it's dangerous to go alone


View Profile
« Reply #1 on: February 20, 2019, 04:21:14 PM »

The bigger issue here is the fact that [ login = '$name' AND password='$password' ] is a receipt for disaster. I dont have the whole escaping code in that section in front of me, but it is a quite inviting attack vector right there, since you are querying the DB using what is passed by the user and not having any control about the format of what to expect, thus opening yourself to all sorts of attacks. I dont see anything in that code that is going to protect the DB against sql injection.

Now, since you must alter the login / user creation process in order to achieve better hashing / encryption anyway ( you have to save the password at creation in the new desired encrypted format ) , why not adding a couple of lines more and have it a bit more secure ?

My advice :

- when creating the user , encrypt the password with password_hash()
- create another hash using the username ( you can use whatever hash algorithm you want, but of course if possible skip md5() and sha1() and save it to that users row
- now, every time a user logs in, hash its username first, and then look for the same value in the DB ( if evil code has been passed , it is now harmless since it has been hashed and the query is "safe" )
- since the hash should be unique ( you really should not have 2 users with the same username ) , you load all the needed data from the found row
- at this stage, you can compare the real username and the password within the array, without querying the DB ( since you did that already )
- if all matches, go ahead and validate the user, if not ( or if a malicious code was passed as the username, nothing will really happen , maybe an warning ) just ignore the user and revoke access

This is not a thorough explanation or a fail safe golden sword that will solve all your problems, but since you are willing to touch that code section, it adds reasonable security.

Oh, and for gods sake, while you are at it, and if you need to compare hashes at any point ( that is not a password ) use hash_equals() ->  http://php.net/manual/de/function.hash-equals.php
Logged
pharis
Militia
**
Offline Offline

Posts: 71


Take this it's dangerous to go alone


View Profile
« Reply #2 on: February 20, 2019, 04:46:26 PM »

from the top of my head, this could be a safer login approach ( assuming you added a username hash and encrypted the password at user creation )



Code:

//  get the name and password
    $name = httppost("name");
    $password = httppost("password");
   

   
//  hash the passed username , effectively neutralizing any potential fabricated unescaped content ( of course, you should always escape it first , one never knows )     
    $hashedName = hash('sha256', $name );

//  now look explicitly for it in the DB ( and for the locked flag ) , nothing else
    $sql = "SELECT * FROM " . db_prefix("accounts") . " WHERE hashedName = '$hashedName' AND locked=0";

//  process result
    $result = db_query($sql);
    $SomeNameVariable = db_fetch_assoc($result);


//  check content
    if ( !empty($SomeNameVariable) )
    {
    #   the passed username exists and we found a hashed version of it -> lets check if the password matches ...
        if ( !password_verify( $password , $SomeNameVariable['password']))
        {
        #   password does not match, revoke user here
            $session['user'] = FALSE;
        }
        else
        {
        #   password did match
            $session['user'] = $SomeNameVariable;
        }
    }
    else
    {
    #   nothing found
        $session['user'] = FALSE; 
    }

Logged
artti
Member
Captain of the Guard
***
Offline Offline

Posts: 176



View Profile
« Reply #3 on: February 23, 2019, 01:56:28 PM »

Thanks for replying! I imagine that those name and password variables are already checked via function httppost(); or most of the lotgd 1.2.0 is securely checked. I just somehow wanted to go away from md5 to hash function.  I definitely look that hash equals function.

I managed to set up creation and login with password_hash() function, but I shall try to be more secure with it.

Thanks again!
Logged
pharis
Militia
**
Offline Offline

Posts: 71


Take this it's dangerous to go alone


View Profile
« Reply #4 on: February 23, 2019, 07:02:37 PM »

I dont think that httppost does anything to secure what is passed by the  user.  ( this is from the 1.1.2 Version )
Sure, there are some stripslashes() here and there in the login code, but that is not security.

The change from md5 to a better algo is a good thing, of course, but pretty useless if the rest stays as it is ( no prepared statements in the db queries, no https:// , no real protection against sql injection... )
That small change is not a golden bullet.

-https://websitebeaver.com/prepared-statements-in-php-mysqli-to-prevent-sql-injection
https://phptherightway.com/


Code:

function httppost($var){
global $HTTP_POST_VARS;

$res = isset($_POST[$var]) ? $_POST[$var] : false;
if ($res === false) {
$res = isset($HTTP_POST_VARS[$var]) ?
$HTTP_POST_VARS[$var] : false;
}
return $res;
}



Logged
Anharat
Codemeister
****
Offline Offline

Posts: 282



View Profile
« Reply #5 on: February 24, 2019, 11:35:58 AM »

If you escape strings correctly or use prepared statements, which natively do that, you don't have to hash anything. But truth is probably that lotgd is not sql injection save.

But to topic, if you want to secure passwords you should also consider adding salt (& pepper) when hashing.
Logged
pharis
Militia
**
Offline Offline

Posts: 71


Take this it's dangerous to go alone


View Profile
« Reply #6 on: February 24, 2019, 01:36:05 PM »

you see, the thing with the hashing ( of the username and the comparison to the db in my earlier post ) was the fastest approach given the state of the lotgd login functions and preventing an open sql injection method.
Someone maintaining the LOTGD codebase should close that gap.

I totally agree with you on the prepared statements and escaping ( although escaping is not that trivial ) and that .. well,  lotgd is not safe against it.

As far as  hashing of passwords goes , salt and pepper are obsolete in the password_hash() function. PHP 5.5 + handles this natively and to be fair its much safer too probably.


« Last Edit: February 24, 2019, 03:35:02 PM by pharis » Logged
Pages: [1]   Go Up
  Print  
 
Jump to:  


*
DragonPrime Notices
Play LoGD on Dragonprime

Support Us
No funds raised yet this year
Your help is greatly appreciated!
Recent Topics
DragonPrime LoGD
Who's Online
134 Guests, 0 Users
Home Forums News Downloads Login Register Advanced Search