Secure Login Code

[fean0r]

Hi,

I was hoping someone could check if this login code is secure?

Please let me know what you think!

Thanks

# Upper
$upass=strtoupper($_REQUEST["username"]);
$uname=strtoupper($_REQUEST["password"]);
# Security
if(get_magic_quotes_gpc()) { $upass = stripslashes($upass); $upass = mysql_real_escape_string($upass); }
else { c4error('Invalid Login String - Please click back and try again!'); }
if(get_magic_quotes_gpc()) { $uname = stripslashes($uname); $uname = mysql_real_escape_string($uname); }
else { c4error('Invalid Login String - Please click back and try again!'); }
# Check DB
$items = mysql_query("SELECT * FROM jusers WHERE uname='".$uname."' AND upass='".$upass."'") or die(mysql_error());
$myrow = mysql_fetch_assoc($items);
if (!$myrow['ID']) { c4error('Invalid Login Details - Please click back and try again!'); }

c4error is just an exit; a nice display with exit; at the end.

[laserlight]

It has a number of problems:

• You did not check if $REQUEST["username"] and $REQUEST["password"] exist before using them.

• You require that magic_quotes_gpc be turned on, but good practice dictates that this setting be turned off. Furthermore, the error message that is printed is incorrect: it is not about an invalid login string, but about a PHP configuration setting.

• You probably want to select a count instead of selecting everything in all the columns. Related to this, if you do want to select the ID column, then you should check the count of number of rows returned instead of using $myrow, which could be false instead of an array.

Other than that, you really should try to indent your code a better: placing many statements on the same line makes it hard to follow the flow of control in the code.

Now, we can talk about security: you should use SSL/TLS if you are really concerned about security. You should protect your users' passwords by using a user specific salt and cryptographic hash algorithm. That said, your code is on the right track for avoiding SQL injection, once you have fixed the problems that I have mentioned.

[fean0r]

Thanks Laser,

I do have a check on the pas and user sorry I should have posted this before!

I have update the error message!

How can I avoid using magic_quotes_gpc ?
Can I just go with $upass = stripslashes($upass); and $upass = mysql_real_escape_string($upass); ?

Thanks

[laserlight]

fean0r wrote:

How can I avoid using magic_quotes_gpc ?
Can I just go with $upass = stripslashes($upass); and $upass = mysql_real_escape_string($upass); ?

If you must cater to the possibility of magic_quotes_gpc being on, you could do it in this way:

if (isset($_REQUEST["username"], $_REQUEST["password"]))
{
    // Normalise to all uppercase.
    $upass = strtoupper($_REQUEST["username"]);
    $uname = strtoupper($_REQUEST["password"]);

// Remove backslashes if necessary.
if (get_magic_quotes_gpc())
{
    $uname = stripslashes($uname);
    $upass = stripslashes($upass);
}

$sql = sprintf("SELECT COUNT(*) FROM jusers WHERE uname='%s' AND upass='%s'",
    mysql_real_escape_string($uname),
    mysql_real_escape_string($upass));
$result = mysql_query($sql)
    or die(mysql_error()); // debugging purposes only

if (mysql_result($result, 0) == 1)
{
    // login successful
}
else
{
    // invalid credentials
}
}
else
{
    // credentials not supplied
}

[fean0r]

Thanks for the advise and example code Laser - I have used that method.

# Sessions
session_start();
header("Cache-control: private");
# Password
session_register("password");
$_SESSION['password']=md5($upass);
# Userid
session_register("user");
$_SESSION['user']=$uname;

I use sessions to keep the user logged in, then I check the sessions on pages were I need to auth the user - is that the best way to do it?

[laserlight]

fean0r wrote:

I use sessions to keep the user logged in, then I check the sessions on pages were I need to auth the user - is that the best way to do it?

There is no point maintaining login credentials (as in username and password combination) in the session since the session identifier is sufficient to authenticate the user without having to check the database. What you should do is to regenerate the session identifier so as to protect against session fixation.

By the way, if you intend the user to use a form to login, you should use $POST['username'] and $POST['password'] instead of $REQUEST["username"] and $REQUEST["password"] respectively. There is no real increase in security, but it makes an attack slightly more difficult.

[fean0r]

laserlight;10914308 wrote:

By the way, if you intend the user to use a form to login, you should use $POST['username'] and $POST['password'] instead of $REQUEST["username"] and $REQUEST["password"] respectively. There is no real increase in security, but it makes an attack slightly more difficult.

Thanks - Updated!

laserlight;10914308 wrote:

What you should do is to regenerate the session identifier so as to protect against session fixation.

So for example I generate a random 10 digit code and add it to a session and a database then check they match when I want to check the user?

[laserlight]

fean0r wrote:

So for example I generate a random 10 digit code and add it to a session and a database then check they match when I want to check the user?

No, you should use [man]session_regenerate_id/man.

[fean0r]

Thanks again!

[effim]

You should really not be uppercasing passwords. You severely reduce the number of possible passwords when you do that because you reduce the number of unique possibilities for each character.

6 characters of 26 letters + 10 numbers = 2 billion possibilities.
6 characters of 52 letters (upper and lower) + 10 numbers = 56 billion possibilities.

It seems like a lot until you consider the fact that there are over 6 billion people in the world and an attacker could easily run 25 million attempts to login in a single month (only 10 a second) even over the web. If the password is susceptible to a dictionary attack, they're very likely to get in. Consider the fact that you eliminate the possibilities for 'giraffE', 'gIrAfFe', 'giraFFe', 'giRAFfe', etc. and instead allow them to just try a single 'GIRAFFE' and get in. 😉

Just for fun, there are 200 trillion possibilities using 62 allowed characters and an 8 character password length and 3 sextillion (1 billion trillions) possibilities for the same allowed characters with a length of 12 characters. Use some non-standard characters to boot (asterisks, commas, periods, math symbols, etc.) and you're super good to go...as long as you don't write your password on your monitor.

[fean0r]

Yea I already removed the upper on password as it was really only needed for the usernames but thanks for the advice I'll keep it in mind!

My main concern was sql injection and any other hole my coding might create.

[effim]

fean0r;10914708 wrote:

Yea I already removed the upper on password as it was really only needed for the usernames but thanks for the advice I'll keep it in mind!

My main concern was sql injection and any other hole my coding might create.

Well you're not protecting against SQL injection in your code because you're not escaping text based on the character encoding of the connection and database. You're asking for garbled characters and unforeseen security holes.

// connect to your database
$variable = mysql_real_escape_string($userInput);
// now perform your query passing $variable to your database.

Note that you must connect to the database BEFORE performing mysql_real_escape_string. This is because mysql_real_escape_string uses the character encoding of the current mysql connection to determine how to escape the content. Also, if you're using a $link to handle the mysql connection you'll need to pass that as the second argument to mysql_real_escape_string.

[laserlight]

effim wrote:

Well you're not protecting against SQL injection in your code because you're not escaping text based on the character encoding of the connection and database. You're asking for garbled characters and unforeseen security holes.

Good point, though I assumed that the database connection was implied, in which your statement is incorrect. In my opinion, this is yet another reason to use the PDO extension or MySQLi extension where the database connection object is required rather than optionally implicit.

[effim]

laserlight;10914711 wrote:

Good point, though I assumed that the database connection was implied, in which your statement is incorrect. In my opinion, this is yet another reason to use the PDO extension or MySQLi extension where the database connection object is required rather than optionally implicit.

I was more referring to the OP's original code than what you had drafted. Still, though, it's always good to point out to newly familiar users that the function is only secure when the appropriate MySQL connection is detectable.

I suppose my biggest argument against PDO is just not wanting to transition over to it. I just realized that PDO supports LOBs as data streams, which is wicked useful for some of the clients I work for (manipulating 100MB+ files through web interfaces). I just might switch over to using it, though I'll probably use it as an adapter via Zend_Db.

[Weedpacket]

fean0r wrote:

Yea I already removed the upper on password as it was really only needed for the usernames but thanks for the advice I'll keep it in mind!

Interestingly, the subject of capitalisation of usernames has come up in the past.