Script for a multi-user webapp database connection

Question

I have the following working script which I use for my database connection for a multi-user webapp. I use a different database for the login, this script is called only after the user is successfully logged in. The login uses PDO.

I would like to know what I can do to

• make this more secure

• harder to fail

include('.dbinc.cred.inc.php');

try {
$dbh = new PDO('mysql:host=' . HOST_ONE . ';dbname=' . DB_ONE . '', USER_ONE, PASS_ONE);

$statement = $dbh->prepare("SELECT user_db, user_db_pass, user_db_user, user_name FROM databases WHERE user_name = :name");
$statement->execute(array('name' => $_SESSION['user_name']));

foreach ($statement as $row) {
   $user_db = $row['user_db'];
   $user_db_user = $row['user_db_user'];
   $user_db_pass = $row['user_db_pass'];
}

}
  catch (PDOException $e) {
print "Error!: " . $e->getMessage() . "<br/>";
die();
}
$dbh = null;

$sql = new mysqli (HOST_ONE,$user_db_user, $user_db_pass,$user_db);
$sql -> set_charset ( 'utf8' );
if ($sql->connect_errno)
{echo "<script language=JavaScript>location.href='#ajax/account_setup.php';</script> ";}

Is it better to do this in 2 files, one for the user check, then define HOST_TWO and so on and include the first file in the second?

Why am I using PDO and then MySQLi? I do this (weird) thing because the whole app uses mysqli_ at the moment and I'm rewriting everything to PDO atm. But actually I need the MySQLi connection, though.

Answer 1

Lets tackle your code point by point:

Security:

In terms of problems with security, I only see 1 problem:

There isn't any kind of encryption on your code.
From what I can see, you connect to a database that has all the password stored in plain text.
Any attacker that puts his hand on your file .dbinc.cred.inc.php will be very happy!

You can use the mcrypt list of function.
For documentation, you can check here: http://php.net/manual/en/ref.mcrypt.php

Edit:

As @EliasVanOotegem pointed out on his comment, the collation isn't being set.
Appending ';charset=latin1' to the string should do it.

If you are using a verson prior to 5.3.6, this will be ignored.
Instead, you must run $dbh->exec("set names latin1");
Source: https://stackoverflow.com/a/4361485
Luckly, PHP has a function to solve this kind of issue: version_compare().
The version can be checked like this: version_compare(PHP_VERSION, '5.3.6', '<')
Documentation: http://php.net/manual/en/function.version-compare.php

Now, you may ask: why not utf8?
The reason is that the default collation for most servers is in fact latin1_swedish_ci.
This means that a password like aç$%dKeä would be returned as scrambled data.
When you try to connect to the other database, using mysqli_*, the connection would be always rejected because the password would be different.

The code itself:

On the code itself, I see some problems too.

Lets make some sections for it.

The try{} block:

Inside the try{} block, you have the following code:

foreach ($statement as $row) {
   $user_db = $row['user_db'];
   $user_db_user = $row['user_db_user'];
   $user_db_pass = $row['user_db_pass'];
}

This code isn't wrong, but this is useful when you fetch multiple rows from the database.
Another thing is that you only use this code to assign the variables to the values from the database.
You should use the list() constructor, using also the $statement->fetch() method.
The above code can be rewritten into this:

list($user_db,$user_db_user,$user_db_pass)=$statement->fetch(PDO::FETCH_NUM);

Documentation:
list() constructor: http://php.net/manual/en/function.list.php
method $statement->fetch(): http://php.net/manual/en/pdostatement.fetch.php

The catch{} block:

Inside the catch block, you are using a print and on the next line you have an empty die().
Removing the print and placing the text on the die() will have the same effect.
This code:

  catch (PDOException $e) {
print "Error!: " . $e->getMessage() . "<br/>";
die();
}

Becomes:

catch (PDOException $e){
    die('Error!: ' . $e->getMessage() . '<br/>');
}

This is not a real issue, but you are concatenating an empty string on the PDO initialization.
Here is the code:

$dbh = new PDO('mysql:host=' . HOST_ONE . ';dbname=' . DB_ONE . '', USER_ONE, PASS_ONE);

Right after DB_ONE, you can remove that concatenation.

Outside the try ... catch{}`:

Outside the try ... catch{} block, you have this line:

$dbh = null;

This line, at the most, should be moved inside the try{} block, which is where you initialize the PDO object.

Other observations:

Your indentation is a little broken.
Indentation allows us to understand better where a block starts and finishes, and what is inside it.

---

This is just an observation, just for the paranoid people.

You fetch the data from the database but you don't free the result set.

You should run $statement->closeCursor(); after you fetch your data.
You can check the documentation here: http://php.net/manual/en/pdostatement.closecursor.php

After you don't need your variables, you should use unset just to free the memory, deleting the variables $statement and $dbh.
This way, I believe, you can skip the line $dbh = null; which is the example given in the php documentation, on the link: http://php.net/manual/en/pdo.connections.php (go to example #3).

Another thing I noticed is the absence of the php tags.

---

Almost at the end of the code, you have this echo:

echo "<script language=JavaScript>location.href='#ajax/account_setup.php';</script> ";

The language attribute is deprecated and can be safely dropped. (Source: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-language )
This was useful in the times when VBScript was used for Internet Explorer, to diferentiate both languages.
Nowadays, you don't need this.
Another thing is that the value of the attribute isn't between quotes.
If you are paranoid about this, you can use the type attribute.
You can set the value text/javascript, being the final code like this:

echo '<script type="text/javascript">location.href="#ajax/account_setup.php";</script>';

Documentation about the type attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-type

The final code (security apart, reflecting the edit):

Applying all the changes, the final code will look like this:

<?php

    include('.dbinc.cred.inc.php');

    try {

        $dbh = new PDO('mysql:host=' . HOST_ONE . ';dbname=' . DB_ONE . ';charset=latin1', USER_ONE, PASS_ONE);

        if(version_compare(PHP_VERSION, '5.3.6', '<'))
        {
            $dbh->exec('set names latin1');
        }

        $statement = $dbh->prepare('SELECT user_db, user_db_pass, user_db_user, user_name FROM databases WHERE user_name = :name');
        $statement->execute(array('name' => $_SESSION['user_name']));

        list($user_db, $user_db_user, $user_db_pass) = $statement->fetch(PDO::FETCH_NUM);

        $statement->closeCursor();

        unset($statement, $dbh);

    }
    catch (PDOException $e){
        die('Error!: ' . $e->getMessage() . '<br/>');
    }

    $sql = new mysqli(HOST_ONE,$user_db_user, $user_db_pass,$user_db);
    $sql -> set_charset('utf8');
    if ($sql->connect_errno)
    {
        echo '<script type="text/javascript">location.href="#ajax/account_setup.php";</script>';
    }

?>

As a micro-optimization, I swapped the quotes.
In terms of real-world usage, using "" would make a negligible difference.
Use the quotes that you prefer, I simply prefer to save some CPU cycles.

Thanks to @EliasVanOotegem for spotting the safety issue regarding the lack of a specification on the used collation to connect to the database.
This is a serious issue because it changes the way that the parameters are escaped and avoids the use of special characters to 'nullify' the escapes, allowing other commands to run.
You can see how this attack is performed in here: http://hakipedia.com/index.php/SQL_Injection#Filter_Bypassing