2
\$\begingroup\$

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;

    }
}
\$\endgroup\$
  • \$\begingroup\$ Please don't invalidate answers by changing your code. \$\endgroup\$ – Ethan Bierlein Nov 11 '15 at 3:22
  • \$\begingroup\$ Correcting it doesn't invalidate the answer, the answers are still correct. It makes it so other answers or comments don't repeat what others have correctly already covered. \$\endgroup\$ – user3154948 Nov 11 '15 at 3:56
  • \$\begingroup\$ Other people can see answers. Stuff won't be repeated. Again, please, don't invalidate answers. \$\endgroup\$ – Ethan Bierlein Nov 11 '15 at 3:57
1
\$\begingroup\$

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).
\$\endgroup\$
  • \$\begingroup\$ OK just starting to read.. but the very first thing I see is this: These are open to reflected XSS (you can try it by inputting "> xss<script>alert(1)</script> as username). I tried it and it gets kicked back with 2 errors produced in validation. they are - HTML Tags are not allowed and - > xss is not alphnumeric So before I go to much further you may have to explain how the validation isn't working because i am now confused. I have been working on this a couple days so I am partially blind to all that is involved at them moment... \$\endgroup\$ – user3154948 Nov 10 '15 at 22:30
  • \$\begingroup\$ @user3154948 You may have to disable the XSS checks of your browser for this to work (but you can't rely on these checks, as users may use old browsers, there may be bugs in the check, etc). In my experience, disabling these checks will be a lot easier in firefox than in chrome. \$\endgroup\$ – tim Nov 10 '15 at 22:34
  • \$\begingroup\$ Yeah disabling chromes web security didn't do it, BUT that is what IE is for! LOL So if I don't echo back the variable in the error will I be alright? Or no? So what do I do to stop this from happening in the first place? \$\endgroup\$ – user3154948 Nov 11 '15 at 1:32
  • \$\begingroup\$ So what can I do, if not eliminate, to at least try to thwart potential issues like the " autofocus onfocus="alert(1)? What other things should I be doing if validation should not be relied upon for primary security? I know csrf is one, but what else No, the SendActivationEmail() function was not included as I only have the basics in place and I know it needs A LOT of work yet. value="<?php echo $_POST['userName']; ?>" produces a warning when the value does not exist, which screws up the form. I don't see an error, could you give me a little more detail? \$\endgroup\$ – user3154948 Nov 11 '15 at 2:15
  • \$\begingroup\$ @user3154948 like I said, for XSS, always use htmlspecialchars (there are a couple of places where this is not enough, but in general, this is secure. And yes, CSRF is another issue you want to look out for, in addition to XSS and SQL injection. The owasp top ten is always a good starting point to get to know security issues. The warning: did you disable showing warnings? \$\endgroup\$ – tim Nov 11 '15 at 9:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.