Site Health: Add test for file checksums

[swissspidy]

For a while now there has been ​a WP-CLI command to verify the current installation's integrity by comparing checksums. This makes it easy to detect suspicious modifications to core files and even plugins (if the plugin is hosted on WordPress.org)

I think Site Health would be a good place to possibly add such a checksum verification to WordPress core itself.

If a user is presented with a warning that core files have been modified, they could be offered the opportunity to re-install WordPress (just like you can do under Dashboard -> Updates)

The same goes for plugin checksums.

[desrosj]

Moving Site Health tickets into their lovely new home, the Site Health component.

[swissspidy]

Updated the description to indicate that this can be applied to both plugins and core.

[killerbishop]

I am considering building this health monitor improvement - it seems straightforward enough - download the checksum list for the files for the current version/locale and verify the files on disk are unmodified. If everything is good, report that everything is in working order. I would build this as an async task and it would report any files that are modified from the original version's list as a security issue. A question I have is how best to test this against the master branch? The API end-point does not work with the trunk version. I can backport to the current stable branch to test it - but I'm concerned about how a unit test could be built and run on the trunk branch.

Aside from my implementation question above - I do have a concern with this feature giving site owners a false sense of security. The CLI tool mentioned in the description is something that is downloaded and run against an install independent of the code - this makes that kind of check trustworthy assuming you check the CLI tool first to make sure it is not compromised. The site monitor will be in the code base and if it is compromised I fear it will be the first target of an attacker to modify this site health tool so that it can report everything is OK even though the hacker is changing files - hence my concern of a false sense of security. It's still possible that this tool will be helpful in cases where the attacker did NOT change the site health module code - but is it good enough to call it secure?

[Clorith]

As with anything in core, it can be modified in some way or another if someone wants to, and the Site Health Check isn't meant to be a security tool in that regard, more as a helping hand, so yes, I think it would be acceptable to do this.

It's fine to skip the test if running an alpha/beta/rc build, where checksums can't be verified.

Some caveats that need to be accounted for, as this does already exist in the ​plugin version and a few scenarios have been discovered through this;

• Any change of locale on the site will make the checksum verification fail unless the user has re-installed core files, or a major update has been performed to ensure they've gotten the locales own files.
• There are hosting panels that do strange things, cPanel used to (I can not confirm if they still do this) modify core files to "break" WordPress updater if you used their softaculous suite to install anything, so that any updates had to be validated and pushed by cPanel them selves. We may need to consider that this is not isolated to that one vendor, and that others do similar things to core to control it through their own systems, even if we don't approve of it, as this may cause unnecessary unease for the user.

I did see your question in the #core room on slack as well, so here is some more input about the approach I envision here.

The test output needs to be simple and to the point, avoid any listings of mismatched files which may cause confusion for the user. All that's needed is a notice that Some core files may have been modified, a short description about what this could mean, to try and avoid user panic as that can sound scary.

End it with an action link to the Dashboard > Update page where the user can re-install core files with the click of a button.

Now, for plugins/themes, I'm not sure that's the best thing to include as well, it gives a greater false indicator if we include those, since we can't check anything coming from outside wordpress.org, so it would be an incorrect indicator of those files states (is my thinking at least).

[killerbishop]

@Clorith - thanks for the feedback.

Regarding the vendor issue with cPanel and similar services - do you think it might be worthwhile providing a way for those services to disable reporting it as an error? The check would still happen and it would even state that the mechanism was disabled by the service provider.

I already have it passing locale, but if they change the locale without downloading the update - should it also check if it matches en_US and not trigger in that event?

Plugins are iffy - and as you have mentioned - may give a false sense of security. I can pull that change and provide as a different patch. So that the changes can be discussed and kept as needed.

[killerbishop]

Patch for #47528

[killerbishop]

I've added a patch that provides a checksum test - it does not list the files that are missing/changed per a conversation on the slack channel. It does use locale as part of the check and it is an async process just in case some server environments are really slow or the file checksum download takes awhile. @Clorith - I saw that you opened #47864 - I assuming that is answer to my question about cPanel. Let me know if I need to make some changes - thanks.

[Clorith]

We already have filters like site_status_tests for that, the ticket you mentioned is not related to this.

I noticed you use code_integrity as the action in 47528.diff​, this should probably be core_integrity as that is what is being checked here.

The patch also lacks some translation functions around a few strings, ideally there's a few possible scenarios, so it would be good to knock out some copy-editing on the wording to use here as well before the patch goes in, could you take us through the various states and strings so we can go over them outside the patch?

[killerbishop]

@Clorith - sure - I'll push an update with the missing __() wrapper function for translations - I only see one right now. I also rename all code_integrity to core_integrity.

Scenarios that are covered:

1. Checksum download failure: If the core_get_checksums() function fails to return checksums for any reason - then an error will be produced.

• Status: critical
• Label: Unable to scan core files for changes
• Description: The checksum file list could not be downloaded. There maybe a connection issue or a list is not available for this version. Please try to run this test again at a later time.

2. Checksum mismatch or missing files: If any file is found missing or with a mismatched checksum - then this recommendation status will be produced. The exception is for any items listed in wp-content - that directory is ignored. The action links to the update-core.php?force_check=1 admin URL.

• Status: recommended
• Label: Some core files may have been modified
• Description: Some WordPress core files may have been changed. One reason this check can fail is that you need to install a version that makes use of the right translation files. If you have the ability to do so, a simple fix is to reinstall WordPress. Reinstall of the core system should not affect any plugins, themes, or content that you have posted.
• Action: Reinstall WordPress manually

3. Everything checks out fine: If everything is OK - then the test is considered passed.

• Status: good
• Label: No changes to the core files are detected
• Description: A scan for changes to the core WordPress files was performed. No changes are detected.

The label declared in the async process list is WordPress Core Files Integrity Check.

[killerbishop]

Patch for #47528 - requested changes

[pgogy]

Heya, first comment so apologies if this is all the wrong

Discussion on site-health slack to possibly store the checksums in an JSON object in a transient post update, or after a check, so a sweep is faster. Apologies if already in there

[killerbishop]

@pgogy - I have the checksum download stored in a transient since that should not change. The checksum download is cached for one hour right now - but that could be changed if that is not long enough.

If you are talking about caching the results - I think you would want it to check every time in case some files were changed since the last run.

[pgogy]

Ah sorry - I forgot a bit - was imagining you could have the checksum added to the DB post upgrade (as the upgrade scripts adds it for core) for all the core files

[killerbishop]

@pgogy - I apologize if I'm not entirely getting this correct - but I want to make sure I understand what your asking for first. So are you thinking that a new DB table would get added to store the checksum list for the current version+locale combination and that the get_core_checksums() function would fetch from that table first before querying the API - or would this be specific to the health check?

Right now, it's just calling the get_core_checksums() function. That function is documented as caching but I didn't see anything implemented inside that function's code to cache. I used the transient API in the health check to avoid repeated calls in a short span.

I can see the value in not fetching unchanging checksum lists for future health checks. I wonder if that is better served in the get_core_checksums() implementation rather than at the health check level. The storage would need to be keyed on version and locale. If stored in a database - would this be global for multi-site? Even without the database storage - is it useful having the transient behavior moved inside the function? Any change here though would be a different ticket or is it OK to include that kind of change in a feature like this?

Sorry if some of my questions seem out of place - I can ping you on Slack if that would work better.

[afragen]

We should omit this check if the site is not running a core release. Running on trunk or point release nightlies will give an error.

Presumably the user knows and understands if they are not running a release version.

[oglekler]

Check if the installation has correct hashes of files in it according to WordPress version can be a great improvement.

I replaced one of the core files with file from another one and got a Warning, but Site Health sort of not notice it. Such things are happening where an update process went wrong. With version 5.5 on a support forum were reported issues when some function cannot be declared because it's previously redeclared and right now was a case where a new file in an installation is missing. It happened because some function was moved to another file and this particular installation went not well and fully. A manual update has helped each time.

I think the developer's version can be not checked as fas as the user sees the correct version number and information that he is using a developer version.

[Michi91]

Refreshed Patch for #47528 - including check for development environment and releases

[Michi91]

Uploaded a refreshed patch including check for development environment ( wp_is_development_mode() ) and releases like alphas, betas, nightlys etc. (Checking if Version contains a - suffix)

I also tested it: (6.3.1 with applied patch)

1. by changing the $wp_version in update.php from 6.3.1 to 6.3.1--alpha-56267-src -> omits the check
2. by define( 'WP_DEVELOPMENT_MODE', 'all'); -> omits the check
3. by manipulating the checksum api request to a local webserver and overwriting the checksums -> works as expected

Feedback is heartily welcomed, I'm new in making WordPress :)