security - How secure is my PHP login system?

itemprop="text">

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?

itemprop="text">

class="normal">Answer

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.