Simple CMS system

Question

I'm working on a simple CMS with the intent of making it as secure as possible (a personal challenge) and the code as clean as possible. I think I've a long way to go so I would appreciate any input, or bug spotting!

Common.php

<?php 

    // Errors, errors everywhere. Let us display them all!
    error_reporting(E_ALL);
    ini_set('display_errors', 1);

    // These variables define the connection information for your MSSQL database 
    $username   = <redacted>; 
    $password   = <redacted>; 
    $host       = <redacted>; 
    $dbname     = <redacted>; 

    // UTF-8 is a character encoding scheme that allows you to conveniently store 
    // a wide varienty of special characters, like ¢ or €, in your database. 
    // By passing the following $options array to the database connection code we 
    // are telling the MSSQL server that we want to communicate with it using UTF-8 
    // See Wikipedia for more information on UTF-8: 
    // http://en.wikipedia.org/wiki/UTF-8 

    //$options = array(PDO::MSSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8'); 

    // A try/catch statement is a common method of error handling in object oriented code. 
    // First, PHP executes the code within the try block.  If at any time it encounters an 
    // error while executing that code, it stops immediately and jumps down to the 
    // catch block.  For more detailed information on exceptions and try/catch blocks: 
    // http://us2.php.net/manual/en/language.exceptions.php 
    try 
    { 
        // This statement opens a connection to your database using the PDO library 
        // PDO is designed to provide a flexible interface between PHP and many 
        // different types of database servers.  For more information on PDO: 
        // http://us2.php.net/manual/en/class.pdo.php 

        //$db = new PDO("mssql:host={$host};dbname={$dbname};charset=utf8", $username, $password, $options); 
        //$db = new PDO('sqlsrv:Server=$host;Database=$dbname','$username','$password');


        $db = new PDO ("sqlsrv:server = tcp:$host,1433; Database = $dbname", "$username", "$password");
        $db->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

    } 
    catch(PDOException $ex) 
    { 
        // If an error occurs while opening a connection to your database, it will 
        // be trapped here.  The script will output an error and stop executing. 
        // Note: On a production website, you should not output $ex->getMessage(). 
        // It may provide an attacker with helpful information about your code 
        // (like your database username and password). 
        die("Failed to connect to the database: " . $ex->getMessage()); 
    } 

    // This statement configures PDO to throw an exception when it encounters 
    // an error.  This allows us to use try/catch blocks to trap database errors. 
    $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); 

    // This statement configures PDO to return database rows from your database using an associative 
    // array.  This means the array will have string indexes, where the string value 
    // represents the name of the column in your database. 
    $db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC); 

    // This block of code is used to undo magic quotes.  Magic quotes are a terrible 
    // feature that was removed from PHP as of PHP 5.4.  However, older installations 
    // of PHP may still have magic quotes enabled and this code is necessary to 
    // prevent them from causing problems.  For more information on magic quotes: 
    // http://php.net/manual/en/security.magicquotes.php 
    if(function_exists('get_magic_quotes_gpc') && get_magic_quotes_gpc()) 
    { 
        function undo_magic_quotes_gpc(&$array) 
        { 
            foreach($array as &$value) 
            { 
                if(is_array($value)) 
                { 
                    undo_magic_quotes_gpc($value); 
                } 
                else 
                { 
                    $value = stripslashes($value); 
                } 
            } 
        } 

        undo_magic_quotes_gpc($_POST); 
        undo_magic_quotes_gpc($_GET); 
        undo_magic_quotes_gpc($_COOKIE); 
    } 

    // This tells the web browser that your content is encoded using UTF-8 
    // and that it should submit content back to you using UTF-8 
    header('Content-Type: text/html; charset=utf-8'); 

    // This initializes a session.  Sessions are used to store information about 
    // a visitor from one web page visit to the next.  Unlike a cookie, the information is 
    // stored on the server-side and cannot be modified by the visitor.  However, 
    // note that in most cases sessions do still use cookies and require the visitor 
    // to have cookies enabled.  For more information about sessions: 
    // http://us.php.net/manual/en/book.session.php 
    session_start(); 

    // Note that it is a good practice to NOT end your PHP files with a closing PHP tag. 
    // This prevents trailing newlines on the file from being included in your output, 
    // which can cause problems with redirecting users.

Login.php (this one likely can be done a LOT better)

<?php 

    // First we execute our common code to connection to the database and start the session 
    require("common.php"); 

    // This variable will be used to re-display the user's username to them in the 
    // login form if they fail to enter the correct password.  It is initialized here 
    // to an empty value, which will be shown if the user has not submitted the form. 
    $submitted_username = ''; 

    // This if statement checks to determine whether the login form has been submitted 
    // If it has, then the login code is run, otherwise the form is displayed 
    if(!empty($_POST) || isset($_COOKIE["qcore"])) 
    { 
        // set the parameter values as if the form has been filled out
        if(!empty($_POST))
        {
            // This query retreives the user's information from the database using 
            // their username. SELECT TOP 1 prevents people from being able to edit
            // their HTTP POST to fetch the entire table.
            $query = " 
                SELECT TOP 1
                    *
                FROM dbo.[User]  
                WHERE 
                    Username = :username 
            "; 

            $query_params = array( 
                ':username' => $_POST['username'] 
            );            
        }
        // if it hasn't, let's use the cooooooooooooookie! Woo!
        else if (isset($_COOKIE["qcore"]))
        {
            $query = " 
                SELECT  TOP 1
                        u.*
                FROM    dbo.[User] AS u
                        INNER JOIN dbo.UserSession AS us
                            ON us.UserId = u.UserId
                WHERE   us.SessionId = :sessiontoken";

            // The parameter values 
            $query_params = array( 
                ':sessiontoken' => $_COOKIE["qcore"] 
            ); 
        }

        try 
        { 
            // Execute the query against the database 
            $stmt = $db->prepare($query); 
            $result = $stmt->execute($query_params); 
        } 
        catch(PDOException $ex) 
        { 
            // Note: On a production website, you should not output $ex->getMessage(). 
            // It may provide an attacker with helpful information about your code.  
            die("Failed to run query: " . $ex->getMessage()); 
        } 

        // This variable tells us whether the user has successfully logged in or not. 
        // We initialize it to false, assuming they have not. 
        // If we determine that they have entered the right details, then we switch it to true. 
        $login_ok = false; 

        // Retrieve the user data from the database.  If $row is false, then the username 
        // they entered is not registered.
        $row = $stmt->fetch(); 
        if($row && !isset($_COOKIE["qcore"])) 
        { 
            // Using the password submitted by the user and the salt stored in the database, 
            // we now check to see whether the passwords match by hashing the submitted password 
            // and comparing it to the hashed version already stored in the database. 
            $check_password = hash('sha256', $_POST['password'] . $row['Salt']); 
            for($round = 0; $round < 65536; $round++) 
            { 
                $check_password = hash('sha256', $check_password . $row['Salt']); 
            } 

            if($check_password === $row['Password']) 
            { 
                // If they do, then we flip this to true 
                $login_ok = true; 
            } 
        } 
        elseif (isset($_COOKIE["qcore"])) 
        {
            $login_ok = true; 
        }

        // If the user logged in successfully, then we send them to the private members-only page 
        // Otherwise, we display a login failed message and show the login form again 
        if($login_ok) 
        { 
            // Here I am preparing to store the $row array into the $_SESSION by 
            // removing the salt and password values from it.  Although $_SESSION is 
            // stored on the server-side, there is no reason to store sensitive values 
            // in it unless you have to.  Thus, it is best practice to remove these 
            // sensitive values first. 
            if(!empty($_POST))
            {
                unset($row['Salt']); 
                unset($row['Password']); 
            }
            // This stores the user's data into the session at the index 'user'. 
            // We will check this index on the private members-only page to determine whether 
            // or not the user is logged in.  We can also use it to retrieve 
            // the user's details. 
            $_SESSION['user'] = $row; 

            // Generate a session token which is used locally as a key between the users cookie
            // and their UserID, this prevents  the user from being able to edit their cookie
            // to login as another user.
            $sessiontoken = dechex(mt_rand(0, 2147483647)) . dechex(mt_rand(0, 2147483647));

            // Save our cookie 'qcore' with the users session id
            setcookie("qcore", $sessiontoken);

            // Insert a new session ID record, or update if one already exists.
            $query = "
                DECLARE @userid AS INTEGER = :userid
                DECLARE @sessionid AS varchar(500) = :sessionid

                IF EXISTS ( SELECT  TOP 1 * 
                            FROM    dbo.UserSession 
                            WHERE   UserId = @userid    )
                                UPDATE  dbo.UserSession 
                                SET     SessionId = @sessionid
                                WHERE   UserId = @userid
                ELSE
                            INSERT INTO dbo.UserSession ( 
                                    UserId ,
                                    SessionId
                            ) VALUES  (
                                    @userid ,
                                    @sessionid)";

            $query_params = array( 
                ':userid' => $row['UserId'], 
                ':sessionid' => $sessiontoken 
            );    

            try 
            { 
                // Execute the query to insert a new user session or update
                // an existing one
                $stmt = $db->prepare($query); 
                $result = $stmt->execute($query_params); 
            } 
            catch(PDOException $ex) 
            { 
                // Note: On a production website, you should not output $ex->getMessage(). 
                // It may provide an attacker with helpful information about your code.  
                // die("Failed to run query: " . $ex->getMessage()); 
                die("Failed to run query: " . $ex->getMessage()); 
            } 

            // Redirect the user to the private members-only page. 
            // This will need to be changed once we have the QUEST logic flow sorted out
            // to be the landing quest page.
            header("Location: private.php"); 
            die("Redirecting to: private.php"); 
        } 
        else 
        { 
            // Tell the user they failed 
            print("Login Failed."); 

            // Show them their username again so all they have to do is enter a new 
            // password.  The use of htmlentities prevents XSS attacks.  You should 
            // always use htmlentities on user submitted values before displaying them 
            // to any users (including the user that submitted them).  For more information: 
            // http://en.wikipedia.org/wiki/XSS_attack 
            $submitted_username = htmlentities($_POST['username'], ENT_QUOTES, 'UTF-8'); 
        } 
    } 
?> 
<h1>Login</h1> 
<form action="login.php" method="post"> 
    Username:<br /> 
    <input type="text" name="username" value="<?php echo $submitted_username; ?>" /> 
    <br /><br /> 
    Password:<br /> 
    <input type="password" name="password" value="" /> 
    <br /><br /> 
    <input type="submit" value="Login" /> 
</form> 
<a href="register.php">Register</a>

Logout.php

<?php 

    // First we execute our common code to connection to the database and start the session 
    require("common.php"); 

    // We remove the user's data from the session 
    unset($_SESSION['user']); 

    // set the cookie expiration date to one hour ago
    setcookie("qcore", "", time()-3600);

    // We redirect them to the login page 
    header("Location: login.php"); 
    die("Redirecting to: login.php");

Private.php

<?php 

    // *** IMPORTANT ***
    // This file will lock a page down to logged in users only. If you would like to secure
    // the page to administrators only then include private_administrator.php instead!

    // First we execute our common code to connection to the database and start the session 
    require("common.php"); 

    // At the top of the page we check to see whether the user is logged in or not 
    if(empty($_SESSION['user'])) 
    { 
        // If they are not, we redirect them to the login page. 
        header("Location: login.php"); 

        // Remember that this die statement is absolutely critical.  Without it, 
        // people can view your members-only content without logging in. 
        die("Redirecting to login.php"); 
    } 

    // Everything below this point in the file is secured by the login system 

    // We can display the user's username to them by reading it from the session array.  Remember that because 
    // a username is user submitted content we must use htmlentities on it before displaying it to the user. 
?> 

Hello <?php echo htmlentities($_SESSION['user']['Username'], ENT_QUOTES, 'UTF-8'); ?>, secret content!<br /> 
<a href="memberlist.php">Memberlist</a><br /> 
<a href="edit_account.php">Edit Account</a><br /> 
<a href="logout.php">Logout</a>

Register.php

<?php 

    // First we execute our common code to connection to the database and start the session 
    require("common.php"); 

    // This if statement checks to determine whether the registration form has been submitted 
    // If it has, then the registration code is run, otherwise the form is displayed 
    if(!empty($_POST)) 
    { 
        // Ensure that the user has entered a non-empty username 
        if(empty($_POST['username'])) 
        { 
            // Note that die() is generally a terrible way of handling user errors 
            // like this.  It is much better to display the error with the form 
            // and allow the user to correct their mistake.  However, that is an 
            // exercise for you to implement yourself. 
            die("Please enter a username."); 
        } 

        // Ensure that the user has entered a non-empty password 
        if(empty($_POST['password'])) 
        { 
            die("Please enter a password."); 
        } 

        // Make sure the user entered a valid E-Mail address 
        // filter_var is a useful PHP function for validating form input, see: 
        // http://us.php.net/manual/en/function.filter-var.php 
        // http://us.php.net/manual/en/filter.filters.php 
        if(!filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)) 
        { 
            die("Invalid E-Mail Address"); 
        } 

        // We will use this SQL query to see whether the username entered by the 
        // user is already in use.  A SELECT query is used to retrieve data from the database. 
        // :username is a special token, we will substitute a real value in its place when 
        // we execute the query. 
        $query = " 
            SELECT 
                1 
            FROM dbo.[User] 
            WHERE 
                Username = :username 
        "; 

        // This contains the definitions for any special tokens that we place in 
        // our SQL query.  In this case, we are defining a value for the token 
        // :username.  It is possible to insert $_POST['username'] directly into 
        // your $query string; however doing so is very insecure and opens your 
        // code up to SQL injection exploits.  Using tokens prevents this. 
        // For more information on SQL injections, see Wikipedia: 
        // http://en.wikipedia.org/wiki/SQL_Injection 
        $query_params = array( 
            ':username' => $_POST['username'] 
        ); 

        try 
        { 
            // These two statements run the query against your database table. 
            $stmt = $db->prepare($query); 
            $result = $stmt->execute($query_params); 
        } 
        catch(PDOException $ex) 
        { 
            // Note: On a production website, you should not output $ex->getMessage(). 
            // It may provide an attacker with helpful information about your code.  
            die("Failed to run query: " . $ex->getMessage()); 
        } 

        // The fetch() method returns an array representing the "next" row from 
        // the selected results, or false if there are no more rows to fetch. 
        $row = $stmt->fetch(); 

        // If a row was returned, then we know a matching username was found in 
        // the database already and we should not allow the user to continue. 
        if($row) 
        { 
            die("This username is already in use"); 
        } 

        // Now we perform the same type of check for the email address, in order 
        // to ensure that it is unique. 
        $query = " 
            SELECT 
                1 
            FROM dbo.[User]  
            WHERE 
                Email = :email 
        "; 

        $query_params = array( 
            ':email' => $_POST['email'] 
        ); 

        try 
        { 
            $stmt = $db->prepare($query); 
            $result = $stmt->execute($query_params); 
        } 
        catch(PDOException $ex) 
        { 
            die("Failed to run query: " . $ex->getMessage()); 
        } 

        $row = $stmt->fetch(); 

        if($row) 
        { 
            die("This email address is already registered"); 
        } 

        // An INSERT query is used to add new rows to a database table. 
        // Again, we are using special tokens (technically called parameters) to 
        // protect against SQL injection attacks. 
        $query = " 
            INSERT INTO dbo.[User]  ( 
                Username, 
                Password, 
                Salt, 
                Email 
            ) VALUES ( 
                :username, 
                :password, 
                :salt, 
                :email 
            ) 
        "; 

        // A salt is randomly generated here to protect again brute force attacks 
        // and rainbow table attacks.  The following statement generates a hex 
        // representation of an 8 byte salt.  Representing this in hex provides 
        // no additional security, but makes it easier for humans to read. 
        // For more information: 
        // http://en.wikipedia.org/wiki/Salt_%28cryptography%29 
        // http://en.wikipedia.org/wiki/Brute-force_attack 
        // http://en.wikipedia.org/wiki/Rainbow_table 
        $salt = dechex(mt_rand(0, 2147483647)) . dechex(mt_rand(0, 2147483647)); 

        // This hashes the password with the salt so that it can be stored securely 
        // in your database.  The output of this next statement is a 64 byte hex 
        // string representing the 32 byte sha256 hash of the password.  The original 
        // password cannot be recovered from the hash.  For more information: 
        // http://en.wikipedia.org/wiki/Cryptographic_hash_function 
        $password = hash('sha256', $_POST['password'] . $salt); 

        // Next we hash the hash value 65536 more times.  The purpose of this is to 
        // protect against brute force attacks.  Now an attacker must compute the hash 65537 
        // times for each guess they make against a password, whereas if the password 
        // were hashed only once the attacker would have been able to make 65537 different  
        // guesses in the same amount of time instead of only one. 
        for($round = 0; $round < 65536; $round++) 
        { 
            $password = hash('sha256', $password . $salt); 
        } 

        // Here we prepare our tokens for insertion into the SQL query.  We do not 
        // store the original password; only the hashed version of it.  We do store 
        // the salt (in its plaintext form; this is not a security risk). 
        $query_params = array( 
            ':username' => $_POST['username'], 
            ':password' => $password, 
            ':salt' => $salt, 
            ':email' => $_POST['email'] 
        ); 

        try 
        { 
            // Execute the query to create the user 
            $stmt = $db->prepare($query); 
            $result = $stmt->execute($query_params); 
        } 
        catch(PDOException $ex) 
        { 
            // Note: On a production website, you should not output $ex->getMessage(). 
            // It may provide an attacker with helpful information about your code.  
            die("Failed to run query: " . $ex->getMessage()); 
        } 

        // This redirects the user back to the login page after they register 
        header("Location: login.php"); 

        // Calling die or exit after performing a redirect using the header function 
        // is critical.  The rest of your PHP script will continue to execute and 
        // will be sent to the user if you do not die or exit. 
        die("Redirecting to login.php"); 
    } 

?> 
<h1>Register</h1> 
<form action="register.php" method="post"> 
    Username:<br /> 
    <input type="text" name="username" value="" /> 
    <br /><br /> 
    E-Mail:<br /> 
    <input type="text" name="email" value="" /> 
    <br /><br /> 
    Password:<br /> 
    <input type="password" name="password" value="" /> 
    <br /><br /> 
    <input type="submit" value="Register" /> 
</form>

User table

CREATE TABLE [dbo].[User](
    [UserId] [int] IDENTITY(1,1) NOT NULL,
    [Title] [varchar](50) NULL,
    [FirstName] [varchar](100) NULL,
    [MiddleName] [varchar](100) NULL,
    [LastName] [varchar](100) NULL,
    [Gender] [varchar](20) NULL,
    [DOB] [date] NULL,
    [Email] [varchar](200) NULL,
    [Phone] [varchar](50) NULL,
    [Mobile] [varchar](50) NULL,
    [ResidentialAddress] [varchar](100) NULL,
    [ResidentialPostCode] [varchar](10) NULL,
    [ResidentialSuburb] [varchar](50) NULL,
    [ResidentialState] [varchar](20) NULL,
    [ResidentialCountry] [varchar](200) NULL,
    [PostalAddress] [varchar](100) NULL,
    [PostalPostCode] [varchar](10) NULL,
    [PostalSuburb] [varchar](50) NULL,
    [PostalState] [varchar](20) NULL,
    [PostalCountry] [varchar](200) NULL,
    [BrowserDetails] [varchar](500) NULL,
    [IsActive] [bit] NULL,
    [Password] [varchar](500) NULL,
    [Salt] [varchar](50) NULL,
    [LastLogin] [datetime] NULL,
    [CompanyID] [int] NULL,
    [Created] [datetime] NULL,
    [CreatedBy] [varchar](50) NULL,
    [LastModified] [datetime] NULL,
    [LastModifiedBy] [varchar](50) NULL
)
SET ANSI_PADDING OFF
ALTER TABLE [dbo].[User] ADD [Username] [varchar](50) NULL
SET ANSI_PADDING ON
ALTER TABLE [dbo].[User] ADD [UserRole] [varchar](30) NULL
 CONSTRAINT [PK_users] PRIMARY KEY CLUSTERED 
(
    [UserId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON)

GO

SET ANSI_PADDING OFF
GO

ALTER TABLE [dbo].[User] ADD  DEFAULT ('User') FOR [UserRole]
GO

UserSession Table

CREATE TABLE [dbo].[UserSession](
    [UserId] [int] NOT NULL,
    [SessionId] [varchar](500) NOT NULL,
    [Created] [datetime] NOT NULL,
    [CreatedBy] [varchar](50) NOT NULL,
    [LastModifed] [datetime] NULL,
    [LastModifiedBy] [varchar](50) NULL,
 CONSTRAINT [PK_SessionID] PRIMARY KEY CLUSTERED 
(
    [SessionId] ASC
)WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON)
)

GO

SET ANSI_PADDING OFF
GO

ALTER TABLE [dbo].[UserSession] ADD  DEFAULT (getdate()) FOR [Created]
GO

ALTER TABLE [dbo].[UserSession] ADD  DEFAULT (user_name()) FOR [CreatedBy]
GO

Answer 1

You want to make your code as secure as possible. Good. You want your code to be as clean as possible: Great. How are you doing? Well, there's some work left to be done, I'm affraid.

Magic quotes
I have very little to say about this, except for: RTFM (it's in the security section BTW):

This feature has been DEPRECATED as of PHP 5.3.0 and REMOVED as of PHP 5.4.0.

Just replace the code that deals with magic quotes with a couple of ini_set's, that ammount to:

magic_quotes_gpc = Off
magic_quotes_runtime = Off
magic_quotes_sybase = Off

PDO
After connecting, in the pointless try-catch block (more on that later), you set the error-mode:

$db->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

Only to repeat this call after the try-catch again. That's not clean code, that's clutter. Besides, wouldn't it be cleaner to connect like this:

$pdo = new PDO( 'sqlsrv:server=tcp:'.$host.',1433;Database='.$dbname,
                (string) $username,
                (string) $password,
                array(
                    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
                    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC
                )
       );

Setting your attributes in one fell swoop? this way, I can see what attributes are being set and what server is being connected to. Why, then, am I not catching an exception, that might be thrown here? Simply because your catch block is a die: if the connection fails, your app fails, why catch what cannot be saved?

Other niggles:

Redundat queries: You're preparing a stmt, to check if the email is taken. If it isn't, you proceed to query to check the existance of the username. Why not do this in one go?

SELECT Email FROM tbl WHERE Email = :email OR username = :username

This does exactly the same thing, but requires only one query.

Redirecting: header("Location: login.php"); is not standard, it'll redirect to the login.php script in your pwd (present working directory). The best way to redirect still is:

header("Location: http://yourdomain.com/login.php", true, 301);//redirect permanently
return;//or exit... I hate die

Wrapping your selects in a try-catch=>die is just a waste of space, just select, if it throws an exception, let the app crash, and get to debugging. When INSERT INTO fails, however, that's a different story. You have to use try-catch there, if you want your code to be as safe as it possibly can be, but you'll have to use transactions, and rollback in case of an exception:

try
{
    $pdo->beginTransaction();
    $pdo->exec($insertStmt);
    $pdo->commit();
}
catch(PDOException $e)
{
    $pdo->rollBack();//revert any changes made during last transaction
    throw $e;//rethrow exception, let it go... don't call die
}

require is not safe
Well, it's not the safest option available to you. require_once is. They do exactly the same thing, except for one thing: require_once (as its name suggests) will make sure the file wasn't included already. That makes it a tad slower, but a whole lot safer. Use require_once, then!

globals are bad
global variables are not safe. Ever. Period. Use functions, classes, namespaces and all that, to avoid name-conflicts.

error settings
If this were production code, I hope you'd set display_errors to 0, right? clients shouldn't be able to see what errors your code contains, that's not safe. setting the error_reporting(E_ALL); is good, but consider: error_reporting(E_STRICT|E_ALL); or error_reporting(-1);, and "go for zero" (as in no notices, warnings or errors)

Now, I've saved best for last:

Silly hash!

You're using sha256 with salt as a hash. That's enough. Honestly! When looping, and hashing the hash 65536 times!!, you're just slowing your code down, not making it extra secure. If anything, however unlikely they are (none found to this date, in fact), this will increase the possibility of collisions, not reduce them! Anyway, sha256 is, to this date, perfectly safe:
If you were to have a machine, dedicated to cracking a sha256 hash, it'd take ~= 10^64 years!. If 10^64 is meaningless, here's the number in full:

100.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000.000

That's years, not tries, years!

Answer 2

You talk security, but you make some really bad mistakes.

To start with the error_reporting. NEVER, simply NEVER have E_ALL as error_reporting level on a production enviroment.

You are catching Exceptions, that is good. But what about E_NOTICE? and E_WARNING? you can catch these by registering an error report function using set_error_handler(); Then to be sure you can also add an exception handler in case a uncaught exception bubbles up to the root.

Then, on to your password hashing. Don't reinvent the wheel Personally I prefer PBKDF2, but the stach overflow answer on 'the' sums it up.

Then, why are you generating a session_token? Ok, to be more secure. But you use mt_rand and then simply convert it to hexadecimal. How is this secure?

The next thing I noticed was all those horrible die(); calls. Yuck, they simply ask for a hard time debugging that code. NEVER display an error using die('my error'); If an error happens, throw an exception or what ever and let the error-reporting function do iets job (i.e. loging, mailing the webmaster, ...). This also gives you more control on the output of the application instead of simply sending a HTTP 200 OK message with only 1 string (the error).

Apart from that I have some overall remarks: seperation of concern. Personally I tend to chuck code away without even looking at it if there is business logic and html in the same file. Seperate them. And while your at it. Create one entry point that decides what to do (i.e. the controller / router).

Then there are some small things.

$row = $stmt->fetch();
if ( $row ) ...

Is weird code. What are you doing with $row? nothing...

if ( $stmt->rowCount() === 1 ) ...

Does the job and its easier and more fun to read.

You save the entire user in the $_SESSION and rely on that being set. But this will make scalability very hard. $_SESSION data is stored in a file on the server itself. If you simply store the session data in the database instead of $_SESSION you give yourself much more flexibility. Just my 5 cents

Answer 3

Since there are multiple answers about the security aspect of your code, I'll comment on the "cleanliness" and generally about architecture.

1. Connecting to the database with each HTTP request can expose your application and database to a Distributed Denial Of Service Attack.

   // First we execute our common code to connection to the database and start the session
   require("common.php");
   

   With this at the top of every file, one HTTP request corresponds to one connection to the database. Imagine a botnet throwing tsunami after tsunami of requests at your server. It takes down your server, application AND database. Yow.

   Don't connect to the database unless you need to.

2. Comments should explain why something is done, not what is being done. Read Coding Without Comments for some inspiration. Don't comment the obvious.

   // Note that it is a good practice to NOT end your PHP files with a closing PHP tag.
   // This prevents trailing newlines on the file from being included in your output,
   // which can cause problems with redirecting users.
   

   While this explains why the trailing closing PHP tag was omitted, you don't need to say this. PHP developers are going to know this, or learn this quickly. You don't need to comment about commonly known facts about a language or design pattern.

The rest of your code, while it works, basically breaks every programming best practice I can think of. You've got business logic mixed in with data access code, mixed in with view logic, mixed in with user interaction code, mixed in with user input validation code. These all should be in their own classes. This is really where object oriented code starts to shine.

Utilizing Object Oriented Programming

The key here is to "separate your concerns." What "concerns" are there?

• Data access
• Business logic
• Form validation
• HTTP request handling
• Rendering a view

Separating HTTP Request Handling From the View

For example, Login.php should be split into two files. Login.php should just contain PHP code and no HTML. It should include LoginForm.php, which would have the HTML code. This separates the HTTP request handling from rendering the view. Now you can make changes in either file with minimal impact on the other. Furthermore, you want to re-render the Login form if there are validation errors instead of calling die. Show the user the validation messages:

if (empty($_POST['username'])) {
    $_POST['username_error'] = 'Username is required';
}

Where your Login.php file ended with:

?>
<h1>Login</h1>

It should end with:

include('path/to/LoginForm.php');

And LoginForm.php would be:

<h1>Register</h1>
<form action="register.php" method="post">
    Username:<br />
    <input type="text" name="username" value="<?php echo $_POST['username']; ?>" />
    <span><?php echo $_POST['username_error']; ?></span>
    <br /><br />
    E-Mail:<br />
    <input type="text" name="email" value="<?php echo $_POST['email']; ?>" />
    <span><?php echo $_POST['email_error']; ?></span>
    <br /><br />
    Password:<br />
    <input type="password" name="password" value="<?php echo $_POST['password']; ?>" />
    <span><?php echo $_POST['password_error']; ?></span>
    <br /><br />
    <input type="submit" value="Register" />
</form>

The last two items are separating your data access from business logic.

Separating Data Access From Business Logic

Since you have two tables in the database, create Domain Models for each table. A Domain Model is a class that represents a row from a particular table. You have a table called "Users" so create a class in PHP called User (the singular form of the table name). After that you need another class to perform CRUD operations on the Users table in the database (Create, Read, Update, Delete). For this data access layer, we can utilize the Repository Pattern to really clean things up.

Creating the User Domain Model

First things first. Let's create the Domain Model for the "Users" table called User:

class User
{
    private $id;
    private $username;
    private $title;
    private $first_name;
    private $middle_name;
    private $last_name;
    private $gender;
    private $birth_date;
    private $email;
    private $phone;
    private $mobile_phone;
    private $residential_address;
    private $postal_address;
    private $browser_details;
    private $active;
    private $password;
    private $salt;
    private $last_login_date;
    private $company_id;
    private $date_created;
    private $created_by;
    private $date_modified;
    private $modified_by;

    public __construct($salt, $password, $username, $id = 0)
    {
        $this->username = $username;
        $this->id = $id;
        $this->salt = $salt;
        $this->password = $password;
    }

    // Getters

    public function getId()
    {
        return $this->id;
    }

    public function getUsername()
    {
        return $this->username;
    }

    public function getTitle()
    {
        return $this->title;
    }

    public function getFirstName()
    {
        return $this->first_name;
    }

    public function getMiddleName()
    {
        return $this->middle_name;
    }

    public function getLastName()
    {
        return $this->last_name;
    }

    public function getFullName()
    {
        return $this->first_name . ' ' . $this->middle_name . ' ' . $this->last_name;
    }

    public function getGender()
    {
        return $this->gender;
    }

    public function getBirthDate()
    {
        return $this->birth_date;
    }

    public function getEmail()
    {
        return $this->email;
    }

    public function getPhone()
    {
        return $this->phone;
    }

    public function getMobilePhone()
    {
        return $this->mobile_phone;
    }

    public function getResidentialAddress()
    {
        return $this->residential_address;
    }

    public function getPostalAddress()
    {
        return $this->postal_address;
    }

    public function getBrowserDetails()
    {
        return $this->browser_details;
    }

    public function getLastLoginDate()
    {
        return $this->last_login_date;
    }

    public function getCompanyId()
    {
        return $this->company_id;
    }

    public function getDateCreated()
    {
        return $this->date_created;
    }

    public function getCreatedBy()
    {
        return $this->created_by;
    }

    public function getDateModified()
    {
        return $this->date_modified;
    }

    public function getModifiedBy()
    {
        return $this->modified_by;
    }

    // Setters

    public function setId($value)
    {
        $this->id = $value;
    }

    public function setTitle($value)
    {
        $this->title = $value;
    }

    public function setFirstName($value)
    {
        $this->first_name = $value;
    }

    public function setMiddleName($value)
    {
        $this->middle_name = $value;
    }

    public function setLastName($value)
    {
        $this->last_name = $value;
    }

    public function setGender($value)
    {
        $this->gender = $value;
    }

    public function setBirthDate($value)
    {
        if ($value > now()) {
            throw new Exception("Birth date cannot be in the future");
        }

        $this->birth_date = $value;
    }

    public function setEmail($value)
    {
        $this->email = $value;
    }

    public function setPhone($value)
    {
        $this->phone = $value;
    }

    public function setMobilePhone($value)
    {
        $this->mobile_phone = $value;
    }

    public function setResidentialAddress(Address $value)
    {
        $this->residential_address = $value;
    }

    public function setPostalAddress(Address $value)
    {
        $this->postal_address = $value;
    }

    public function setBrowserDetails($value)
    {
        $this->browser_details = $value;
    }

    public function setPassword($value)
    {
        $this->password = $value;
    }

    public function setLastLoginDate($value)
    {
        $this->last_login_date = $value;
    }

    public function setCompanyId($value)
    {
        $this->company_id = $value;
    }

    public function setDateCreated($value)
    {
        $this->date_created = $value;
    }

    public function setCreatedBy($value)
    {
        $this->created_by = $value;
    }

    public function setDateModified($value)
    {
        $this->date_modified = $value;
    }

    public function setModifiedBy($value)
    {
        $this->modified_by = $value;
    }

    // Actions

    public function activate()
    {
        $this->active = true;
    }

    public function changePassword($old_password_raw, $new_password_raw)
    {
        $password_changed = false;
        $old_password = // Encrypt $old_password_raw using $this->salt
        $new_password = // Encrypt $old_password_raw $this->salt

        if (/* $old_password equals $this->password */) {
            $this->password = $new_password;
            $password_changed = true;
        }

        return $password_changed;
    }

    public function deactivate()
    {
        $this->active = false;
    }

    public function isActive()
    {
        return $this->active;
    }

    public function logIn($password)
    {
        // Use $this->salt to encrypt $password and compare it to $this->password
    }
}

There's a lot going on here. First, create private properties for each column in the Users table. We create getter and setter methods for each property, except a few key properties:

• username: This is required just to have a User object, and should get supplied in the constructor.
• salt: This is security related and should be provided in the constructor. Code outside the User class should not ever need to know the salt. Any code requiring the salt should be a method in the User class.
• password: This should be provided in the constructor and never accessed outside of the User object for the same reasons as the salt.

We have additional "actions" that can be performed on a user. For instance, activating and deactivating a user, or changing their password:

class User
{
    // ...

    // Actions

    public function activate()
    {
        $this->active = true;
    }

    public function changePassword($old_password_raw, $new_password_raw)
    {
        $password_changed = false;
        $old_password = // Encrypt $old_password_raw using $this->salt
        $new_password = // Encrypt $old_password_raw $this->salt

        if (/* $old_password equals $this->password */) {
            $this->password = $new_password;
            $password_changed = true;
        }

        return $password_changed;
    }

    public function deactivate()
    {
        $this->active = false;
    }

    public function isActive()
    {
        return $this->active;
    }

    public function logIn($password)
    {
        // Use $this->salt to encrypt $password and compare it to $this->password
    }
}

These "actions" are the Business Logic of a User. Bundling data (private properties) with methods that operate on those properties is an advantage of creating Domain Models. This Business Logic becomes very portable. Everywhere you have a User object, you have all the necessary business logic for users.

You'll also notice that the set_residential_address and set_postal_address methods have a PHP Type Hint requiring you to pass in an Address object:

class Address
{
    private $address;
    private $postal_code;
    private $suburb;
    private $state;
    private $country;

    public __construct($address, $postal_code, $suburb, $state, $country)
    {
        $this->address = $address;
        $this->postal_code = $postal_code;
        $this->suburb = $suburb;
        $this->state = $state;
        $this->country = $country;
    }

    public function getAddress()
    {
        return $this->address;
    }

    public function getPostalCode()
    {
        return $this->postal_code;
    }

    public function getSuburb()
    {
        return $this->suburb;
    }

    public function getState()
    {
        return $this->state;
    }

    public function getCountry()
    {
        return $this->country;
    }
}

Why create an Address class? You have two sets of columns on the Users table with similar sounding names. Creating an Address class allows you to follow the "Don't Repeat Yourself" guideline (DRY up your code).

• ResidentialAddress
• ResidentialPostCode
• ResidentialSuburb
• ResidentialState
• ResidentialCountry
• PostalAddress
• PostalPostCode
• PostalSuburb
• PostalState
• PostalCountry

This means your User class needs only two properties to represent these two similar pieces of information, rather than 10.

Creating the Data Access Layer

For the data access layer, we will use the Repository Design Pattern. This is where you will use the PDO class to connect to the database. We will create a parent class called DatabaseRepository and then create a UserRepository class that provides methods allowing you to perform CRUD operations on the Users table.

The DatabaseRepository class:

class DatabaseRepository
{
    private $db;
    private $connection_string;

    public __construct()
    {
        $this->connection_string = // Get connection string from a config file
        $this->db = new PDO($this->connection_string);
    }

    protected function executeQuery($sql, $params)
    {
        $statement = $this->db->prepare($sql);

        return $statement->execute($params);
    }
}

This doesn't contain much. There are a couple of keys to this class:

1. The $db property is private. Anytime code needs to use the $db object, you must define a protected method in this class so child classes have controlled access to the database. This will make refactoring code later much easier since no child classes directly access $this->db.

2. The connection string to the database should be read from a config file. Don't hard code this in PHP. Putting this in a config file allows you to define multiple "environments" for your application: Dev, Test and Production, for instance, should all connect to different databases.

Now let's see the UserRepostory class, which inherits from DatabaseRepository:

class UserRepository extends DatabaseRepository
{
    private static $table_name = 'dbo.[User]';
    private static $sql_find_by_username = "SELECT TOP 1 * FROM {UserRepository::$table_name} WHERE Username = :username";

    public function findByUsername($username)
    {
        $params = array(':username' => $username);
        $data = $this->executeQuery(self::$sql_find_by_username, $params);

        if (empty($data)) {
            return null;
        }

        return $this->mapToUser($data);
    }

    private function mapToUser($data)
    {
        $user = new User($data['Salt'], $data['Password'], $data['Username'], $data['UserID']);

        $user->set_first_name($data['FirstName']);
        $user->set_middle_name($data['MiddleName']);

        // Call more setters...

        // Map a complex property...
        $residential_address = new Address(
            $data['ResidentialAddress'],
            $data['ResidentialPostCode'],
            $data['ResidentialSuburb'],
            $data['ResidentialState'],
            $data['ResidentialCountry']
        );

        // Set the complex property
        $user->setResidentialAddress($residential_address);

        return $user;
    }
}

Some things to note:

• The table name is a static (class level) property
• Any hard coded SQL should also be kept in constants or static properties
• The findByUsername method takes a username and returns a fully populated User object
• The map_to_user method takes an array of data directly from the database and calls the appropriate methods on the User object.

Cleaning up Login.php

Now that we have our view, business logic and data access separated out, we can really shorten up Login.php:

<?php

// Login.php

if (isset($_POST)) {
    // Display the form with a GET request
    include('path/to/LoginForm.php');
}
else {
    if (empty($_POST['username'])) {
        $_POST['username_error'] = 'Username is required';

        // Redisplay the form if a POST results in validation errors
        include('path/to/LoginForm.php');
    } elseif (empty($_POST['password'])) {
        $_POST['password_error'] = 'Password is required';

        // Redisplay the form if a POST results in validation errors
        include('path/to/LoginForm.php');
    } else {
        // Now we connect to the database on a POST request
        $repository = new UserRepository();
        $user = $repository.findByUsername($_POST['username']);

        if (!isset($user)) {
            header('location: register.php');
            die();
        }

        if (!$user->logIn($_POST['password'])) {
            $_POST['password_error'] = 'Username or Password is incorrect';
        }

        // Set the user on the session
        // Get/create the session data from the database

        // User successfully logged in
        header('location: private.php');
    }
}

Login.php gets reduced to the bare minimum code required to handle the HTTP request and validate user input. The rendering of the view is delegated to LoginForm.php. Data access is delegated to UserRepository and the business logic of logging in is delegated to User. I intentionally left out finding and mapping the UserSession as an exercise for you (and because this answer is already long enough).