Help improving sanitizing user input & registration?


#1

What improvements should I make?

Because the site is so form dependent I wrote a simple function to sanitize the user inputs and do a little formatting of the input data. This function also sanitizes the GET input from an activation email:
[php]function sanitizeAll() {

array_walk_recursive($_POST, 'trim');
array_walk_recursive($_GET, 'trim');

$data = array
	(
	"role" => FILTER_SANITIZE_STRING,
	"code" => FILTER_SANITIZE_STRING,
	"userName" => FILTER_SANITIZE_STRING,
	"activationCode" => FILTER_SANITIZE_STRING,
	"firstName" => FILTER_SANITIZE_STRING,
	"lastName" => FILTER_SANITIZE_STRING,
	"street" => FILTER_SANITIZE_STRING,
	"city" => FILTER_SANITIZE_STRING,
	"state" => FILTER_SANITIZE_STRING,
	"zip" => FILTER_SANITIZE_NUMBER_INT,
	"busName "=> FILTER_SANITIZE_STRING,
	"busName" => FILTER_SANITIZE_MAGIC_QUOTES,
	"email" => FILTER_SANITIZE_EMAIL,
	"phone" => FILTER_SANITIZE_NUMBER_INT,
	"fax" => FILTER_SANITIZE_NUMBER_INT,
	"licenseNum" => FILTER_SANITIZE_NUMBER_INT,
	"wsaddr" => FILTER_SANITIZE_URL,
  );
  
if($_POST){
	$var = filter_input_array(INPUT_POST, $data);
}else {
	$var = filter_input_array(INPUT_GET, $data);
}

$phone = preg_replace("/[^0-9]/","",$var['phone']);
$var['phone'] = "(".substr($phone, 0, 3).") ".substr($phone, 3, 3)."-".substr($phone,6);

$fax = preg_replace("/[^0-9]/","",$var['fax']);
$var['fax'] = "(".substr($fax, 0, 3).") ".substr($fax, 3, 3)."-".substr($fax,6);

$parsed = parse_url($var['wsaddr']);
	if (!isset($parsed['scheme'])) {
		$url = $var['wsaddr'];
		$var['wsaddr'] = "http://".$url."/";
	}

return $var;[/php]

Then there is the registration page that first utilizes the above function, obviously there are other forms/pages that use this but for now this is the only one that is “complete”.
[php]include(‘pg_top.php’);

echo ‘

User Registration

’;
echo ‘
’;

$registerForm = ’

I am a buyer!

I am a auctioneer!


Username





Email





Password





Retype Password






';
		if(isset($_POST['submitbtn'])){
			
			$data = sanitizeAll();
			
			if(!empty($data['role'])){
			
				if(!empty($data['userName'])){
					
					if(ctype_alnum($data['userName'])){
						
						if (!preg_match('/\s/',$data['userName'])){
							
							if(!empty($data['email'])){
								
								if (filter_var($data['email'], FILTER_VALIDATE_EMAIL)) {
								
									if(!preg_match('/(?=.{8,})(?=.*\d)(?=.*[a-z])(?=.*[A-Z]).*/', $data['pswd'])){
										
										if($data['pswd'] == $data['retypepswd']){
											
											require('includes/db_connect.php');
											
											$username = $data['userName'];
											
												$sql = mysqli_query($conn, "SELECT count(*) FROM members WHERE username = '$username'");
												$row = mysqli_fetch_row($sql);
												
												if($row[0] == 0){
													
													$salt =  substr(md5(microtime()),rand(0,26),128);
														$pepper = substr(md5(microtime()),rand(0,26),128);
														$garlic = $salt.'!@#$%^&*()_+-'.$pepper;
														
														$code = md5(str_shuffle($garlic));
														
														$email = $data['email'];
														
														$headers = 'From: <activate@auctionhunterpro.com>';
														$headers .= "Reply-To: No Reply \r\n";
														$headers .= "MIME-Version: 1.0\r\n";
														$headers .= 'X-Mailer: PHP/' . phpversion();
														$headers .= "Content-Type: text/html; charset=ISO-8859-1\r\n";
														$subject = 'Your Auction Hunter Pro Registration';
														
														$message = "<p>Thank you for registering at Auction Hunter Pro.</p>";
														$message .= '<p>You must activate your account before you can login, please click <a href="http://auctionhunterpro.com/activate.php?activationCode='.$code.'&email='.$email.'">THIS LINK</a> to continue.</p>';
														$message .= 'If you can not view HTML email you must enter the following mannually at  http://auctionhunterpro.com/activate.php</p>';
														$message .= '<p>Activation Code: '.$code.'</p>';
														$message .= '<p>Email: '.$email.'</p>';
														$message .= 'If you have any problems activating your account please feel free to contact us at Support@AuctionHunterPro.com';
														
														if(@mail($email, $subject, $message, $headers)){
															
															$hash = password_hash($getpassword, PASSWORD_DEFAULT);
															
															$role = $data['role'];
															
															$sql = mysqli_query($conn, "INSERT INTO members
																(id, username, email, password, active, code, role, setup_complete)
															VALUES 
																('', '$username', '$email', '$hash', '0', '$code', '$role', '0')	
															");
															$numrows = mysqli_affected_rows($conn);
															
																if($numrows === 1){
																	
																	echo '<div class="red t-center">Registration Sucessful!</div><div class="t-center">An email has been sent to the email address you supplied, <span class="red">you must activate your account</span> before logging in for the first time.</div>';
																	
																} else $errormsg = 'An error has occured please contain Customer Support.';
															
														}else{ $errormsg = "There was an error sending your activation email."; }
													
												}else { $errormsg = 'Sorry but this username is already in use.'; }
												
											mysqli_close($conn);
											
										}else { $errormsg = "Your passaords do not match."; }
										
									}else { $errormsg = 'Your Password must contain 1 uppercase letter, 1 lowercase letter and 1 number.'; }
								
								}else { $errormsg = 'This is not a valid email address.'; }
								
							}else { $errormsg = "You must enter an email address"; }
							
						}else { $errormsg = "Username may not contain spaces."; }
						
					}else {$errormsg = "Usernames may only contain letters and numbers.";}
					
				}else { $errormsg = "A username is required."; }
			
			}else { $errormsg = 'You must chose a role, either buyer or auctioneer.'; }
			
			
		}else { echo $registerForm; }
		
		if(!empty($errormsg)){ echo '<div class="red">'.$errormsg.'</div>'.$registerForm ; }

echo ‘

’;

include(‘pg_bot.php’);[/php]

The code functions as I intended but my fear it is not enough sanitation or I have other mistakes. So there it is, where am I making mistakes?


#2

Enough sanitation for what? It will depend on what you’re actually using it for :slight_smile:

[hr]

I suggest reading up on http://www.phptherightway.com/

[hr]

A few comments

It’s generally considered good convention to separate logic and view. So at the very least it’s advised to have the PHP code above the output data in the file, it’s usually best to separate the logic and view into their own (controller and view) files. Apart from the good convention of separation of concerns this helps us as IDEs will now be able to highlight the HTML properly.

Consider this code

[php]<?php
include (‘pg_top.php’);

if (isset($POST[‘submitbtn’])) {
$data = sanitizeAll();
if (!empty($data[‘role’])) {
if (!empty($data[‘userName’])) {
if (ctype_alnum($data[‘userName’])) {
if (!preg_match(’/\s/’, $data[‘userName’])) {
if (!empty($data[‘email’])) {
if (filter_var($data[‘email’], FILTER_VALIDATE_EMAIL)) {
if (!preg_match(’/(?=.{8,})(?=.\d)(?=.[a-z])(?=.[A-Z])./’, $data[‘pswd’])) {
if ($data[‘pswd’] == $data[‘retypepswd’]) {
require (‘includes/db_connect.php’);
$username = $data[‘userName’];
$sql = mysqli_query($conn, "SELECT count() FROM members WHERE username = ‘$username’");
$row = mysqli_fetch_row($sql);
if ($row[0] == 0) {
$salt = substr(md5(microtime()) , rand(0, 26) , 128);
$pepper = substr(md5(microtime()) , rand(0, 26) , 128);
$garlic = $salt . '!@#$%^&
()
±’ . $pepper;
$code = md5(str_shuffle($garlic));
$email = $data[‘email’];
$headers = ‘From: activate@auctionhunterpro.com’;
$headers.= “Reply-To: No Reply \r\n”;
$headers.= “MIME-Version: 1.0\r\n”;
$headers.= ‘X-Mailer: PHP/’ . phpversion();
$headers.= “Content-Type: text/html; charset=ISO-8859-1\r\n”;
$subject = ‘Your Auction Hunter Pro Registration’;
$message = “

Thank you for registering at Auction Hunter Pro.

”;
$message.= ‘

You must activate your account before you can login, please click THIS LINK to continue.

’;
$message.= ‘If you can not view HTML email you must enter the following mannually at http://auctionhunterpro.com/activate.php’;
$message.= ‘

Activation Code: ’ . $code . ‘

’;
$message.= ‘

Email: ’ . $email . ‘

’;
$message.= ‘If you have any problems activating your account please feel free to contact us at Support@AuctionHunterPro.com’;
if (@mail($email, $subject, $message, $headers)) {
$hash = password_hash($getpassword, PASSWORD_DEFAULT);
$role = $data[‘role’];
$sql = mysqli_query($conn, "INSERT INTO members
(id, username, email, password, active, code, role, setup_complete)
VALUES
(’’, ‘$username’, ‘$email’, ‘$hash’, ‘0’, ‘$code’, ‘$role’, ‘0’)
");
$numrows = mysqli_affected_rows($conn);
if ($numrows === 1) {
echo ‘
Registration Sucessful!
An email has been sent to the email address you supplied, you must activate your account before logging in for the first time.
’;
}
else $errormsg = ‘An error has occured please contain Customer Support.’;
}
else {
$errormsg = “There was an error sending your activation email.”;
}
}
else {
$errormsg = ‘Sorry but this username is already in use.’;
}
mysqli_close($conn);
}
else {
$errormsg = “Your passaords do not match.”;
}
}
else {
$errormsg = ‘Your Password must contain 1 uppercase letter, 1 lowercase letter and 1 number.’;
}
}
else {
$errormsg = ‘This is not a valid email address.’;
}
}
else {
$errormsg = “You must enter an email address”;
}
}
else {
$errormsg = “Username may not contain spaces.”;
}
}
else {
$errormsg = “Usernames may only contain letters and numbers.”;
}
}
else {
$errormsg = “A username is required.”;
}
}
else {
$errormsg = ‘You must chose a role, either buyer or auctioneer.’;
}
}

?>

User Registration

<?php if (!empty($errormsg)) { ?>
<?php $errormsg ?>
<?php } ?>
I am a buyer!
I am a auctioneer!

Username

Email

Password

Retype Password

<?php include ('pg_bot.php'); ?>[/php]

Second you have way too many indention levels from (the above) line 6-13. Having this structure also results in only one validation kicking off at a time. Potentially leaving the user submitting the form n times before having seen all errors. This can be changed to something like this in order to resolve both problems:

[php]<?php
include (‘pg_top.php’);

if (isset($_POST[‘submitbtn’])) {
$data = sanitizeAll();

$errors = array();
if (empty($data[‘role’])) {
$errors[] = ‘You must chose a role, either buyer or auctioneer.’;
}

if (empty($data[‘userName’])) {
$errors[] = “A username is required.”;
}

if (ctype_alnum($data[‘userName’])) {
$errors[] = “Usernames may only contain letters and numbers.”;
}

if (!preg_match(’/\s/’, $data[‘userName’])) {
$errors[] = “Username may not contain spaces.”;
}

if (!empty($data[‘email’])) {
$errors[] = “You must enter an email address”;
}

if (filter_var($data[‘email’], FILTER_VALIDATE_EMAIL)) {
$errors[] = ‘This is not a valid email address.’;
}

if (!preg_match(’/(?=.{8,})(?=.\d)(?=.[a-z])(?=.[A-Z])./’, $data[‘pswd’])) {
$errors[] = ‘Your Password must contain 1 uppercase letter, 1 lowercase letter and 1 number.’;
}

if ($data[‘pswd’] == $data[‘retypepswd’]) {
$errors[] = “Your passwords do not match.”;
}

if (empty($errors)) {
require (‘includes/db_connect.php’);

$username = $data['userName'];
$sql = mysqli_query($conn, "SELECT count(*) FROM members WHERE username = '$username'");
$row = mysqli_fetch_row($sql);

if (!empty($row[0]) {
  $errors[] = 'Sorry but this username is already in use.';
}
else {
  $salt = substr(md5(microtime()) , rand(0, 26) , 128);
  $pepper = substr(md5(microtime()) , rand(0, 26) , 128);
  $garlic = $salt . '!@#$%^&*()_+-' . $pepper;
  $code = md5(str_shuffle($garlic));
  $email = $data['email'];
  $headers = 'From: <activate@auctionhunterpro.com>';
  $headers.= "Reply-To: No Reply \r\n";
  $headers.= "MIME-Version: 1.0\r\n";
  $headers.= 'X-Mailer: PHP/' . phpversion();
  $headers.= "Content-Type: text/html; charset=ISO-8859-1\r\n";
  $subject = 'Your Auction Hunter Pro Registration';
  $message = "<p>Thank you for registering at Auction Hunter Pro.</p>";
  $message.= '<p>You must activate your account before you can login, please click <a href="http://auctionhunterpro.com/activate.php?activationCode=' . $code . '&email=' . $email . '">THIS LINK</a> to continue.</p>';
  $message.= 'If you can not view HTML email you must enter the following mannually at  http://auctionhunterpro.com/activate.php</p>';
  $message.= '<p>Activation Code: ' . $code . '</p>';
  $message.= '<p>Email: ' . $email . '</p>';
  $message.= 'If you have any problems activating your account please feel free to contact us at Support@AuctionHunterPro.com';
  if (@mail($email, $subject, $message, $headers)) {
    $hash = password_hash($getpassword, PASSWORD_DEFAULT);
    $role = $data['role'];
    $sql = mysqli_query($conn, "INSERT INTO members
      (id, username, email, password, active, code, role, setup_complete)
      VALUES 
      ('', '$username', '$email', '$hash', '0', '$code', '$role', '0')
      ");
    $numrows = mysqli_affected_rows($conn);
    if ($numrows === 1) {
      echo '<div class="red t-center">Registration Sucessful!</div><div class="t-center">An email has been sent to the email address you supplied, <span class="red">you must activate your account</span> before logging in for the first time.</div>';
    }
    else $errormsg = 'An error has occured please contain Customer Support.';
  }
  else {
    $errormsg = "There was an error sending your activation email.";
  }
}

mysqli_close($conn);

}

}

?>

User Registration

<?php foreach ($errors as $error) { ?>
<?php $error ?>
<?php } ?>
I am a buyer!
I am a auctioneer!

Username

Email

Password

Retype Password

<?php include ('pg_bot.php'); ?>[/php]

Then I’d move lines 7-38, and 51-84 into their own functions. leaving us with something like this

[php]<?php
include (‘pg_top.php’);

if (isset($_POST[‘submitbtn’])) {
$data = sanitizeAll();

$errors = validateRegistrationForm($data);

// only register a user if the registration form is valid (has no errors)
if (empty($errors)) {
// register function should return array($user, $errors);
list($user, $errors) = registerUserFromRegistrationData($data);
}

// only send welcome message if we were able to register the user
if (empty($errors)) {
$errors = sendWelcomeEmailToUser($user);
}

}

?>

User Registration

<?php foreach ($errors as $error) { ?>
<?php $error ?>
<?php } ?>
I am a buyer!
I am a auctioneer!

Username

Email

Password

Retype Password

<?php include ('pg_bot.php'); ?>[/php]

#3

Well let’s see, what am I using it for. The short answer, storage and retrieval of user info for login purposes.

The long answer. It is a site that will have up to 9000 registered members, eventually. Additionally we expect roughly 12k-25k page views per day. We have a very limited number of competitors in our market, these are the low end of their numbers.

This particular portion of the site will allow users both auctioneers and buyers access to other parts of the site that are not public, as well as give us firm metrics data about users and locations so that we know where to target our marketing. It is just very first step in the registration process, followed up by activation and account setup that collects more in depth information to be used in our directory.

Everything you suggested makes perfect sense and is exactly why I came here with the question!


#4

You should actually strike the term “sanitization” from your vocabulary and forget all this [tt]FILTER_SANITIZE_*[/tt] voodoo.

You cannot “clean” data like you would clean dirty shoes. This notion is very naïve and downright harmful, because it gives you a false sense of security and will actually damage your data. For example, see what your beloved [tt]FILTER_SANITZE_STRING[/tt] does to a perfectly legitimate username:
[php]<?php

// before: a nice username
$username = ‘I <3 PHP’;

// unleash the filter_var() mangler
$username = filter_var($username, FILTER_SANITIZE_STRING);

// after: the username is entirely broken, it now says "I "
var_dump($username);[/php]
Oops. And that’s just a silly example. Imagine what happens if your application deals with critical data for a couple of days, and then you suddenly realize that your entire database is filled with unfixable garbage instead of the actual data. You don’t want that.

Even worse, none of those filter gymnastics will make you secure. While [tt]FILTER_SANITIZE_STRING[/tt] sounds like some kind of universal security function, it’s actually just meant for HTML contexts, and it can’t even get that right. So when you use those “sanitized” strings in SQL queries or HTML markup, you’ll quickly end up with SQL injections and cross-site scripting vulnerabilities all over the place.

So I suggest you forget everything that starts with “FILTER_SANITIZE” and try to get a more precise understanding of how to deal with user input:

[ul][]Never change the user input. You may validate and reject it, but never change it. Silently altering the input will corrupt your data and can lead to catastrophic mistakes. Imagine the following scenario: The user asks you to delete a certain record from your database, but somehow they get the numeric ID wrong and request the removal of record “52~7”. What do you do? Take a shot in the dark and remove the record “527” instead? Or maybe “52”? That’s obviously a terrible idea. The only valid answer is: You reject the input so that the user can provide the correct ID.
[
]Security is context-sensitive. There is no magical forever-secure filter. For example, HTML and SQL are two entirely different languages, so they require two entirely different security strategies. You also have to take additional factors like the character encoding into a account. A function that expects, say, UTF-8 strings will fail miserably when it encounters ISO 8859-1 strings. So context is everything.[/ul]

Regarding your code: Sorry, but I think this is unfixable. I’ve tried to make a list of defects and suggestions, but then I gave up, because pretty much every line is a defect. Your HTML is vulnerable to cross-site scripting, your queries are vulnerable to SQL injections, your “random numbers” can easily be guessed, your e-mails expose your exact PHP version and are vulnerable to cross-site scripting as well etc.

I’m not saying this to put you down. A lot of those issues probably boil down to crappy online tutorials and the weirdness of PHP. Nevertheless: If you want your website to survive more than 5 minutes, you’ll need to learn the basics of security and start from scratch.


#5

but that’s two different scenarios. If you use prepared statements / parameterized queries (like you should) you will not need to do any input validation.

Sanitazion for output depends on the context (html, js, cli etc)


#6

Oh lord, no. :frowning:

I thought this was just a hobby project or maybe homework for school, and now you’re saying this code will be used in production? For 9,000 users?

Wow. I already knew that the IT industry is fudged up beyond repair, but this is taking it to a new level. What’s next? Banking software written by preschoolers?

At least the spam mafia won’t have to worry about getting new addresses.


#7

The only thing useful and constructive you have said was your comment about the following being a valid username. [php]$username = ‘I <3 PHP’;[/php] I can not think of anywhere that would be a valid username. Special chars and white spaces. Nope. And all that did was remind me to check for special chars and throw an errormsg if they are found. So why don’t you take your comments elsewhere if you can’t be useful.


#8

I will get there with prepared statements. I haven’t written any useful code in YEARS(think late 90’s), so I am having to relearn as I go for this project. Right now I am trying to get the basics in place. I will go back later and rewrite it again. I know it seems like a waste of time but I need to relearn everything and this is how I learn it best. Doing it and then improving upon it in the next go around.

With that said, the following has me confused.[php]list($user, $errors)…[/php]I can’t figure out for the life of me where $user is coming from.


#9

You are using deprecated code and md5 was hacked like 50 years ago. You need to use PDO with prepared statements. The code pretty much needs to be started from scratch. It would take way more time to try and “fix” it.


#10

Thanks for skimming… Yes, I know the code is depreciated. Had you read you would have seen that I haven’t written any code since the later 90’s and I am having to relearn everything and that is the reason I am here. Looking for CONSTRUCTIVE criticism that actually points me in a direction of the flaws, not just states the obvious.


#11

Sure, we all know how we learn best, and if you’ve done this before then you’re probably able to handle it just fine. Just as long as you know there’s a new way of doing things before you start running code in production.

A big change since “the good old days” is that international competition has pressured price down, which has made it “impossible” to deliver 100% custom applications to every client.This is why most use a very modular approach of writing applications these days. With web and even java script projects being assembled by including lots of dependencies. The instant tradeoffs are that you (may) get individual dependencies that have lots of contributors, are properly isolated, have unit tests and “know” which version of “some other dependency” they’re compatible with.

Of course there are also the not so good dependencies, but a quick search on google and a glimpse at their github page should reveal what the status is (note, open issues aren’t always a bad thing).

For PHP what you need to look into is a package manager called Composer. I believe it was one of the things that saved PHP, or at least breathed a long awaited new life into it.

That’s the reason I included the comment. I never implemented the functions so the code isn’t there :wink:

[php] // register function should return array($user, $errors);
list($user, $errors) = registerUserFromRegistrationData($data);[/php]


#12

You have been pointed in the right direction. Apparently you cant see it.

Right Direction:

You need to use PDO with prepared statements.

Do I actually need to google links for you?


#13

To be fair whitespaces are allowed in the usernames here… :stuck_out_tongue:


#14

How dare you. I demand that all whitespace be removed from this site. And I want at least three layers of [tt]FILTER_SANITIZE_STRING[/tt] for my own safety. Because that’s how programming works.

You kids with your silly prepared statements.


#15

Thank you :slight_smile:


#16

How dare they! :o :stuck_out_tongue:

I should have been more clear. I get you never implemented the function, I guess I should have said I have no freaking idea what that function needs to do…LOL I will figure it out when I get that far. I decided to take a look at prepared statements and see if I can speed up the time spent learning new concepts, it would be nice to have a functioning site before next year… (I actually have a functioning site that is being tested by a couple local auctioneers, so far the premise works, it just need to be updated to today’s standards. Since it is just a directory if SHTF it isn’t a huge deal with only a couple users and a couple thousand listings. Later once it is public SHTF would be a HUGE deal.)


#17

If you view the second last code block I posted it’s kinda clear how you can divide that logic into (at least) three parts, validating the form fields, registering the user, and sending the welcome email.

So something like this (just copy pasted, not working). I considered checking if the username was already registered to be part of the “validate form fields” function

[php] require (‘includes/db_connect.php’);

      $hash = password_hash($getpassword, PASSWORD_DEFAULT);
      $role = $data['role'];
      $sql = mysqli_query($conn, "INSERT INTO members
        (id, username, email, password, active, code, role, setup_complete)
        VALUES 
        ('', '$username', '$email', '$hash', '0', '$code', '$role', '0')
        ");
      $numrows = mysqli_affected_rows($conn);
      if ($numrows === 1) {
        echo '<div class="red t-center">Registration Sucessful!</div><div class="t-center">An email has been sent to the email address you supplied, <span class="red">you must activate your account</span> before logging in for the first time.</div>';
      }
      else $errormsg = 'An error has occured please contain Customer Support.';
    }
    else {
      $errormsg = "There was an error sending your activation email.";
    }
  }

  mysqli_close($conn);

return array($user, $errors);[/php]


#18

Oh I would have gotten here eventually, I just fell into the deep hole pf prepared statements… argh!

I am trying, trying does not constitute succeeding at, creating functions to handle insert, select, update and delete prepared statements. I am stuck on insert at the moment…LOL

Something about wrong parameter counts… I hate prepared statements! 2 values to insert = 3 bind_param values “ss”,$u ,$p evidently I am missing something somewhere else. Next year might turn into the year after for this thing…lol


#19

If you post your current code it’s at least someone here spots the binding error you’re having :slight_smile:


#20

I figured I would try it a few more times before going that route. Part of the learning process, a little struggle does the mind good. Well, that and you never forget it…lol