Are there any serious flaws in this?

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

User avatar
dhrosti
Forum Commoner
Posts: 90
Joined: Wed Jan 10, 2007 5:01 am
Location: Leeds, UK

Are there any serious flaws in this?

Post 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.
Charles256
DevNet Resident
Posts: 1375
Joined: Fri Sep 16, 2005 9:06 pm

Post by Charles256 »

cann i see the validate function?
User avatar
dhrosti
Forum Commoner
Posts: 90
Joined: Wed Jan 10, 2007 5:01 am
Location: Leeds, UK

Post 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)
User avatar
Buddha443556
Forum Regular
Posts: 873
Joined: Fri Mar 19, 2004 1:51 pm

Post 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.
User avatar
WaldoMonster
Forum Contributor
Posts: 225
Joined: Mon Apr 19, 2004 6:19 pm
Contact:

Post 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>
wildwobby
Forum Commoner
Posts: 66
Joined: Sat Jul 01, 2006 8:35 pm

Post 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.
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Re: Are there any serious flaws in this?

Post 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?
User avatar
dhrosti
Forum Commoner
Posts: 90
Joined: Wed Jan 10, 2007 5:01 am
Location: Leeds, UK

Post by dhrosti »

Thanks for all the feedback, much appreciated.
User avatar
dhrosti
Forum Commoner
Posts: 90
Joined: Wed Jan 10, 2007 5:01 am
Location: Leeds, UK

Post 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?
User avatar
daedalus__
DevNet Resident
Posts: 1925
Joined: Thu Feb 09, 2006 4:52 pm

Post 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.
User avatar
WaldoMonster
Forum Contributor
Posts: 225
Joined: Mon Apr 19, 2004 6:19 pm
Contact:

Post by WaldoMonster »

Thanks wildwobby and Daedalus-
User avatar
RobertGonzalez
Site Administrator
Posts: 14293
Joined: Tue Sep 09, 2003 6:04 pm
Location: Fremont, CA, USA

Post 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.
User avatar
Buddha443556
Forum Regular
Posts: 873
Joined: Fri Mar 19, 2004 1:51 pm

Post 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: ).
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post 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.
User avatar
dhrosti
Forum Commoner
Posts: 90
Joined: Wed Jan 10, 2007 5:01 am
Location: Leeds, UK

Post 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;
	}
}
Post Reply