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 tableCREATE 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 OFFALTER TABLE [dbo].[User] ADD [Username] [varchar](50) NULLSET ANSI_PADDING ONALTER 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)GOSET ANSI_PADDING OFFGOALTER TABLE [dbo].[User] ADD  DEFAULT ('User') FOR [UserRole]GOUserSession TableCREATE 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))GOSET ANSI_PADDING OFFGOALTER TABLE [dbo].[UserSession] ADD  DEFAULT (getdate()) FOR [Created]GOALTER TABLE [dbo].[UserSession] ADD  DEFAULT (user_name()) FOR [CreatedBy]GO
Simple CMS system
php;sql;security;sql server
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 quotesI 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 = Offmagic_quotes_runtime = Offmagic_quotes_sybase = OffPDOAfter 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 = :usernameThis 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 permanentlyreturn;//or exit... I hate dieWrapping 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 safeWell, 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 badglobal variables are not safe. Ever. Period. Use functions, classes, namespaces and all that, to avoid name-conflicts.error settingsIf 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.000That's years, not tries, years!