WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 7 months ago

Last modified 6 months ago

#18201 closed feature request (fixed)

Verify updates with md5 checks

Reported by: nacin Owned by: dd32
Milestone: 3.7 Priority: low
Severity: normal Version:
Component: Upgrade/Install Keywords: has-patch needs-testing
Focuses: Cc:

Description

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?

Attachments (8)

18201.diff (5.1 KB) - added by pento 8 months ago.
18201.2.diff (4.2 KB) - added by pento 7 months ago.
18201.3.diff (5.2 KB) - added by pento 7 months ago.
18201.4.diff (4.8 KB) - added by pento 7 months ago.
18201.5.diff (6.1 KB) - added by pento 7 months ago.
18201.6.diff (6.1 KB) - added by pento 7 months ago.
18201.7.diff (10.2 KB) - added by dd32 7 months ago.
18201-no_partial.patch (2.4 KB) - added by TobiasBg 7 months ago.
Don't use inverted naming for booleans

Download all attachments as: .zip

Change History (40)

comment:1 sbressler3 years ago

  • Cc sbressler@… added

comment:2 chacha1023 years ago

  • Cc chacha102 added

comment:3 mbijon3 years ago

  • Cc mike@… added

comment:4 nacin3 years ago

  • Milestone changed from 3.3 to Future Release
  • Priority changed from normal to low
  • Type changed from task (blessed) to feature request

comment:5 dempsey2 years ago

  • Cc dempsey@… added

comment:7 nacin8 months ago

  • Milestone changed from Future Release to 3.7

comment:8 follow-up: nofearinc8 months ago

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?

comment:9 in reply to: ↑ 8 ; follow-up: rmccue8 months ago

Replying to nofearinc:

  1. 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.

comment:10 in reply to: ↑ 9 nofearinc8 months ago

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.

pento8 months ago

comment:11 pento8 months ago

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
Last edited 8 months ago by pento (previous) (diff)

pento7 months ago

comment:12 pento7 months ago

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.

pento7 months ago

comment:13 pento7 months ago

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).

comment:14 dd327 months ago

  • 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).

comment:15 dd327 months ago

#11733 was marked as a duplicate.

pento7 months ago

comment:16 pento7 months ago

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.

comment:17 follow-up: dd327 months ago

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.

comment:18 in reply to: ↑ 17 pento7 months ago

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.

comment:19 follow-up: dd327 months ago

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

comment:20 in reply to: ↑ 19 nofearinc7 months ago

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.

comment:21 dd327 months ago

  • Keywords has-patch needs-testing added

pento7 months ago

pento7 months ago

comment:22 pento7 months ago

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?

dd327 months ago

comment:23 dd327 months ago

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
}
Last edited 7 months ago by dd32 (previous) (diff)

comment:24 dd327 months ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

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

comment:25 dd327 months ago

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

comment:26 TobiasBg7 months ago

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]).

TobiasBg7 months ago

Don't use inverted naming for booleans

comment:27 TobiasBg7 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:28 nacin7 months ago

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

In 25674:

Don't use a double negative.

props TobiasBg.
fixes #18201.

comment:29 nacin6 months ago

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.

comment:30 nacin6 months ago

[25801] also adds core support for locale checksums.

comment:31 nacin6 months ago

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.

comment:32 dd326 months ago

In 25811:

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

Note: See TracTickets for help on using tickets.