Is this Form Secure?

mikemcleanuk

I have been doing some reading up on security of php forms and mysql insertion and such. As a result i have redesigned a form and below i have posted some of the code. Could some let me know if they think i have done all that is possible to ensure that the form is totally secure?

Are their any improvements possible and if so what are they?

<?php
include('session.php');

function mres($input) {
    return strip_tags(mysql_real_escape_string($input));
} 

if(isset($_POST['submit'])) 
{
	if (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token'])
	{
		$clean = array();
		$email_pattern = '/^[^@\s<&>]+@([-a-z0-9]+\.)+[a-z]{2,}$/i';

	$clean['username'] = ( isset( $_POST['username'] ) ) ? mres( $_POST['username'] ) : '';
	$clean['password'] = ( isset( $_POST['password'] ) ) ? mres(sha1( $_POST['password'] )) : '';
	$clean['password2'] = ( isset( $_POST['password2'] ) ) ? mres(sha1( $_POST['password2'] )) : '';
	if (preg_match($email_pattern, $_POST['email'])) 
	{ 
		$clean['email'] = mres($_POST['email']); 
	}
	if (preg_match($email_pattern, $_POST['email2'])) 
	{ 
		$clean['email2'] = mres($_POST['email2']); 
	}
	$clean['fname'] = ( isset( $_POST['fname'] ) ) ? mres( $_POST['fname'] ) : '';
	$clean['lname'] = ( isset( $_POST['lname'] ) ) ? mres( $_POST['lname'] ) : '';
	$clean['dobd'] = ( isset( $_POST['dobd'] ) AND ctype_digit( $_POST['dobd'] ) ) ? $_POST['dobd'] : 0;
	$clean['dobm'] = ( isset( $_POST['dobm'] ) ) ? mres( $_POST['dobm'] ) : '';
	$clean['doby'] = ( isset( $_POST['doby'] ) AND ctype_digit( $_POST['doby'] ) ) ? $_POST['doby'] : 0;
	$clean['sex'] = ( isset( $_POST['sex'] ) ) ? mres( $_POST['sex'] ) : '';
	$clean['country'] = ( isset( $_POST['country'] ) ) ? mres( $_POST['country'] ) : '';
	$clean['lordname'] = ( isset( $_POST['lordname'] ) ) ? mres( $_POST['lordname'] ) : '';
	$query = mysql_query("SELECT * FROM members WHERE username ='$clean[username]' ");
	$result = mysql_fetch_row($query);
	$query2 = mysql_query("SELECT * FROM resources WHERE lordname ='$clean[lordname]' ");
	$result2 = mysql_fetch_row($query2);

	if($clean['sex'] == "sexm")
	{
		$clean['sex'] = "male";
	}
	else
	{
		$clean['sex'] = "female";
	}

	if($result != 0)
	{
		$error = "Username taken please choose another";
	}
	else if(!($clean['password'] == $clean['password2']))
	{
		$error = "Your passwords do not match.";
	}
	else if(!($clean['email'] == $clean['email2']))
	{
		$error = "Your emails do not match.";
	}
	else if($result2 != 0)
	{
		$error = "Lordname taken please choose another";
	}
	else
	{		
		mysql_query("INSERT INTO mem 
				(id, username, user_password, fname, 
lname, email, dob, sex) 
			VALUES	('NULL','$clean[username]','$clean[password]','$clean[fname]',
'$clean[lname]','$clean[email]','$clean[dobd]-$clean[dobm]-$clean[doby]',
'$clean[sex]')
			") 
			or die(mysql_error()); 

		mysql_query("INSERT INTO res (lordname) VALUES	('$clean[lordname]')
		")		
		or die(mysql_error());

		echo"<p>Member Added";
	}
}
}

if(isset($error))
{
	echo"$error";
}

$token = md5(uniqid(rand(), true));
$_SESSION['token'] = $token;
?>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>Member Signup</title>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
</head>
<body>
<form name="login-form" id="login-form" method="post" action="signup.php">
	<fieldset>
		<legend>Member Signup</legend>
		<dl>
			<dt>
				<label>Username: <input name="username" type="text" maxlength="20" id="username" /></label>
			</dt>
		</dl>
		<dl>
			<dt>
				<label>Password: <input name="password" type="password" maxlength="20" id="password" /></label>
				<label>Repeat Password: <input name="password2" type="password" maxlength="20" id="password2" /></label>
			</dt>
		</dl>
		<dl>
			<dt>
				<label>E-Mail: <input name="email" type="text" maxlength="100" id="email" /></label> 
				<label>Repeat E-Mail: <input name="email2" type="text" maxlength="100" id="email2" /></label>
			</dt>
		</dl>
		<dl>
			<dt>
				<label>First Name: <input name="fname" type="text" maxlength="100" id="fname" /></label>
				<label>Last Name: <input name="lname" type="text" maxlength="100" id="lname" /></label>
			</dt>
		</dl>
		<dl>
			<dt>
				<label title="Username">Date of Birth: 
				<label for="day">Day</label>

			<select name="dobd" id="day">
				<option value="1" selected="selected">1</option>
				...
			</select>

			<label for="month">Month</label>
			<select name="dobm" id="month">
				<option value="January" selected="selected">January</option>
				....
			</select>

			<label for="year">Year</label>
			<select name="doby" id="year">
			<option value="2008" selected="selected">2008</option>
				....
			</select>
		</label>
		</dt>
	</dl>
	<dl>
		<dt>
			<label>Sex: Male<input name="sex" type="radio" id="sex" value="sexm" />Female<input name="sex" type="radio" id="sex" value="sexf" /></label>
		</dt>
	</dl>
	<dl>
		<dt>  
			<label for="country">Country</label>
				<select name="country" id="country">
				<option value="NONE" selected="selected">Please Choose One</option>
				.....
			</select> 
		</dt>
	</dl>
	<dl>
		<dt>
			<label title="Username">Lord Name: 
			<input name="lordname" type="text" maxlength="25" id="lordname" />
			</label>
		</dt>
	</dl>
	<dl>
		<dt>
			<input type="hidden" name="token" value="<?php echo $token; ?>" />
			<label title="Submit"><input type="submit" name="submit" value="Signup" /></label>
		</dt>
	</dl>
</fieldset>
</form>
</body>
</html>

mikemcleanuk

Are their any improvements possible and if so what are they?

<?php
include('session.php');

function mres($input) {
    return strip_tags(mysql_real_escape_string($input));
} 

if(isset($_POST['submit'])) 
{
	if (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token'])
	{
		$clean = array();
		$email_pattern = '/^[^@\s<&>]+@([-a-z0-9]+\.)+[a-z]{2,}$/i';

	$clean['username'] = ( isset( $_POST['username'] ) ) ? mres( $_POST['username'] ) : '';
	$clean['password'] = ( isset( $_POST['password'] ) ) ? mres(sha1( $_POST['password'] )) : '';
	$clean['password2'] = ( isset( $_POST['password2'] ) ) ? mres(sha1( $_POST['password2'] )) : '';
	if (preg_match($email_pattern, $_POST['email'])) 
	{ 
		$clean['email'] = mres($_POST['email']); 
	}
	if (preg_match($email_pattern, $_POST['email2'])) 
	{ 
		$clean['email2'] = mres($_POST['email2']); 
	}
	$clean['fname'] = ( isset( $_POST['fname'] ) ) ? mres( $_POST['fname'] ) : '';
	$clean['lname'] = ( isset( $_POST['lname'] ) ) ? mres( $_POST['lname'] ) : '';
	$clean['dobd'] = ( isset( $_POST['dobd'] ) AND ctype_digit( $_POST['dobd'] ) ) ? $_POST['dobd'] : 0;
	$clean['dobm'] = ( isset( $_POST['dobm'] ) ) ? mres( $_POST['dobm'] ) : '';
	$clean['doby'] = ( isset( $_POST['doby'] ) AND ctype_digit( $_POST['doby'] ) ) ? $_POST['doby'] : 0;
	$clean['sex'] = ( isset( $_POST['sex'] ) ) ? mres( $_POST['sex'] ) : '';
	$clean['country'] = ( isset( $_POST['country'] ) ) ? mres( $_POST['country'] ) : '';
	$clean['lordname'] = ( isset( $_POST['lordname'] ) ) ? mres( $_POST['lordname'] ) : '';
	$query = mysql_query("SELECT * FROM members WHERE username ='$clean[username]' ");
	$result = mysql_fetch_row($query);
	$query2 = mysql_query("SELECT * FROM resources WHERE lordname ='$clean[lordname]' ");
	$result2 = mysql_fetch_row($query2);

	if($clean['sex'] == "sexm")
	{
		$clean['sex'] = "male";
	}
	else
	{
		$clean['sex'] = "female";
	}

	if($result != 0)
	{
		$error = "Username taken please choose another";
	}
	else if(!($clean['password'] == $clean['password2']))
	{
		$error = "Your passwords do not match.";
	}
	else if(!($clean['email'] == $clean['email2']))
	{
		$error = "Your emails do not match.";
	}
	else if($result2 != 0)
	{
		$error = "Lordname taken please choose another";
	}
	else
	{		
		mysql_query("INSERT INTO mem 
				(id, username, user_password, fname, 
lname, email, dob, sex) 
			VALUES	('NULL','$clean[username]','$clean[password]','$clean[fname]',
'$clean[lname]','$clean[email]','$clean[dobd]-$clean[dobm]-$clean[doby]',
'$clean[sex]')
			") 
			or die(mysql_error()); 

		mysql_query("INSERT INTO res (lordname) VALUES	('$clean[lordname]')
		")		
		or die(mysql_error());

		echo"<p>Member Added";
	}
}
}

if(isset($error))
{
	echo"$error";
}

$token = md5(uniqid(rand(), true));
$_SESSION['token'] = $token;
?>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>Member Signup</title>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
</head>
<body>
<form name="login-form" id="login-form" method="post" action="signup.php">
	<fieldset>
		<legend>Member Signup</legend>
		<dl>
			<dt>
				<label>Username: <input name="username" type="text" maxlength="20" id="username" /></label>
			</dt>
		</dl>
		<dl>
			<dt>
				<label>Password: <input name="password" type="password" maxlength="20" id="password" /></label>
				<label>Repeat Password: <input name="password2" type="password" maxlength="20" id="password2" /></label>
			</dt>
		</dl>
		<dl>
			<dt>
				<label>E-Mail: <input name="email" type="text" maxlength="100" id="email" /></label> 
				<label>Repeat E-Mail: <input name="email2" type="text" maxlength="100" id="email2" /></label>
			</dt>
		</dl>
		<dl>
			<dt>
				<label>First Name: <input name="fname" type="text" maxlength="100" id="fname" /></label>
				<label>Last Name: <input name="lname" type="text" maxlength="100" id="lname" /></label>
			</dt>
		</dl>
		<dl>
			<dt>
				<label title="Username">Date of Birth: 
				<label for="day">Day</label>

			<select name="dobd" id="day">
				<option value="1" selected="selected">1</option>
				...
			</select>

			<label for="month">Month</label>
			<select name="dobm" id="month">
				<option value="January" selected="selected">January</option>
				....
			</select>

			<label for="year">Year</label>
			<select name="doby" id="year">
			<option value="2008" selected="selected">2008</option>
				....
			</select>
		</label>
		</dt>
	</dl>
	<dl>
		<dt>
			<label>Sex: Male<input name="sex" type="radio" id="sex" value="sexm" />Female<input name="sex" type="radio" id="sex" value="sexf" /></label>
		</dt>
	</dl>
	<dl>
		<dt>  
			<label for="country">Country</label>
				<select name="country" id="country">
				<option value="NONE" selected="selected">Please Choose One</option>
				.....
			</select> 
		</dt>
	</dl>
	<dl>
		<dt>
			<label title="Username">Lord Name: 
			<input name="lordname" type="text" maxlength="25" id="lordname" />
			</label>
		</dt>
	</dl>
	<dl>
		<dt>
			<input type="hidden" name="token" value="<?php echo $token; ?>" />
			<label title="Submit"><input type="submit" name="submit" value="Signup" /></label>
		</dt>
	</dl>
</fieldset>
</form>
</body>
</html>

sneakyimp

I'm not certain, but just using strip_tags to 'clean' something before you stuff it directly into an sql statement is not secure enough. The reason being that SQL injection is still possible. You should probably read more on what SQL injection means. People don't just stick in html tags, they stick in quote marks and attempt to do things like buffer overflows and such.

You should be using [man]mysql_real_escape_string[/man] on any text strings taken from user input. Also, you are echoing mysql_error's output directly to the user -- this will result in all kinds of useful information being communicated to a hacker. You should have a single generic failure message that doesn't reveal any information about what went wrong with the query. Write the specific failure info to a log file that only you can read.

mikemcleanuk

thanks for the notes, but i am using mysql_real_escape_string as is in the function:

function mres($input) {
return strip_tags(mysql_real_escape_string($input));
}

as for the mysql errors i will work on that thanks!

sneakyimp

My mistake. I looked it over rather hastily.

I don't see you requiring SSL anywhere. If you really want it to be secure, you should host it using HTTPS.

if($_SERVER['HTTPS']!="on") {
  $redirect= "https://".$_SERVER['HTTP_HOST'].$_SERVER['REQUEST_URI'];
  header("Location:$redirect");
}

Another thing I notice is that you let the user supply a session token in the form and accept it without any hesitation. This could allow a user to hijack someone else's session. Hopefully the code in session.php has code to prevent someone from doing this.

I haven't checked each and every form input to be sure that you clean it first. using mysql_real_escape_string in your mres func is a pretty good start.

BuzzPHP

Depends what you mean by "secure". Ive coded a lot of bots that exploit forms like this, and Im guessing you are trying to stop that.

1) The session token is a great idea, but easierly circumvented.
2) Checking the user agent can help stop bots, but easierly circumvented.
3) Checking the referer can help stop bots, but easierly circumvented.
4) Failing on incorrect tokens can slow down bots, but they will still attack.
5) Multi step forms help weed out the people who cant code to attack these, but I can.

Fact is, the only way to protect against bots is some form of captcha.

As far as SQL injection, you have covered that. I wouldn't have coded it that way, but thats just me.

As far as data validation, there is no way to validate an email, unless you send an email to the account to force people to "validate your email". Um, but thats more coding.

Bjom

if you look for maximum security, why do you stick to an old standard like mysql, when there is an improved one that is even propagated by the PHP devs? Use mysqli instead and switch from relying on the ...real_escape_string approach to using prepared statements

mysqli in the manual

and stop using or die... it even can have security implications.

mikemcleanuk

Bjom;10930656 wrote:
if you look for maximum security, why do you stick to an old standard like mysql, when there is an improved one that is even propagated by the PHP devs? Use mysqli instead and switch from relying on the ...real_escape_string approach to using prepared statements

mysqli in the manual

and stop using or die... it even can have security implications.

thanks for the tips, i will begin investigatin mysqli. But one basic question. Can i just replace all my mysql statements with mysqli and everything will work the same way? Or would it require a new aproach to programming?

laserlight

mikemcleanuk wrote:
Can i just replace all my mysql statements with mysqli and everything will work the same way?

If you are talking about SQL statements, then yes. If you are talking about PHP code that involves the MySQL extension to PHP code that involves the MySQLi extension, then mostly, but there will be some work involved in making that change, particularly because you would need an explicit connection object and you would want to take advantage of prepared statements (and the latter means making some changes to the text of your SQL statements).