| Both sides previous revisionPrevious revisionNext revision | Previous revision |
| rfc:throwable_string_param_max_len [2020/06/27 15:01] – 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.1 | * 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 |
| ===== Introduction ===== | ===== Introduction ===== |
| |
| Since 2003, ''%%Throwable->getTraceAsString()%%'' and ''%%Throwable->__toString()%%'' have limited the length of string function arguments in stringified stack traces to 15 bytes (e.g. ''#0 /path/to/file.php(line) function("012345678901234...")''. This is not enough space to render information such as paths, URLs, UUIDs, etc. if an end user wants to see it when debugging an issue. | Since 2003, ''%%Throwable->getTraceAsString()%%'' and ''%%Throwable->__toString()%%'' have limited the length of string function arguments in stringified stack traces to 15 bytes (including lines such as ''%%#0 /path/to/file.php(line) function("012345678901234...")%%''). This is not enough space to render information such as paths, URLs, UUIDs, etc. if an end user wants to see it when debugging an issue or during local development. |
| |
| While 15 bytes may be a reasonable default for many use cases (e.g. allowing packing more stack frames on a screen or within a byte limit), it would be useful to be able to raise that default. | While 15 bytes may be a reasonable default for many use cases (e.g. allowing packing more stack frames on a screen or within a byte limit), it would be useful to be able to raise that default. |
| |
| - ''echo $throwable;'' | - ''echo $throwable;'' |
| - ''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 ==== |
| |
| * hardcoded default values: 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. |
| * php.ini-development values: 15 | |
| * php.ini-production values: 15 | * hardcoded default value: 15 |
| | * php.ini-development 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. | |
| |
| * 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. | |
| |
| ==== 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''. | Add a new ini setting ''zend.exception_string_param_max_len'' as described in the RFC. (Yes/No, requiring 2/3 majority) |
| | |
| | Voting opened 2020-07-11 and closes 2020-07-25. A 2/3 majority is required. |
| | |
| | <doodle title="Add a new ini setting zend.exception_string_param_max_len" auth="tandre" voteType="single" closed="true"> |
| | * Yes |
| | * No |
| | </doodle> |
| |
| ==== Poll ==== | ==== Poll ==== |
| |
| * Informal poll: Interest in allowing it to be set to 0 | <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> |
| |
| * Informal poll: Interest in raising the default setting value | ===== Changelog ===== |
| | |
| | 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 ===== |
| | ==== 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). |
| | |
| | |
| | <code php> |
| | function unsafeHTMLRenderingExample(string $rawUserInput, string $appSecret) { |
| | echo "<h1>Heading</h1>\n"; |
| | try { |
| | process($rawUserInput); |
| | } catch (Exception $e) { |
| | // The output will include both $rawUserInput and $appSecret. |
| | // Previously, only 15 bytes would be displayed. |
| | echo "This should not happen: $e\n"; |
| | } |
| | } |
| | </code> |
| | |
| | |
| | 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. |
| |
| | 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 |