Improving Login Form security

Hello,

I’ve recently built around 6 or so web sites which have all used the same kind of Login form with PHP.

However, I’m wondering how ‘secure’ it really is, and wondered if any kind SitePointers would be able to ‘audit’ the code.

I understand there’s a lot of code to look over but I would really appreciate any help at all.

The scripts allow a user to register, log in and then log out.

SignUp.php


<?php

include ('includes/functions.php');
include ('includes/config.inc.php');
include('includes/header.php');

$ip = getenv("REMOTE_ADDR");
$hr = getenv("HTTP_REFERER");
$hst = gethostbyaddr( $_SERVER['REMOTE_ADDR'] );
$ua = $_SERVER['HTTP_USER_AGENT'];


// If Form Submitted
if (isset($_POST['saveForm'])) {
	
	if (isset($_SESSION['token']) && $_POST['token'] == $_SESSION['token']) {
		// valid token
	} else {
		header("Location: /signup/signout.php");
	}
	

	$token = md5(uniqid(rand(), TRUE));
	$_SESSION['token'] = $token;
	$_SESSION['token_time'] = time();

	 require_once('connect.php');
	 
	 // Start to set & clean-up variables
	 
  	 // Check for a name.
	 if (eregi ('^[[:alpha:]\\.\\' \\-]{2,30}$', stripslashes(trim($_POST['name'])))) {
		//$name = escape_data($_POST['name']);
		$name = substr($name,0,30);
	 } else {
		$name = FALSE;
		$errmessage = TRUE;
	 }
	

	 // validate age
	 $age = getIntValue($_POST['age']);
	 

   	 // validate & confirm email address   	
	 $email = $_POST['email'];
	 if(eregi("^[_a-z0-9-]+(\\.[_a-z0-9-]+)*@[a-z0-9-]+(\\.[a-z0-9-]+)*(\\.[a-z]{2,3})$", $email)) {
	  $email = stripslashes(strip_tags(trim(strtolower($_POST['email']))));
	  $email = substr($email,0,120);
	 } else {
	  $email = FALSE;
	  $errmessage = TRUE;
   	 }

	 $email2 = $_POST['email2'];
	 if(eregi("^[_a-z0-9-]+(\\.[_a-z0-9-]+)*@[a-z0-9-]+(\\.[a-z0-9-]+)*(\\.[a-z]{2,3})$", $email2)) {
	  $email2 = stripslashes(strip_tags(trim(strtolower($_POST['email2']))));
	  $email2 = substr($email2,0,120);
	 } else {
	  $email2 = FALSE;
	  $errmessage = TRUE;
     }	 

	 if ($email == $email2) {
		$e = TRUE;
	 } else {
  		$e = FALSE;
	  	$errmessage = TRUE;
	 }
	 

	 // Terms
	 $terms = (isset($_POST['terms'])) ? TRUE : FALSE;

	// time stamp - watch our for multiple submissions
	if (strlen($_POST['stamp']) == 32) {
		$s = escape_data($_POST['stamp']);
	} else {
		$s = FALSE;
		$errmessage = TRUE; 
	}
	

	// Check if all fields have been completed

   if ($e) {

         if(!isset($name,$age,$e,$terms,$s) || empty($name) || empty($email) || empty($email2) || empty($age) || empty($terms) || empty($s)) {

    		 $feedbackstyle = "error"; 
			 $feedback = "Please complete ALL the form fields.";

    		 } else {

    		// Make sure the email address is available.
			$email = mysql_real_escape_string($email);
    		$query = "SELECT id FROM pusers WHERE email='$email'";
    		$result = mysql_query($query);

    		if (mysql_num_rows($result) == 0) { // Available.          

            $pass = createRandomPassword();
            $salt = randomString();
           
            // prepend the salt to the password
            $p = $salt . sha1($salt.$pass);

            $ipaddress = $_SERVER['REMOTE_ADDR'];
            $name = mysql_real_escape_string($name);
            $age = mysql_real_escape_string($age);
            $s = mysql_real_escape_string($s);

      		$result = mysql_query("INSERT INTO pusers(name,age,email,password,date_added,ip,timestamp) VALUES('$name','$age','$email','$p',NOW(),'$ip','$s') LIMIT 1");
        	
        	if (mysql_affected_rows() == 1) { // If it ran OK.

      				// Send the email.
      				$body = "Thank you for signing up for an account with Profiler.";
      				$body .= "\
\
Your temporary password is ".$pass;
      				mail($_POST['email'], 'Profiler - Account Confirmation', $body, 'From: no-reply@meownsite.co.uk');

					$feedbackstyle = "success";
					$feedback = "Great success! Grab your Password in your Inbox and Log In.";

        		}

      	 	}
      	 	
      	}

  	}

}

?>

<form id="form1" name="form1" class="wufoo leftLabel" autocomplete="off" enctype="multipart/form-data" method="post" action="<?php echo(''.htmlentities($_SERVER["PHP_SELF"]).''); ?>">
<input type="hidden" name="token" value="<?php echo $token; ?>" />

<div class="info">

	<h2>Sign up</h2>
	<div>Complete the form below to sign-up at our web site.</div>

</div>

<?php

if (isset($feedback)) {
	 echo '<div class="' . $feedbackstyle . '">';
	 echo $feedback;
	 echo '</div>';
}

?>

<ul>

<li id="foli1">

	<label class="desc" id="title1" for="name">Name<span id="req_1" class="req">*</span></label>

	<div>
		<input id="name" name="name" type="text" class="field text medium" value="<?php echo (isset($_POST['name'])) ? htmlspecialchars($_POST['name']) : ''; ?>" maxlength="255" tabindex="1" <?php if ((isset($_POST['saveForm'])) && ($name == FALSE)) { echo 'style="border:2px solid red"'; } ?> />
		<label for="name">Must be between <var id="rangeMinMsg1">3</var> and <var id="rangeMaxMsg1">50</var> characters.&nbsp;&nbsp;&nbsp; <em class="currently">Currently Used: <var id="rangeUsedMsg1">0</var> characters.</em></label>
</div>

	</li>

<li id="foli4">

	<label class="desc" id="title4" for="age">Age<span id="req_4" class="req">*</span></label>

	<div>
		<input id="age" name="age" type="text" class="field text small"  value="<?php echo (isset($_POST['age'])) ? htmlspecialchars($_POST['age']) : ''; ?>" maxlength="255" tabindex="3" <?php if ((isset($_POST['saveForm'])) && ($age == FALSE)) { echo 'style="border:2px solid red"'; } ?> />
		<label for="age">Enter a number between <var id="rangeMinMsg4">15</var> and <var id="rangeMaxMsg4">130</var>.</label>
	</div>

</li>

<li id="foli7">

	<label class="desc" id="title7" for="email">Email<span id="req_7" class="req">*</span></label>

	<div>
		<input id="email" name="email" type="text" class="field text medium" value="<?php echo (isset($_POST['email'])) ? htmlspecialchars($_POST['email']) : ''; ?>" maxlength="255" tabindex="4" <?php if ((isset($_POST['saveForm'])) && ($email == FALSE)) { echo 'style="border:2px solid red"'; } ?> /> 
	</div>

</li>


<li id="foli8">

	<label class="desc" id="title8" for="email2">Confirm Email<span id="req_8" class="req">*</span></label>

	<div>
		<input id="email2" name="email2" type="text" class="field text medium" value="<?php echo (isset($_POST['email2'])) ? htmlspecialchars($_POST['email2']) : ''; ?>" maxlength="255" tabindex="5" <?php if ((isset($_POST['saveForm'])) && (($email2 == FALSE) OR ($e == FALSE))) { echo 'style="border:2px solid red"'; } ?> /> 
	</div>

</li>


<li id="foli314">

	<label class="desc" id="title314" for="terms">Terms & Conditions</label>

	<div class="col">
		<span>
			<input id="terms" name="terms" type="checkbox" class="field checkbox" value="yes" tabindex="8" <?php if(isset($_POST['terms']) && $_POST['terms'] == 'yes') { echo 'checked="checked"'; } ?> 	/>
			<label class="choice" for="terms">I agree  <?php if ((isset($_POST['saveForm'])) && ($terms == FALSE)) { echo '<span style="error">Please tick box.</span>'; } ?></label>
		</span>
	</div>

</li>
	

<li class="buttons">

	<input id="saveForm" name="saveForm" class="btTxt submit" type="submit" value="Submit" />

</li>


</ul>


<input type="hidden" name="stamp" id="stamp" value="<?php echo md5(uniqid(rand(),true)); ?>" />


</form>


</div><!--container-->

<img id="bottom" src="images/bottom.png" alt="" />

<?php

include('includes/footer.php');

?>

SignIn.php


<?php

include ('includes/functions.php');
include ('includes/config.inc.php');
include('includes/header.php');

$ip = getenv("REMOTE_ADDR");
$hr = getenv("HTTP_REFERER");
$hst = gethostbyaddr( $_SERVER['REMOTE_ADDR'] );
$ua = $_SERVER['HTTP_USER_AGENT'];

if (isset($_POST['saveForm'])) { // Check if the form has been submitted.

	$token = md5(uniqid(rand(), TRUE));
	$_SESSION['token'] = $token;
	$_SESSION['token_time'] = time();

	require_once ('connect.php'); // Connect to the Database.


	if (checkNotEmpty($_POST['email'])) {
	  if ((strlen($_POST['email']) > '80') || (strlen($_POST['email']) < '7')) {
  			$email = FALSE;
  		} else {
      		$email = strtolower(strip_tags($_POST['email']));
	  }
	} else {
  		$email = FALSE;
	}	

	if (checkNotEmpty($_POST['password'])) {
	  if ((strlen($_POST['password']) > '30') || (strlen($_POST['password']) < '8')) {
  			$p = FALSE;
  		} else {
  			$p = strtolower(strip_tags($_POST['password']));
  		}
	} else {
  			$p = FALSE;
	}	

  	if (strlen($_POST['stamp']) == 32) {
	   $s = TRUE;
  	}

	if ($email && $p && $s) { // If everything's OK.

	  // clean up for database
	  $email = mysql_real_escape_string($email);
	  $ip = mysql_real_escape_string($ip);

	  // basic brute force protection
  	  /*$ip = $_SERVER['REMOTE_ADDR'];
  	  $sql = "SELECT id, email FROM login_attempts WHERE email = '$email' AND ip = '$ip'";
  	  $res = mysql_query($sql);

  	  if (mysql_num_rows($res) > '8') {
        $feedbackstyle = "error";
        $feedback = "You have made 8 failed login attempts.<br />Your Account has now been blocked.";
        include ('includes/footer.php');
        exit;
      }
	  mysql_free_result($res);*/

		// Query the Database.
		$query = "SELECT id, email, name FROM pusers WHERE (email='$email' AND password = CONCAT(SUBSTRING(password, 1, 32), SHA1(CONCAT(SUBSTRING(password, 1, 32), '$p')))) AND disabled = 'N' LIMIT 1";
		$result = mysql_query ($query);

		// A match was made
		if (mysql_num_rows($result) == 1) {

			// Register the values and redirect.
			$row = mysql_fetch_array($result, MYSQL_NUM);
			mysql_free_result($result);
			mysql_close(); // Close the db connection.			

			$_SESSION['name'] = escape_data(htmlspecialchars($row[2]));
			$_SESSION['email'] = $row[1];
			$_SESSION['u_id'] = $row[0];
			$user_id = $row[0];
			session_regenerate_id();
			$_SESSION['ip'] = $_SERVER['REMOTE_ADDR'];
			$_SESSION['timestamp'] = time();
			$_SESSION['theagent'] = md5($_SERVER['HTTP_USER_AGENT']);
			$_SESSION['HTTP_USER_AGENT'] = $_SERVER['HTTP_USER_AGENT'];

			// Start defining the URL
			$url = 'http://' . $_SERVER['HTTP_HOST'] . dirname($_SERVER['PHP_SELF']);			

			// Check for trailing slash.
			if ((substr($url, -1) == '/') OR (substr($url, -1) == '\\\\') ) {
				$url = substr ($url, 0, -1); // Chop off the slash.
			}			

			// Add the Page
			$url .= '/index.php';

			ob_end_clean(); // Delete the buffer.
			header("Location: $url");
			exit(); // Quit the script.

		} else { // No match was made.

			  // Alert Administrator of Failed Login Attempt
			  $to      = "michael@meownsite.co.uk";
              $subject = "Profiler | Failed Login Attempt";
              $message = "Hello\
\
Email: $email\
\
IP: $ip";
              $headers = 'From: no-reply@meownsite.co.uk' . "\\r\
" .
                  'Reply-To: no-reply@meownsite.co.uk' . "\\r\
" .
                  'X-Mailer: PHP/' . phpversion();
              mail($to, $subject, $message, $headers);

				// log failed attempt, in case brute force
				$ip = $_SERVER['REMOTE_ADDR'];
				$sql = "INSERT INTO login_attempts VALUES ('','$email','$ip',now()) LIMIT 1";
				mysql_query($sql);			

				$feedbackstyle = "error"; 
        		$feedback = "Either the email address and password entered do not match those on file, or you have not yet activated your account.";

      			$sqler = "SELECT * FROM login_attempts WHERE ip = '$ip' AND date_added  >= (NOW()-1)";
      			$reser = mysql_query($sqler);     				  

      			if (mysql_num_rows($reser) > 3) {

      				    $sqlupdate = "UPDATE pusers SET disabled = 'Y' WHERE email = $email";
      				    mysql_query($sqlupdate);   				  

    			        // uh-oh notify
    			        $to      = "michael@michaelmcg.co.uk";
    			        $subject = "Profiler | 3 Failed Login Attempts...";
    			        $message = "Hello\
\
Email: $email\
\
IP: $ip";
    			        $headers = 'From: no-reply@meownsite.co.uk' . "\\r\
" .
                      'Reply-To: no-reply@meownsite.co.uk' . "\\r\
" .
                      'X-Mailer: PHP/' . phpversion();
                  		mail($to, $subject, $message, $headers);
    			}
    			mysql_free_result($reser);
			}

		} else { // If everything wasn't OK.
		
				$feedbackstyle = "error";
				$feedback = "Please try again.";

		}

} // End of Submit conditional.

?>

<form id="form1" name="form1" class="wufoo leftLabel" autocomplete="off" enctype="multipart/form-data" method="post" action="<?php echo(''.htmlentities($_SERVER["PHP_SELF"]).''); ?>">

<div class="info">

	<h2>Sign in</h2>

	<div>Complete the form below to sign-in to our web site.</div>

</div>

<?php

if (isset($feedback)) { 

	 echo '<div class="' . $feedbackstyle . '">';
	 echo $feedback;
	 echo '</div>';
	 
}

?>

<ul>

<li id="foli7">

	<label class="desc" id="title7" for="email">Email<span id="req_7" class="req">*</span></label>

	<div>
		<input id="email" name="email" type="text" class="field text medium" value="<?php echo (isset($_POST['email'])) ? htmlspecialchars($_POST['email']) : ''; ?>" maxlength="80" tabindex="4" <?php if ((isset($_POST['saveForm'])) && ($email == FALSE)) { echo 'style="border:2px solid red"'; } ?> /> 
	</div>

</li>

<li id="foli414">

	<label class="desc" id="title414" for="password">Password<span id="req_414" class="req">*</span></label>

	<div>
		<input id="password" name="password" type="password" class="field text medium" value="<?php echo (isset($_POST['password'])) ? htmlspecialchars($_POST['password']) : ''; ?>" maxlength="30" tabindex="9" <?php if ((isset($_POST['saveForm'])) && ($p == FALSE)) { echo 'style="border:2px solid red"'; } ?> />
		<label for="password">Must be between <var id="rangeMinMsg414">8</var> and <var id="rangeMaxMsg414">30</var> characters.&nbsp;&nbsp;&nbsp; <em class="currently">Currently Used: <var id="rangeUsedMsg414">0</var> characters.</em></label>
	</div>

</li>

	<li class="buttons">
	<input id="saveForm" name="saveForm" class="btTxt submit" type="submit" value="Submit" />
	</li>
	
</ul>

<input type="hidden" name="stamp" id="stamp" value="<?php echo md5(uniqid(rand(),true)); ?>" />

</form>

</div><!--container-->

<img id="bottom" src="images/bottom.png" alt="" />

<?php

include('includes/footer.php');

?>

Index.php


<?php # index.php

include ('includes/functions.php');
include ('includes/config.inc.php');
include('includes/header.php');

if (isset($_SESSION['HTTP_USER_AGENT'])) {
	if ($_SESSION['HTTP_USER_AGENT'] != md5($_SERVER['HTTP_USER_AGENT'])) {
		header("Location: /signup/signout.php");
	}
} else {
	$_SESSION['HTTP_USER_AGENT'] = md5($_SERVER['HTTP_USER_AGENT']);
}

require_once ('connect.php'); // Connect to the Database.

$clean = array();
$html = array();

$clean['name'] = $_SESSION['name'];
$html['name'] = htmlentities($clean['name'], ENT_QUOTES, 'UTF-8');

echo "<h2>Welcome, {$html['name']}</h2>";
echo "<p><strong>Not {$html['name']}? <a href='signout.php'>Click here</a>.</strong></p>";

include ('includes/footer.php');

?>

Header.php


<?php

ob_start();
session_start();

//error_reporting(E_ALL & ~E_NOTICE);

$currentFile = $_SERVER["SCRIPT_NAME"];
$foo = $_SERVER['PHP_SELF'];

if (isset($_SESSION['u_id']) && ($currentFile == '/signup/signin.php')) {
  header("Location: index.php");
}

if ((!isset($_SESSION['u_id'])) && ($currentFile == '/signup/index.php')) {
  header("Location: signin.php");
}

$ip = $_SERVER['REMOTE_ADDR'];

if (isset($_SESSION['ip'])) {
	if ($_SESSION['IP'] != $_SERVER['REMOTE_ADDR']) {
		header("Location: /signup/signout.php");	
	}
}

if (isset($_SESSION['timestamp'])) {
	if ((time() - $_SESSION['timestamp']) > 1800) {
		header("Location: /signup/signout.php");
	} else {
		$_SESSION['timestamp'] = time();	
	}
}


require_once ('connect.php'); // Connect to the Database.

?>

<!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>Profiler</title>

<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />

  <!-- CSS -->
  <link rel="stylesheet" href="css/structure.css" type="text/css" />
  <link rel="stylesheet" href="css/form.css" type="text/css" />        	

</head>

<body id="public">	

  <img id="top" src="images/top.png" alt="" />

  <div id="container">

Footer.php


</body>

</html>

<?php

if (isset($_POST['saveForm'])) {

  // close my db connection
  mysql_close($dbc);

}

?>

Snippet from Functions.php


  function checkNotEmpty($s) {
  	return (trim($s) !== '');
  }  

  function getIntValue($s) {
    if (!is_numeric($s)) {
      return false;
    } else {
      return (int)$s;
    }
  }

Like I say I understand there is a large amount of code on display here, but I would really value some of the professionals taking a look over it and maybe making same obvious notes of things I haven’t considered.

I’ve tried to consider things like XSS and SQL Injection.

I’m going to use htaccess/htpasswd on my administrator directory as well as try enforce a strict password scheme for users.

Many thanks for any assistance

A couple of remarks:

  • eregi is deprecated in recent PHP-versions, use preg_match with the i modifier, see Deprecated features in PHP 5.3.x
  • users can disable the referer, so using the HTTP_REFERER is unreliable
  • with some providers you get a new ip for every request made, so the users’ ip might not be static, which also means using the ip address is unreliable
  • the validation for the e-mail address could be simplified and improved:

    php <?php $email = trim(stripslashes($email)); if (!preg_match('/^[a-z0-9_\\-]+(\\.[_a-z0-9\\-]+)*@(([_a-z0-9\\-]+\\.)+([a-z]{2}|aero|arpa|biz|com|coop|edu|gov|info|int|jobs|mil|museum|name|nato|net|org|pro|travel))$/i', $email)) { // E-mail address is invalid } ?>
  • Use mysql_real_escape_string to escape the users’ input to prevent SQL-injections
  • To prevent cross-site scripting, you can use htmlspecialchars when outputting the users’ input.
    You can also use a session variable with a random key, hash it, and put it in the form as a hidden field so you can check if the users’ input comes from your login page and not from another host.

That’s good advice there.

I’ll add though, that if you’re using using at least PHP 5.2 then you also have filter_var and [url=“http://php.net/manual/en/function.filter-input.php”]filter_input at your disposal for sanitizing and validating.


$email = filter_input(INPUT_POST, 'email', FILTER_VALIDATE_EMAIL);

Thank you.
But, remember that those filters are not as reliable as one might think.
For example:


<?php
    // Should all return false
    filter_var('http://.', FILTER_VALIDATE_URL); // returns http://.
    filter_var('a@b-.c', FILTER_SANITIZE_EMAIL); // returns a@b-.c
    filter_var('http://.', FILTER_SANITIZE_URL); // returns http://.
?>

Sanitize only removes characters, it doesn’t check it it’s valid. That’s why there are separate sanitize and validate filters.

That first example with http://. is an interesting variation though.
I do see though on the validate filters page that there is an optional FILTER_FLAG_PATH_REQUIRED flag that can be applied. Would that help to deal with it?

When FILTER_FLAG_PATH_REQUIRED is ommitted, filter_var returns false.

Another interesting issue with the e-mail validation filter:

<?php
    filter_var('a.@b.c', FILTER_VALIDATE_EMAIL); // returns a.@b.c
?>

Also, the domain names’ TLD should be validated too, which filter_var does not.
I don’t think filter_var is the most reliable function for validation, for now.

Your top level validation doesn’t include any country codes, and with ICANN considering opening more and more domains, you’re excluding people who might be using those.

‘.c’ is a valid TLD according to the spec, it’s just not distributed by ICANN. Excluding the filter_var function based on your example is silly. filter_var is significantly easier to use than rolling your own function, and as such is usually a much better idea.

Your entirely right.
The syntax is correct, according to the specifications in RFC 822.
And surely, one should not disregard filter_var based on my assumptions above.
But, .c is not a registered TLD, and therefor unexisting and, more importantly, invalid.

In this case the validity of the e-mail address is not important, since it will be matched to an e-mail address stored in the database.
As such, the syntax of the e-mail address does not matter.

The best way to validate an e-mail address, is to send an e-mail to that address with a verification link when an e-mail address is stored.
But when e-mail validition is important, where the situation does not allow you to send a verification link to a given e-mail address, filter_var does not suffy, in my opinion.

In that last situation, not only the syntax matters, but also the validity of the TLD, the existence of the domain itself and the existence of MX-records for that domain.
I have to admit, it is reeeee…aly far fetched, but some government organisations require such intensive validation, like FOV (Federation of Organisations active in the popular (non-formal) adult education scene).

For really thorough e-mail validation, you would need:

Input validation is a pain, but if such things are high priority requirements by the client, then I will meet the clients’ demands and Thorough will be my middle name.

And my last name will be Nuts :slight_smile:

Many thanks for the replies guys. Really appreciate your input. It’s always valid :wink:

Hexburner, many thanks for your email validation reg exp. I’m going to use it from now on, works great!

-------------------

Hexburner, in #2 you mention:

Use mysql_real_escape_string to escape the users’ input to prevent SQL-injections

To prevent cross-site scripting, you can use htmlspecialchars when outputting the users’ input.

You can also use a session variable with a random key, hash it, and put it in the form as a hidden field so you can check if the users’ input comes from your login page and not from another host.

In my signin.php and signup.php I make use of mysql_real_escape_string. Although, I’m thinking of using prepared statements for future work.

XSS
In my index.php I used:



$clean = array();
$html = array();

$clean['name'] = $_SESSION['name'];
$html['name'] = htmlentities($clean['name'], ENT_QUOTES, 'UTF-8');

echo "<h2>Welcome, {$html['name']}</h2>";

echo "<p><strong>Not {$html['name']}? <a href='signout.php'>Click here</a>.</strong></p>";

would this suffice as escaping output?

-------------------

users can disable the referer, so using the HTTP_REFERER is unreliable

Is there any way around this, or is it not really too big of an issue to be concerned over?

-------------------

You can also use a session variable with a random key, hash it, and put it in the form as a hidden field so you can check if the users’ input comes from your login page and not from another host.

Could you perhaps show me a brief example of how I could do this?
I tried to use a token verfication method in my signup.php script above. Is this the best way to do it?

In terms of Security on the signup.php and signin.php, do you think I have sufficiently covered everything I need to?

Also, many thanks also to pmw57. I think I should look to upgrade more of my sites to PHP 5.2. I’ll speak to my host on this so I can use the FILTER functions.

I really value the feedback everyone has given on this thread so far. It means a lot.

I think I’m going to try stick with these standards on future apps. Or just use CodeIgiter/ZF :slight_smile:

Yes, that does the job well.

Yes, that’s a good way of doing it that you’re using there.

Nearly, but not quite. Your code appears to rely on the server aplying magic quotes to things like the $_POST variables. Magic quotes are not good or reliable, so if they are enabled, you need to remove them, and then at the database side of things you should use mysql_real_escape_string instead. A good attempt to do that has been made, but the $p variable appears to have been missed.

A code smell to watch out for is where get_magic_quotes_gpc() is not used when retrieving input. The signup code strips the slashes, but it shouldn’t if the server already has magic_quotes disabled.

$email = $_POST['email'];
if (get_magic_quotes_gpc()) {
    $email = stripslashes($email);
}
$email = strip_tags(trim(strtolower($email)));

Then where the variable is used in the database, another useful code smell is if mysql_real_escape_string is not used with the SQL statement.

On the signin form for example, there’s a chunk of commented code between the escaped variables and where they’re used in the SQL statement. This means that it’s easy to miss whether the variables have ben escaped or not, which is an existing problem in the code with the $p variable.

That problem can be prevented by moving the escaping to where the variables are placed in the SQL string.

The query page shows a good way to do this, where sprintf is used to place sanitised variables within the SQL statement.


$sql = sprintf('SELECT id, email, name FROM pusers WHERE (email="&#37;s" AND password = CONCAT(SUBSTRING(password, 1, 32), SHA1(CONCAT(SUBSTRING(password, 1, 32), "%s")))) AND disabled = "N" LIMIT 1',
    mysql_real_escape_string($email),
    mysql_real_escape_string($p)
); 
$result = mysql_query ($sql); 

Where numbers are used, you would use %d instead of “%s” and of source as they’re numbers you wouldn’t use mysql_real_escape_string, but something like intval($id) instead.

So in the end, it’s basically code smells that you’re looking for.

[list][]Are the inputs being appropriately stripped, sanitised and validated.
[
]Are the variables for SQL statements being appropriately escaped to protect the database.
[*]Are the outputs being appropriately escaped to protect your page.[/list]

Many many thanks for the detailed response Paul.

The 6 sites these appear on are fairly old and it’s definitely not the way I build sites now. I like to split it up(as you have done) by filter, db escape, output escaping.

My current Data Escaping function is like this:


// Create a function for escaping the data.

function escape_data ($data) {

// Use Magic Quotes.
if(ini_get('magic_quotes_gpc')) {
	$data = stripslashes($data);
}

// Check for mysql_real_escape_string() support.
if (function_exists('mysql_real_escape_string')) {
		global $dbc; // Need the connection.
		$data = mysql_real_escape_string (trim($data), $dbc);
} else {
		$data = mysql_escape_string (trim($data));
}	

// Return the escaped value.
return $data;		

} // End of Function

Does this fix the magic quotes issue? Does this mean, I should just use my escape_data function for db input rather than stripslashes?

Concur with your thoughts on keeping mysql_real_escape_string in the actual query. Keeps things nice and simple.

Again, really appreciate you taking the time to look over my work.

I’m going to try roll this out across my existing 6 apps.

I think I’ll then take a look at all my web site scripts for SQL Injection and XSS issues as I think this is where I may have holes.

Thanks again Paul!

There are some issues with that quoted code.

  • The ini_get function may return “off” which in your if condition would evaluate to true. Please use get_magic_quotes_gpc() instead.
  • Checking if mysql_real_escape_string exists looks to be really old code, from when PHP older than 4.3.0 is being used. The earlier mysql_escape_string had some serious security issues, so if you’re going to use that then you also need to protect against those other security problems too.
  • The escape function has a significant problem, in that it cannot correctly escape form inputs that are not destined for a database, and it cannot correctly escape PHP values that have not had magic_quotes applied to them.

[list][*]Using that escape function means that you have to escape the values either when you receive the input, or when you apply the values to the SQL string.

When you escape the values only you receive the input, later on through the code when the values are applied to the SQL statement you need to remember to not apply the mysql_real_escape_string function to those specific values, and to apply it to other values that aren’t directly from the input. The SQL statement would then have some values escaped that did not directly originate from user input, and other values not escaped because that has already been done to them. That can only lead to bad things happening.

When the values have magic quotes and mysql_real_escape_string applied only at the SQL statement, the part of the code where the inputs are retrieved then need to remain unescaped where those values are later on escaped at the sql code. That can only lead to bad things happening too.[/list]

So please don’t use that code. You’re only setting yourself up for failure should you do so.

Instead, set up a demarcation point where the inputs have magic quotes stripped from them, are sanitised, and validated.
Then later on, where you handle the database work, escape all of the values going in to the database, whether that means using mysql_real_escape_string or intval or other techniques.

You can use separate functions to perform these tasks, but just ensure that the next person coming along (who may also be yourself in 6 months time) can clearly understand that the values are being properly treated.

That has been found to be the only appropriate way to sufficiently protect yourself from problems that may occur.

Eeek. Sounds like I should be ditching that escape_data function altogether.

Would you suggest sticking with mysql_real_escape_string for all database input and not using stripslashes at all?

Stripslashes is important and necessary to perform for input, but only when magic_quotes are enabled. Why? Some inputs are destined for the database, and some are not.

If you don’t strip slashes, they will still be there in the code when you later on escape the data for the database.

When working with a new server you can entirely disable magic quotes, and PHP 6.0 will not allow magic quotes so that stripping slashes is not needed, but where there is pre-existing code that hasn’t been protected, or where your code will be used in unknown environments, you need to strip slashes on all inputs.

escaping strings is a completely separate task that applies only to data being sent to the database. That data can come from user inputs, and that data can come from other processes within your code. That’s why escaping the data at the point where the data gets added to the database, is the only way to consistently ensure that the correct behaviour occurs.

Hi Paul,

Aaah, thanks for clearing this up.

From now on I will escape input data solely at the SQL query.

I’m going to work on a very basic Guestbook app shortly and try include everything I’ve learned from this. Building both a procedural and OOP version :slight_smile: Stay tuned :smiley:

Again, can’t thank you enough for your help. I’ve a feeling I’ll be returning to this thread often.

By the way, as a good example of why to escape the output, the first 5 minutes of The End of All Things provides a very good run-down of the dangers that cross-site-scripting exposes your web site to.

If header.php is what protects your content from non logged in users, you have no protection.

header() does not stop script execution. Sending a location header is merely a recomendation to the client that they should load another url. They might not listen.

use exit() to stop script execution.

Try it


header('content-type: text/plain');
$opts = array('http' => array('max_redirects'  => 0, 'ignore_errors' => true));
readfile('http://example.com/members_only.php', false, stream_context_create($opts));

Manythanks again Paul. I hope to work on a very small Guestbook ap and perhaps post the results here too.

crmalibu, many thanks for your thoughts.
So I would include the header, and then follow it with an exit() statement?

Could you show me an example, taking my header.php above in consideration?
Sorry, I’m just a bit unsure how to use your example.

Many thanks for your help.

The email regex hexburner provides is equally flawed. It rejects legitimate Gmail addresses.

A more inclusive regex for email addresses would be:

preg_match('/^[a-z0-9!#$&#37;&\\'*+\\\\/=?^_`{|}~-]+(?:\\\\.[a-z0-9!#$%&\\'*+\\\\/=?^_`{|}~-]+)*@(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\\\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$/i', $email))

Though beware, it does not properly handle quoted addresses such as "John Doe"@example.com (not that anyone uses those).

You can combine sprintf() and mysql_query() in a way that makes it easy for you to escape strings and yet make it very hard to do something dumb (like not escaping). Here’s an example:
http://php.net/manual/en/function.mysql-query.php#81188

Many thanks sk89q. I use your PHP Security checklist a lot. Brilliant resource.

Thank you. Will update the RegEx shortly and take a look at the SQL query example.