Make WordPress Core

Opened 2 years ago

Closed 12 days ago

#57304 closed enhancement (fixed)

Add SensitiveParameter attribute to DB connection and login variables

Reported by: tobiasbg's profile TobiasBg Owned by: johnbillion's profile johnbillion
Milestone: 6.8 Priority: normal
Severity: normal Version:
Component: Security Keywords: php82 has-patch
Focuses: Cc:

Description

PHP 8.2 introduces a SensitiveParameter attribute that can "mark a parameter that is sensitive and should have its value redacted if present in a stack trace."

WordPress deals with user passwords, database login credentials, etc. To protect these from appearing in logs or stack traces (which sometimes get copied into bug reports in forum threads and similar), using the #[\SensitiveParameter] attribute for such variables should be explored.

As that attribute starts with a #, which indicates a comment, it's safe to use this with older versions of PHP as well (similar to the #[\AllowDynamicProperties] attribute that is already used in Core).

Change History (21)

#1 @jrf
2 years ago

I fully support this proposal.

Note for the implementation: the attribute can only be added to individual parameters in function declarations, so can not be applied to global or function local variables.

Related RFC: https://wiki.php.net/rfc/redact_parameters_in_back_traces

Side-note: we may need to verify that existing tooling, like the documentation generation tooling and WordPressCS, are able to deal with attributes for function parameters in function declarations. If not, those will need to be updated.

#2 @desrosj
2 years ago

Thanks for this one @TobiasBg! I also support this change. Would you be able to create a first pass PR for this?

This ticket was mentioned in Slack in #core by costdev. View the logs.


2 years ago

#4 @costdev
2 years ago

  • Milestone changed from 6.2 to Future Release

This ticket was discussed during the bug scrub. As a patch hasn't been submitted yet, and 6.2 Beta 1 is only a few days away, I'm moving this ticket to the Future Release milestone.

Additional Props: @audrasjb @petitphp

#5 @petitphp
2 years ago

Started working on a patch for this ticket and I encounter a small quirk.

To make this change compatible with PHP7.x and below we'll need to use a custom formatting and include the attribute on its own line or this will cause a Fatal error since it's interpreted as a comment and discard the end of the line (see the 3v4l.org demo).

Example for the wpdb constructor we could end up with :

public function __construct(
        $dbuser,
        #[\SensitiveParameter] <-- should be on its own line
        $dbpassword,
        $dbname,
        $dbhost
) {

This looks ok to me, but I wanted to have other opinions before putting up a patch.

Last edited 2 years ago by petitphp (previous) (diff)

#6 @johnbillion
2 years ago

That's fine, we do similar for function calls with multi-line parameters. Does this trigger a coding standards violation? If so we can put forward the case to change that.

#7 @petitphp
2 years ago

That's fine

Great! I'll finish the work on the patch. Thanks.

Does this trigger a coding standards violation?

It doesn't on my local machine when running the linter.

This ticket was mentioned in PR #4147 on WordPress/wordpress-develop by @petitphp.


2 years ago
#8

  • Keywords has-patch added; needs-patch removed

Mark credentials with the new SensitiveParameter attribute available in PHP 8.2 to hide them in stack traces or log.

Trac ticket: https://core.trac.wordpress.org/ticket/57304

#9 @petitphp
2 years ago

I've created a PR to implement this change.

It adds the SensitiveParameter attribute to "password" parameters in various place in the codebase.

The PasswordHash class has not been touch as I wasn't sure if this is still considered an "external library" or if it's part of the core.

#10 @petitphp
8 months ago

Could we add this ticket to the 6.7 milestone to move it forward ?

#11 @jrf
8 months ago

  • Milestone changed from Future Release to 6.7

@johnbillion commented on PR #4147:


5 months ago
#12

@jrfnl If you'd like to give this the nod then I'll get it in.

@johnbillion commented on PR #4147:


5 months ago
#13

Ah we still need to verify that WP Parser handles this change ok so the developer.wordpress.org site can still be generated correctly. I'll test it.

#14 @davidbaumwald
5 months ago

  • Milestone changed from 6.7 to 6.8

With 6.7 Beta 1 releasing in a few hours, this is being moved to 6.8 given recent momentum towards a resolution.

If any committer feels the remaining work can be resolved in time for Beta 1 and wishes to assume ownership, feel free to update the milestone accordingly.

#15 @johnbillion
5 months ago

I've so far been unable to get wp-parser running locally to verify that this works. Even running PHP 7.4 and respecting the lock file, the output shows endless syntax errors from the php-parser library. Not sure what's going wrong.

#16 @TobiasBg
5 months ago

@johnbillion: To be sure I understand, this is more of an issue with WP Parser and not directly with #[\SensitiveParameter] and PHP 7.4 compatibility, right? To PHP < 8.2, #[\SensitiveParameter] should just be a comment?!

#17 @johnbillion
5 months ago

Correct, this is to ensure the docs parser handles those inline attributes/comments without tripping over.

This ticket was mentioned in Slack in #docs by johnbillion. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


13 days ago

@johnbillion commented on PR #4147:


12 days ago
#20

Okay I finally got WP-Parser running locally on a PHP 7.4 container, at least enough to get it to parse the source and populate the parsed data. It looks to be working as expected and parsing each of the function parameters. The data displayed on the page comes from the @param tags anyway. This is good to go.

#21 @johnbillion
12 days ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from new to closed

In 59754:

Security: Add the SensitiveParameter attribute to sensitive parameters.

Values passed to parameters with this attribute will be redacted if present in a stack trace when using PHP 8.2 or later. This reduces the chance that passwords and security keys get accidentally exposed in debug logs and bug reports.

Props petitphp, TobiasBg, jrf, johnbillion.

Fixes #57304

Note: See TracTickets for help on using tickets.