Anyone willing to do a quick security review or a registration page?

MutantJohn · September 11, 2015

Hey all,

I'm writing a small user registration page in MySQL and, of course, PHP.

I'm wondering if someone would like to take a look and tell me if there's something obvious I'm missing or something I just did completely wrong.

Here are the pages :

register.html

<!DOCTYPE html>
<html>
<head>
    <title>ditacms User Registration</title>
</head>
<body>
 
    <p>Complete the registration form below</p>
 
    <form method="post" action="register.php" id="registration_form">
        
        <label>Username : </label>
        <input type="text" name="username" />
        <br/>
 
        <label>Email : </label>
        <input type="text" name="email" value="" />
        <br/>
 
        <label>Password : </label>
        <input type="text" name="password" value="" />
        <br/>
 
        <label>Re-Type Password : </label>
        <input type="text" name="confirm_password" value="" />
        <br/>
 
    </form>
 
    <button type="submit" form="registration_form">Register</button>
 
</body>
</html>

register.php

<!DOCTYPE html>
<html>
<head>
    <title>Registration Processing</title>
</head>
<body>
 
<?php
 
define( "EOL", "<br />\n" );
 
// data source name
define( "DSN", "mysql:host=localhost;dbname=ditacms;charset=utf8" );
 
define( "USER", "account_creator" );
define( "PASSWORD", "UrsaOwnsRoshan" );
 
function db_connect()
{
    try
    {
        $db = new PDO( DSN, USER, PASSWORD );
    }
    catch( PDOException $ex )
    {
        // echo $ex->getMessage();
        // echo $ex->getTraceAsString();
        echo "Attempt to connect to database failed!" . EOL;
 
        exit();
    }
    
    return $db;
}
 
function verify_post_register_params()
{
    $username = $_POST[ "username" ];
    $email = $_POST[ "email" ];
    $password = $_POST[ "password" ];
    $confirm_password = $_POST[ "confirm_password" ];
 
    // if the user left any field blank...
    if ( empty( $username ) ||
         empty( $email ) ||
         empty( $password ) ||
         empty( $confirm_password ) )
    {
        echo "Empty field found in form submission!" . EOL;
        echo "Please complete the form." . EOL;
        return false;
    }
 
    // if the passwords do not exactly match...
    if ( strcmp( $password, $confirm_password ) !== 0 )
    {
        echo "Password mismatch!";
        return false;
    }
 
    $username = filter_var( $username, FILTER_SANITIZE_STRING );
    $email = filter_var( $email, FILTER_SANITIZE_EMAIL );
    $password = filter_var( $password, FILTER_SANITIZE_STRING );
 
    if ( $username === false ||
         $email === false ||
         $password === false )
    {
        echo "Sanitization failed! Potential attack!!!" . EOL;
        return false;
    }
 
    if ( filter_var( $email, FILTER_VALIDATE_EMAIL ) === false )
    {
        echo "Invalid email address!" . EOL;
        return false;
    }
 
    $register = array( "username" => $username,
                       "email" => $email,
                       "password" => $password );
 
    return $register;
}
 
function user_exists( $db, $username )
{
    $query = $db->prepare( "SELECT username FROM `ditacms`.`members` WHERE username = :username" );
    $query->bindValue( ":username", $username, PDO::PARAM_STR );
    $query->execute();
 
    $rows = $query->fetchAll( PDO::FETCH_ASSOC );
 
    // if the rows returned are empty, the user
    // does NOT exist so return false
    if ( empty( $rows ) === true )
    {
        return false;
    }
 
    // if the rows returned are NOT empty, the
    // user DOES exist so return true
    else
    {
        echo "A user with that username already exists!" . EOL;
        return true;
    }
}
 
function create_new_user( $db, $username, $email, $password )
{
    echo "Creating new user..." . EOL;
 
    $insert = $db->prepare( "INSERT INTO `ditacms`.`members` (username, email, password)
                             VALUES(:username, :email, :password)" );
 
    $hash = password_hash( $hash, PASSWORD_DEFAULT );
 
    $insert->bindValue( ":username", $username, PDO::PARAM_STR );
    $insert->bindValue( ":email", $email, PDO::PARAM_STR );
    $insert->bindValue( ":password", $hash, PDO::PARAM_STR );
 
 
    if ( $insert->execute() === false )
    {        
        echo "Insertion failure..." . EOL;
        return false;
    }
    else
    {
        echo "Successfully registered new account!" . EOL;
        return true;
    }
}
 
 
/*
 * main() loop
 */
 
echo "<p>Processing user registration request...</p>";
 
$register = verify_post_register_params();
 
if ( $register === false )
{
    echo "Bad POST parameters. Exiting script..." . EOL;   
}
else
{
    $db = db_connect();
 
    // if the user does NOT exist, create one
    if ( user_exists( $db, $register[ "username" ] ) === false )
    {
        create_new_user( $db, $register[ "username" ],
                              $register[ "email" ],
                              $register[ "password" ] );
    }    
}
 
 
?>
 
    <a href="/ditacms.com/register.html">Return to registration page</a>
 
    <br />
 
    <a href="/ditacms.com/">Return to homepage</a>
 
</body>
</html>

QuickOldCar · September 12, 2015

Does this even work? Are passing an undefined $hash variable through password_hash() when should be $password

I guess I was bored, tried to simplify it more.

Lot of changes including some error checking, html form and server side validation.

Didn't see the point doing the functions in it because I kept checking for an empty error array.

Since their passwords are saved as hashed values is no point filtering them but I added a minimum length.

Untested database related, try it out.

<?php
$errors = array();
$msg    = "Complete the registration form below";

if (isset($_POST['submit'])) {
    
    define("EOL", "<br />\n");
    
    // data source name
    define("DSN", "mysql:host=localhost;dbname=ditacms;charset=utf8");
    
    define("USER", "account_creator");
    define("PASSWORD", "UrsaOwnsRoshan");
    
    
    try {
        $db = new PDO(DSN, USER, PASSWORD);
    }
    catch (PDOException $ex) {
        // echo $ex->getMessage();
        // echo $ex->getTraceAsString();
        $errors[] = "database";
        $msg      = "Attempt to connect to database failed!" . EOL;
        die($msg);
    }
    
    if (empty($errors)) {
        
        if (isset($_POST['username']) && ctype_alnum(str_replace(array(
            "-",
            "_"
        ), '', trim($_POST['username']))) && strlen(trim($_POST['username'])) >= 5) {
            $username = trim($_POST['username']);
        } else {
            $username = '';
            $errors[] = 'username';
        }
        
        if (isset($_POST['email']) && filter_var(trim($_POST['email']), FILTER_VALIDATE_EMAIL)) {
            $email = trim($_POST['email']);
        } else {
            $email    = '';
            $errors[] = 'email';
        }
        
        if (isset($_POST['password']) && strlen(trim($_POST['password'])) >= 5) {
            $password = trim($_POST['password']);
        } else {
            $password = '';
            $errors[] = 'password';
        }
        
        if (isset($_POST['confirm_password']) && strlen(trim($_POST['confirm_password'])) >= 5) {
            $confirm_password = trim($_POST['confirm_password']);
        } else {
            $confirm_password = '';
            $errors[]         = 'confirm password';
        }
        
        
        // if the passwords do not exactly match...
        if ($password !== $confirm_password) {
            $password         = '';
            $errors[]         = 'password';
            $confirm_password = '';
            $errors[]         = 'confirm password';
        }
        
        if (empty($errors)) {
            
            $query = $db->prepare("SELECT username FROM `ditacms`.`members` WHERE username = :username");
            $query->bindValue(":username", $username, PDO::PARAM_STR);
            
            if (!$query->execute()) {
                $errors[] = "query fail";
                $msg      = "The user check query failed" . EOL;
            } else {
                if ($query->fetchColumn()) {
                    $errors[] = "user exists";
                    $msg      = "A user with that username already exists!" . EOL;
                }
                
            }
            
            
        }
        
        if (empty($errors)) {
            
            $msg = "Creating new user..." . EOL;
            
            $insert = $db->prepare("INSERT INTO `ditacms`.`members` (username, email, password)
                             VALUES(:username, :email, :password)");
            
            $hash = password_hash($password, PASSWORD_DEFAULT);
            
            $insert->bindValue(":username", $username, PDO::PARAM_STR);
            $insert->bindValue(":email", $email, PDO::PARAM_STR);
            $insert->bindValue(":password", $hash, PDO::PARAM_STR);
            
            
            if (!$insert->execute()) {
                $errors[] = "insert failure";
                $msg      = "Insertion failure..." . EOL;
            } else {
                $msg = "Successfully registered new account!" . EOL;
            }
            
        }
        
    }
    if (empty($errors)) {
        //redirect to login or main page
        header("Location: http://" . $_SERVER['SERVER_NAME']);
        exit;
    } else {
        $msg .= "<p style='color:#FF3300;'>You have the following errors: " . implode($errors, ", ") . "</p>";
    }
    
    
}
?>

<!DOCTYPE html>
<html>
<head>
    <title>ditacms User Registration</title>
<style>
*:focus {
    outline: none;
}
body {
    font: 14px/21px "Lucida Sans", "Lucida Grande", "Lucida Sans Unicode", sans-serif;
    max-width:99%;
}
a {
    text-decoration:none;
}
.wrap{
    position:relative;
    width:75%;
    margin-left:auto;
    margin-right:auto;
    padding:5px;
}
ul{
    list-style-type: none;
    padding:0;
    margin:0;
}
.register_form h2, .register_form label {
    font-family:Georgia, Times, "Times New Roman", serif;
}
.form_hint, .required_notification {
    font-size: 11px;
}
.register_form ul {
    width:750px;
    list-style-type:none;
    list-style-position:outside;
    margin:0px;
    padding:0px;
}
.register_form li{
    padding:12px;
    border-bottom:1px solid #eee;
    position:relative;
}
.register_form li:first-child, .register_form li:last-child {
    border-bottom:1px solid #777;
}
.register_form h2 {
    margin:0;
    display: inline;
}
.required_notification {
    color:#d45252;
    margin:5px 0 0 0;
    display:inline;
    float:right;
}
.register_form label {
    width:150px;
    margin-top: 3px;
    display:inline-block;
    float:left;
    padding:3px;
}
.register_form input {
    height:20px;
    width:300px;
    padding:5px 8px;
    -moz-transition: padding .25s;
    -webkit-transition: padding .25s;
    -o-transition: padding .25s;
    transition: padding .25s;
}
.register_form textarea {
    padding:8px;
    width:300px;
    -moz-transition: padding .25s;
    -webkit-transition: padding .25s;
    -o-transition: padding .25s;
    transition: padding .25s;
}
.register_form button {
    margin-left:156px;
}
.register_form input, .register_form textarea {
    padding-right:30px;
    border:1px solid #aaa;
    box-shadow: 0px 0px 3px #ccc, 0 10px 15px #eee inset;
    border-radius:2px;
}
.register_form input:focus, .register_form textarea:focus {
    background: #fff;
    border:1px solid #555;
    box-shadow: 0 0 3px #00FF00;
    padding-right:70px;
}
/* Button Style */
button.submit {
    background-color: #68b12f;
    background: -webkit-gradient(linear, left top, left bottom, from(#68b12f), to(#50911e));
    background: -webkit-linear-gradient(top, #68b12f, #50911e);
    background: -moz-linear-gradient(top, #68b12f, #50911e);
    background: -ms-linear-gradient(top, #68b12f, #50911e);
    background: -o-linear-gradient(top, #68b12f, #50911e);
    background: linear-gradient(top, #68b12f, #50911e);
    border: 1px solid #509111;
    border-bottom: 1px solid #5b992b;
    border-radius: 3px;
    -webkit-border-radius: 3px;
    -moz-border-radius: 3px;
    -ms-border-radius: 3px;
    -o-border-radius: 3px;
    box-shadow: inset 0 1px 0 0 #9fd574;
    -webkit-box-shadow: 0 1px 0 0 #9fd574 inset ;
    -moz-box-shadow: 0 1px 0 0 #9fd574 inset;
    -ms-box-shadow: 0 1px 0 0 #9fd574 inset;
    -o-box-shadow: 0 1px 0 0 #9fd574 inset;
    color: white;
    font-weight: bold;
    padding: 6px 20px;
    text-align: center;
    text-shadow: 0 -1px 0 #396715;
}
button.submit:hover {
    opacity:.85;
    cursor: pointer;
}
button.submit:active {
    border: 1px solid #20911e;
    box-shadow: 0 0 10px 5px #356b0b inset;
    -webkit-box-shadow:0 0 10px 5px #356b0b inset ;
    -moz-box-shadow: 0 0 10px 5px #356b0b inset;
    -ms-box-shadow: 0 0 10px 5px #356b0b inset;
    -o-box-shadow: 0 0 10px 5px #356b0b inset;
    
}
input:required, textarea:required {
    background: #fff;
    border-color:#FF0000;
    
}
.register_form input:required:valid, .register_form textarea:required:valid { /* when a field is considered valid by the browser */
    background: #fff;
    box-shadow: 0 0 5px #5cd053;
    border-color: #28921f;
}
.form_hint {
    background: #d45252;
    border-radius: 3px 3px 3px 3px;
    color: white;
    margin-left:8px;
    padding: 1px 6px;
    z-index: 999; /* hints stay above all other elements */
    position: absolute; /* allows proper formatting if hint is two lines */
    display: none;
}
.form_hint::before {
    content: "\25C0"; /* left point triangle in escaped unicode */
    color:#d45252;
    position: absolute;
    top:1px;
    left:-6px;
}
.register_form input:focus + .form_hint {
    display: inline;
}
.register_form input:required:valid + .form_hint {
    background: #28921f;
}
.register_form input:required:valid + .form_hint::before {
    color:#28921f;
}

</style>
</head>
<body>
<div class="wrap">
    <form class="register_form" method="post" action="" novalidate>
<ul>
<li>
    <h2>Register</h2>
    <button style="float:right;"><a href="login.php">LOGIN</a></button>
</li>
        <p><?php echo $msg;?></p>
<li>
        <label>Username : </label>
        <input type="text" pattern="[\w-_]{5,}" name="username" value="<?php echo $username;?>" required title="Allowed: Minimum 5 charactersers,A-Z,a-z,0-9,-_"/>
        <span class="form_hint">Allowed: Minimum 5 charactersers,A-Z,a-z,0-9,-_</span>
</li>
<li>
        <label>Email : </label>
        <input type="text" name="email" value="<?php echo $email;?>" required  title="Format: name@domain.com"/>
        <span class="form_hint">Format: "name@domain.com"</span>
</li>
<li>
        <label>Password : </label>
        <input type="password" pattern=".{5,}" name="password" value="<?php echo $password;?>" required title="Minimum 5 characters"/>
        <span class="form_hint">Minimum 5 characters</span>
</li>
<li>
        <label>Re-Type Password : </label>
        <input type="password" pattern=".{5,}" name="confirm_password" value="<?php echo $confirm_password;?>" required title="Minimum 5 characters"/>
        <span class="form_hint">Minimum 5 characters</span>
</li>
<li>
        <button class="submit" type="submit" name="submit" >Register</button>
</li>
</ul>    
    </form>
 
 </div>   
 
</body>
</html>

MutantJohn · September 14, 2015

Holy crap, dude O_o

I'm going to have to take my time and really comb through that.

And yeah, I finally caught that error with the password_hash() thing XD

Edit :

Your PHP is a pleasure to read, actually. Very interesting approach. I dig it. I was also going to save the CSS and JS for the end but thank you for taking the time to draft something up.

Edited September 14, 2015 by MutantJohn

MutantJohn · September 14, 2015

One thing I really like is how you condensed it all down to just one page.

One question though, why are you filtering out the underscores from the username? Is it for security reasons or aesthetic?

Psycho · September 14, 2015

One thing I will point out in QuickOldCar's code that may not be obvious. You will notice that the file doesn't start with "<HTML>". It instead starts with the PHP processing logic. Then, after all the processing is done, he builds the page. There are many reasons why this format should be followed and his implementation includes one such example. After all the processing logic is done there is a condition to test whether registration passed or if there was a failure. If registration failed the logic will continue to the output of the form (as well as the error message). however, if registration passed, there is a header() redirect to a login or main page. That header function would fail if ANY content had been sent to the browser - even if it was just the opening "<HTML>" tag or a line-break before the opening PHP tag.

Psycho · September 14, 2015

One thing I really like is how you condensed it all down to just one page.

One question though, why are you filtering out the underscores from the username? Is it for security reasons or aesthetic?

If you are referring to the pattern in the input element

<input type="text" pattern="[\w-_]{5,}" name="username" . . .

It is actually superfluous (i.e. not needed). That pattern is defining the allowed characters. The \w represents "word" characters: a-z, A-Z, 0-9 and underscore. Then there is a dash and an underscore in the approved list as well. Since the underscore is included in the \w it is not needed to add it explicitly.

EDIT: I just noticed this in his code as well

        if (isset($_POST['username']) && ctype_alnum(str_replace(array(
            "-",
            "_"

        ), '', trim($_POST['username']))) && strlen(trim($_POST['username'])) >= 5) {

That is verifying that (not including underscores and dashes) that the username is at least 5 characters and all alphanumeric characters. The replacement is only done for the purpose of validation. If validation passes, the entire username (including dashes and underscores) is used. Unless there was a specific business need for such exclusion I wouldn't be so strict. Also, excluding the dashes and underscores before checking the length could cause isses. for example, if a user tried to use the name "ab_de" the validation code would output the error that the username was not valid - even though it uses the allowed characters and is 5 characters long. But, considering how fast he put this together, I'm sure it was not something he would have done in a final version.

Edited September 14, 2015 by Psycho

QuickOldCar · September 14, 2015

Yeah Psycho is right, just an example, validate how you see fit.

The html form validation is just a helper, always validate server side.

MutantJohn · September 14, 2015

Huh, okay. Cool.

This is some really helpful stuff!

Also, I'm loving all the HTML5 features you included! This website is going to look so modern now! I can't thank you guys enough!

QuickOldCar · September 15, 2015

That is verifying that (not including underscores and dashes) that the username is at least 5 characters and all alphanumeric characters. The replacement is only done for the purpose of validation. If validation passes, the entire username (including dashes and underscores) is used. Unless there was a specific business need for such exclusion I wouldn't be so strict. Also, excluding the dashes and underscores before checking the length could cause isses. for example, if a user tried to use the name "ab_de" the validation code would output the error that the username was not valid - even though it uses the allowed characters and is 5 characters long. But, considering how fast he put this together, I'm sure it was not something he would have done in a final version.

My bad...I had an A-Za-z0-9-_ originally for a pattern and replaced the number and letter ranges with a \w

All would need is \w- so the underscore was extra

The dashes and underscores are included in the length check

strlen(trim($_POST['username'])) >= 5

ab_de passes server side validation because excluding the - and _ is still all letters and numbers, then a minimum 5 characters checked on the actual post value

I personally only allow letters,numbers,dash and underscore to keep them url friendly because people do some crazy names with all sorts of odd characters otherwise.

Glad you like it.

Edited September 15, 2015 by QuickOldCar

MutantJohn · September 15, 2015

Yeah, it's great to see some code that I can use as a reference. I'm digging some of the approaches and it's changing how I'm coding up my application.

I know it's silly to design a login system when there's more sophisticated tools out there but this is a really good learning experience for me.

Jacques1 · September 16, 2015

Some additional suggestions:

1)

PDO uses a potentially insecure default configuration which you should override. In particular, prepared statements are deactivated by default, so your prepare() doesn't prepare anything. It merely auto-escapes the input values and inserts them into the query string, which is known to cause vulnerabilities in some scenarios. PDO also doesn't use exceptions by default, so the script will ignore failed queries and crash uncontrolled at a later stage.

A proper configuration looks something like this:

$database_connection = new PDO(DATABASE_DSN, DATABASE_USERNAME, DATABASE_PASSWORD, [
    PDO::ATTR_EMULATE_PREPARES => false,                // use real prepared statements instead of "emulated" ones
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,        // throw an exception in case of an error
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,   // fetch associative arrays by default (this is not critical)
]);

2)

Do not mangle the user input with questionable PHP features like FILTER_SANITIZE_STRING. This is extremely confusing and even dangerous for the user, because the filter cuts off anything that looks like HTML. So a good password like “sfjk3<§45%$§%g” is silently turned into the weak password “sfjk3”. It's best to leave the user input alone (except maybe for trimming the username). Store the original data and escape it when needed.

Speaking of which: Any data you wish to insert into an HTML context must first be escaped with htmlspecialchars() to prevent cross-site-scripting attacks.

<?php

function html_escape($input, $encoding)
{
    return htmlspecialchars($input, ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML5, $encoding);
}

$username = 'foo';

?>

Welcome, <?= html_escape($username, 'UTF-8') ?>.

3)

Your way of enforcing unique usernames doesn't work reliably. If two clients request the same unused name at the same time, the application allows both of them to have it, because the check happens before the rows are inserted. This will crash the application or lead to two different users with the same name, depending on whether the database system enforces unique values.

A better solution is to let the database handle the uniqueness check: Make sure the username column is a primary key or has a UNIQUE constraint. Then simply try to insert the row with no prior database checks. If that causes a constraint violation, you know that the name is already taken. This approach is also much shorter than your current code, because you only need one query:

<?php

// MySQL error codes
const MYSQL_ER_DUP_UNIQUE = '23000';

// ... database connection etc.

$username = 'foo';
$email = 'foo@example.com';
$password_hash = '$2y$12$nGbRUyeeFfS/xr1uceoSz.mTRypyBR2rTi1gJL20mEhyXgAP3UvKS';

$member_query = $database_connection->prepare('
    INSERT INTO members
    SET
      username = :username,
      email = :email,
      password = :password
');

// Try to insert the new member; this will trigger an exception if the name is already taken.
try
{
    $member_query->execute([
        'username' => $username,
        'email' => $email,
        'password' => $password_hash,
    ]);
}
catch (PDOException $member_query_error)
{
    // If the exception was caused by a constraint violation, tell the user about the duplicate name; otherwise propagate the exception.
    if ($member_query->errorCode() ===  MYSQL_ER_DUP_UNIQUE)
    {
        echo 'Sorry, this name is already taken.';
    }
    else
    {
        throw $member_query_error;
    }
}

4)

Don't catch exceptions unless you know exactly why you're doing it. Internal database problems are none of the user's business, so there's no need to tell them. Even worse, you throw away all the error information and won't be able to analyze the problem.

The default error handler is usually better than any custom solution, because it can be fine-tuned with the php.ini. If you want to have a user-friendly error message, just set up a custom error page for 500 errors (the manual of your webserver will tell you how).

Catching exceptions only makes sense if you need to solve a specific problem in a special way (e. g. retry an action which is known to fail from time to time). This is very rare. Most of the time, you won't need any try statements at all.

5)

Your use of password_hash() is not ideal. One of the main features of modern password hash algorithms is that they're adjustable. You can and should fine-tune them for your specific hardware and your security requirements.

Sure, you can rely on the default configuration. But this means the hashing procedure is probably either too weak or too strong. It's better to explicitly choose an algorithm and set the parameters.

The current standard algorithm is bcrypt which has a single strength parameter. You should decide how long the hashing should take (more time means more password security but also less usability) and then try different strengths until you end up with the right duration, e. g. one second for normal user accounts.

<?php

const PASS_ALGORITHM = PASSWORD_BCRYPT;
const PASS_STRENGTH = 12;    // increase or decrease as needed

$password = 'testtest';
$hash = password_hash($password, PASS_ALGORITHM, ['cost' => PASS_STRENGTH]);

QuickOldCar · September 16, 2015

Some good suggestions Jacques

MutantJohn · September 19, 2015

That's amazing advice O_o

Thank you for taking the time to write all of that. That's... really, really awesome! Thank you!

Sign In

Anyone willing to do a quick security review or a registration page?

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information