Boot exception exposes db username and file system path

matteocontrini

Hello,

I've noticed that when Flarum cannot connect to the database it shows the following error message:

Flarum encountered a boot error (Doctrine\DBAL\Driver\PDOException)
SQLSTATE[HY000] [1045] Access denied for user 'flarum'@'localhost' (using password: YES)
thrown in /[REDACTED]/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php on line 31

Is there a way to change this behaviour so that it doesn't expose potentially sensitive information like the database username and the path where Flarum files are?

Debug mode is off.

Thanks

clarkwinkelmann

matteocontrini the relevant code is in https://github.com/flarum/core/blob/master/src/Http/Server.php

The problem is that the catch block is outside of the whole boot logic, which means it also handles error that could happen before Flarum's config/logger is initialized.

We can't just hide that information altogether, otherwise there would be no way of accessing it and it would be impossible to troubleshoot if it happens to someone.

We'd need some way of accessing whether the app is in debug mode, but the problem is that if an exception occurs at this point, the $app object won't be returned. We'd need to split this into multiple calls, then we could check whether debug mode and/or the logger are available in the application and only default to printing the full error if nothing was loaded.

Given config is always loaded before the database provider boots, this would at least cover the database issues.

Ideally the logger should be used, but an issue is that at that point it could also be in a broken state itself if the boot error was caused by an extension. Only debug mode seems reliable enough to use in an error state since it's just an attribute in an array.

matteocontrini

clarkwinkelmann that would be better, but it would still expose the same information to everyone when you enable debug mode. I understand that in the PHP world it's a common thing to spit errors (exceptions) to the user, but ideally the error details should just be logged to a file for the admins to take a look at. (With other languages/frameworks that would be writing to stdout/stderr, but I guess that in PHP it's a bit trickier since the standard output is actually the web page, I think.)

Looking at the code, wouldn't it be a solution to print a generic error to the page and then rethrow the exception? In this way, wouldn't the exception be logged in the PHP error log and printed only if PHP is configured to do so?

clarkwinkelmann

matteocontrini yes that makes sense.

The risk with trowing the exception without any catch is that the look of a native 500 error page varies a lot from hosting to hosting. On some hostings it might be a completely blank page, which isn't ideal either.

If we can get Flarum logging to kick in during extension loading and provider registration/boot, this would already be a huge improvement. The only problem I see there is the possible discrepancy between the core logger and any extension-provided logger. For example the Sentry extension might report some errors and not others depending on the order the extensions are loaded and which one caused an error. It will still be an improvement over not logging the error like we do now, but I think the possible inconsistency needs consideration.

The problem really is that we can't write anything to disk until Flarum's config has been loaded, because we can't know the location of the storage folder before that, and we shouldn't write anywhere else. Once that is loaded, we could write to a special storage/logs/boot.txt file without having to initialize the full logger with the use of a second catch block.

Not sure how other frameworks/softwares handle that. I believe Laravel does log everything including database connection errors. But in Flarum we do something special, we do a database request before registering/booting all the providers because we need to get the list of enabled extensions.

SKevo

matteocontrini Looking at the code, wouldn't it be a solution to print a generic error to the page and then rethrow the exception? In this way, wouldn't the exception be logged in the PHP error log and printed only if PHP is configured to do so?

Yes, why not just display a general "error" message and direct the admin to look in the flarum logs at the server instead? That would be the best choice IMO, making sure that only people that have access to the server files can read the logs. These exceptions are meant to be logged server-side, and if that's the case and Flarum can leak the database password, then it's a serious security issue that must be addressed.

Also, why would normal users need to see the logs if not the admin? It's useless for them. If we just display "An error occurred, please check server logs if you're admin" message, user will know that there is an error on the server's side, but won't need to know the details and admins will know that the error was logged server-side, where they can fix it.

matteocontrini

clarkwinkelmann The risk with trowing the exception without any catch is that the look of a native 500 error page varies a lot from hosting to hosting. On some hostings it might be a completely blank page, which isn't ideal either.

Can that happen even if the script outputs something before throwing? 🤔

It's a tricky problem anyway. Should I create an issue on GitHub pointing to this discussion?

clarkwinkelmann

matteocontrini yes a GitHub issue would definitely be good so we don't forget about this.

I'd love to hear the input from other members of the team. But I know everyone's focused on beta 14 right now.

matteocontrini

Done: flarum/core2290

luceos

I'd like to point out that we previously had a similar issue and we already removed the database password. I'm not sure to which extend you want information redacted, because there should be some limitation to that madness 😛

I honestly don't think exposing db user and file system path are a huge problem. It's security by obscurity to think otherwise.

matteocontrini

luceos I don't think it's madness. I've never seen a production software expose the username of the database or the file path to the user, it's just not something that should be exposed, IMO. Logs exist for that reason.

WordPress for example doesn't show an exception if the database connection cannot be established. It just tells you gently that it happened. If there are other fatal errors, I think they're just captured by the PHP runtime.

Regarding security, I'm not concerned about that, but I don't like potentially exposing infrastructure choices, like the web server I'm using, the file system structure, whether I'm using a local instance of MySQL, etc.

I hope it makes sense 🙂

Ralkage

matteocontrini well, to be fair, other forum software such as Vbulletin and Xenforo has done or still does similar to how Flarum does it 😆

tankerkiller125

matteocontrini The only software that does things differently than Flarum that I can think of off the top of my head is Wordpress, every single other application I know of just tosses out the raw PHP error.

rob006

tankerkiller125 every single other application I know of just tosses out the raw PHP error.

Any examples? I think that lack of nice error page is pretty rare in these days, and displaying internal exception details is just something that any decent software left 20 years behind. Especially if you do this intentionally, not just overlooked exception exposed by PHP configuration.

tankerkiller125

rob006 All laravel apps I know of (although they now make it slightly better with whoops I think, but still shows the default error) phpBB for sure last I checked

matteocontrini

tankerkiller125 I still don't think that I/we should be fighting for not having exceptions exposed in that way. Maybe it's normal in the PHP world but definitely not with polished software in general. I perfectly understand that it won't be considered as a priority, but we should agree that it's an issue and not madness 🙂

Also, let me link Security by Obscurity is Underrated and Secrecy (Obscurity) is a Valid Security Layer...

askvortsov

I think this discussion can continue in the github issue as we work to decide on an implementation. My current thinking is that instead of exiting, we should just throw an exception so that if display_errors is off, nothing will be displayed. I don't think adding a second layer of whoops at the boot level is necessary: boot errors apply to the whole server, and must be fixed by the server admin.

sylv38

Just to add my case in the discussion to help : today the mysql database returned a too many connections exception (it seems there is a problem with my host) and the boot message was displayed to everybody with the password of the db... it was panic on board when I saw that ! I activated maintenance mode urgently !

I'm running beta 15 with debug off.

The full message hiding critical information with stars :

Flarum encountered a boot error (Doctrine\DBAL\Driver\PDO\Exception)

SQLSTATE[08004] [1040] Too many connections
thrown in /**********************************/beta/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDO/Exception.php on line 18

PDOException: SQLSTATE[08004] [1040] Too many connections in *********************************/beta/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:38
Stack trace:
#0 *********************************/beta/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php(38): PDO->__construct('mysql:host=**********.', '******************', '****************', Array)
#1 *********************************/beta/vendor/illuminate/database/Connectors/Connector.php(67): Doctrine\DBAL\Driver\PDOConnection->__construct('mysql:host=******.', '********', '**********', Array)
#2 *********************************/beta/vendor/illuminate/database/Connectors/Connector.php(46): Illuminate\Database\Connectors\Connector->createPdoConnection('mysql:host=********', '********', '**************', Array)
#3 *********************************/beta/vendor/illuminate/database/Connectors/MySqlConnector.php(24): Illuminate\Database\Connectors\Connector->createConnection('mysql:host=*****', Array, Array)
#4 *********************************/beta/vendor/illuminate/database/Connectors/ConnectionFactory.php(182): Illuminate\Database\Connectors\MySqlConnector->connect(Array)
#5 [internal function]: Illuminate\Database\Connectors\ConnectionFactory->Illuminate\Database\Connectors\{closure}()
#6 *********************************/beta/vendor/illuminate/database/Connection.php(924): call_user_func(Object(Closure))
#7 *********************************/beta/vendor/illuminate/database/Connection.php(906): Illuminate\Database\Connection->getPdo()
#8 *********************************/beta/vendor/illuminate/database/Connection.php(892): Illuminate\Database\Connection->getDoctrineConnection()
#9 *********************************/beta/vendor/flarum/core/src/Database/Migrator.php(67): Illuminate\Database\Connection->getDoctrineSchemaManager()
#10 [internal function]: Flarum\Database\Migrator->__construct(Object(Flarum\Database\DatabaseMigrationRepository), Object(Illuminate\Database\MySqlConnection), Object(Illuminate\Filesystem\Filesystem))
#11 *********************************/beta/vendor/illuminate/container/Container.php(843): ReflectionClass->newInstanceArgs(Array)
#12 *********************************/beta/vendor/illuminate/container/Container.php(681): Illuminate\Container\Container->build('Flarum\\Database...')
#13 *********************************/beta/vendor/illuminate/container/Container.php(629): Illuminate\Container\Container->resolve('Flarum\\Database...', Array)
#14 *********************************/beta/vendor/illuminate/container/Container.php(945): Illuminate\Container\Container->make('Flarum\\Database...')
#15 *********************************/beta/vendor/illuminate/container/Container.php(873): Illuminate\Container\Container->resolveClass(Object(ReflectionParameter))
#16 *********************************/beta/vendor/illuminate/container/Container.php(834): Illuminate\Container\Container->resolveDependencies(Array)
#17 *********************************/beta/vendor/illuminate/container/Container.php(261): Illuminate\Container\Container->build('Flarum\\Extensio...')
#18 *********************************/beta/vendor/illuminate/container/Container.php(799): Illuminate\Container\Container->Illuminate\Container\{closure}(Object(Illuminate\Container\Container), Array)
#19 *********************************/beta/vendor/illuminate/container/Container.php(681): Illuminate\Container\Container->build(Object(Closure))
#20 *********************************/beta/vendor/illuminate/container/Container.php(629): Illuminate\Container\Container->resolve('Flarum\\Extensio...', Array)
#21 *********************************/beta/vendor/flarum/core/src/Extension/ExtensionServiceProvider.php(30): Illuminate\Container\Container->make('flarum.extensio...')
#22 [internal function]: Flarum\Extension\ExtensionServiceProvider->Flarum\Extension\{closure}(Object(Flarum\Foundation\Application))
#23 *********************************/beta/vendor/flarum/core/src/Foundation/Application.php(322): call_user_func(Object(Closure), Object(Flarum\Foundation\Application))
#24 *********************************/beta/vendor/flarum/core/src/Foundation/Application.php(263): Flarum\Foundation\Application->fireAppCallbacks(Array)
#25 *********************************/beta/vendor/flarum/core/src/Foundation/InstalledSite.php(148): Flarum\Foundation\Application->boot()
#26 *********************************/beta/vendor/flarum/core/src/Foundation/InstalledSite.php(81): Flarum\Foundation\InstalledSite->bootLaravel()
#27 *********************************/beta/vendor/flarum/core/src/Http/Server.php(56): Flarum\Foundation\InstalledSite->bootApp()
#28 *********************************/beta/vendor/flarum/core/src/Http/Server.php(33): Flarum\Http\Server->safelyBootAndGetHandler()
#29 *********************************/beta/public/index.php(26): Flarum\Http\Server->listen()
#30 {main}

Next Doctrine\DBAL\Driver\PDO\Exception: SQLSTATE[08004] [1040] Too many connections in *********************************/beta/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDO/Exception.php:18
Stack trace:
#0 *********************************/beta/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php(42): Doctrine\DBAL\Driver\PDO\Exception::new(Object(PDOException))
#1 *********************************/beta/vendor/illuminate/database/Connectors/Connector.php(67): Doctrine\DBAL\Driver\PDOConnection->__construct('mysql:host=*****', '*****', '************', Array)
#2 *********************************/beta/vendor/illuminate/database/Connectors/Connector.php(46): Illuminate\Database\Connectors\Connector->createPdoConnection('mysql:host=*****', '*****', '************', Array)
#3 *********************************/beta/vendor/illuminate/database/Connectors/MySqlConnector.php(24): Illuminate\Database\Connectors\Connector->createConnection('mysql:host=*****', Array, Array)
#4 *********************************/beta/vendor/illuminate/database/Connectors/ConnectionFactory.php(182): Illuminate\Database\Connectors\MySqlConnector->connect(Array)
#5 [internal function]: Illuminate\Database\Connectors\ConnectionFactory->Illuminate\Database\Connectors\{closure}()
#6 *********************************/beta/vendor/illuminate/database/Connection.php(924): call_user_func(Object(Closure))
#7 *********************************/beta/vendor/illuminate/database/Connection.php(906): Illuminate\Database\Connection->getPdo()
#8 *********************************/beta/vendor/illuminate/database/Connection.php(892): Illuminate\Database\Connection->getDoctrineConnection()
#9 *********************************/beta/vendor/flarum/core/src/Database/Migrator.php(67): Illuminate\Database\Connection->getDoctrineSchemaManager()
#10 [internal function]: Flarum\Database\Migrator->__construct(Object(Flarum\Database\DatabaseMigrationRepository), Object(Illuminate\Database\MySqlConnection), Object(Illuminate\Filesystem\Filesystem))
#11 *********************************/beta/vendor/illuminate/container/Container.php(843): ReflectionClass->newInstanceArgs(Array)
#12 *********************************/beta/vendor/illuminate/container/Container.php(681): Illuminate\Container\Container->build('Flarum\\Database...')
#13 *********************************/beta/vendor/illuminate/container/Container.php(629): Illuminate\Container\Container->resolve('Flarum\\Database...', Array)
#14 *********************************/beta/vendor/illuminate/container/Container.php(945): Illuminate\Container\Container->make('Flarum\\Database...')
#15 *********************************/beta/vendor/illuminate/container/Container.php(873): Illuminate\Container\Container->resolveClass(Object(ReflectionParameter))
#16 *********************************/beta/vendor/illuminate/container/Container.php(834): Illuminate\Container\Container->resolveDependencies(Array)
#17 *********************************/beta/vendor/illuminate/container/Container.php(261): Illuminate\Container\Container->build('Flarum\\Extensio...')
#18 *********************************/beta/vendor/illuminate/container/Container.php(799): Illuminate\Container\Container->Illuminate\Container\{closure}(Object(Illuminate\Container\Container), Array)
#19 *********************************/beta/vendor/illuminate/container/Container.php(681): Illuminate\Container\Container->build(Object(Closure))
#20 *********************************/beta/vendor/illuminate/container/Container.php(629): Illuminate\Container\Container->resolve('Flarum\\Extensio...', Array)
#21 *********************************/beta/vendor/flarum/core/src/Extension/ExtensionServiceProvider.php(30): Illuminate\Container\Container->make('flarum.extensio...')
#22 [internal function]: Flarum\Extension\ExtensionServiceProvider->Flarum\Extension\{closure}(Object(Flarum\Foundation\Application))
#23 *********************************/beta/vendor/flarum/core/src/Foundation/Application.php(322): call_user_func(Object(Closure), Object(Flarum\Foundation\Application))
#24 *********************************/beta/vendor/flarum/core/src/Foundation/Application.php(263): Flarum\Foundation\Application->fireAppCallbacks(Array)
#25 *********************************/beta/vendor/flarum/core/src/Foundation/InstalledSite.php(148): Flarum\Foundation\Application->boot()
#26 *********************************/beta/vendor/flarum/core/src/Foundation/InstalledSite.php(81): Flarum\Foundation\InstalledSite->bootLaravel()
#27 *********************************/beta/vendor/flarum/core/src/Http/Server.php(56): Flarum\Foundation\InstalledSite->bootApp()
#28 *********************************/beta/vendor/flarum/core/src/Http/Server.php(33): Flarum\Http\Server->safelyBootAndGetHandler()
#29 *********************************/beta/public/index.php(26): Flarum\Http\Server->listen()
#30 {main}

sylv38

I reup this issue as the database on my host was again down for a moment... and the password was shown again... and I have to change it again and again... and again, each time the database connection goes wrong and that I see it.
This is really a very very big security issue !

luceos

sylv38 I agree. I'm on mobile right now. If someone can find or create an issue for this? I'll try to look into this asap.

clarkwinkelmann

We have agreed to change the error message, that will be done for the next release.

However while it should be fixed, we still haven't found a setup to replicate the password leak. That would really help if we could replicate it so that we can test the fix on one of those setup as well.

sylv38 can you share the details about your hosting? Company, operating system version, MySQL version, PHP version?

askvortsov

There's already an issue on the milestone, with a proposed solution: flarum/core2290

sylv38

askvortsov Because it's critical, I edited the core file to patch the function with your proposition on the milestone ! Thank you !