Homebrewed Authentication - For fun

DeadlySin3

Granted, there are some really good authentication systems out there and I sincerely doubt anything I could write would be as high quality. Still - I'm having fun tinkering and i'm curious about what others out there may think about certain methods i'm using.

Firstly, I'm using a homebrewed MySQLi 'DataLayer' Class to connect and perform basic CRUD functions. (Something of a Data Access Object, but not at all abstract)
Second, I have a login form that offers a visitor the oppurtunity to type in a username and password combination that gets sent via _post to the form validator/processor.

Please keep in mind that i'm still in the early stages of writing the login processing, it's not as clean as it could be. in the end, I intend on creating a subclass to handle the logins, just working out the logic first.

I first check if the user is logged in, in which case I regenerate the session id - I read this could help prevent session hi-jacking. Then I sanitize, doLogin is the variable associated with the submit button on the form. I don't know if this really & truly needs to be sanitized, but I don't think passing it through the filter hurts.

I'll post the code I have to process the form and i'll throw some add'l comments in where i'm looking for help.

<?php

if (isset($_SESSION['loggedin']) && $_SESSION['loggedin'] === TRUE) {
    session_regenerate_id();
}

#
# Sanitizing form variables
#

$doLogin = filter_input(INPUT_POST, 'doLogin', FILTER_SANITIZE_STRING); /* var passed from the login form submit button */
$admin_login = filter_input(INPUT_POST, 'admin_login', FILTER_SANITIZE_STRING);
$admin_pass = filter_input(INPUT_POST, 'admin_pass', FILTER_SANITIZE_STRING);
$login_token = filter_input(INPUT_POST, 'login_token', FILTER_SANITIZE_STRING); /* copy of server generated token */
$err = "NULL"; # Still working on error handling - this is incomplete

# Ensuring we're processing strings
/* I read somewhere that ensuring you're dealing w/the exact type of data you expect is important. Someone, somewhere will find a way to throw other types at your processing methods. I'm almost certain that this is an unnecessary step at this point. Maybe if I put it before the filters, it may be useful? Not sure on this */

$var_chk = array($doLogin, $admin_login, $admin_pass);
foreach ($var_chk as $string) {
    if (!is_string($string)) {
        settype($string, 'string');
    }
}

#
# Authenticating Server Token
#

if (isset($doLogin)) {

/* I'm certain some work needs to be done here- I get an error if TOKEN_EXP_TIME (in this case, it's defined as 180 seconds) has passed and the token expires.
Notice: Undefined index: login_token_time - at this time i'm not overly concerned with this so I haven't toyed with it much but I have some ideas to remedy this issue */

$login_token_age = time() - $_SESSION['login_token_time'];

if (!isset($login_token) || !isset($_SESSION['login_token']) || empty($login_token) || $login_token !== $_SESSION['login_token'] || $login_token_age > TOKEN_EXP_TIME) {
    $err = "bad_token";

    ImportantMsg::writeBadTokenMsg();

# disallow continuation 
        $continue = "0";
    } else {
        unset($_SESSION['login_token']);
        unset($_SESSION['login_token_time']);
        $continue = "1";
    }
}

if ($continue === "1") {
#
# Validating the user
#
    if (isset($doLogin) && $admin_login == '' || $admin_pass == '') {
        $err = "inc_cred";
        ImportantMsg::writeRequiredFieldMsg();
    }

#
# If form was completed and submitted
#

if (isset($doLogin) && isset($admin_login) && isset($admin_pass) && $admin_login != '' && $admin_pass != '') {

#
# Verify this user exists in the database
#

    $table = $dl->select_specific("id, user_access, user_login, user_first, user_last, user_email", "code_users", array('user_login' => $admin_login, 'user_pass' => Users::hash_pass($admin_pass)), "", '1') or $err = "inv_cred";
    if ($err == "inv_cred") {
        ImportantMsg::writeInvalidCredentialsMsg();

/* So I feel I need to explain the definition of the variable $table here; the select_specific function is part of my DataLayer class that allows to more easily write SQL statements on the fly.
I have another function that works alot like it called select() but it uses just a table name, sort & condition as arguments. Since i'm dealing with a password, I didn't want to select the password hash & other misc stuff so I chose not to use it here for obvious security reasons.

$table resolves to : SELECT id, blah, blah, etc, from code_users WHERE user_login = $admin_login AND user_pass = passwordhash LIMIT 1
To break it down a bit further the result set is returned as a multidimensional array
$table = array( # Typical breakdown of dataLayer select() method
    array(
        'id' => '10',
        'user_access' => "admin",
        'user_login' => 'j.doe',
        'user_first' => 'Jonathan',
        'user_last' => 'Doe',
        'user_email' => 'j.doe@isp.com'
    ),
);

so $table[0]['id'] = 10 in this instance

I have a utility method designed that takes variables like $admin_login and passes them through mysqli_real_escape_string automatically - I'm working on learning PDO though so maybe one day soon i'll convert, but for now i love this datalayer!
*/

    } else {

#
# If user exists - set sessions
#
# Define user_hash
            $_SESSION['user_hash'] = md5(microtime());
# Insert user_hash on login
            $dl->update("code_users", array('user_hash' => $_SESSION['user_hash']), array('id' => $table[0]['id'])) or die($dl->getError());

        foreach ($table as $d_row) {
            foreach ($d_row as $field => $val) {
                $_SESSION[$field] = $val;
            }
        }

#
# Advance to home page
#

        $_SESSION['loggedin'] = TRUE;
        echo "<meta http-equiv=\"REFRESH\" content=\"3;url=$base_url/index.php\">";

        ImportantMsg::writeLoginMsg();
    }
}
}

So i'm really curious about the user_hash; It seems like a good idea to store such a thing in the database as I have a method called isLoggedIn() that checks for the session[user_hash] and matches it to the database variable and if they match, I know the user is currently logged in and I can display user/admin related material - of course, I only check the database if session[loggedin]==TRUE. Is it worthwhile to check for the user hash or is just checking the sessions good enough?

I think checking for the user_hash adds an extra layer of security and that's why i implemented it but if it's just a waste of resources, it's easily removed.

The last thing I wanted to discuss was how I'm hashing passwords. As shown above, I have a public static function in my Users class that does the work.

Firstly, I have two "Salts" if you will.
One is 63 random printable ASCII characters (Site Key)
Another is 63 random alpha-numeric characters (a-z, A-Z, 0-9) (Site Salt)
( https://www.grc.com/passwords.htm generated them for me )

Both of which are hard-coded. Right now, both are stored in a .php file with other config settings however, eventually, i will store the site key in the database eventually.

Then, I take the password, concatenate the site salt, user id then site key to it and spit out a hash; the function looks something like this:

function hash_pass($password) {
        hash_hmac('sha512', $password . S_SALT . (int) self::getIDFromLogin(), S_KEY);
        return $hashed_pw;
    }

The only real difference between each hash will be the actual password and the id of course will be different for each user as the other two salts are static I'm curious about the integrity of this solution.
Would it be better to actually generate a random salt and store it in the database as opposed to using the users id? One would still have to connect to the database to fetch it so I doubt it would be less of a stressful operation on the server but given the length of the two static salts - a 1 or 2 digit user id is enough to make even the same password result in a different hash for each unique user

Should I use a different algo? I'm not looking for hashing speed - just strength really. I've tried the password_hash function but i'm using PHP 5.4.4-14+deb7u11 (cli) (built: Jun 16 2014 09:46:53) - the most up to date version of PHP available for Debian wheezy and it generates a fatal error to an undefined function so for now something like the function above will have to suffice.

I was considering creating a secondary hash_pass2() function that rearranged the salts and the id so that every user with odd (or even, whatever) id #'s would use the secondary hash but I haven't done this yet.

I'd sure love some feedback on this as any help is greatly appreciated to improve the code

For anyone curious - here's the getIDFromLogin function

public static function getIDFromLogin() {
        $admin_login = filter_input(INPUT_POST, 'admin_login', FILTER_SANITIZE_STRING);
        $count = $dl->select_specific('id, count(*) as count', 'code_users', array('user_login' => $admin_login), '', '1'); # SELECT id, count(*) as count FROM code_users WHERE user_login = $admin_login LIMIT 1
        if ($count[0]['count'] > 0) {
            return $count[0]['id'];
        } else {
            return false;
        }
    }

big_nerd

Hello,

For all it's worth, don't use * anywhere in SQL statements.

it boils down to efficiency, you can COUNT(id) or COUNT() and get the same result - except that with any "" MySQL needs to lookup the columns before performing the operation.

I am not an expert on Crypto so I can't speak much on your methods - but I can say, even if the salt's are found - you are still doing your part to discourage bad behaviour since a "hacker" (term used loosely) would have to get your data (hashed passwords), then generate a hash list. Realistically your idea of just making it a pain in the rear end for someone to have to generate MASSIVE lists in order to crack it - so on that note, well done.

If you want to add a unique salt, you could also add the userid to the salt, because userid #1762 will always be 1762, it will effectively create a unique salt per person, but still be able to be referenced via SQL & PHP if required.

And - even if the person does know you've done this - it will help trying to generate a massive rainbow table for comparison. Of course - if they are after 1 specific password it adds nothing to what you already have since the first part of the hash is always known.

Keep up the coding...

laserlight

DeadlySin3 wrote:
Would it be better to actually generate a random salt and store it in the database as opposed to using the users id? One would still have to connect to the database to fetch it so I doubt it would be less of a stressful operation on the server but given the length of the two static salts - a 1 or 2 digit user id is enough to make even the same password result in a different hash for each unique user

Use a random salt of significant length. It is true that if you use the user id the salt will be unique, but on the other hand this is so predictable an approach that an attacker might already be prepared for it (at least for specific numbers), whereas the idea here is to make life as difficult for the attacker as possible so your users have enough time to change their passwords once they hear that your database has been compromised.

DeadlySin3 wrote:
Should I use a different algo? I'm not looking for hashing speed - just strength really. I've tried the password_hash function but i'm using PHP 5.4.4-14+deb7u11 (cli) (built: Jun 16 2014 09:46:53) - the most up to date version of PHP available for Debian wheezy and it generates a fatal error to an undefined function so for now something like the function above will have to suffice.

Yes, the password hashing set of functions was introduced in PHP 5.5.

DeadlySin3 wrote:
Should I use a different algo? I'm not looking for hashing speed - just strength really. I've tried the password_hash function but i'm using PHP 5.4.4-14+deb7u11 (cli) (built: Jun 16 2014 09:46:53) - the most up to date version of PHP available for Debian wheezy and it generates a fatal error to an undefined function so for now something like the function above will have to suffice.

If you are looking for strength then you should use bcrypt (the default for password_hash), scrypt, PBKDF2, or at least repeat the hashing with the salt for some sufficiently large number of iterations.

DeadlySin3

laserlight;11041767 wrote:
Use a random salt of significant length. It is true that if you use the user id the salt will be unique, but on the other hand this is so predictable an approach that an attacker might already be prepared for it (at least for specific numbers), whereas the idea here is to make life as difficult for the attacker as possible so your users have enough time to change their passwords once they hear that your database has been compromised.

Yes, the password hashing set of functions was introduced in PHP 5.5.

If you are looking for strength then you should use bcrypt (the default for password_hash), scrypt, PBKDF2, or at least repeat the hashing with the salt for some sufficiently large number of iterations.

Hi Laserlight thanks for the reply. I was starting to think that maybe my post wasn't making sense in what I was asking - I'll definitely look into the bcrypt library a bit more. I read into it when i was initially looking at PHP5.5 updates. By no means am I a crypto guy either. I only know what i've read about in a few books and honestly that wasn't much lol.

That being said, the current set up outlined above shows that I have two salts stored in PHP documents while a third, the userID is stored in the database. Of course, If a "hacker" was able to get access to the database then there's a a major issue. Then - this attacker would still need access to the PHP documents that are on the server to get to the salts used.

While not impossible, I would imagine this would be a major attack and likely wouldnt go unnoticed.

I've always known not to use "*" in SQL statements - but damn if it's not easy. You're right though - I need to be less lazy. Duly Noted and thank you sir.

What about the user_hash? it's stored in DB upon successful login and checked later when needing to be sure the user is logged in. Do ya think that's wasteful ? Or is it worthwhile to dip into the db to check this everytime you need to know whether or not it's ok to let this user access restricted areas?

Weedpacket

laserlight wrote:
DeadlySin3 wrote:
Should I use a different algo? I'm not looking for hashing speed - just strength really. I've tried the password_hash function but i'm using PHP 5.4.4-14+deb7u11 (cli) (built: Jun 16 2014 09:46:53) - the most up to date version of PHP available for Debian wheezy and it generates a fatal error to an undefined function so for now something like the function above will have to suffice.

Yes, the password hashing set of functions was introduced in PHP 5.5.

Note in the documentation for [man]password_hash[/man] that it also refers to a drop-in workaround for PHP 5.4.

laserlight

DeadlySin3 wrote:
That being said, the current set up outlined above shows that I have two salts stored in PHP documents

Whether you have one or two from the PHP script probably matters less since if an attacker has access to one of those values he/she will probably have access to the other.

DeadlySin3 wrote:
while a third, the userID is stored in the database. Of course, If a "hacker" was able to get access to the database then there's a a major issue.

Passwords are hashed for storage because:

The storage used to store passwords can be compromised, sometimes even as an inside job.
Users reuse passwords.

Therefore, when you are considering the strength of password hashing for storage, you must assume that the attacker not only knows the password hash value and the algorithm, but also knows other related data like user id and the salt value (whether stored in the database on in the script).

DeadlySin3 wrote:
Then - this attacker would still need access to the PHP documents that are on the server to get to the salts used.

While not impossible, I would imagine this would be a major attack and likely wouldnt go unnoticed.

Yeah, so this makes life more difficult for the attacker with negligible cost to you, which is a Good Thing.

DeadlySin3 wrote:
What about the user_hash? it's stored in DB upon successful login and checked later when needing to be sure the user is logged in. Do ya think that's wasteful ? Or is it worthwhile to dip into the db to check this everytime you need to know whether or not it's ok to let this user access restricted areas?

The session id already functions as a "user hash". Just setting a session variable to indicate that the user has been logged in should be fine. Note that you may or may not "dip into the db" to check session variables: it depends on what is used as the session storage.

DeadlySin3

Thanks Weedpacket - I read the php.net man page and I do see the work around; I've ran the password compat test and it shows a pass - For the last couple of days i've been relaxing a bit and not coding really. I've been eating, drinking and dreaming code the last month or so. Time away will be good lol!

From what you're saying: having two salts doesn't matter due to having one = having the other.

True; again this is why i use the ID as part of a salt as well. It's a static # thats not stored with my PHP docs but in the database. (Also, an attacker may mistake 'user_hash' as the password salt!)

I'll be re-working the hashing to use stronger encryption.

I'm going to re-read a portion of a book I have about security; It was highly recommended in the book that something like user_hash is stored in the database and a session['user_hash'] is set as well. Then when checking to see if the user is logged in, you match session['user_hash'] to the database stored variable user_hash

since user_hash gets updated in the database as well as in the session on login -- even if somehow the "attacker" were able to get a session['loggedin'] = TRUE the Users::IsLoggedIn(); check would fail due to to the hashes not matching. It took me a bit of time of reading and re-reading the first time around to really grasp the concept but I think, it makes sense.

laserlight

DeadlySin3 wrote:
I'm going to re-read a portion of a book I have about security; It was highly recommended in the book that something like user_hash is stored in the database and a session['user_hash'] is set as well. Then when checking to see if the user is logged in, you match session['user_hash'] to the database stored variable user_hash

since user_hash gets updated in the database as well as in the session on login -- even if somehow the "attacker" were able to get a session['loggedin'] = TRUE the Users::IsLoggedIn(); check would fail due to to the hashes not matching. It took me a bit of time of reading and re-reading the first time around to really grasp the concept but I think, it makes sense.

The user cannot manipulate session data other than via your script because that data is stored serverside. Therefore, if the attacker was able to set session['loggedin'] to true, then you have a script vulnerability that needs to be fixed, and perhaps the vulnerability would also allow the attacker to set the hash in both the session and in the database. That said, maybe the vulnerability only allows manipulation of session data, in which case this defense in depth would work, but there are other considerations...

You need to decide when to generate (or regenerate) the hash. If you only generate it at signup, then you do not get much out of the defense in depth because the chances are higher that an attacker can somehow find out the hash for a user (improbable, but remember that such manipulation of session data is improbable to begin with). If you regenerate it at login or better yet, on every page load, then you get more margin of security, but at the cost of preventing users from staying logged in on multiple devices unless you want a more complicated system of multiple user hashes (and for the latter: an extra database update on each page load, which may matter if you have high traffic since you probably want the field to be indexed).

DeadlySin3

laserlight;11041803 wrote:
The user cannot manipulate session data other than via your script because that data is stored serverside. Therefore, if the attacker was able to set session['loggedin'] to true, then you have a script vulnerability that needs to be fixed, and perhaps the vulnerability would also allow the attacker to set the hash in both the session and in the database. That said, maybe the vulnerability only allows manipulation of session data, in which case this defense in depth would work, but there are other considerations...

You need to decide when to generate (or regenerate) the hash. If you only generate it at signup, then you do not get much out of the defense in depth because the chances are higher that an attacker can somehow find out the hash for a user (improbable, but remember that such manipulation of session data is improbable to begin with). If you regenerate it at login or better yet, on every page load, then you get more margin of security, but at the cost of preventing users from staying logged in on multiple devices unless you want a more complicated system of multiple user hashes (and for the latter: an extra database update on each page load, which may matter if you have high traffic since you probably want the field to be indexed).

Nah I see where you're coming from. I've done some indepth reading about session hijacking & fixation and it seems the easiest way around that is just to regenerate the session id after a state change (going from visitor to logged in user); I'm likely just going to remove that bit - if it's unnecessary code it's not worth keeping.

Thanks for the tips!

laserlight

You're welcome! 🙂

johanafm

You should also make certain that it's not possible to have sessions created with a known id by passing along an unused session id, which is known as "session fixation". This type of attack lets a user easily hijack another users session, "session adoption", because the attacker would already know the session id.

Php 5.5.2 added ini setting session.use_strict_mode which prevents session fixation.

More on the topic here, including information on how to prevent session fixation without session.use_strict_mode enabled.

DeadlySin3

Thanks for the info!