Everything

php

[PATCH] Add configuration value to enable/disable stack trace logging

php.internals

Erik Lundin

7 years ago

Background:
The latest version of PHP seems to handle fatal errors as exceptions which
results in stack traces being logged. Stack traces can potentially contain
sensitive information and should not be logged in a production
environment.

Test code:
<?php
function handle_password($a) {
does_not_exist();
}
handle_password('s3cretp4ssword');

PHP 5.4.16:
Jun 17 15:58:01 server php[29650]: PHP Fatal error:  Call to undefined
function does_not_exist() in /var/www/html/index.php on line 3

PHP 7.4 (dev):
Jun 17 15:58:01 server php[18159]: PHP Fatal error:  Uncaught Error: Call
to undefined function does_not_exist() in
/var/www/html/index.php:3#012Stack trace:#012#0
/var/www/html/index.php(5): handle_password('s3cretp4ssword')#012#1
{main}#012  thrown in /var/www/html/index.php on line 3

Suggested patch:
Add a configuration value to be able to prevent exceptions from logging
stack traces.

log_exception_trace = On/Off

It would be optimal to have this disabled as default as novice
administrators would perhaps not be aware that this information would be
logged. For debugging purposes it would be helpful to be able to enable
this but maybe the default value should be set conservatively to minimize
unnecessary problems?

I've added this configuration value in Zend/zend.c as the exception
message is compiled in Zend/zend_exceptions.c. Adding it to main/main.c
would change the scope from zend_compiler_globals to php_core_globals and
I guess that you wouldn't want to mix them?

Link to pull request: https://github.com/php/php-src/pull/4281

Regards, Erik Lundin

Mark Randall

7 years ago

On 17/06/2019 18:10, Erik Lundin wrote:

> Background:
> The latest version of PHP seems to handle fatal errors as exceptions
> which results in stack traces being logged. Stack traces can potentially
> contain sensitive information and should not be logged in a production
> environment.

Having access to the full stack trace is, in my opinion, an essential
tool for debugging.

Standard PHP Error reporting should always be disabled on production.

On the other hand, security in layers, and the less information you
hold, the less probability there is of it getting out, or being
catastrophic when you do, so having the option to turn it off wouldn't hurt.

As it happens I was dealing with some issues last month where my first
order exception handler was failing, and logs were being put into
stackdriver where they could potentially have been accessed by those who
don't have direct access to the processes but do have access to logging.

I was wondering at the time if it would be possible to supply a public
key via the PHP.ini and have the outputs encrypted before being written
- as this is how I handle the stack traces in my userland exception
logging database and IMHO would provide best-of-both-worlds.

The benefits of public key vs a symmetric key are that the logs remain
secure even with read access to php.ini.

--
Mark Randall

Björn Larsson

7 years ago

Den 2019-06-17 kl. 19:10, skrev Erik Lundin:

> Background:
> The latest version of PHP seems to handle fatal errors as exceptions
> which results in stack traces being logged. Stack traces can
> potentially contain sensitive information and should not be logged in
> a production environment.
>
> Test code:
> <?php
> function handle_password($a) {
>         does_not_exist();
> }
> handle_password('s3cretp4ssword');
>
> PHP 5.4.16:
> Jun 17 15:58:01 server php[29650]: PHP Fatal error:  Call to undefined
> function does_not_exist() in /var/www/html/index.php on line 3
>
> PHP 7.4 (dev):
> Jun 17 15:58:01 server php[18159]: PHP Fatal error:  Uncaught Error:
> Call to undefined function does_not_exist() in
> /var/www/html/index.php:3#012Stack trace:#012#0
> /var/www/html/index.php(5): handle_password('s3cretp4ssword')#012#1
> {main}#012  thrown in /var/www/html/index.php on line 3
>
> Suggested patch:
> Add a configuration value to be able to prevent exceptions from
> logging stack traces.
>
> log_exception_trace = On/Off
>
> It would be optimal to have this disabled as default as novice
> administrators would perhaps not be aware that this information would
> be logged. For debugging purposes it would be helpful to be able to
> enable this but maybe the default value should be set conservatively
> to minimize unnecessary problems?
>
> I've added this configuration value in Zend/zend.c as the exception
> message is compiled in Zend/zend_exceptions.c. Adding it to
> main/main.c would change the scope from zend_compiler_globals to
> php_core_globals and I guess that you wouldn't want to mix them?
>
> Link to pull request: https://github.com/php/php-src/pull/4281
>
> Regards, Erik Lundin
>

Hi,

In our environment these kind of errors goes to the Apache error log.
We have full control of the complete LAMP stack and it's only our
ISP who can access these. So little risk for leakage of sensitive info.

Now we have a large legacy code base that has been migrated from
PHP 5.2 to PHP 7.3. Even if we have tested a lot we have had great
usage of these kind of stack traces in the production environment.
It helped us fixing the (hopefully) last issues. We also have a kind of
beta site, but we didn't got enough traffic on that one to trigger the
errors we got in production.

My 50c on this subject, well aware of that having "ownership" of the
LAMP stack is one prerequisite to not disclose sensitive info.

r//Björn Larsson

Erik Lundin

7 years ago

Joe’s solution seems to fix the problem. I havent tested it yet though. I would have been forced to patch this reguardless before bringing php 7+ into production. His fix would be enough to protect the data provided proper config files are enforced.

Thanks Joe! Hopefully this will be merged which would be one less thing to maintain.

/Erik

Thomas Lamy

7 years ago

Am 17.06.19 um 21:27 schrieb Björn Larsson:

> Den 2019-06-17 kl. 19:10, skrev Erik Lundin:
>> Background:
>> The latest version of PHP seems to handle fatal errors as exceptions
>> which results in stack traces being logged. Stack traces can
>> potentially contain sensitive information and should not be logged in
>> a production environment.
>>
>> Test code:
>> <?php
>> function handle_password($a) {
>>         does_not_exist();
>> }
>> handle_password('s3cretp4ssword');
>>
>> PHP 5.4.16:
>> Jun 17 15:58:01 server php[29650]: PHP Fatal error:  Call to
>> undefined function does_not_exist() in /var/www/html/index.php on line 3
>>
>> PHP 7.4 (dev):
>> Jun 17 15:58:01 server php[18159]: PHP Fatal error:  Uncaught Error:
>> Call to undefined function does_not_exist() in
>> /var/www/html/index.php:3#012Stack trace:#012#0
>> /var/www/html/index.php(5): handle_password('s3cretp4ssword')#012#1
>> {main}#012  thrown in /var/www/html/index.php on line 3
>>
>> Suggested patch:
>> Add a configuration value to be able to prevent exceptions from
>> logging stack traces.
>>
>> log_exception_trace = On/Off
>>
>> It would be optimal to have this disabled as default as novice
>> administrators would perhaps not be aware that this information would
>> be logged. For debugging purposes it would be helpful to be able to
>> enable this but maybe the default value should be set conservatively
>> to minimize unnecessary problems?
>>
>> I've added this configuration value in Zend/zend.c as the exception
>> message is compiled in Zend/zend_exceptions.c. Adding it to
>> main/main.c would change the scope from zend_compiler_globals to
>> php_core_globals and I guess that you wouldn't want to mix them?
>>
>> Link to pull request: https://github.com/php/php-src/pull/4281
>>
>> Regards, Erik Lundin
>>
> Hi,
>
> In our environment these kind of errors goes to the Apache error log.
> We have full control of the complete LAMP stack and it's only our
> ISP who can access these. So little risk for leakage of sensitive info.
>
> Now we have a large legacy code base that has been migrated from
> PHP 5.2 to PHP 7.3. Even if we have tested a lot we have had great
> usage of these kind of stack traces in the production environment.
> It helped us fixing the (hopefully) last issues. We also have a kind of
> beta site, but we didn't got enough traffic on that one to trigger the
> errors we got in production.
>
> My 50c on this subject, well aware of that having "ownership" of the
> LAMP stack is one prerequisite to not disclose sensitive info.
>
> r//Björn Larsson
>

A "me too" from here.

For stacktraces with fully qualified class names, I would rather like to
have the max string length of arguments in the backtrace configurable
instead of the currently hard coded value.

Also, what about cutting a base path from the stack trace? Having
something like "/opt/projects/customer/project/" no longer in the trace
would be more readable for the developer, and, in case somebody has
dispay_errors=on, a bit less of information disclosure.


Just my 2c

Thomas