Make WordPress Core

Opened 12 months ago

Closed 4 days ago

Last modified 4 days ago

#60638 closed enhancement (fixed)

Gravatar: Upgrade md5 hashing algorithm to sha256

Reported by: henrywright's profile henry.wright Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.8 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: privacy Cc:

Description

Gravatar now seems to support the sha256 hashing algorithm.

Source https://docs.gravatar.com/general/hash/

Functions such as get_avatar_data() use the old md5 hashing algorithm. That's still supported by Gravatar but considering the md5 string can be reversed, privacy may be impacted in cases where a user's email is obtained from the md5 hash.

Change History (30)

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


12 months ago

#2 @swissspidy
12 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

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


12 months ago
#3

  • Keywords has-patch added; needs-patch removed

Gravatar is changing the hash algorithm from md5 to sha256.

We currently have support for both, but we know that md5's altarithm is weak.

The function output should change as in the example below:
https://1.gravatar.com/avatar/18301bd96ed0ce394ded59bd34de229a?s=96&d=mm&r=g
to:
https://1.gravatar.com/avatar/397b0d12b48f689fad6730036b75e61fac515b62ed32340d0485fc719e073ae3

Final result:

Before:
https://github.com/WordPress/wordpress-develop/assets/252078/ecd0a845-7c06-4a5a-be08-2cf5fa0a8f3a

After:
https://github.com/WordPress/wordpress-develop/assets/252078/bd22a687-a038-483e-82ad-683e6592007c

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

#4 @haozi
9 months ago

Hi, in which version will this feature be released? 6.6?

Last edited 9 months ago by haozi (previous) (diff)

#5 @haozi
9 months ago

  • Version set to trunk

#6 @johnbillion
5 months ago

  • Focuses privacy added
  • Version 6.6 deleted

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


3 months ago

#8 @desrosj
3 months ago

  • Keywords needs-testing added
  • Milestone changed from Future Release to 6.8

Moving to 6.8 to consider.

@jucaduca just to confirm, is the plan to continue supporting md5 in addition to the new, preferred sha256? Sites running old versions of WordPress will continue using md5 after this change, so just wanted to confirm.

@haozi commented on PR #6179:


3 months ago
#9

@worais are you still working on this? If not I can take over.

#10 @dd32
2 months ago

This should be merged in 6.8.

I believe the plan is that md5 will remain supported initially, but sha256 will become highly preferred for cache hit rates, so backporting this could be considered.

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


2 months ago
#11

  • Keywords has-unit-tests added

#12 @haozi
2 months ago

  • Keywords needs-testing removed

Hello, I added a new patch that maintain compatibility with @md5.gravatar.com, can anyone test it and give feedback?

Last edited 2 months ago by haozi (previous) (diff)

#13 @haozi
2 months ago

In my patch the credits page works well (still use md5).

We can modify it if necessary.
(but need to ask the meta team to modify the credits api first https://meta.trac.wordpress.org/ticket/7860 )

https://s2.loli.net/2024/12/17/8iQS9shTaUwL52y.png

Last edited 2 months ago by haozi (previous) (diff)

#14 @SergeyBiryukov
2 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#15 @SergeyBiryukov
2 months ago

In 59532:

Privacy: Use SHA-256 hashing algorithm for Gravatar.

This aims to improve privacy by switching to a more secure algorithm, as an MD5 string can be reversed.

Follow-up to [6748], [31107].

Props henry.wright, jucaduca, haozi, desrosj, dd32, SergeyBiryukov.
See #60638.

#16 @SergeyBiryukov
2 months ago

In 59533:

Coding Standards: Fix WPCS issues in get_avatar_data().

Follow-up to [59532].

See #60638.

@SergeyBiryukov commented on PR #6179:


2 months ago
#17

Thanks for the PR! Merged in r59532.

@SergeyBiryukov commented on PR #8013:


2 months ago
#18

Thanks for the PR! Merged in r59532.

#19 @SergeyBiryukov
2 months ago

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

In 59541:

Privacy: Replace hardcoded MD5 references in wp_credits_section_list().

The Credits API has been updated to return SHA-256 email hashes.

Follow-up to [59532], [meta14307].

Props haozi.
Fixes #62706, #60638.

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


6 weeks ago
#21

@johnbillion commented on PR #8097:


6 weeks ago
#22

Oh that's nasty. Good spot. In practice, do any hosts have hash disabled? I guess we'll only know by committing the change to core and waiting for the host tests to run. There's no way to run the host tests against a PR, correct?

@johnbillion commented on PR #8097:


6 weeks ago
#23

I ask because if there are none then I would personally be happy to retain the hardcoded references to sha256 in the test suite. If we can get the minimum supported version of PHP bumped to 7.4 later this year then the issue goes away then anyway.

@dd32 commented on PR #8097:


6 weeks ago
#24

In practice, do any hosts have hash disabled?

Personally: I don't think so.

The stats included in WP 6.1+ show that a single site in the data-set has json extension but doesn't have the hash extension. That could easily be a rounding error or other data-error.

The extension being disabled did cross my mind when I saw this originally, but thought it was already a required extension, due to it being installed almost everywhere. Core does conditionally check for it in a few places, but PHPMailer doesn't (although it looks to be only in the HTML emails code paths, which is not at all run by many)

There's no way to run the host tests against a PR, correct?

AFAIK, that's correct.

@peterwilsoncc commented on PR #8097:


6 weeks ago
#25

Oh that's nasty. Good spot. In practice, do any hosts have hash disabled?

I'd be very surprised if this is the case. I can't see why anyone would want to do this but I've been surprised before.

I guess we'll only know by committing the change to core and waiting for the host tests to run. There's no way to run the host tests against a PR, correct?

That's correct, there's no way to run them against a PR. I think commit and see is the best we can do.

Reviewing the PHP stats, it appears likely we're a few months from being able to bump the minimum version. The KISS approach would be to revert the change and reapply it once the minimum is bumped as Automattic are retaining MD5 support for the foreseeable future.

Hash lookup tables exist for all one way hashes, so the benefit of switching can wait.

@peterwilsoncc commented on PR #8097:


6 weeks ago
#26

The stats included in WP 6.1+ show that a single site in the data-set has json extension but doesn't have the hash extension. That could easily be a rounding error or other data-error.

The extension being disabled did cross my mind when I saw this originally, but thought it was already a required extension, due to the stats showing it being installed almost everywhere. Core does conditionally check for it in a few places, but PHPMailer doesn't (although it looks to be only in the HTML emails code paths, which is not at all run by many)

Being generous with the rounding error and assuming a few thousand sites don't have the extension available, I think it's fine to require the extension rather than do all this juggling.

@johnbillion What do you think of a make post/trac ticket to discuss this so contributors can figure out if that's a better approach?

@johnbillion commented on PR #8097:


4 weeks ago
#27

Closing in favour of #8138.

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


5 days ago
#28

This marks hash as a required extension and implements checks for required extensions in the same manner as PHP and MySQL version checks during installation, upgrade, and regular runtime. The hash extension is required for the following tickets:

If a site doesn't have hash (or json) installed then this prevents the site from updating to 6.8 from a prior version because update-core.php gets put into place during the update routine. More details at https://meta.trac.wordpress.org/ticket/7900 .

## Effects

If a version of PHP is being used that does not have the hash extension installed:

  • During an update from any prior version, the update will terminate with the error "The update cannot be installed because WordPress x.y.z requires the hash PHP extension", whether the update is performed via the Dashboard -> Updates menu or via the wp core update command in WP-CLI. The site will remain at the prior version and will remain functional because at the point of this error the only file that has been overwritten is update-core.php. If the user installs the missing extension they can proceed with the update.
  • If a user manually updates WordPress by overwriting the core files with the new version, their site will become inaccessible and will show the "WordPress x.y.z requires the hash PHP extension" error in the same way it would for an unmet PHP version requirement.
  • During a fresh installation, the user will be presented with an error message that says "WordPress x.y.z requires the hash PHP extension", and installation won't proceed. This is the same behaviour as it would be for an unmet PHP version requirement.

## Testing steps

This is best tested by manually adjusting the $required_php_extensions array in wp-includes/version.php to include an extension that doesn't exist on your installation. My preferred method is adding extensions named after fruit, but Pete prefers characters from musicals. Do what feels right for you.

  1. Ensure missing extensions are caught during bootstrap and shown as an error in the same way that an unmet PHP or MySQL version does. Multiple missing extensions should all be shown at once. An unmet PHP version dependency should always take precedence over an unmet extension dependency.
  2. Reset your environment and try to install WordPress. Ensure missing extensions prevent installation. Multiple missing extensions should all be shown at once.
  3. Testing an upgrade is difficult because it relies on the incoming update having $required_php_extensions present. Testing steps to follow.
  4. Ensure that a site where all required extensions are installed can be installed, upgraded, and run without issue.

## Testing on GitHub Actions

I've opened a separate PR at #8285 which combines this branch with #8252 in order to test the upgrade routine on GitHub Actions with an additional extension requirement that is not met. The wp core update step successfully fails with the appropriate error, preventing the upgrade from continuing. After the error the site remains operational at the previous version because no core files have been overwritten at that point except update-core.php. This is the same behaviour as an unmet PHP, MySQL, or MariaDB requirement.

#29 @johnbillion
4 days ago

  • Resolution set to fixed
  • Status changed from reopened 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

#30 @johnbillion
4 days ago

In 59804:

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

Props swissspidy.

See #60638, #62815, #56017

Note: See TracTickets for help on using tickets.