2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 2 simple things: include isn't a function, so you can drop the () around it; and you forgot to close the string with the filename on the include. \$\endgroup\$ – Ismael Miguel Nov 4 '14 at 15:56
  • \$\begingroup\$ Thanks, edited it, only forgot the close ' in this question. The script is working, I'm mainly interested in security aspects of this script \$\endgroup\$ – baao Nov 4 '14 at 15:57
  • \$\begingroup\$ I will tell 1 security problem: the password is stored without any kind of encryption. Even a simple xor cypher would be helpful. Both on the file and on the file .dbinc.cred.inc.php (hope I'm wrong) and on the database. And please, tell me you don't have all your databases' credentials stored in that database you are connecting with PDO. \$\endgroup\$ – Ismael Miguel Nov 4 '14 at 16:01
  • 1
    \$\begingroup\$ @jsanc623 You caught me on my gaming session. But I will make the answer now. It might take 10-20 minutes. \$\endgroup\$ – Ismael Miguel Nov 4 '14 at 18:52
  • 1
    \$\begingroup\$ @jsanc623 I have posted as an answer, as you advised. I also added some tiny issues that his code had. Anything that isn't accurate or that escaped, feel free to correct. \$\endgroup\$ – Ismael Miguel Nov 4 '14 at 19:39
3
\$\begingroup\$

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

\$\endgroup\$
  • 1
    \$\begingroup\$ This is one of the best, if not the best answer, I have gotten on SO so far. Thank you very much, the code looks like in your answer now (and pw and stuff are encrypted with mcrypt) \$\endgroup\$ – baao Nov 4 '14 at 19:42
  • \$\begingroup\$ @michael Thank you. It gave me a bit of work and took longer than I though. Any inaccuracy you find, please edit the answer or leave a comment. I forgot to do something regarding that last echo that sends Javascript code. I will tackle it now. \$\endgroup\$ – Ismael Miguel Nov 4 '14 at 19:51
  • \$\begingroup\$ I learn PDO since today, so your code examples were a nice help, too! \$\endgroup\$ – baao Nov 4 '14 at 19:55
  • \$\begingroup\$ @michael PDO is a little difficult to learn in 1 day. But PHP has a decent documentation, in my opinion. I have made the changes. Check the information I have provided about the last echo on your code. \$\endgroup\$ – Ismael Miguel Nov 4 '14 at 20:04
  • \$\begingroup\$ Yes, I think I'll need another day, or some more for that... Got the change. Once again, thank you very much for your time and help! Happy gaming! \$\endgroup\$ – baao Nov 4 '14 at 20:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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