Implement PHP 8.2 SensitiveParameter to redact sensitive/password data in backtraces.

[convenient]

Summary

Currently if an exception is thrown while handling a sensitive parameter, part of that parameter is exposed in a stack trace.

We could enable zend.exception_ignore_args to turn off all the arguments from the traces, but that would affect the ability to support the sites and debug issues as we'd have no data.

As of PHP 8.2 there is support to provide annotations to function parameters to ensure they're excluded from back traces for this exact purpose.

It looks like

function test(
    $foo,
    #[\SensitiveParameter] $bar,
    $baz
) {

And would prevent the contents of $bar being exposed in any traces

Examples

For example it looks like this at a basic level

<?php
class Example
{
    public function login($username, $password)
    {
        throw new \Exception('something unexpected happened');
    }
}

(new Example)->login('SomeCoolUsername', 'SomeCoolerPassword12345');

PHP Fatal error:  Uncaught Exception: something unexpected happened in /Users/lukerodgers/src/example.php:6
Stack trace:
#0 /Users/lukerodgers/src/example.php(10): Example->login('SomeCoolUsernam...', 'SomeCoolerPassw...')
#1 {main}
  thrown in /Users/lukerodgers/src/example.php on line 6

See the the log contains SomeCoolerPassw which is not my full example password, but it is a lot.

The extensible nature of Magento means this kind of thing can happen in many core places

For example inside performIdentityCheck if someone had hooked into the observer admin_user_authenticate_after with some flaky logic that could trigger an exception, it could cause this function to fail like in the above demo.

This would result in part of the password being exposed in the logs

magento2/app/code/Magento/User/Model/User.php

Lines 965 to 997 in 1819863

	public function performIdentityCheck($passwordString)
	{
	try {
	$isCheckSuccessful = $this->verifyIdentity($passwordString);
	} catch (\Magento\Framework\Exception\AuthenticationException $e) {
	$isCheckSuccessful = false;
	}
	$this->_eventManager->dispatch(
	'admin_user_authenticate_after',
	[
	'username' => $this->getUserName(),
	'password' => $passwordString,
	'user' => $this,
	'result' => $isCheckSuccessful
	]
	);
	// Check if lock information has been updated in observers
	$clonedUser = clone($this);
	$clonedUser->reload();
	if ($clonedUser->getLockExpires()) {
	throw new \Magento\Framework\Exception\State\UserLockedException(
	__('Your account is temporarily disabled. Please try again later.')
	);
	}
	if (!$isCheckSuccessful) {
	throw new \Magento\Framework\Exception\AuthenticationException(
	__('The password entered for the current user is invalid. Verify the password and try again.')
	);
	}
	return $this;
	}

Proposed solution

When it becomes time to drop PHP 8.1 support add #[\SensitiveParameter] to all password / token fields in the codebase.

• https://wiki.php.net/rfc/redact_parameters_in_back_traces
• php/php-src@9085197

Release note

No response

Triage and priority

• Severity: S0 - Affects critical data or functionality and leaves users without workaround.
• Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
• Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
• Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
• Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.

[m2-assistant]

Hi @convenient. Thank you for your report.
To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:

• @magento give me 2.4-develop instance - upcoming 2.4.x release
• For more details, review the Magento Contributor Assistant documentation.
• Add a comment to assign the issue: @magento I am working on this
• To learn more about issue processing workflow, refer to the Code Contributions.

---

Join Magento Community Engineering Slack and ask your questions in #github channel.
⚠️ According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.
🕙 You can find the schedule on the Magento Community Calendar page.
📞 The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

[convenient]

Here's a rector rule for anyone wanting to add to their codebase https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#addsensitiveparameterattributerector

[m2-assistant]

Hi @engcom-Hotel. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

• 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
• 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
• 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
• 4. Verify that the issue is reproducible on 2.4-develop branch
  Details

  - Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
  - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
  - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
• 5. Add label Issue: Confirmed once verification is complete.
• 6. Make sure that automatic system confirms that report has been added to the backlog.

[engcom-Hotel]

Hello @convenient,

Thanks for the report and collabration!

I agree with your point we should have SensitiveParameter added for the sensitive parameters, but as you know this feature has been introduced in PHP 8.2, and currently Magento supports PHP 8.1 as well.

Hence marking this issue as a feature request for now.

Thanks

[convenient]

@engcom-Hotel While PHP 8.1 does not support this functionality, I do not believe that prevents this moving along. It would simply not take effect until PHP 8.2.

See this example file has #[\SensitiveParameter] $password in the function declaration

// example.php
<?php
class Example
{

    public function login($username, #[\SensitiveParameter] $password)
    {
        throw new \Exception('something unexpected happened');
    }
}

(new Example)->login('SomeCoolUsername', 'SomeCoolerPassword12345');

PHP 8.1

Runs as normal, still shows SomeCoolerPassw...

$ /opt/homebrew/opt/[email protected]/bin/php example.php
PHP Fatal error:  Uncaught Exception: something unexpected happened in /Users/lukerodgers/example.php:7
Stack trace:
#0 /Users/lukerodgers/example.php(11): Example->login('SomeCoolUsernam...', 'SomeCoolerPassw...')
#1 {main}
  thrown in /Users/lukerodgers/example.php on line 7

Fatal error: Uncaught Exception: something unexpected happened in /Users/lukerodgers/example.php:7
Stack trace:
#0 /Users/lukerodgers/example.php(11): Example->login('SomeCoolUsernam...', 'SomeCoolerPassw...')
#1 {main}
  thrown in /Users/lukerodgers/example.php on line 7

PHP 8.2

Runs as normal, shows Object(SensitiveParameterValue)

$ /opt/homebrew/opt/[email protected]/bin/php example.php
PHP Fatal error:  Uncaught Exception: something unexpected happened in /Users/lukerodgers/example.php:7
Stack trace:
#0 /Users/lukerodgers/example.php(11): Example->login('SomeCoolUsernam...', Object(SensitiveParameterValue))
#1 {main}
  thrown in /Users/lukerodgers/example.php on line 7

Fatal error: Uncaught Exception: something unexpected happened in /Users/lukerodgers/example.php:7
Stack trace:
#0 /Users/lukerodgers/example.php(11): Example->login('SomeCoolUsernam...', Object(SensitiveParameterValue))
#1 {main}
  thrown in /Users/lukerodgers/example.php on line 7

[tr33m4n]

This is potentially blocked by laminas/laminas-code#145. Magento's code generation will not currently create attributes in \Magento\Framework\Interception\Code\Generator\Interceptor when intercepting a class, leaving generated parameters in stack traces un-obscured. The underlying Laminas library doesn't support attributes currently, and the open PR to add this seems to have lost traction. We may need to patch/extend the library ourselves or gently encourage laminas/laminas-code#145 to be merged haha