Is my login script safe using session variables

[Noppy]

Hi,

i am creating a login script for my site and was hoping someone would be nice enough to have a look at my script and check i am not missing an important security hole. Basically is it safe once i have checked the cookie variables against the DB to set a session variable that is used while they are logged in, rather than having to query the DB each page load.


here is the code that does the checking to see if a cookie is already remembering the password to use to check against the DB.

hope this makes sense, any help/suggestions much appreciated

<?php
//Let's check if there is a cookie set with password
if(isset($_COOKIE['username']) && isset($_COOKIE['password']) ){
    
    //Check if there is a session set so we don't need to check the database otherwise do a database check
    if($_SESSION['logged'] !== 'user' || $_SESSION['logged'] !== 'member' || $_SESSION['logged'] !== 'admin'){

        
        //lets get the variables
        $username = mysql_real_escape_string($_COOKIE['username']);
        $password = mysql_real_escape_string($_COOKIE['password']);
        
        //lets do a query on the database now we know they are wanting to login
        $query_login = mysql_query("SELECT * FROM users WHERE username = '$username' and password = '$password'")or die(mysql_error()); 
        //get the results for the login
        $row_login = mysql_fetch_array($query_login);
        
        $total_login = mysql_num_rows($query_login);
        if($total_login == 0){
                //it failed so we set an error message
        $_SESSION['unwelcome'] = 'username and/or password not recognised';
        $_SESSION['welcome'] = '<a href="login.php">Login</a>';    
    }
    else{
        //it worked and we have a match so set a welcome message
                $_SESSION['welcome'] = '<H4>Welcome '.$row_login['username'].' - <a href="logout.php">log out</a></h4>';
        $_SESSION['unwelcome'] = '';    
        //ok so they are good lets set a session variable to allow certain bits to be visible
        //options user,member,admin
        $_SESSION['logged'] = $row_login['type'];
    }


        
    }
}
else {
//no cookie was found so we just ask them to log in
$_SESSION['welcome'] = '<H4><a href="login.php">Login</a></h4>';
}

?>

then on my pages i just have something simple like

<?php echo $_SESSION['welcome'].' '.$_SESSION['unwelcome'];?>
<h1>hello page</h1>
<?php if($_SESSION['logged'] == 'user'){ ?>
<p>some text only registered people see</p>
<?php ;}
elseif($_SESSION['logged'] == 'admin'){ echo 'lets do some admin';}
else{?>
<p>Log in to see extra stuff</p>
<?php }?>

[Serenarules]

I don't see anything syntactically wrong here, but I would make a few suggestions and comments.

1) RE: "if($total_login == 0)" What if the row count is 3? A better check would be against the actual expected value of 1, failing on anything else.
2) I don't see any usage of the md5/sha256 hashing methods in your call to check password. Are you storing the passwords in plain text in the cookie? If so, this is not very secure. Other sites can read cookies. I would store the password in the DB as a sha256 salted hash. Also store the salt. In the cookie, store only the hashed pass. Fetch the user by username only, rehash the cookie's stored password, then compare.

[Noppy]

great thanks for having a look. I'll look at changing the total_login number as what you've pointed out makes sense.

Yep definitely Hashing the password with a salt. just haven't added that bit in yet as i would only confuse myself whilst getting the basic parts to work

thanks much appreciated

[cpradio]

2) I don't see any usage of the md5/sha256 hashing methods in your call to check password. Are you storing the passwords in plain text in the cookie? If so, this is not very secure. Other sites can read cookies. I would store the password in the DB as a sha256 salted hash. Also store the salt. In the cookie, store only the hashed pass. Fetch the user by username only, rehash the cookie's stored password, then compare.

This might be a mis-understanding (as I haven't had my caffeine yet), but are you saying to store the hash in the cookie (without the salt taken into consideration)? As I'd argue, that is just as bad as storing it plain text in the cookie anymore.

Wouldn't you store the salted hash in the cookie and then you can compare it with the username in your query (since your password column should contain the salted hash too)?

I guess I am not seeing why you would need to rehash the cookie's stored password, as the database should have the sha256 salted hash and the cookie should have stored that same value too.

[Noppy]

best get this bit right - i was going to do this

$salt = "whatever1234";


 setcookie('password', md5($salt.$_POST['password']), time()+60*60*24*365,'/','www.mysite.org'); 

i think i was going to insert the same hashed salted password into my password column. is this the correct thing to do?

thanks

[cpradio]

i think i was going to insert the same hashed salted password into my password column. is this the correct thing to do?

Yes, that is what I would have done, and then your SQL could remain AS IS. But I'm still curious if I missed something that @Serenarules ; was implying.

[Serenarules]

This might be a mis-understanding (as I haven't had my caffeine yet), but are you saying to store the hash in the cookie (without the salt taken into consideration)? As I'd argue, that is just as bad as storing it plain text in the cookie anymore.

Wouldn't you store the salted hash in the cookie and then you can compare it with the username in your query (since your password column should contain the salted hash too)?

I guess I am not seeing why you would need to rehash the cookie's stored password, as the database should have the sha256 salted hash and the cookie should have stored that same value too.

Not exatly. Let me elaborate.

First, I wouldn't use a global salt. If guessed, all your users base are belong to me. Sorry, bad joke. =)

I prefer using unique salts, generated at the time of registration, store that in the users record. Not in the cookie. Never in the cookie.

When logging in, the record is fetched by username only. Then whatever he input for his password is rehashed using the stored salt (from above) and compared.

If login is successful, the hashed password and username is stored in the cookie like normal.

The reason this setup is good is because it will accomodate both global and per-user salts.

I apologize for the unclarity. I must be off my game today, I've had to clarify a few things I've said so far. LOL

[cpradio]

Not exatly. Let me elaborate.

First, I wouldn't use a global salt. If guessed, all your users base are belong to me. Sorry, bad joke. =)

I prefer using unique salts, generated at the time of registration, store that in the users record. Not in the cookie. Never in the cookie.

Okay, I agree with that.

When logging in, the record is fetched by username only. Then whatever he input for his password is rehashed using the stored salt (from above) and compared.

Agree with you there too

If login is successful, the hashed password and username is stored in the cookie like normal.

Just a bit more clarity, the salted hashed password and username is stored in the cookie, correct? So when they revisit you can compare the values in the cookie against the values in the database (they should match equally without any additional work).

I apologize for the unclarity. I must be off my game today, I've had to clarify a few things I've said so far. LOL

No worries, just wanted to make sure you were implying what I was hoping but wasn't necessarily reading

[Serenarules]

Just a bit more clarity, the salted hashed password and username is stored in the cookie, correct? So when they revisit you can compare the values in the cookie against the values in the database (they should match equally without any additional work).

Correct. =)

However, if you REALLY want to get into security, you could go all out and implement a ticket system.

Read on for ticket system.

Upon successful login, cache browser type/ver, client ip, time of login, user ID, and generate a ticket id.

Store ticket ID only in the cookie. Do not even need username!

Upon page refresh, lookup the ticket, compare current browser type/ver, client ip, check time of login for timeout status (reset if required), then, if all pass, just fetch the user's record via user ID.

No need to mess with messy passwords. Even if a ticket cookie is stolen, it's properties won't match up against what the thief is using.

In addition, you could seed each FORM based page with a one-up authorization code, to be compared upon POST. Both the ticket cookie and the posted auth code must be correct in order to process.

But this is only if you really want to go all-the way.

[Noppy]

thanks, this is all very useful info. Not sure i need to be super secure as not storing any sensitive info but may just give unique salts a go to be on the safe side and future proof

thanks for looking over my code

[logic_earth]

You shouldn't even put the hashed version of a password into a cookie. What you need is a token, one that is continually regenerated after periods of activity from the user.

[kduv]

I personally wouldn't use md5 or sha1. Neither hashing method was meant for passwords ... but instead for speed which makes brute force attacks much easier. I instead prefer to use mcrypt VIA PHPass. It also takes care of all your salting and stretching needs.

Here's a great article on password security: http://www.openwall.com/articles/PHP-Users-Passwords