How secure is my PHP login system?

Question

I'm new to PHP and this is also my first log in system so it would be great if you guys could look over my code and see if you can spot any security holes:

note: I am sanitizing all user input although it's not shown here.

Sign Up:

Step 1: I take the password the user chose and run it through this function:

encrypt($user_chosen_password, $salt);

function encrypt($plain_text, $salt) {
    if(!$salt) {
        $salt = uniqid(rand(0, 1000000));
    }
    return array(
        'hash' => $salt.hash('sha512', $salt.$plain_text),
        'salt' => $salt
    );
}

Step 2: I then store the hash and the salt ($password['hash'] and $password['salt']) in the users table in the database:

id | username | password  | salt       | unrelated info...
-----------------------------------------------------------
1  | bobby    | 809a28377 | 809a28377f | ...
                fd131e5934
                180dc24e15
                bbe5f8be77
                371623ce36
                4d5b851e46

Log In:

Step 1: I take the username the user entered and do a look up on the database to see if any rows are returned. On my site no 2 users can share the same username so the username field always has a unique value. If I get 1 row returned I grab the salt for that user.

Step 2: Then I run the user entered password through the encrypt function (as previously posted above) but this time I also supply the salt retrieved from the database:

encrypt($user_entered_password, $salt);

Step 3: I now have the proper password to match against in this variable: $password['hash']. So I so a second lookup on the database to see if the username entered and the hashed password together return a single row. If so then the user's credentials are correct.

Step 4: In order to log the user in after their credentials passed I generate a random unique string and hash it:

$random_string = uniqid(rand(0, 1000000));
$session_key = hash('sha512', $random_string);

I then insert the $session_key into the active_sessions table in the database:

user_id | key
------------------------------------------------------------
1       | 431b5f80879068b304db1880d8b1fa7805c63dde5d3dd05a5b

Step 5:

I take the unencrypted unique string generated in the last step ($random_string) and set that as the value of a cookie which I call active_session:

setcookie('active_session', $random_string, time()+3600*48, '/');

Step 6:

At the top of my header.php include there is this check:

if(isset($_COOKIE['active_session']) && !isset($_SESSION['userinfo'])) {
   get_userinfo();
}

The get_userinfo() function does a lookup on the users table in the database and returns an associative array which is stored in a session called userinfo:

// first this function takes the value of the active_session cookie and hashes it to get the session_key:

hash('sha512', $random_string);

// then it does a lookup on the active_sessions table to see if a record by this key exists, if so it will grab the user_id associated with that record and use this to do a second lookup on the users table to get the userinfo:

    $_SESSION['userinfo'] = array(
        'user_id'           => $row->user_id,
        'username'          => $row->username,
        'dob'               => $row->dob,
        'country'           => $row->country,
        'city'              => $row->city,
        'zip'               => $row->zip,
        'email'             => $row->email,
        'avatar'            => $row->avatar,
        'account_status'    => $row->account_status,
        'timestamp'         => $row->timestamp,
    ); 

If the userinfo session exists I know the user is authenticated. If it doesn't exist but the active_session cookie exists then that check at the top of the header.php file will create that session.

The reason why I am using a cookie and not sessions alone is to persist the login. So if the user closes the browser the session may be gone but the cookie will still exist. And since there is that check at the top of header.php, the session will be recreated and the user can function as a logged in user as normal.

Log Out:

Step 1: Both the userinfo session and the active_session cookie are unset.

Step 2: The associated record from the active_sessions table in the database is removed.

---

Notes: The only issue I can see (and perhaps there are many others), is if the user fakes that active_session cookie by creating it themselves in their browser. Of course they must set as that cookie's value a string which after it is encrypted must match a record in the active_sessions table from where I will retrieve the user_id to create that session. I am not sure what the chances of this is realistically, for a user (perhaps using an automated program) to guess a string correctly which they don't know will then be sha512 encrypted and matched against the string in the active_sessions table in the database to get the user id to build that session.

Sorry for the big essay but since this is such a critical part of my site and due to my inexperience I just wanted to run it by more experienced developers to make sure it's actually safe.

So do you see any security holes in this route and how can it be improved?

Solution 1

You should include some kind of timeout or failover to prevent against brute-force attacks. There are a number of ways to do this, including IP-based blocking, incremental timeouts, etc. None of these will ever stop a hacker, but they can make it much more difficult.

Another point (which you haven't mentioned, so I don't know your plan) is failure messages. Make failure messages as vague as possible. Providing an error message like 'That username exists, but the passwords did not match' might be helpful to the end-user, but it kills login functionality. You just converted a brute-force attack that should take O(n^2) time to O(n) + O(n). Instead of needed to try every permutation in a rainbow table (for example), the hacker just tries all values for username (with a set password) first, until the failure message changes. Then, it knows a valid user, and just has to brute force the password.

Along those lines, you should also make sure that the same amount of time elapses when a username exists and doesn't exist. You are running additional processes when a username actually exists. As such the response time would be longer when a username exists vs when it doesn't. An incredibly skilled hacker could time page requests to find a valid username.

Similarly, you should make sure that, in addition to expiring cookies, you also expire the sessions table.

Lastly, in the get_user_info() call, you should terminate all open sessions if there are multiple concurrent, active logins. Make sure you timeout sessions after a set amount of inactivity (like 30 minutes).

Along the lines of what @Greg Hewgill mentioned, you haven't included any of the following:

• SSL/encrypted connection between Server-Client
• Other transport protocols you much be using to process authentication (like OAuth)

You server is secure, but it doesn't matter how awesomely secure your algorithm is if someone can read the data that's exchanged (MITM). You should make sure you are only communicating over an encrypted protocol.

Solution 2

...run the user entered password through the encrypt function...

So how does the password get from the browser to the server? You haven't mentioned protecting against man-in-the-middle attacks.

Solution 3

This code...

function encrypt($plain_text, $salt) {
    if(!$salt) {
        $salt = uniqid(rand(0, 1000000));
    }
    return array(
        'hash' => $salt.hash('sha512', $salt.$plain_text),
        'salt' => $salt
    );
}

...is bad. Use the new password API and be done with it. Unless you are an expert, you should not try to design your own authentication system. They are extremely difficult to get right.

In order to log the user in after their credentials passed I generate a random unique string and hash it:

Just let PHP handle session management. rand() and mt_rand() are both very insecure random number generators.

Solution 4

It looks like the code you created is not testable through automated unit and integration tests. This makes it hard to spot any errors that might be included in your implementation over time and while running in a production environment.

This normally leads to security issues, because the security of a strict and correct execution and sane data handling is not tested/verified.

(It's just another point on the list, see as well the answer about how to secure the transport layer, also you have not specified how you protect your session data from being tampered.)

• Authentication Cheat Sheet
• Session Management Cheat Sheet

Solution 5

Regarding passwords in php, you shouldnt be encrypting them. You should hash them using the password_hash() then on login, use password_verify() to verify that the password via the html form matches the stored hash in the database

View more solutions

Comments

[Admin]

Are you referring to someone listening over the user's network, Like the need for SSL?

[Greg Hewgill]

Yes, that's correct. If I can see the packets between browser and server (there are many ways), then I can see the user's password go by in plaintext.

[Admin]

Yes I was planning on introducing x number of failed attempts login timeout. At least that would delay automated bots from attempting brute force attacks.

[Jared Farrish]

I hate it when a site tells me "the password doesn't match the email address", and when I then try a nonsensical email, "the user doesn't exist". And... when the site emails me the password I've "forgotten". Thanks. I've seen supposedly risk-savvy sites do this, and refuse to acknowledge why it's a problem to do so.

[Jared Farrish]

This is an interesting answer. Do you have any additional links or information that help explain how to approach this problem?

[hakre]

I prefer an example: If Paula has the same pw-hash and hash in the db, she might be able to obtain the identity of Bobby. Unit-Testing abstracting the store, it's easily possible to spot such issues with boundary tests, e.g. duplicated values. It might be a detail that has been overlooked in implementation by the wish to make everything correct. Such cases are better to be spotted early. It's just standard testing stuff, so you should find a lot on the topic online. Apart from that I suggest nostarch.com/codecraft.htm

[Jared Farrish]

Well, I'd say that most of the people who would be interested in how unit testing and integration tests are beneficial are the one-off and small team groups that may not have the practice behind them to see the actual payoff. I'm wondering, what does same pw-hash and hash in the db mean? Is this a collision test?

[hakre]

password and salt. it's a test what happens if those collide across rows, yes. I don't understand the part you wrote about teams and so.

[hakre]

Well if you're concerned about a unit of your application you want to know it properly works, you need to test it. Review alone does not work. I'm not saying you need to do TDD, but I'm saying you need to test it. You need to try to break it even for such components.

[Jared Farrish]

That's boilerplate. I'm not sure you entirely grasp the context I'm proposing. Is "properly works" a function of "how often it works for the end user" or a quotient of "how often it proves problematic for the [same] end-user"? Sometimes we don't work in an environment that's entirely per-ordained or tested. TDD is great and all, but so is worst-is-best when the risk of "failure" is not "abject failure", even if the developer (or developers) don't want either.