Best practice for login-script?

I currently have the current login-script for logging in user on a webpage:

<?php
session_start();
include ('db_login.php');

//information gathered from the login-form
$user_name=mysql_real_escape_string($_POST['user_name']);
$user_pass=mysql_real_escape_string(md5($_POST['user_pass']));

$result_login_check=mysql_query("SELECT * FROM $members WHERE username='$user_name' AND user_password='$user_pass' AND (group_id='5' OR group_id='7' OR group_id='2')");
$user_count=mysql_num_rows($result_login_check);

$ua_login = mysql_fetch_array($result_login_check,MYSQL_BOTH);
$gid=$ua_login['group_id'];

if($user_count==1) {
$_SESSION['user_name']=$user_name;
$_SESSION['user_pass']=$user_pass;
$_SESSION['groupid']=$gid;
header("location: $_SERVER[HTTP_REFERER]");
}
else {
$_SESSION['error']="Feil i login";
header("location: $_SERVER[HTTP_REFERER]");
}
?>

PHP:

I'm just wondering if this is the "correct way" to do this? This php-file takes the values posted by the user in the login-form, processes those values, and logs in/denies the user, depending.

Now, the question I have is - is this a secure way? Not secure? If not, why? If not, how can I rectify this, and have a secure way of logging in?

 

Wherever you insert the password make sure it goes through md5, ie: md5($_POST['password']);

 

What's the point of storing the password in the session variable? On many setups, that leaves it vulnerable to being read by other PHP scripts.

You aren't checking mysql_num_rows() until after trying to fetch a result row, that seems the wrong way 'round.

And it's not a security issue (well it might be, I don't know what else goes on in your code), but using redirects to dance around during the login process seems to be unnecessarily precarious. Why not just handle the login on the same page the user navigated to?

 

Hey. Yeah, you are absolutely right - the password stored in session is a left-over from earlier days, and is not needed for anything. I've changed the code as follows:

<?php
session_start();
include ('db_login.php');

//information gathered from the login-form
$user_name=mysql_real_escape_string($_POST['user_name']);
$user_pass=mysql_real_escape_string(md5($_POST['user_pass']));

$result_login_check=mysql_query("SELECT * FROM $members WHERE username='$user_name' AND user_password='$user_pass' AND (group_id='5' OR group_id='7' OR group_id='2')");
$user_count=mysql_num_rows($result_login_check);
if($user_count==1) {
$ua_login = mysql_fetch_array($result_login_check,MYSQL_BOTH);
$gid=$ua_login['group_id'];
$_SESSION['user_name']=$user_name;
$_SESSION['groupid']=$gid;
header("location: $_SERVER[HTTP_REFERER]");
}
else {
$_SESSION['error']="Feil i login";
header("location: $_SERVER[HTTP_REFERER]");
}
?>

PHP:

The referer is basically just to handle the return to whatever page the user choses to log in on - I know it isn't a 100% way of doing it, but it seemed the easiest way to do it. I could probably handle the whole login/out part on the same page, but I haven't really looked into changing the script to handle that, and while it might not be a lot of work, it's not a priority right now. Might change that later, though.

(When I think about it, I think perhaps the reason why it is made like that is because there was some issues using the "header:"-statement on an earlier server)

 

Can i just suggest you salt all passwords etc in the database for extra security.

<?php
define('SALT','s0m,3_compl35ety-ran10k3m');

$password = md5($whatever . SALT);
?>

Code (markup):

 

Adding on to Agent Smith's suggestion, perhaps you can even do a:

<?php
$password_to_insert_to_db = md5($password_string . md5($salt));
?>

PHP:

Salt has to be constant though. To check:

$sql = "blah blah blah... AND password = '" . md5($_POST['password'] . md5($salt)) . "'";

PHP:

 

Currently salting the password is not an option

It has to do with it being parsed by a separate part of the webpage too, and using another system for managing and creating user/passwords. Until I've spent some time fixing that part, there won't be any possibilities for changing the way the password is generated.

This isn't really top security or anything, so... how "bad" is it to not add salt to a password encrypted with a md5 hash? I know that md5 has been broken, but how high are the chances someone doing a random attack would be able to break this?

 

Basically, md5 is still irreversible. But it doesn't stop people. I cant remember the name of this process:

So yeah, i md5 hash my password which is "pass", i then post it on a forum for whatever reason. Google caches it.

If i was a hacker, i would search google for a random md5 string, say i registered at your site with the same password, he searches google with the md5 hash and there you go. He has the unhashed password.


However, if you add a salt to it which is not known and completely random, your safe. As all unhashed strings will be different from the normal md5.

Such sites are http://gdataonline.com/ with 168,678,430 unhashed strings. So please, hash them!

 

Ok, so one basically have to have some look-up table for registered hashes to be able to break it - that means it is basically "bad" passwords that are most likely to be cracked. Random passwords with "secure" password creation enforced is probably not so likely to be broken by a hash-lookup.