#60638 closed enhancement (fixed)
Gravatar: Upgrade md5 hashing algorithm to sha256
Reported by: |
|
Owned by: |
|
---|---|---|---|
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
@
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
This ticket was mentioned in Slack in #core by haozi. View the logs.
3 months ago
#8
@
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.
#10
@
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
Trac ticket: https://core.trac.wordpress.org/ticket/60638
#12
@
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?
#13
@
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 )
@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.
#20
@
6 weeks ago
- Resolution fixed deleted
- Status changed from closed to reopened
Re-opening this because the hash
extension can be disabled on PHP 7.2 and 7.3, meaning hash()
isn't guaranteed to be available. Only on PHP 7.4 and higher is it a core extension.
See https://github.com/WordPress/wordpress-develop/pull/7333#issuecomment-2574024332 where I'm waiting to see what Dion says.
The other effect that this has is not being able to use sha256 in the hash_hmac()
compat function in core, and we've got a few places where this is handled:
- https://github.com/WordPress/wordpress-develop/blob/4a9a928dbcd1c91d3633c8de51614dd90d8ea0ac/src/wp-includes/class-wpdb.php#L2409-L2414
- https://github.com/WordPress/wordpress-develop/blob/4a9a928dbcd1c91d3633c8de51614dd90d8ea0ac/src/wp-includes/pluggable.php#L770-L772
- https://github.com/WordPress/wordpress-develop/blob/4a9a928dbcd1c91d3633c8de51614dd90d8ea0ac/src/wp-includes/pluggable.php#L873-L875
- https://github.com/WordPress/wordpress-develop/blob/4a9a928dbcd1c91d3633c8de51614dd90d8ea0ac/src/wp-includes/class-wp-session-tokens.php#L71-L76
We might need to fall back to md5()
when hash()
isn't available.
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.
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 thehash
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:
- Trac ticket: https://core.trac.wordpress.org/ticket/60638
- Trac ticket: https://core.trac.wordpress.org/ticket/21022
- Trac ticket: https://core.trac.wordpress.org/ticket/62815
- Trac ticket: https://core.trac.wordpress.org/ticket/56017
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 thewp 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 isupdate-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.
- 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.
- Reset your environment and try to install WordPress. Ensure missing extensions prevent installation. Multiple missing extensions should all be shown at once.
- Testing an upgrade is difficult because it relies on the incoming update having
$required_php_extensions
present. Testing steps to follow. - 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.
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:
Trac ticket: https://core.trac.wordpress.org/ticket/60638