#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)
Change History (40)
#4
@
13 years ago
- Milestone changed from 3.3 to Future Release
- Priority changed from normal to low
- Type changed from task (blessed) to feature request
#8
follow-up:
↓ 9
@
11 years 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):
- Verify for PHP compiled with OpenSSL support. If true, check for sha1_file or md5_file functions locally. Then perform the
get_core_updates
orfind_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). - 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 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?
#9
in reply to:
↑ 8
;
follow-up:
↓ 10
@
11 years ago
Replying to nofearinc:
- 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.
#10
in reply to:
↑ 9
@
11 years 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.
#11
@
11 years ago
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
#12
@
11 years 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.
#13
@
11 years 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).
#14
@
11 years 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).
#16
@
11 years ago
- 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 ofmd5_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.
#17
follow-up:
↓ 18
@
11 years 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.
#18
in reply to:
↑ 17
@
11 years 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.
#19
follow-up:
↓ 20
@
11 years 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
#20
in reply to:
↑ 19
@
11 years 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.
#22
@
11 years 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?
#23
@
11 years 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:
while( true ) { svn revert -R . && cat 18201.diff | patch -p0 #make changes svn diff > 18201.diff #hit update }
#24
@
11 years ago
- Owner set to dd32
- Resolution set to fixed
- Status changed from new to closed
In 25540:
#26
@
11 years 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]).
Related: #22704