Both sides previous revisionPrevious revisionNext revision | Previous revision |
rfc:throwable_string_param_max_len [2020/07/01 18:47] – tandre | rfc:throwable_string_param_max_len [2020/07/25 14:00] (current) – tandre |
---|
====== PHP RFC: throwable_string_param_max_len: Configurable string length in getTraceAsString() ====== | ====== PHP RFC: zend.exception_string_param_max_len: Configurable string length in getTraceAsString() ====== |
* Version: 0.2 | * Version: 0.5 |
* Date: 2020-06-27 | * Date: 2020-06-27 |
* Author: Tyson Andre, tandre@php.net | * Author: Tyson Andre, tandre@php.net |
* Status: Draft | * Status: Implemented |
* First Published at: https://wiki.php.net/rfc/throwable_string_param_max_len | * First Published at: https://wiki.php.net/rfc/throwable_string_param_max_len |
* Implementation: https://github.com/php/php-src/pull/5769 | * Implementation: https://github.com/php/php-src/pull/5769 |
- ''%%log('something', $throwable->getTraceAsString());%%'' | - ''%%log('something', $throwable->getTraceAsString());%%'' |
- Uncaught Throwables that crashed an application. | - Uncaught Throwables that crashed an application. |
| |
| Note that PHP 7.4 introduced the setting ''zend.exception_ignore_args'', which allowed removing argument information from exceptions completely (in ''getTrace()'', ''getTraceAsString()'', etc.). Setting ''zend.exception_string_param_max_len=0'' still provides more information than completely disabling tracking args (you still know the argument is a string, and types of non-strings), which is why this RFC also allows decreasing the setting from the previous hardcoded value of 15. |
| |
| * Being able to set the minimum value to ''0'' may have the benefit of avoiding accidentally exposing sensitive information in external dependencies or legacy applications even if enabling ''zend.exception_ignore_args'' didn't make sense for an application. See [[throwable_string_param_max_len#Impact of raising string param length limit]] for examples of those issues. |
| * The name of ''zend.exception_string_param_max_len'' was chosen for its similarity to ''zend.exception_ignore_args''. |
| |
===== Proposal ===== | ===== Proposal ===== |
| |
Add a new ini setting ''throwable_string_param_max_len'' that would allow changing the string byte limit to any value between 15 and 1000000, keeping the current default of 15 bytes. (Changeable by ''PHP_INI_ALL'') | Add a new ini setting ''zend.exception_string_param_max_len'' that would allow changing the string byte limit to any value between 0 and 1000000, keeping the current default of 15 bytes. (Changeable by ''PHP_INI_ALL'') |
| |
A maximum value is enforced to make it harder to accidentally run out of memory or disk space (e.g. if long strings occur multiple times in a stack trace). ''%%Throwable->getTrace()%%'' can be used if the full argument values are needed. | A maximum value is enforced to make it harder to accidentally run out of memory or disk space (e.g. if long strings occur multiple times in a stack trace). ''%%Throwable->getTrace()%%'' can be used if the full argument values are needed. |
==== php.ini Defaults ==== | ==== php.ini Defaults ==== |
| |
To keep backwards compatibility for reasons such as [[throwable_string_param_max_len#Raise the default value|Future scope: Raise the default value]], and to ensure stack traces remain similar in development/production, all proposed values are 15. | To keep backwards compatibility for reasons such as [[throwable_string_param_max_len#Raise the default value|Future scope: Raise the default value]], php.ini-development remains at 15. Note that ''zend.exception_ignore_args'' defaults to On in PHP 7.4+, so anything using php.ini-production would not be logging stack traces regardless of this setting. |
| |
* hardcoded default value: 15 | * hardcoded default value: 15 |
* php.ini-development value: 15 | * php.ini-development value: 15 |
* php.ini-production value: 15 | * php.ini-production value: 0 |
| |
===== Open Issues ===== | ===== Open Issues ===== |
| |
===== Future Scope ===== | ===== Future Scope ===== |
| |
==== Decrease the ini setting's minimum ==== | |
| |
Future RFCs may suggest allowing ''0'' in ''throwable_string_param_max_len''. This RFC keeps a minimum of 15 bytes because application developers/users may prefer to have the same level of detail in bug reports if stringified exceptions are included in bug reports. | |
| |
Being able to set the minimum value to ''0'' may have the benefit of avoiding accidentally exposing sensitive information in external dependencies or legacy applications. See [[throwable_string_param_max_len#Impact of raising string param length limit]] for examples of those issues. | |
| |
| |
==== Raise the default value ==== | ==== Raise the default value ==== |
Application may be unexpectedly relying on the hardcoded limit of 15 to avoid logging sensitive information such as full urls, full paths, or full file contents. | Application may be unexpectedly relying on the hardcoded limit of 15 to avoid logging sensitive information such as full urls, full paths, or full file contents. |
| |
===== Proposed Voting Choices ===== | ===== Vote ===== |
| |
Add a new ini setting ''throwable_string_param_max_len'' as described in the RFC. (Yes/No, requiring 2/3 majority) | Add a new ini setting ''zend.exception_string_param_max_len'' as described in the RFC. (Yes/No, requiring 2/3 majority) |
| |
==== Poll ==== | Voting opened 2020-07-11 and closes 2020-07-25. A 2/3 majority is required. |
| |
* Informal poll: Interest in allowing it to be set to 0 | <doodle title="Add a new ini setting zend.exception_string_param_max_len" auth="tandre" voteType="single" closed="true"> |
| * Yes |
| * No |
| </doodle> |
| |
* Informal poll: Interest in raising the default setting value | ==== Poll ==== |
| |
| <doodle title="Informal poll: Interest in raising the default string parameter max length from 15 bytes in future RFCs" auth="tandre" voteType="single" closed="true"> |
| * Yes |
| * No |
| </doodle> |
| |
===== Changelog ===== | ===== Changelog ===== |
| |
0.2: Add "Impact of raising string param length limit" section. | 0.2: Add "Impact of raising string param length limit" section. |
| |
| 0.3: Allow decreasing ini setting value to a minimum of 0 (previously 15). Change the recommended value in php.ini-production to 0. |
| |
| 0.4: Update external links, formatting. |
| |
| 0.5: Rename from ''throwable_string_param_max_len'' to ''zend.exception_string_param_max_len''. Add reference to RFC thread. Rename "Proposed Voting Choices" section to "Vote" |
| |
===== References ===== | ===== References ===== |
| |
https://externals.io/message/110717 "Making the hardcoded string length limit of Throwable->getTraceAsString() configurable" | https://externals.io/message/110717 "Making the hardcoded string length limit of %%Throwable->getTraceAsString()%% configurable" |
| |
| https://externals.io/message/110744 "[RFC] throwable_string_param_max_len: Configurable string length in getTraceAsString()" |
| |
===== Appendix ===== | ===== Appendix ===== |
==== Impact of raising string param length limit ==== | ==== Impact of raising string param length limit ==== |
| |
For example, code such as the following already had multiple issues such as exposing $appSecret and the potential for XSS from echoing $rawUserInput without html escaping (e.g. ''<script>...</script>''), and should be rewritten to stop doing that. If more than 15 bytes are output, the severity of that bug may be much higher (e.g. if ''$appSecret'' was ''%%-----BEGIN RSA PRIVATE KEY-----...%%'', which was previously truncated to 15 bytes and would omit the private key itself). | For example, code such as the following already had multiple issues such as exposing $appSecret and the potential for XSS from echoing $rawUserInput without html escaping (e.g. ''<script>...</script>''), and should be rewritten to stop doing that. If more than 15 bytes are output, the severity of that bug may be much higher (e.g. if ''$appSecret'' was ''%%"-----BEGIN RSA PRIVATE KEY-----..."%%'', which was previously truncated to 15 bytes and would omit the private key itself). |
| |
| |
| |
| |
Static analyzers may be able to detect potentially unsafe uses of ''getTraceAsString()'' and ''%%__toString()%%'' in the future. Currently, though, I don't believe they check for this specific issue. https://psalm.dev/docs/security_analysis/ and https://gerrit.wikimedia.org/g/mediawiki/tools/phan/SecurityCheckPlugin/#mediawiki-security-check-plugin are projects in that area. | Static analyzers may be able to detect potentially unsafe uses of ''getTraceAsString()'' and ''%%__toString()%%''. Open-source projects in this area include the following: |
| |
| * https://psalm.dev/docs/security_analysis/ (Partial checks added in [[https://github.com/vimeo/psalm/pull/3731|this pr]]) |
| * https://gerrit.wikimedia.org/g/mediawiki/tools/phan/SecurityCheckPlugin/#mediawiki-security-check-plugin (no checks at the time of writing) |
| |
Because the default remains at 15 bytes, this RFC should not make unsafe code like this worse unless the ini setting is changed deliberately. | Because the default remains at 15 bytes, this RFC should not make unsafe code like this worse unless the ini setting is changed deliberately. |
| |
| A related ini setting is ''zend.exception_ignore_args'', which was added in PHP 7.4 to force the omission of arguments from stack traces collected for exceptions, to prohibit the output of sensitive information in stack traces. See http://github.com/php/php-src/commit/0819e6dc9b4788e5d44b64f8e606a56c969a1588 |