| Both sides previous revisionPrevious revisionNext revision | Previous revision |
| rfc:throwable_string_param_max_len [2020/07/01 20:40] – 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.3 | * 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 |
| - 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 ''throwable_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. | 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. | * 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 0 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. |
| |
| ===== 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. | |
| |
| |
| |
| ==== 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 raising the default setting value | <doodle title="Add a new ini setting zend.exception_string_param_max_len" auth="tandre" voteType="single" closed="true"> |
| | * Yes |
| | * No |
| | </doodle> |
| |
| | ==== 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 throwable_string_param_max_len to a minimum of 0 (previously 15). Change the recommended value in php.ini-production to 0. | |
| | 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 | 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 |