4
\$\begingroup\$

I have the following code to do remember-me style login:

<?php
session_start();
if(!isset($_SESSION['login'])) $_SESSION['login'] = 0;
if(isset($_COOKIE['web_login_cookie']) && $_SESSION['login'] == 0) {
    $sql = mysql_query("SELECT * FROM users");
    while($row = mysql_fetch_array($sql)) {
        $pass = sha1($row['password'] . '@#$%$#%dsf@#$SDFg');
        if($_COOKIE['web_login_cookie'] == $pass) {
            $_SESSION['userID'] = $row['userID'];
            $_SESSION['login'] = 1;
            if($row['level'] == 1) {
                $_SESSION['admin'] = 1;
            }
        }
    }
    if(mysql_num_rows($sql) == 0) {
        setcookie("web_login_cookie", "", time()+(60*60*24*14));
        session_destroy();
    }
}
?>

I have this code in my header file. Is this secure enough or will it pose any security risks in the future?

\$\endgroup\$
  • 2
    \$\begingroup\$ Please read about the WHERE clause in SQL. \$\endgroup\$ – Álvaro G. Vicario Jan 24 '12 at 10:21
  • \$\begingroup\$ hi Avocado, i have read that, then what should i do? \$\endgroup\$ – cicakman Jan 24 '12 at 10:22
  • \$\begingroup\$ Calculating and comparing every user's cookie every time will be insanely slow rather soon i think. Three links to read: stackoverflow.com/questions/549/… en.wikipedia.org/wiki/HMAC openwall.com/phpass \$\endgroup\$ – biziclop Jan 24 '12 at 11:16
  • 1
    \$\begingroup\$ It's not related to security, but downloading an iterating through the whole user table on every page request will be a bottleneck in the application. \$\endgroup\$ – palacsint Jan 24 '12 at 11:27
5
\$\begingroup\$

I don’t like the idea of storing the password hash in a cookie. It is easy to steal and grants unlimited access. The cookie is transmitted with every single HTTP request. I would prefer using an access token with limited lifetime. Anyway most people store passwords in their browser and this is easy to steal as well – so this seems not to be a massive security break.

The bigger problem is that your login only relies on the password – you don’t need to know the corresponding username to login. This makes brute forcing a lot easier. And more important: your login will break, if two users use the same password. Then user2 will (unintentionally) gain access to user1’s account.

PS: As mentioned in comments, don’t loop over every line in your table, use a WHERE clause instead. And I don’t understand, what you are trying to achieve with your mysql_num_rows code block…

\$\endgroup\$
  • \$\begingroup\$ So is it better to store username inside cookies to prevent user2 will (unintentionally) gain access to user1’s account \$\endgroup\$ – cicakman Jan 24 '12 at 10:34
  • \$\begingroup\$ Your edit is a huge improvement. Don't use substr to divide userid and passhash, instead define a delimeter (which must not occur both in username or passhash). And also be aware of SQL Injections - cookies can be manipulated as easy as GET and POST params. \$\endgroup\$ – DerVO Jan 24 '12 at 11:55
2
\$\begingroup\$

With that:$_COOKIE['web_login_cookie'] == $pass, you store password (hash) in cookie. best, not to be put in the cookie. Just said the status of connection in the session (server side).

Control the login/password with PHP et put just $_SESSION['login'] = 1 in session.

You must check your login form against the injection MYSQL.

\$\endgroup\$
1
\$\begingroup\$

Your system makes it easy to do a bulk guessing attack on all users if the ‘salt’ is known to an attacker: All he has to do is to provide the hash of the salted password that might be used by an arbitrary user. Due to the while loop, your script will pick the right one out whose password hash matches the one provided in the cookie.

Chances are that someone of the users uses 123456 as their password, so using 5ecec7b7b6672e8f419f34bf1c5fa38298e08a70 as cookie value would be a good guess.

Besides the issue that you store the password in a cookie (no matter in what form), you use that value for both identification (Who is it?) and authentication (Is it really him/her?). This is also the reason why the mentioned attack would work.

Instead of using a cookie for authentication, just use the session mechanism. Although here the session ID is also used for both identification and authentication, a session is only valid for a limited time and the session ID should be unguessable.

\$\endgroup\$
  • \$\begingroup\$ Hi, please check the updated code, do you think this is any better? \$\endgroup\$ – cicakman Jan 24 '12 at 11:11
1
\$\begingroup\$

As others have mentioned, storing the password in the cookie leaves you wide open to replay attacks. If uoi ever try to scale this up to more than single character user ids, then you're going to run into problems with SQL injection - particularly since you then use the userid returned from the query rather than the user id which was supplied in the cookie. And you destroy the cookie when the user logs in - so it only remembers you once?

Sorry - but I'd recommend you go and read the many duplicate questions on SO regarding remember me functionality and start again.

\$\endgroup\$
0
\$\begingroup\$

I usually generate a login cookie like this and then I only have to verify the hash when the user tries to login through cookie authentication. Will further explain this answer when the time is on my side :)

$expiration = time() + 2629743;

$key  = hash_hmac('md5', $user_id . $expiration . $_SERVER['REMOTE_ADDR'], 'my-secret-key');
$hash = hash_hmac('md5', $user_id . $expiration . $_SERVER['REMOTE_ADDR'], $key);

$cookie = $user_id . '|' . $expiration . '|' . $hash;
setcookie('auth_cookie', $cookie, $expiration);
\$\endgroup\$
0
\$\begingroup\$

The right way to manage passwords in PHP was covered in great detail in this article that won the "Month of PHP security" in 2010.

I realise this isn't a great answer for this site, but I would strongly recommend anyone writing a login system read that article, even if they are not using PHP.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.