what is wrong with my php login code? Beyond Security Support keeps sending me warnings

Question

I keep getting emails from Beyond Security Support website telling me of all my site security issues. First on the list is the word 'username' in my register.php script. I can post the result of that error if needed. I would at some point like to change it to secure login using https instead of http but for now i just want to solve this issue. It tells me to use prepared statments which it already does as you can see from the code. It would be nice to make it secure from sql injection and made rather simple. this is the username section of the code in php.

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

//very basic validation
if (!preg_match("/^[a-zA-Z0-9_-]*$/",$_POST['username'])) {
    $error[] = 'Usernames can only be numbers, letters, and characters _ -';
}

if(strlen($_POST['username']) < 3){
    $error[] = 'Username is too short,3 chars min.';
} elseif (strlen($_POST['username']) > 25){
    $error[] = 'Username is too long, 25 chars max.';
}
else
    {
    $stmt = $db->prepare('SELECT username FROM members WHERE username = :username');
    $stmt->execute(array(':username' => $_POST['username']));
    $row = $stmt->fetch(PDO::FETCH_ASSOC);

    if(!empty($row['username'])){
        $error[] = 'Username provided is already in use.';
    }
}

and this the login form below

            <form role="form" method="post" action="" autocomplete="off">
            <h2>Please Sign Up</h2>
            <p>Already a member? <a href='/login.php'>Login</a></p>
            <hr>

            <?php
            //check for any errors
            if(isset($error)){
                foreach($error as $error){
                    echo '<p class="bg-danger">'.$error.'</p>';
                }
            }

            //if action is joined show sucess
            if(isset($_GET['action']) && $_GET['action'] == 'joined'){
                echo "<h2 class='bg-success'>Registration successful, please check your email to activate your account.<br />Do not forget to check in your junk mail!.</h2>";
            }
            ?>

            <div class="form-group">
                <input type="text" name="username" id="username" class="form-control input-lg" placeholder="User Name" value="<?php if(isset($error)){ echo $_POST['username']; } ?>" tabindex="1">
            </div>
            <div class="form-group">
                <input type="email" name="email" id="email" class="form-control input-lg" placeholder="Email Address" value="<?php if(isset($error)){ echo $_POST['email']; } ?>" tabindex="2">
            </div>
            <div class="row">
                <div class="col-xs-6 col-sm-6 col-md-6">
                    <div class="form-group">
                        <input type="password" name="password" id="password" class="form-control input-lg" placeholder="Password" tabindex="3">
                    </div>
                </div>
                <div class="col-xs-6 col-sm-6 col-md-6">
                    <div class="form-group">
                        <input type="password" name="passwordConfirm" id="passwordConfirm" class="form-control input-lg" placeholder="Confirm Password" tabindex="4">
                    </div>
                </div>
            </div>
            <div class="form-group">
                <input type="text" name="question" id="question" class="form-control input-lg" placeholder="Fill in the missing word. A bird likes to ???? loudly!" value="<?php if(isset($error)){ echo $_POST['question']; } ?>" tabindex="5">
            </div>
            <div class="row">
                <div class="col-xs-6 col-md-6"><input type="submit" name="submit" value="Register" class="btn btn-primary btn-block btn-lg" tabindex="6"></div>
            </div>
        </form>

I wasnt sure if the whole form code was needed but thought i'd post it all 'just in case'. this is the report they sent me below.

We discovered vulnerabilities in the scripts listed below. Next to each script, there is a description of the type of attack that is possible, and the way to recreate the attack. If the attack is a simple HTTP GET request, you can usually paste it into your browser to see how it works. If it's a POST attack, the parameters for the POST request will be listed in square parenthesis.

Cross Site Scripting URL: http://www.finchkeeper.com/register.php Affected Parameter: username Vector Used: ">alert('foo'); Pattern found: ">alert('foo'); Complete Attack: http://www.finchkeeper.com/register.php [username=">alert('foo'); &email= &password= &passwordConfirm= &question= &submit=Register]

Comments

You shouldn't echo user submitted values directly in your form as they can type anything into the username field, including code, and you are going to just echo that out. Which means that code/script would run as if from your source.

---

Also, given the code above, you are doing your prepared statements and binding correctly. There is no sql injection issue here. However, this is obviously not the whole code and so no one can say for certain that there are no sql injection issues elsewhere.

Answer 1

I think the problem is that you are not sanitizing the POST variables when an error occurs. When an error does occur you are echoing the POST variables back to the user.

Try sanitizing the POST array.

what is a good method to sanitize the whole $_POST array in php?

Comments

While this link may answer the question, it is better to include the essential parts of the answer here and provide the link for reference. Link-only answers can become invalid if the linked page changes. - From Review

Answer 2

It's because you're echo'ing the raw $_POST['username'] back into the form's <input value=""> when there's an error, without escaping any HTML entities.

So when they submit the form with the username ">alert('foo');, the string alert('foo'); is written back to the page, because the "> part closes the <input> element. They see that as a cross-site scripting vulnerability.

When you echo back the username into the value, you should escape it. Something like this should do:

value="<?php if(isset($error)){ echo htmlspecialchars($_POST['username']); } ?>"

This will break the username if it actually contains HTML-entities, but given the regular expression you use for username validation, you don't want to allow those anyway.

Comments

This won't break the username as the input field will convert the entities back to their correct character. The username would be broken because it contains non-alphanumeric characters and they are regex matching it.

---

thank you everyone, I will try making the adjustments where you advised. I have been looking at this code for months but couldnt find a solution until now. I do like to try and figure it out for myself first and ask as a last resort.

---

Ok just got my weekly report from Beyond Security Support website. in the report it no longer tells me i have 3 high risk security errors, It finally says zero high risk errors :) you guys are awesome and I really appreciate your help. thank you once again.