Registration, Validation & Storage functions

Question

The last time I wrote any serious code was the 90's, so a lot has changed and I am finding I am having to relearn pretty much everything.

I have leaned a lot from previous posts and this is the result. So as the title states I need a code review to help me identify where I am making mistakes by today's standards. To thwart any PDO comments, I have decided against it for now.

One note, the ValidateForms() function is just one big catch all at the moment, later I will break it down and put the parts and pieces into their corresponding functions. And yes, there are errors in that function as I am fixing them write the corresponding page/function. But the registration page and the corresponding functions do successfully complete their tasks.

It's a lot, I know but I am looking for constructive criticism for overall security , better/more validation, ways to compact the code and just overall improvements I should make.

The User Registration Page:

include('pg_top.php');

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

    $errors = ValidateForms();

    if(empty($errors)){
        $errors = UserRegistration();
    }
}
if($errors[0] == "You must activate your account before logging in. Please check your email."){
    $success = $errors[0];
    echo $success;
}else {
?>

<h1>User Registration</h1>
<div class="allForms">
    <?php 
    if(!empty($errors)){
        foreach($errors as $error){
        echo '<div class="red">- '.$error.'</div>';
        }
    }
    ?>
    <form action="<?php echo esc_self(); ?>" method="POST" id="registerForm">
        <div><input name="role" type="radio" value="buyer" required /> I am a buyer!</div>
        <div><input name="role" type="radio" value="auctioneer" required checked /> I am a auctioneer!</div>
        <p>
            <label>Username</label>
                <br />
            <input type="text" name="userName" value="<?php echo $_POST['userName']; ?>" placeholder="Username" required />
        </p>
        <p>
            <label>Email</label>
                <br />
            <input class="email" type="email" name="email" value="<?php echo $_POST['email']; ?>" placeholder="Email Address" required />
        </p>
        <p>
            <label>Password</label>
                <br />
            <input type="password" name="pswd" value="" pattern="(?=.*\d)(?=.*[a-z])(?=.*[A-Z]).{8,}" placeholder="Password" title="Must contain at least one number and one uppercase and lowercase letter, and at least 8 or more characters" required />
        </p>
        <p>
            <label>Retype Password</label>
                <br />
            <input type="password" name="retypepswd" value="" placeholder="Retype Your Password" required />
        </p>
            <input type="submit" name="submitbtn" value="Register" />
    </form>
</div>

<?php
}
include('pg_bot.php');
?>

User input validation

function ValidateForms() {

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

$errormsg = array();

foreach($_POST as $key => $value){
        if($key == 'auction_desc'){ continue; }
        if($key == 'imageUploads'){ continue; }

        if($value != strip_tags($value)) {
            $errormsg[] = 'HTML Tags are not allowed';
        }
    }
if(in_array($_POST['userName'] || $_POST['pswd'] || $_POST['activationCode'] || $_POST['licenseNum'] || $_POST['street'],$_POST)){

    $toCheck = array($_POST['userName'],$_POST['pswd'],$_POST['activationCode'],$_POST['licenseNum'], $_POST['street']);

    foreach($toCheck as $key => $var) {
        if(empty($var)){ continue; }
            if(!ctype_alnum($var)){
            $errormsg[] = $var.' is not alphnumeric';
        }
    }
}
if(in_array($_POST['firstName']||$_POST['ampm']||$_POST['role'],$_POST)){

    $toCheck = array($_POST['firstName'],$_POST['ampm'],$_POST['role']);

    foreach($toCheck as $key => $var) {
        if(empty($var)){ continue; }
            if(!ctype_alpha($var)){
            $errormsg[] = $var.' must only contain letters.';
        }
    }
}
if(in_array($_POST['zip']||$_POST['month']||$_POST['day']||$_POST['year']||$_POST['hour']||$_POST['min']||$_POST['sort'],$_POST)){

    $toCheck = array($_POST['zip'],$_POST['month'],$_POST['day'],$_POST['year'],$_POST['hour'],$_POST['min'],$_POST['sort']);

    foreach($toCheck as $key => $var) {
        if(empty($var)){ continue; }
            if(!ctype_digit($var)){
            $errormsg[] = 'You have an error in either zip code or auction date/time.';
        }
    }
}

if(isset($_POST['city'])){
    if (!preg_match("/^[A-Za-z\ \']+$/",$_POST['city'])) {
        $errormsg[] = 'City name may only contain letters, single quote';
    }
}

if(isset($_POST['state'])){
    if (!preg_match("/^[A-Za-z\ \']+$/",$_POST['state'])) {
        $errormsg[] = 'City name may only contain letters, single quote';
    }
}

if(isset($_POST['lastName'])){
    if (!preg_match("/^[A-Za-z\\- \']+$/",$_POST['lastName'])) {
        $errormsg[] = 'Last name may only contain letters,a hyphen or a single quote';
    }
}

if(isset($_POST['busName'])){
    if (!preg_match("/^[0-9A-Za-z\\- \&\.\\']+$/",$_POST['busName'])) {
        $errormsg[] = "Business name may only contain letters, numbers and . & ' -";
    }
}
if(isset($_POST['title'])){
    if (!preg_match("/^[0-9A-Za-z\\- \&\.\\']+$/",$_POST['title'])) {
        $errormsg[] = "Business name may only contain letters, numbers and . & ' -";
    }
}
if(isset($_POST['bio'])){
    if (!preg_match("/^[0-9A-Za-z\\- \&\.\,\(\)\$\?\%\\']+$/",$_POST['bio'])) {
			$errormsg[] = "Bio name may only contain letters, numbers and ?$%&()-,.";
		}
	}
	if(isset($_POST['question'])){
		if (!preg_match("/^[0-9A-Za-z\\- \&\.\,\(\)\$\?\%\\']+$/",$_POST['question'])) {
        $errormsg[] = "Question name may only contain letters, numbers and ?$%&()-,.";
    }
}

if(isset($_POST['category_1'])){
    if (!preg_match("/^[A-Za-z\/\&\.\']+$/",$_POST['category_1'])) {
        $errormsg[] = "Bio name may only contain letters, numbers and ?$%&()-,.";
    }
}

if(isset($_POST['category_2'])){
    if (!preg_match("/^[A-Za-z\/\&\.\']+$/",$_POST['category_2'])) {
        $errormsg[] = "Bio name may only contain letters, numbers and ?$%&()-,.";
    }
}
if(isset($_POST['category_3'])){
    if (!preg_match("/^[A-Za-z\/\&\.\']+$/",$_POST['category_3'])) {
        $errormsg[] = "Bio name may only contain letters, numbers and ?$%&()-,.";
    }
}

if(isset($_POST['phone'])){
    if(!preg_match($toMatch,$_POST['phone'])){
        $errormsg[] = 'Phone number must be <b>(###)###-####</b> format';
    }
}
if(isset($_POST['fax'])){
    if(!preg_match($toMatch,$_POST['fax'])){
        $errormsg[] = 'Fax number must be <b>(###)###-####</b> format';
    }
}

if (isset($_POST['email']) && !filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)) {
    $errormsg[] = "The email address entered is invalid."; 
}
//does pswd contain 1 upper, 1 lower and 1 number_format
if(isset($_POST['pswd']) && !preg_match('/(?=.{8,})(?=.*\d)(?=.*[a-z])(?=.*[A-Z]).*/', $_POST['pswd'])){
    $errormsg[] = 'Password must contain one uppercase and lowercase letter and one number.';
}

//Is state length longer than 2 chars
if(isset($_POST['state']) && strlen($_POST['state'])!=2){
    $errormsg[] = 'State must be abbreviated to two characters';
}

//is zipcode longer than 5 chars
if(isset($_POST['zip']) && strlen($_POST['zip']) !=5){
    $errormsg[] = 'Zipcode must be 5 digits.';
}

//month length 
if(isset($_POST['month']) && strlen($_POST['month']) !=2){
    $errormsg[] = 'Something is wrong with the month field.';
}

//day length
if(isset($_POST['day'])){
    if(strlen($_POST['day']) >2 || strlen($_POST['day'])< 1){
    $errormsg[] = 'Something is wrong with the day field.';
    }
}

//year length
if(isset($_POST['year']) && strlen($_POST['year']) !=4){
    $errormsg[] = 'Something is wrong with the year field.';
}

//hour length
if(isset($_POST['hour'])){
    if(strlen($_POST['hour']) >2 || strlen($_POST['hour'])< 1){
    $errormsg[] = 'Something is wrong with the hour field.';
    }
}

if(isset($_POST['min']) && strlen($_POST['min']) !=2){
    $errormsg[] = 'Something is wrong with the minute field.';
}

if(isset($_POST['ampm']) && strlen($_POST['ampm']) !=2){
    $errormsg[] = 'Something is wrong with the am/pm field.';
}

if (isset($_POST['wsaddr']) && filter_var($_POST['wsaddr'], FILTER_VALIDATE_URL) === false) {
 $errormsg[] = 'The websiite address you entered is invalid.';
}

if(isset($_POST['bio']) && strlen($_POST['bio'])>255){
    $errormsg[] = 'Bio can not be longer than 255 characters';
}

//is title longer than 75 chars.
if(isset($_POST['title'])){
    if(strlen($_POST['title'])>75){
    $errormsg[] = 'Title can not be longer than 75 characters';
    }
}

if(isset($_POST['auction_desc'])){
    if(strlen($_POST['auction_desc'])>5000){
    $errormsg[] = 'Auction description is limited to 3000 characters';
    }
}

if(isset($_POST['sort'])){
    if(strlen($_POST['sort']) >1){
    $errormesg[] = 'Something has gone wrong with the sort feature, please try again.';
    }
}
if(isset($_POST['role'])){
    if(strlen($_POST['role']) > 10 || strlen($_POST['role']) < 5){
        $errormsg[] = 'Something has gone wrong please try again.';
    }
}
return $errormsg;
}

and finally comparisons & storage

function UserRegistration(){

 $errormsg = array();

if(!isset($_POST['role'])){
    $errormsg[] = 'You must chose if you are buyer or an auctioneer.';
}
if(!isset($_POST['email'])){
    $errormsg[] = 'You must enter a valid email address.';
}
if(!isset($_POST['pswd'])){
    $errormsg[] = 'You must enter a password.';
}
if(!isset($_POST['retypepswd'])){
    $errormsg[] = 'You must enter a password.';
}
if($_POST['pswd'] !== $_POST['retypepswd']){
    $errormsg[] = 'Your passwords do not match.';
}

if(!empty($errormsg)){

    return $errormsg;

}else {

    $token = md5(uniqid(mt_rand(), true));

    $hash = password_hash($_POST['pswd'], PASSWORD_DEFAULT);

    $a = 0;

    require('includes/db_connect.php');

    $sql = "SELECT username FROM members WHERE username=?";

    if(!($stmt = $conn->prepare($sql))){
         $errormsg[] = "Prepare failed: (" . $mysqli->errno . ") " . $mysqli->error;
        }

    if(!$stmt->bind_param("s",$_POST['userName'])){
        $errormsg[] =  "Binding parameters failed: (" . $stmt->errno . ") " . $stmt->error;
        }

    if(!$stmt->execute()) {
        $errormsg[] = "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
        }

    $stmt->bind_result($username);

    $stmt->fetch();

    $stmt->close();

    if($_POST['userName'] == $username){

        $errormsg[] = "This username taken, please choose another.";

    }else {

        $sql = "INSERT INTO members (username, email, password, active, code, role, setup_complete) VALUES (?, ?, ?, ?, ?, ?, ?)";

        if(!($stmt = $conn->prepare($sql))){
             $errormsg[] = "Prepare failed: (" . $mysqli->errno . ") " . $mysqli->error;
            }

        if(!$stmt->bind_param("sssissi",$_POST['userName'],$_POST['email'],$hash,$a,$token,$_POST['role'],$a)){
            $errormsg[] =  "Binding parameters failed: (" . $stmt->errno . ") " . $stmt->error;
            }

        if(!$stmt->execute()) {
            $errormsg[] = "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
            }

        if(empty($errormsg)){

            $rows = $stmt->affected_rows;
            }
        $stmt->close;
        $conn->close();

        if ($rows == 1) {
            SendActivationEmail($to,$token);
            }else {
                $errormsg[] = 'There was a database error Please contact support';
            }

        if(@mail($email, $subject, $message, $headers)){
            $errormsg[] = "You must activate your account before logging in. Please check your email.";
        }else { 
            $errormsg[] = 'There was an error sending your activation email.';
        }
    }

    return $errormsg;

    }
}

Answer 1

Security

XSS

<input type="text" name="userName" value="<?php echo $_POST['userName']; ?>" placeholder="Username" required />
<input class="email" type="email" name="email" value="<?php echo $_POST['email']; ?>" placeholder="Email Address" required />

and

$toCheck = array($_POST['firstName'],$_POST['ampm'],$_POST['role']);

foreach($toCheck as $key => $var) {
    if(empty($var)){ continue; }
        if(!ctype_alpha($var)){
        $errormsg[] = $var.' must only contain letters.';
    }
}
foreach($errors as $error){
    echo '<div class="red">- '.$error.'</div>';
}

These are open to reflected XSS (you can try it by inputting "> xss<script>alert(1)</script> as username).

I know that you have some validation, but validation is not the preferred method of handling XSS (and in this case it's not really working).

You should always use htmlspecialchars($string, ENT_QUOTES, 'UTF-8'); right at the moment when you are echoing variables/inserting them into HTML.

As to your validation: You check if there are tags in the input, and if there are, you display an error. But you still echo the payload! Also, even if you would not echo it if tags are present, an attacker could break out of existing tags via eg " autofocus onfocus="alert(1).

All the other checks in ValidateForms are nice to have but should not be relied upon for primary security.

Misc

Your code seems safe from SQL injection, and you use bcrypt for password hashing, both of which are good.

• @mail($email, $subject, $message, $headers): The variables do not seem to exist in the code you posted. But you should check if they are open to email injection.
• don't echo db error messages to the user. A legitimate user will not know what to do with them, and an attacker may use the information gained in further attacks.
• neither uniqid nor mt_rand produce cryptographically secure values. I would just use something like openssl_random_pseudo_bytes.
• Personally, I do not like restrictions on passwords. Attackers gain information through them, with which they can personalize their wordlists for easier bruteforcing, and users will generally choose bad passwords either way.

Other

• You really don't want to use @. If something goes wrong, you generally want to know about it, not suppress it.
• label should always have a for attribute for usability.
• If you don't need the key of an array, just write foreach($toCheck as $var) instead of foreach($toCheck as $key => $var).
• do not be too strict with your validation. For example, there are lots of people who have more than letters in their names (same goes for companies, etc).
• City name may only [...] for the state check is a copy-paste error.
• Something is wrong is never a good error message. Try to be specific to help your users (eg Days must be between 1 and 99 (although is that really what you want to check?)).
• You have quite a lot of newlines, which stretches your code and makes it a bit harder to read. Try to build logical units which you order into blocks instead of writing each statement on its own line.
• Your spacing is sometimes inconsistent (eg strlen($_POST['title'])>75 vs strlen($_POST['sort']) >1 vs strlen($_POST['role']) > 10.
• Your indentation is also sometimes inconsistent (missing indentation, closing } indented, etc).
• function names should start with a lowercase letter (classes start with an uppercase letter).
• value="<?php echo $_POST['userName']; ?>" produces a warning when the value does not exist, which screws up the form. You should check if the value is set before echoing it (html encoded).