How many security problems does this login script have?

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

How many security problems does this login script have?

Post by matthijs »

As was discussed in this thread, a big problem with PHP security is that many articles don't pay enough attention to security. So I just found this snippet of code for a login-script, and as an exercise I'd like to put it to the test here. I could have placed it in the coding critique section, but as it's so much security related I thought it would fit in here.

Code: Select all

<?

// Use session variable on this page. This function must put on the top of page.
session_start();

////// Logout Section. Delete all session variable.
session_destroy();

$message="";

////// Login Section.
$Login=$_POST['Login'];
if($Login){ // If clicked on Login button.
$username=$_POST['username'];
$password=$_POST['password'];

// Connect database.
$host="localhost"; // Host name.
$db_user=""; // MySQL username.
$db_password=""; // MySQL password.
$database="tutorial"; // Database name.
mysql_connect($host,$db_user,$db_password);
mysql_select_db($database);

// Check matching of username and password.
$result=mysql_query("select * from admin where username='$username' and password='$password'");
if(mysql_num_rows($result)!='0'){ // If match.
session_register("username"); // Craete session username.
header("location:main.php"); // Re-direct to main.php
}else{ // If not match.
$message="--- Incorrect Username or Password ---";
}

} // End Login authorize check.
?>

<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Untitled Document</title>
</head>

<body>
<? echo $message; ?>
<form id="form1" name="form1" method="post" action="<? echo $PHP_SELF; ?>">
<table>
<tr>
<td>User : </td>
<td><input name="username" type="text" id="username" /></td>
</tr>
<tr>
<td>Password : </td>
<td><input name="password" type="password" id="password" /></td>
</tr>
</table>
<input name="Login" type="submit" id="Login" value="Login" />
</form>
</body>
</html>
So I'll start. First thing I see: missing db escape, therefore risking sql injection.

Code: Select all

$result=mysql_query("select * from admin where username='$username' and password='$password'");
Second thing I see, missing escape for html output:

Code: Select all

<? echo $message; ?>
There's more of course, but I'll leave the rest to you guys. How many flaws can we spot?
User avatar
DaveTheAve
Forum Contributor
Posts: 385
Joined: Tue Oct 03, 2006 2:25 pm
Location: 127.0.0.1
Contact:

Post by DaveTheAve »

All my login scripts have a captcha and a login monitoring system. Basically all the monitoring system does is you can only login once from an ip once every 30 seconds, if you fail to enter the right password three times or you submit your account & password without waiting 30 between trails, you get banned for one hour. Complex I know.
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Re: How many security problems does this login script have?

Post by timvw »

ini_set('error_reporting', E_ANAL); // Going to mention all the issues i can come up with (not only security related).
matthijs wrote:

Code: Select all

// Use session variable on this page. This function must put on the top of page.
session_start();
This one bits if you include it from another file where you've already called session_start... I usually prefer to write:

Code: Select all

if (!isset($_SESSION)) { session_start(); }
matthijs wrote:

Code: Select all

////// Logout Section. Delete all session variable.
session_destroy();
Why do you logout by default? Anyway, you might want to read the documentation on the first example at http://be.php.net/session_destroy (about cookies propagating the session id).
matthijs wrote:

Code: Select all

////// Login Section.
$Login=$_POST['Login'];
if($Login){ // If clicked on Login button.
$username=$_POST['username'];
$password=$_POST['password'];
Use array_key_exists (or a friend) in order to verify the values exist...
matthijs wrote:

Code: Select all

// Connect database.
$host="localhost"; // Host name.
$db_user=""; // MySQL username.
$db_password=""; // MySQL password.
$database="tutorial"; // Database name.
mysql_connect($host,$db_user,$db_password);
mysql_select_db($database);
What happens if the connection could not be established?
matthijs wrote:

Code: Select all

$result=mysql_query("select * from admin where username='$username' and password='$password'");
$username and $password are not prepared for use in a MySQL query (http://www.php.net/mysql_real_escape_string).

Since you're only interested in the number of found rows, not in the data that exists in the rows i would recommend to SELECT COUNT(username) AS count instead..

matthijs wrote:

Code: Select all

session_register("username"); // Craete session username.
That's old-style php. Read the documentation: http://www.php.net/session_register.

I presume that the setting of the username in the setting leads to a 'higher' access level.. In this case i would recommend to call session_regenerate_id in order to avoid session stealing attacks (i believe the correct name is 'stealth attacks').
matthijs wrote:

Code: Select all

header("location:main.php"); // Re-direct to main.php

Code: Select all

[/quote]

Afaik the HTTP protocol only supports absolute URIs for redirection... you also may want to call session_write_close to make sure the session data is flushed to disk... (And perhaps php should output a 303 status code instead of 302... but that's another matter )

[quote="matthijs"]

Code: Select all

<form id="form1" name="form1" method="post" action="<? echo $PHP_SELF; ?>">

Code: Select all

[/quote]

$PHP_SELF is oldstyle for $_SERVER['PHP_SELF']... Anyway this variable has not been prepared for use in a html context... And is known as a variable that can be easily exploited for XSS attacks... Using the action='#' instead seems like a good suggestion.
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

Great list Tim. Didn't know about ini_set('error_reporting', E_ANAL); but I might use it in the future :)

To be clear: I did not write this script, I just got it from a tutorial today as an example of what we discussed in the other thread about lack of security awareness. Sorry if I wasn't clear.

One other thing I noticed about the code is the use of shorttags <? instead of <?php.

Furthermore, no hashing is used on username /password. Use of at least an md5() would be a good idea, in case the db gets hacked.
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

matthijs wrote:To be clear: I did not write this script, I just got it from a tutorial today as an example of what we discussed in the other thread about lack of security awareness. Sorry if I wasn't clear.
It's just easier to talk to someone when i'm commenting on something ;) (I was aware you didn't write that code... I think most regulars at phpdn don't write code like that ;))
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

It's ok :) Thanks for your input.
User avatar
Oren
DevNet Resident
Posts: 1640
Joined: Fri Apr 07, 2006 5:13 am
Location: Israel

Post by Oren »

Very poorly written code, so sad :cry:
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

But very common... (And yes, there used to be a time that i was proud if i wrote code like that...)
Z3RO21
Forum Contributor
Posts: 130
Joined: Thu Aug 17, 2006 8:59 am

Post by Z3RO21 »

With my login systems I monitor users, log actions, and use a security system that checks for sql injection like queries, monitor login attempts (prevent brute force), and monitor for forgery in script elements (inserting certain data).
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

And what do you do with the logs?
Z3RO21
Forum Contributor
Posts: 130
Joined: Thu Aug 17, 2006 8:59 am

Post by Z3RO21 »

Store them, I needed to add the logs because I am doing a CMS for my school. The IT person wants to be able to see what people are doing.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: How many security problems does this login script have?

Post by Mordred »

timvw wrote:
matthijs wrote:

Code: Select all

////// Logout Section. Delete all session variable.
session_destroy();
Why do you logout by default?
To avoid session fixation.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Re: How many security problems does this login script have?

Post by John Cartwright »

Mordred wrote:
timvw wrote:
matthijs wrote:

Code: Select all

////// Logout Section. Delete all session variable.
session_destroy();
Why do you logout by default?
To avoid session fixation.
Typically when any account actions are taken, session_regenerate_id() is used to avoid fixation.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Oh, nice, thanks ;)
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

For a second i thought i forgot to talk about that ;)
timvw wrote: I presume that the setting of the username in the setting leads to a 'higher' access level.. In this case i would recommend to call session_regenerate_id in order to avoid session stealing attacks (i believe the correct name is 'stealth attacks').
Post Reply