HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Simple CMS system

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
systemsimplecms

Problem

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

```
;
$password = ;
$host = ;
$dbname = ;

// 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

Solution

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!

Code Snippets

magic_quotes_gpc = Off
magic_quotes_runtime = Off
magic_quotes_sybase = Off
$db->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
$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
                )
       );
SELECT Email FROM tbl WHERE Email = :email OR username = :username
header("Location: http://yourdomain.com/login.php", true, 301);//redirect permanently
return;//or exit... I hate die

Context

StackExchange Code Review Q#31396, answer score: 6

Revisions (0)

No revisions yet.