Make WordPress Core

Opened 5 weeks ago

Closed 7 days ago

Last modified 6 days ago

#62815 closed task (blessed) (fixed)

Explicitly require the `hash` extension

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

Description

In #60638 the Gravatar hashes have been switched from sha1 to sha256. In #21022 sha384 will be used for pre-hashing user passwords. The hash extension is required in order for either of these extensions to be available for use by hash() or hash_hmac().

In the discussion on https://github.com/WordPress/wordpress-develop/pull/8097, Dion observed that one single site out of all those tracked on 6.1+ had hash disabled while otherwise being compatible.

As a result, let's add hash to the list of extensions that are required to run WordPress 6.8 so we can ship safer Gravatar hashes and safer password hashing, and correspondingly remove some compatibility code.

Change History (20)

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


5 weeks ago
#1

  • Keywords has-patch has-unit-tests added; needs-patch removed

@johnbillion commented on PR #8138:


5 weeks ago
#2

@dd32 I would like to ask you to double check those stats you pulled about the hash extension. It's great that only a single site on 6.1+ has json but not hash, but it's also a suspiciously low number. Is there any chance it may not be accurate? Better to be safe than sorry. Thanks!

@ayeshrajans commented on PR #8138:


5 weeks ago
#3

PHP 7.4 is the first version to properly bundle the hash extension, so prior to PHP 7.4, it is technically possible to compile PHP without the hash extension.

@peterwilsoncc commented on PR #8138:


5 weeks ago
#4

@johnbillion In it's current form, the error will only show the first extension that is missing. If an install is missing both required extensions, I think it would be helpful to include that in the WP_Error. Does something like this work?

// Add a warning when required PHP extensions are missing.
        $required_php_extensions = array( 'json', 'hash' );
        $missing_php_extensions  = array();
        foreach ( $required_php_extensions as $required_php_extension ) {
                if ( ! extension_loaded( $required_php_extension ) ) {
                        $missing_php_extensions[] = $required_php_extension;
                }
        }

        if ( ! empty( $missing_php_extensions ) ) {
                $php_extension_error = new WP_Error();
                foreach ( $missing_php_extensions as $missing_php_extension ) {
                        $php_extension_error->add(
                                "php_not_compatible_{$missing_php_extension}",
                                sprintf(
                                        /* translators: 1: WordPress version number, 2: The PHP extension name needed. */
                                        __( 'The update cannot be installed because WordPress %1$s requires the %2$s PHP extension.' ),
                                        $wp_version,
                                        strtoupper( $missing_php_extension )
                                )
                        );
                }
                return $php_extension_error;
        }

@johnbillion commented on PR #8138:


5 weeks ago
#5

@Ayesh Yep that's correct. Dion ran the numbers in #8097 and saw almost zero affected sites with hash disabled, hence this proposal. I've asked him to run the numbers again to double check!

@johnbillion commented on PR #8138:


5 weeks ago
#6

@peterwilsoncc Good point, I'll take a look

@johnbillion commented on PR #8138:


5 weeks ago
#7

So we've a catch-22 situation here, the code in update_core() cannot introduce any new logic to prevent an update from occurring because its code won't yet be in place during the update. The update_core() function specifically reads $required_php_version and $required_mysql_version from wp-includes/version.php in the update package for the PHP and MySQL version checks.

For future-proofing we could introduce a $required_php_extensions global in this file, but it wouldn't be able to take effect during any update from a version earlier than 6.8.

@peterwilsoncc commented on PR #8138:


4 weeks ago
#8

@johnbillion It could be done on the API by only presenting the 6.8 update if the hash extension is available. The main consideration there would be how to handle two situations:

  • updating from a version prior to 6.1
  • updates from sites running a plugin to remove passing extension data to wordpress.org

Each of these will remain a problem, even if a two step approach is taken (prep in 6.8, implement in 6.9) so the API approach will be needed in some form regardless.

I'm sure patches to meta are welcome but I was unable to find where to create a PR. in the meta repo. It's possible the API is in the private systems repo.

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


2 weeks ago
#11

This introduces a new workflow which tests the upgrade process from the three latest major versions to the current branch version. The current branch is built and zipped up, and the zip is used by WP-CLI for the update.

This uses a smaller set of matrix values than the "Upgrade Tests" workflow. I think this is sufficient for now, it can always be expanded at a later date if necessary.

Workflow runs can be seen here: https://github.com/WordPress/wordpress-develop/actions/workflows/upgrade-develop-testing.yml

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

@audrasjb commented on PR #8252:


2 weeks ago
#12

Thanks for the ping @johnbillion. While I'm not the best person to checkout this proposal, I just wanted to ask a question about the "a new workflow which tests the upgrade process from the three latest major versions to the current branch" part: if this proposal is accepted, does it mean that we'll have to manually bump the related versions on each major release? This is not a blocker, but definitely something that will need some specific documentation on the major release handbook page :)

@johnbillion commented on PR #8252:


2 weeks ago
#13

@audrasjb Yes that's correct. We already have that in several other workflows, eg the performance tests, old branch tests, and upgrade tests. I'll make a note to get the major release handbook page udpated.

@johnbillion commented on PR #8252:


13 days ago
#14

we already generate a ZIP of the code in the current branch during the build test workflow

Yeah, if you take a look at the first few commits in this branch you'll see that I was initially trying to use that build, but it requires using the workflow_run trigger in order to make use of the build artifact once the workflow completes, and that trigger can't be tested in a PR because it only functions when the workflow with the trigger exists in the default branch. I tried workflow_dispatch but similarly this can't be tested in a PR from a fork. So I just went with another build.

I think this could be accomplished with some conditional checks, but we'd likely need to add a 3rd-party action like changed-files to know when to run each job

I understand your point but I'm not overly keen on adding further third party actions. Is a bunch of conditionals and an extra third party action better than an additional workflow? I don't think there's much between them.

#15 @johnbillion
7 days ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 59803:

Security: Explicitly require the hash PHP extension and add requirement checks during installation and upgrade.

This extension provides the hash() function and support for the SHA-256 algorithm, both of which are required for upcoming security related changes. This extension is almost universally enabled, however it is technically possible to disable it on PHP 7.2 and 7.3, hence the introduction of this requirement and the corresponding requirement checks prior to installing or upgrading WordPress.

Props peterwilsoncc, ayeshrajans, dd32, SergeyBiryukov, johnbillion.

Fixes #60638, #62815, #56017

See #21022

#16 @johnbillion
7 days ago

In 59804:

Security: Delete a test file that was missed in [59803].

Props swissspidy.

See #60638, #62815, #56017

@joemcgill commented on PR #8252:


7 days ago
#17

Thanks, @johnbillion. I think this is a good idea and things are looking good to me. Just to confirm one of my assumptions here, in addition to needing to bump the WP versions in the matrix passed for the reusable upgrade testing workflow, we'll also need to make sure the matrix stays up to date with the supported PHP and DB versions as that support changes over time, correct?

I also notice that the current matrix doesn't include any object cache tests. Is that intentional or do you think there would be some value in including at least one object cache test in the matrix?

@johnbillion commented on PR #8252:


7 days ago
#18

@joemcgill That's correct. We have that case already with the other upgrade tests. It's unfortunate, and perhaps going forward we could populate the matrix from .version-support-php.json and .version-support-mysql.json, but that would be better proposed in a separate PR I think.

Good spot on the object caching. Looking at the Installation Tests workflow there's a matrix entry for memcached but it's not actually used. The Upgrade Tests workflow doesn't include any handling for memcached.

Adding testing with and without memcached would require a change to the way these installation and upgrade workflow files work because they all use PHP directly on the host runner instead of using the containers. I'll open a follow-up ticket.

Note: See TracTickets for help on using tickets.