PHP Sessions and Security (Login Script) - Expert's Help Needed URGENT!

[BlackTyger]

If you are a PHP expert, then I really your help. I have a question regarding PHP sessions and their security. So here is my story ...

 

I created a login script (login.php) for my website. When a user goes to the login.php page, they see a login form that they must fill with their username and password to login to the members' area and view their profile, etc.

 

On that login page, when the user enters their username and password and then clicks the "Login" button, my script filters the data, sends MySQL query and checks if the login is valid. If the login is NOT valid, then they get a "Login Failed" message. If the login is valid, I register their username and the password in sessions and redirect them to the members.php page.

 

Here is some of my code for my login.php page after mysql confirms the login is valid

 

<?php

$query = mysql_query('SELECT * FROM `users` WHERE username='$user' AND password='$pass'");
$numRows = mysql_num_rows($query);

if ( $numRows ) {
     
      // login is valid
      $_SESSION['username'] = $user;
      $_SESSION['pass'] = $pass;

      // redirect user to members area
      header('Location: /members.php');

} else {
     
       // login is invalid
       echo "Login failed";

}

?>

 

My question is ... is this login script secured? I mean, I am not generating any session id or any cookie. I am just storing the username and the password in two session variables and those are the things that i will use to display the user's profile, etc. Can attackers attack this script? Is this secured or is there any other way I can make it stronger?

[gizmola]

There's a couple of obvious issues.  The first is that I don't see you filtering user input.  That is the #1 thing you need to do here, or alternatively you could use mysqli with bind variables.  Currently you would be vulnerable to a simple SQL injection exploit.

 

Secondarily, your members.php would need to check for a valid $_SESSION['username'] or redirect back to your login form, because people can figure out that you're redirecting to members.php, and just use that url directly.

 

I also don't see where you call session_start() or where you check that the value of $_SESSION['username'] is already set.

 

Otherwise, I don't see any major problems with your snippet.  However, you should be aware of a couple of things:

 

Sometimes people will do :

 

if (isset($_SESSION['username'])) {
  //logged in
} else {
  header('Location: login.php');
}
// Do other stuff for member.

 

It's important to follow up the header('Location:') calls with a die() so that people don't attempt to use tools that can selectively refuse to follow the Location redirect, and thus continue to execute code and fall through.  Even in your example, having a die() wouldn't be bad. 

 

The other obvious thing to mention is that the username and pw will be sent across the network from the user's browser in cleartext, and is susceptible to sniffing.  The way around that is to have things like login redirect to https, but some sites simply choose to live with the possibility that an individual user will be compromised.

[KevinM1]

Some other general tips:

 

I don't see why you'd want to store the user's password in a session.  They're already logged in, right?

 

Passwords should be stored as a hashed value in the db.

 

For even tighter password security, use a salt.

 

Whenever a user does something important/critical, regenerate their session id (session_regenerate_id).

 

MySQLi with prepared statements is really the way to go.

[BlackTyger]

Okay, friends! I forgot to mention some stuff ....

 

The code that i posted above is just part of the actual code. Yes, I have data filtering in my actual code and all mysql filtering, too. My login and members page both have session_start(); at the top.

 

And yes, the members.php file checks if the $_SESSION['username'] is registered or not. if it's not, then it sends the user back to the login page. I was worried about session stealing and fixation and session hijacking.

 

Is there any way an attack can hijack my users' sessions and control their accounts? I read about session id hijacking on php.net and got worried.

[gizmola]

Nightslyr makes a really good point, which is there is no reason to store the password in the session, although there's also no big danger there, as the session data lives on the server.  I do however agree with his point, that what you don't need, you shouldn't have stuffed in there.

 

The general rule of thumb for avoiding session fixation is that you regenerate the session id anytime there's an escalation of privilege.  The default session id is an md5() hash which for most people offers sufficient unpredictability, but if you want to do something different, like your own custom session id, you can do that in your code.  Seems you already are familiar with the manual, but I'd recommend just going through all the functions in the session section of the php manual.

 

The session id is going to be stored as a cookie, so if people can get access to someone's machine or cookie then it's always possible to masquerade as them.  That is why you will often see that (along with the session id regeneration) there will be reprompts for authentication at places where people might change account information (like change password, email etc.) where you will see a prompt for the original password there.

 

At any rate read up on:  http://us3.php.net/manual/en/function.session-regenerate-id.php

[BlackTyger]

Okay, thanks a million for your great help. I am kinda confused with this session id stuff. Can you please explain what do I need to do to regenerate an ID. I mean, do I just place the session_regenerate_id() anywhere in my code or are there any serious changes that i need to make?

[KevinM1]

Okay, thanks a million for your great help. I am kinda confused with this session id stuff. Can you please explain what do I need to do to regenerate an ID. I mean, do I just place the session_regenerate_id() anywhere in my code or are there any serious changes that i need to make?

 

You just call the function.