Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question

2 Answers 2

up vote 5 down vote accepted

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!

share|improve this answer
    
I assume sha256 is being called repeatedly as a form of key-strengthening. If your goal is to prevent an external user from hacking, a delay will work just as well, yet will avoid burning CPU. If your goal is to make it more painful to crack your database (i.e., if someone manages to steal your password database and you want to slow down their efforts to extract plain-text passwords), then key-strengthening will help. That being said, I agree with Pinoniq that it's better to use an existing key-strengthening function. –  Brian Sep 17 '13 at 13:24
    
@Brian: My point is: sha256 is secure enough... there's other things you'd want secure first. You're not going to spend time applying sealant around your bath tub when your roof is leaking, are you? –  Elias Van Ootegem Sep 17 '13 at 13:38
    
Good thing to point out that sha256 multiple times won't help as you pointed out. It will make it weaker because of a bigger chance of collisions. –  Pinoniq Sep 17 '13 at 14:07
    
@Pinoniq: "this will increase the possibility of collisions, not reduce them"... already pointed that out... though the increase of probability is ≃ 0%, but about twice as big a chance of a natural collision. Ah well... there is a some increased risk of collision –  Elias Van Ootegem Sep 17 '13 at 14:28
    
@EliasVanOotegem: Maybe not, but you're also not going to start ripping off existing sealant from your bath tub while your roof is leaking. The OP's use repeated calls to sha256 is accomplishing something. Whether it is important to protect the database after it has been stolen is a decision the OP can make. It's good practice to do so (mitigates attacks on your users' passwords, since they are probably using the same password everywhere). I admit this particular protection is low priority, but that isn't justification to remove it if it is already in place. –  Brian Sep 17 '13 at 14:41

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

share|improve this answer
    
I disagree with your saying catching errors is good. If the catch-block only contains a die satement, catch is just dead-weight, and will bypass any exception handlers registered. If something your app needs throws an exception, don't catch it 'till the very end (exception handler)... BTW $row is used in some cases, though –  Elias Van Ootegem Sep 17 '13 at 13:17
    
Personally I think depending on the situation some exceptions need to be catched and others not. It also depends on what programming style you are using. Are you returning a boolean success? or are you throwing an exception? Do you throw exception for every little problem (i.e. no an int)? And as I also pointed out, heshouldnt simply die. But I do get what you mean. Overall try-catch will be dead weight –  Pinoniq Sep 17 '13 at 14:03
    
Of course, some exceptions have to be caught, just like I pointed out in my example: an insert fails -> rollback the transaction, this should be done using try-catch. –  Elias Van Ootegem Sep 17 '13 at 14:21

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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