Re: [PHP-DEV] [PATCH] Add configuration value to enable/disable stack trace logging

2019-06-17 Thread Erik Lundin

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

> 17 juni 2019 kl. 21:27 skrev 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:
>> > 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


Re: [PHP-DEV] Re: [PATCH] Add configuration value to enable/disable stack tracelogging

2019-06-17 Thread Erik Lundin
Encrypting logs could potentially impact performance alot. My opinion is that 
core dumps and full stack traces should be disabled by default and activated 
only when needed to minimize the risk of data leaks. However, logging is 
needed. You need to get information about what went wrong. 

Maybe this should be enabled in steps?

No stack trace (Default?)
Stack trace with obfuscated argument-data
Full stack trace
/Erik Lundin


> 17 juni 2019 kl. 19:45 skrev Mark Randall :
> 
>> 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
> 
> -- 
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
> 


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

2019-06-17 Thread 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:
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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php



[PHP-DEV] [PATCH] Raw logging to syslog

2019-06-15 Thread Erik Lundin

Background:
In newer versions of PHP the syslog logging is forced to multiline logging 
and it's not possible to turn this off. This is especially problematic 
when you have a lot of logging and need to search and find connected lines 
from a single call to syslog(). For example when debugging multiline 
sql-queries.


Test code:
syslog(LOG_INFO, "test1\t\ttest2\ntest3");

Old behaviour (PHP 5.4.16 (cli) - CentOS 7):
Jun 15 08:55:45 server php: test1#011#011test2#012test3

New behaviour (PHP 7.4 (dev)- CentOS 7):
Jun 15 08:55:45 server php: test1\x09\x09test2
Jun 15 08:55:45 server php: test3

After patch and with configuration value (syslog.filter=raw):
Jun 15 08:55:45 server php: test1#011#011test2#012test3

Suggested change:
This patch includes a new configuration value for syslog.filter (raw) 
which allows to send the logging data in it's raw form to the syslog. The 
code for the other configuration values should be untouched by this 
change.


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

--
Regards, Erik Lundin

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php