Verify updates with md5 checks

[nacin]

Before an update, we should verify core files with a list of md5s, returned through the API. We should do this in order to determine whether we can safely do a partial build, or if we should just copy over everything.

During the update, we can avoid copying over any files that are up to date, as verified by md5 results beforehand. This should save a number of FTP calls, in particular.

After the update, we should md5 files we've copied over, and see if anything went wrong, and try again if necessary. Unfortunately we don't always know why updates fail, so we're going to need to test the hell out of this and see what kinds of cases we can cover. (Maybe it's time for error reporting back to WP.org?)

Any other thoughts?

[SergeyBiryukov]

Related: #22704

[nofearinc]

The way I see the process here is the following (let me know if it's way out of line or there is a better way to approach it):

1. Verify for PHP compiled with OpenSSL support. If true, check for sha1_file or md5_file functions locally. Then perform the get_core_updates or find_core_update functions to fetch the updates, get the right update. Download the file, check the checksum, extract the content and perform to update the WordPress. If the update fails, check whether the file has already been downloaded (important for large servers with thousands of WordPress installs) and work with it instead (otherwise download again).
2. If PHP isn't compiled with OpenSSL support, then check for SSL support in system curl/wget calls. Perform system download/update with the root CA certificate bundled in WordPress. Then verify and install as described in 1.
3. If SSL is not installed at all, perform basic HTTP download with PHP or wget/curl and update accordingly.

Best practice in different platforms/tools is providing two checksums (both md5 and sha1) from the vendor (in version.php or some other new URL on wordpress.org) as md5 or sha1 might not be available on the server.

Does it sound feasible and would we need to apply core updates in the WP_Http class or other update functions from wp-admin/includes/update.php in addition to the autoupdater itself?

[rmccue]

Replying to nofearinc:

2. If PHP isn't compiled with OpenSSL support, then check for SSL support in system curl/wget calls. Perform system download/update with the root CA certificate bundled in WordPress. Then verify and install as described in 1.

If you mean the cURL extension, then cURL only has SSL support if you also have OpenSSL (that is, cURL depends on OpenSSL for support) from memory. If you mean using command-line curl/wget, calling exec/shell_exec/... is a Bad Idea(tm) and it's better to avoid that.

As far as I can see though, this ticket is more for integrity checking than security, security issues are being handled in #18577.

[nofearinc]

Replying to rmccue:

If you mean the cURL extension, then cURL only has SSL support if you also have OpenSSL

IIRC, PHP could be compiled without SSL support but you could still have OpenSSL on your server. The binary of SSL-supported PHP is large and some hosting vendors compiled it without SSL support while the server has SSL capabilities for various tasks.

[pento]

attachment:18201.diff​

So, here's a first pass.

I decided not to use an API, instead opting to include a (currently imaginary) file that includes the hashes for the file, based on the format of the ​Exploit Scanner plugin. The file would be created as part of the build process.

Format:

array(
	'dir1/file1.foo' => 'hash1',
	'dir2/file2.bar' => 'hash2,
	...
);

TODO:

• Add support for FTP servers that provide MD5 functionality
• Don't copy files that haven't changed
• Check files after they're copied, to make sure it all worked

[pento]

Upon reflection, attachment:18201.diff​ was completely wrong, and totally missed the point.

So, let's try this again with attachment:18201.2.diff​.

This one does the following:

• Checks the state of the current WP install, to decide if it can get away with a partial upgrade
• Checks the current WP install against the upgrade version checksums, to see if we can skip any files

It doesn't do the check after the upgrade (as suggested in the original ticket), as I think we need to consider how we want that to behave. Blindly retrying the copy feels like the wrong solution, and I suspect the answer will be huge and will bog down this ticket, so it'd be good to tackle that separately.

[pento]

Okay, okay. There's no excuse for not doing the rest of this ticket. So, here's attachment:18201.3.diff​, complete with sanity checking after it tries to upgrade core.

I'm still not wild about blindly trying again, but it's better than nothing at all. Apart from the "disk full" check, does anyone have other thoughts for things we should check?

I put in a 3 second sleep in case there's some sort of short, temporary glitch (i.e., a network glitch to a NAS). The length of the wait has no basis in reality, I just like '3' as a number. If anyone likes another number better, I'm open to arguments (whether they be artistic, philosophical or fact based).

[dd32]

• get_home_path() != ABSPATH. get_home_path() will return the parent directory when WordPress is installed in a subdirectory.
• function_exists( 'md5_file' ), can/is md5_file actually disabled by any hosting providers? I can imagine some shitty hosts would.. but.. google doesn't reveal any cases where that's being done.
• sleep(3) ideally shouldn't be needed, a fair amount of IO time should've taken place between the copy and the md5, delaying 3s shouldn't cause issues though, as that code branch ideally shouldn't ever be hit, however with #25237 _copy_dir() may fail to copy a file when using FTP if a file write doesn't succeed, so that's more likely to happen now than previously.

This code is going to be extremely hard to test properly with trunk, since $wp_version will not map directly to a set of hashes (for either the package being downloaded, or, for the installation of WordPress).

[dd32]

#11733 was marked as a duplicate.

[pento]

attachment:18201.4.diff​

• Fixed get_home_path()/ABSPATH
• Removed the function_exists( 'md5_file' ) check. I originally put it in after nofearinc's suggestion that PHP can be compiled without it, but I'm seeing very few reported instances of md5_file() missing, and nothing recently.
• Removed the sleep( 3 ). I agree, it probably wouldn't actually be useful.

Perhaps we could set up the API to also return the correct hashes for nightly builds? We could at least use that as a limited basis for testing.

[dd32]

Perhaps we could set up the API to also return the correct hashes for nightly builds? We could at least use that as a limited basis for testing.

The Auto-Updates API responses are currently returning 3.7-alpha-20130912 as the version number, if we could get the nightly builds script to update the $wp_version field to either a date, or an up-to-date revision, I'm sure we can make it happen. The major problem at present is that 1 version string can refer to any build in the last week or so normally..
The crucial part is that we need to get everything returning the same version strings, so that things will match up.

[pento]

Replying to dd32:

The Auto-Updates API responses are currently returning 3.7-alpha-20130912 as the version number, if we could get the nightly builds script to update the $wp_version field to either a date, or an up-to-date revision, I'm sure we can make it happen. The major problem at present is that 1 version string can refer to any build in the last week or so normally..

I'm 100% in favour of the nightly build script updating $wp_version. I'd use the revision number, as that also matches up to a know state of the codebase.

[dd32]

Removed the function_exists( 'md5_file' ) check. I originally put it in after nofearinc's suggestion that PHP can be compiled without it, but I'm seeing very few reported instances of md5_file() missing, and nothing recently.

Ah, nofearinc has suggested it would be unavailable when OpenSSL was unavailable, which isn't the case. PHP ​includes it's own MD5 implementation which can't be disabled at compile time. The only way to disable it is to include it in disable_functions

[nofearinc]

Replying to dd32:

Ah, nofearinc has suggested it would be unavailable when OpenSSL was unavailable, which isn't the case. PHP ​includes it's own MD5 implementation which can't be disabled at compile time. The only way to disable it is to include it in disable_functions

Thanks for the update. I started my update component with those ideas at first and I had to stop working on it as you and pento seem to be getting along pretty well.

[pento]

attachment:18201.5.diff​​ / attachment:18201.6.diff​​

• Adds the ability to request the checksums for more than one version from the API, so we can combine our HTTP requests
• Renames the API endpoint to /checksums/, now that it does multiple versions at once
• Some extra sanity checking on various lumps of data

Currently, we're planning on restricting the API to returning data for a maximum of two versions, to prevent potential DDoS attacks. Is there any reason we would need to return more?

[dd32]

Attachment attachment:18201.7.diff​​ added

That's attachment:18201.6.diff​​ merged to 3.6 and a bunch of syntax errors, and code fixes..

• _copy_dir() uses a regular expression internally for the skip list, apparently a 40,000-character long string isn't advisable as a regular expression.. The replacement strpos() one I tried (included in patch) also didn't work as it'd find 'wp-includes' within '/wp-includes/some/file' and fail.. The whole skip list functionality is really broken and doesn't work for anything more than a simple unique-named folder (ie. wp-content).
• The about.php redirect keeps kicking in, although, as far as I could tell, I neutered it.. get ready to hit escape after the update process finishes
• The get_core_checksums() function won't exist when upgrading from pre-3.7, so it needs checking it exists before it's called in wp-admin/includes/update-core.php
• See all the TODO and TESTING comments throughout
• includes PHP5.4-only debug included (short-form array notation), as a FYI.
• Uses $from / $to (which is a remote FTP path) as a local path often, many cases updated to ABSPATH, but, some things such as the $working_dir cannot be easily mapped from FTP path to Local path
• Added a few more upgrade messages (maintainence mode / verifying files) so it's easier to see what's happening

That's all I've got right now..
If you want to test this patch on an install, some helpful commands: (Use this in a /tags/3.6 checkout)

while( true ) {
   svn revert -R . && cat 18201.diff | patch -p0
   #make changes
   svn diff > 18201.diff
   #hit update
}

[dd32]

In 25540:

Upgrader: Perform a MD5 file verification check on the files during upgrade. This ensures that both a Partial upgrade build can be used, and that all the files were copied into place correctly.
Props pento for initial patch. Fixes #18201

[dd32]

In 25648:

MD5 file verification: Prevent md5_file() warnings when files don't exist, additionally, don't verify wp-content files as they can be updated separately, as well as WP_CONTENT_DIR being set elsewhere. See #22704 See #18201

[TobiasBg]

We should not use inverted naming in booleans, as that just twists brains too many times.
18201-no_partial.patch inverts $no_partial (introduced in [25540]).

[TobiasBg]

Don't use inverted naming for booleans

[TobiasBg]

Forgot that dd32 might not get notified about the last comment as the ticket was already closed, so reopening to allow him to review it.

[nacin]

In 25674:

Don't use a double negative.

props TobiasBg.
fixes #18201.

[nacin]

In 25801:

Significantly simplify get_core_checksums(), as the caching and chunking was causing too much grief.

Make sure we only do our pre-flight is_writable check when the file exists.

see #18201. see #22704.

[nacin]

[25801] also adds core support for locale checksums.

[nacin]

In 25805:

Avoid numerous potential PHP warnings when dealing with the pre-r25801 get_core_checksums() return value.

Warnings included current(), filestat(), and md5_file().

see #18201. see #22704.

[dd32]

In 25811:

Avoid a few PHP Warnings when files don't exist and use a better method to locate the local filepath.
See #18201