Page 1 of 2

Are there any serious flaws in this?

Posted: Tue Feb 06, 2007 9:46 am
by dhrosti
Iv just written a login page for a small site im building and i just wanted to check with the pros if its alright to use...

Code: Select all

require('inc/db_cnx.php');
include('inc/function_validate.php');

if (isset($_POST['submit'])) {
	$company = $_POST['company'];
	$passcode = $_POST['passcode'];
	// Account for servers timezone
	$H = date('H')-1;
	$date = date('Y-m-d '.$H.':i');
	
	validate($passcode);
	validate($company);
	
	$company = addslashes($company);
	$query = "SELECT passcode, client_id, access FROM details WHERE company='$company'";
	$q = mysql_query($query);
	
	// Check if company is registered
	if (mysql_num_rows($q) > 0) {
		$a = mysql_fetch_array($q);
		if ($a['passcode'] == $passcode) {
			// Update last_login
			$sql = "UPDATE details SET last_login='$date' WHERE company='$company'";
			$qe = mysql_query($sql);
			// If company name should be remembered
			if ($_POST['remember'] == 1) {
				setcookie("company", stripslashes($company), +15552000); 
			}
			// check access
			if ($a['access'] == "Admin") {
				session_start();
				$_SESSION['id'] = $a['client_id'];
				$_SESSION['access'] = "admin";
				$_SESSION['login'] = 1;
				header("Location: overview.php");
			} else {
				session_start();
				$_SESSION['id'] = $a['client_id'];
				$_SESSION['access'] = "client";
				$_SESSION['company'] = stripslashes($company);
				$_SESSION['login'] = 1;
				header("Location: download.php");
			}
		} else {
			die("Login Error");
		}
	} else {
		die("$company is not registered, please contact us.");
	}
}
Thanks in advance.

Posted: Tue Feb 06, 2007 10:19 am
by Charles256
cann i see the validate function?

Posted: Tue Feb 06, 2007 10:26 am
by dhrosti
sure...

Code: Select all

function validate($string) {
	if (!preg_match("#[_a-zA-Z0-9.\'-]#s", $string)) {
		die('<p>Invalid input.</p><p><a href="javascript: history.go(-1)">Back</a></p>');
	} else {
		return $string;
	}
}
its pretty basic but its stops dangerous characters (i think)

Posted: Tue Feb 06, 2007 1:00 pm
by Buddha443556
Use mysql_ real_ escape_ string() instead of addslashes().

Header location should be an absolute URL as require by HTTP 1.1.

How does the "remember me" work? All cookies beyond the session id deserve extra scrutiny. I'm already against this cookie because it contains the username.

Posted: Tue Feb 06, 2007 6:22 pm
by WaldoMonster
Buddha443556 wrote:Header location should be an absolute URL as require by HTTP 1.1.
Is this also required for JavaScript window.location?

Code: Select all

<script type="text/javascript">window.location="index.php";</script>

Posted: Tue Feb 06, 2007 6:44 pm
by wildwobby
WaldoMonster wrote:
Buddha443556 wrote:Header location should be an absolute URL as require by HTTP 1.1.
Is this also required for JavaScript window.location?

Code: Select all

<script type="text/javascript">window.location="index.php";</script>
I wouldn't think so because I don't think it is sending raw headers.

Re: Are there any serious flaws in this?

Posted: Wed Feb 07, 2007 12:53 am
by timvw
- You are using $_POST['company'] and $_POST['passcode'] before you have checked that they exist.
- You don't prepare $company for use in a MySQL query
- If you selected count(company) you wouldn't need to use mysql_num_rows, and pass in the $passcode, this way you can remove the check in code
- $q seems like an odd name for a variable that holds a resultset
- You use $_POST['remember'] before you have checked if the variable exists.
- header('Location' with a relative url might lead to problems in some browsers, making a call to session_write_close before the header might prevent potential problems too...
- You have not prepared the companyname for use in html, so you're open for XSS and other nice stuff :P
- I Wonder why you want the script to die?

Posted: Wed Feb 07, 2007 3:08 am
by dhrosti
Thanks for all the feedback, much appreciated.

Posted: Wed Feb 07, 2007 3:28 am
by dhrosti
Buddha443556 wrote:Use mysql_ real_ escape_ string() instead of addslashes().

Header location should be an absolute URL as require by HTTP 1.1.

How does the "remember me" work? All cookies beyond the session id deserve extra scrutiny. I'm already against this cookie because it contains the username.
Would it be better for the cookie to store a unique ID referring to the client instead?

Posted: Wed Feb 07, 2007 11:30 am
by daedalus__
WaldoMonster wrote:Is this also required for JavaScript window.location?

Code: Select all

<script type="text/javascript">window.location="index.php";</script>
I don't think so but I use absolute URLs, anyway.

Posted: Wed Feb 07, 2007 2:18 pm
by WaldoMonster
Thanks wildwobby and Daedalus-

Posted: Wed Feb 07, 2007 3:24 pm
by RobertGonzalez
Why is the call to session_start() inside of a conditional? If either one is going to call session_start(), why not call it to begin with?

You have no error checking/trapping on your calls to mysql_query(). If you attempt to read a result identifier that returned false, you are going to generate all sorts of warnings/notices/errors.

Shortened variable names are enough to make a maintainer want to puke on your shoes. Use descriptive variable names. This is not a performance issue, but it is certainly a code-cleanliness and maintenance issue.

You should be checking for isset($_POST['var_name']) instead of just throwing them into conditional, lest you want to see undefined index errors.

The validate function returns a value, yet you call it without an assignment into something.

Code: Select all

<?php
validate($passcode);
validate($company);
?>
Should be:

Code: Select all

<?php
$passcode = validate($passcode);
$company = validate($company);
?>
die() is typically not a good form of error handling.

All in all, will it work? Probably. Can it be improved? Definitely.

Posted: Wed Feb 07, 2007 10:03 pm
by Buddha443556
dhrosti wrote:Would it be better for the cookie to store a unique ID referring to the client instead?
Probably not, especially since the session id pretty much serves this purpose already. :wink:

I don't see it mentioned above but you should use session_ regenerate_ id() to avoid session fixation exploitation (say that three times fast :lol: ).

Posted: Thu Feb 08, 2007 4:16 am
by Mordred
The validate() function is just horrible!
If the string is not okay, the script dies, whereas if it is okay, it is returned, which doesn't do much though, as the caller is not assigning the returned value. Ah, wait, Everah already wrote about that.

I hope that in overview.php and download.php you also check the user credentials, otherwise the whole purpose of the script will be defeated.

Posted: Thu Feb 08, 2007 5:21 am
by dhrosti
Yea iv tidied up that function since posting, it wasnt nice. It now returns 1 or 0 and is set into a variable. I think thats a better way to do things. Thanks for all the info though guys, your a lot of help!

Code: Select all

function validate($string) {
	if (preg_match("/^[\W]+[^,\.\'\"]*$/s", $string)) {
		return 1;
	} else {
		return 0;
	}
}