copona / core

Copona Core
1 stars 0 forks source link

Remove server credentials from Whoops #1

Open PieterjanDeClippel opened 3 years ago

PieterjanDeClippel commented 3 years ago

Many developers make mistakes eg. running composer install instead of composer install --no-dev on public servers. This causes the Whoops package to be installed and the possibility of the server credentials being exposed.

https://webcache.googleusercontent.com/search?q=cache:s3DL2jFmOjsJ:https://www.morisson.lv/index.php%3Froute%3Dproduct/category%26path%3D88order%253DASC_194+&cd=20&hl=nl&ct=clnk&gl=be

For the sake of love, use the following package so that the personal information of the users of your users is not exposed to unauthorized audience...

Apart from that, You're never certain that disabling debug mode prevents Whoops from being triggered: https://hacken.io/researches-and-investigations/dangers-of-laravel-debug-mode-enabled/

Whoops can be triggered even with debug mode disabled

prhost commented 3 years ago

Hello @PieterjanDeClippel

I understand your point of view but the whoops in Copona are literally tied to our debug mode configuration as you can see in the code I marked: https://github.com/copona/copona/blob/master/system/startup.php#L185-L195

I believe that in this case the developer is more neglected to leave debug mode enabled. In a way I find this package that hides credentials interesting, but it is only for laravel.

arnisjuraga commented 3 years ago

Yes, @PieterjanDeClippel and @prhost - You are both right. Whoops is loaded if and only dev mode is enabled.

Good thing would be to "remove" dumping of certain sensitive variables in Whoops output. As Copona is not 'Laravel', just uses some packages, I suggest to implement this simply in Whoops handler.

Like this, in startup.php ?

$handler = new \Whoops\Handler\PrettyPageHandler;
foreach (['_ENV', '_GET', '_POST', '_COOKIE', '_SERVER', '_SESSION'] as $global) {
    $handler->blacklist($global, 'DB_PASSWORD');
    $handler->blacklist($global, 'DB_HOSTNAME');
    $handler->blacklist($global, 'DEBUG_ALLOW_IP');
}
$whoops->pushHandler($handler);

Is it enough to have it in PrettyPageHandler only, or should be added for JsonResponseHandler and PlainTextHandler?

PieterjanDeClippel commented 3 years ago

Please don't print server credentials in webpages. It's a bad idea. It really is... 😢

arnisjuraga commented 3 years ago

You are right. this IS just for any debug purposes. You can't protect system from developers, who will just print them anyway. What we can do, is to avoid any "mistakes". Limiting "debug" mode output of sensitive data is always a good practice.

PieterjanDeClippel commented 3 years ago

Thanks for the response. As far as I can tell the JsonResponseHandler only prints the call stack and error. It's only the PrettyPageHandler that provides an entry for Environment variables.

prhost commented 3 years ago

Yes, @PieterjanDeClippel and @prhost - You are both right. Whoops is loaded if and only dev mode is enabled.

Good thing would be to "remove" dumping of certain sensitive variables in Whoops output. As Copona is not 'Laravel', just uses some packages, I suggest to implement this simply in Whoops handler.

Like this, in startup.php ?

$handler = new \Whoops\Handler\PrettyPageHandler;
foreach (['_ENV', '_GET', '_POST', '_COOKIE', '_SERVER', '_SESSION'] as $global) {
    $handler->blacklist($global, 'DB_PASSWORD');
    $handler->blacklist($global, 'DB_HOSTNAME');
    $handler->blacklist($global, 'DEBUG_ALLOW_IP');
}
$whoops->pushHandler($handler);

Is it enough to have it in PrettyPageHandler only, or should be added for JsonResponseHandler and PlainTextHandler?

Hello @arnisjuraga if you follow this path I think that only that would not be enough, I would have to disable all env in this case, imagine that I have a MAILGUN_TOKEN or several database connections DB_OTHER_PASSWORD, how would you identify these customized and sensitive env? I believe that the right thing is to disable them all, just like the library indicated by @PieterjanDeClippel does.

PieterjanDeClippel commented 3 years ago

Yes, @PieterjanDeClippel and @prhost - You are both right. Whoops is loaded if and only dev mode is enabled. Good thing would be to "remove" dumping of certain sensitive variables in Whoops output. As Copona is not 'Laravel', just uses some packages, I suggest to implement this simply in Whoops handler. Like this, in startup.php ?

$handler = new \Whoops\Handler\PrettyPageHandler;
foreach (['_ENV', '_GET', '_POST', '_COOKIE', '_SERVER', '_SESSION'] as $global) {
    $handler->blacklist($global, 'DB_PASSWORD');
    $handler->blacklist($global, 'DB_HOSTNAME');
    $handler->blacklist($global, 'DEBUG_ALLOW_IP');
}
$whoops->pushHandler($handler);

Is it enough to have it in PrettyPageHandler only, or should be added for JsonResponseHandler and PlainTextHandler?

Hello @arnisjuraga if you follow this path I think that only that would not be enough, I would have to disable all env in this case, imagine that I have a MAILGUN_TOKEN or several database connections DB_OTHER_PASSWORD, how would you identify these customized and sensitive env? I believe that the right thing is to disable them all, just like the library indicated by @PieterjanDeClippel does.

In this SO thread there's an extensive discussion where the variables appear and so on (like they're also stored in $_ENV and $_SERVER).

I like the solution of Raheel Haisan which removes the keys of $_ENV, $_SERVER and $_COOKIE all the way.

Besides that, never blacklist, but whitelist. Applications grow over time and to tighten security holes at all levels it's for the best to whitelist...