57

I have a simple website where I establish a connection to a MySQL server using PDO.

$dbh = new PDO('mysql:host=localhost;dbname=DB;port=3306',
               'USER',
               'SECRET', 
               array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));

I had some traffic on my site and the server's connection limit was reached, and the website throws this error, with my plain password in it!

Fatal error: Uncaught exception 'PDOException' with message 'SQLSTATE[08004] [1040] Too many connections' in /home/domain/html/index.php:xxx Stack trace: #0 /home/domain/html/index.php(64): PDO->__construct('mysql:host=loca...', 'USER', 'SECRET', Array) #1 {main} thrown in /home/domain/html/index.php on line 64

Ironically I switched to PDO for security reasons, so this really shocked me, because this exact error is something you can provoke very easily on most sites using simple HTTP flooding.

I have now wrapped my connection in a try/catch block, but still I think this is catastrophic!

I am new to PDO and so my question is: what do I have to do to consider to be safe? How do I establish a connection in a secure way? Are there other known security holes like this one that I have to be aware of?

Peter Mortensen
  • 28,342
  • 21
  • 95
  • 123
The Surrican
  • 26,829
  • 23
  • 111
  • 159
  • See: http://stackoverflow.com/questions/5811834/why-would-this-be-poor-php-code/5811853#5811853 For a hole when using dynamic table/db/column names, and how to plug that hole. – Johan Jun 23 '11 at 13:47
  • I wholeheartedly agree with turning off errors in production, try/catch and stuff like that but consider if you are having an offshore "team" of progammers where the password should not be known to "junior" programmers, this is, as you said a "catastrophic" security leak. Not to mention, novice programmers who don't care about turning off errors at all. With that said, I am baffled by this decision to reveal the password on error. – IMB May 29 '12 at 13:43
  • 1
    Holy moly this is SICK! OH MY GOD! This is absolutely outrageous! You need more upvotes just for keeping your cool and not going into CAPS RAGE. – Sharky Jun 08 '13 at 17:40
  • Possible duplicate: *[Uncaught PDOException reveals username and password](https://stackoverflow.com/questions/23571128/uncaught-pdoexception-reveals-username-and-password)* – Peter Mortensen Apr 21 '20 at 16:46

4 Answers4

22

You should have display_errors = off in your PHP.ini anyway to avoid this problem. Errors that reveal details like these come from many places, in addition to PDO.

Yes, you should also have it in a try/catch block.

You can also $pdo->setAttribute(PDO::ERRMODE_SILENT), but then you need to be checking the error codes manually rather than using a try/catch block. See http://php.net/manual/en/pdo.setattribute.php for more error constants.

Brad
  • 146,404
  • 44
  • 300
  • 476
  • 3
    @Joe, then you should take that up with the PDO developers. I see no issue with it returning information from the stack like this. Once you are aware of it, it isn't an issue. Of course, the try/catch will be forgotten on some applications... inevitably it will be a problem for some... you're right about that. – Brad Jun 23 '11 at 14:23
  • 2
    Both of these suggestions don't seem to work for me. When the connection fails, the error stack is printed to the screen with the plain text password visible. I am using PHP ActiveRecord. – Alexandre R. Janini Jun 30 '15 at 20:24
15

A simple workaround is to catch the PDOException thrown by the PDO constructor:

try {
    $dbh  =  new PDO('mysql:host=localhost;dbname=DB;port=3306', 'USER',
    'SECRET', array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
} catch (PDOException $e) {
    throw new Exception($e->getMessage());
}
Peter Mortensen
  • 28,342
  • 21
  • 95
  • 123
Matthias
  • 439
  • 1
  • 5
  • 15
  • 3
    ...that is leaving a developer without an error message that otherwise would help them to solve a problem. – Your Common Sense Jun 02 '16 at 06:00
  • 2
    Does getting exception "Could not connect to database" leave developer with no info? I don't think so – Matthias Jun 02 '16 at 14:03
  • 11
    The base concept is bad. Nobody should place a plain password into an object and keep it in memory. The correct solution would be to change PDO code, not to put the plain password into the message of an exception. I consider this as a bug and a security hole in PDO itself. – nagylzs Apr 11 '17 at 12:00
  • @YourCommonSense That's easily fixed (and I've included how in an edit). – duskwuff -inactive- Aug 21 '18 at 20:53
  • @duskwuff thank you a lot. It really does solve a problem. I already updated all my tutorials. The only thing I have to add is that if a function is used to wrap the connection code (as it usually is), it shouldn't accept raw credentials as its parameters, but accept them as an array or an object or retrieve in the body. Otherwise the problem will be moved one level up but persists. – Your Common Sense Apr 01 '19 at 10:23
  • I still don't understand why developers justify printing credentials in logfiles/error messages/web responses. It does not help anybody but hackers, and is one of the main reasons PHP developers are transitioning to ASP.NET Core – Pieterjan Jul 07 '20 at 20:59
8

Ok, this made me giggle a little. The usage of error reporting is for debugging purposes, and it allows you to quickly find and fix issues.

When you're within a live environment your server should be configured for internal logging only, and not direct output, so basically you will need to turn off the output of errors within your php.ini.

display_errors = Off

But while you're within your test environment, this stack is merely a tool to help you and is configurable.

When errors occur within a live environment they would be logged, so you should always be checking your log files and then fix accordingly.

People may specify that you can manage errors within your PHP application, but by personal preference I think this is the wrong way to go about it. Configuring the INI and configuration files for your web-server and MySQL / SQL Server will result in more acute management.

If your application is a public application then it would also be a good idea to handle errors within the application as a large percentage of clients may be on shared hosting and not have full access to server configurations.

Peter Mortensen
  • 28,342
  • 21
  • 95
  • 123
RobertPitt
  • 54,473
  • 20
  • 110
  • 156
2

We use encoded username and passwords, and decode those in the PDO constructor. Then we catch the PDOException and throw a new PDOException with the old exception its message, so that the trace will show only the encoded username and password.

A good encryption library for PHP is defuse/php-encryption.

Example code:

<?php
class myPDOWrapper extends PDO
    {

        public function __construct(string $dns, string $encodedUser, string $encodedPassword)
        {
            try {
                parent::__construct($dns, $this->decodeFunction($encodedUser), $this->decodeFunction($encodedPassword),
                    [
                        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
                    ]
                );
            }
            catch (PDOException $exception) {
                throw new PDOException($exception->getMessage());
            }
        }

        private function decodeFunction(string $encoded): string
        {
            return \Defuse\Crypto\Crypto::decrypt($encoded, $this->decodeKey());
        }

        private function decodeKey(): \Defuse\Crypto\Key
        {
            static $key = null;

            if(null === $key) {
                $key = \Defuse\Crypto\Key::loadFromAsciiSafeString(getenv('MY_PDO_DECODE_KEY'));
            }

            return $key;
        }
    }
Peter Mortensen
  • 28,342
  • 21
  • 95
  • 123
Wesley Abbenhuis
  • 642
  • 11
  • 17