WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 4 weeks ago

Last modified 5 days ago

#39309 closed task (blessed) (fixed)

Secure WordPress Against Infrastructure Attacks

Reported by: paragoninitiativeenterprises Owned by: pento
Milestone: 5.2 Priority: normal
Severity: critical Version: 4.8
Component: Upgrade/Install Keywords: has-patch
Focuses: Cc:

Description (last modified by peterwilsoncc)

(Similar to #25052 but much more focused on the implementation details)

Background

Recommended reading:

  1. http://seclists.org/oss-sec/2016/q4/478
  2. https://www.wordfence.com/blog/2016/11/hacking-27-web-via-wordpress-auto-update/
  3. https://paragonie.com/blog/2016/10/guide-automatic-security-updates-for-php-developers

Currently, if an attacker can compromise api.wordpress.org, they can issue a fake WordPress update and gain access to every WordPress install on the Internet that has automatic updating enabled. We're two minutes to midnight here (we were one minute to midnight before the Wordfence team found that vulnerability).

Given WordPress's ubiquity, an attacker with control of 27% of websites on the Internet is a grave threat to the security of the rest of the Internet. I don't know how much infrastructure could withstand that level of DDoS. (Maybe Google?)

The solution is to make the automatic update mechanism secure even if the update server is totally owned up. As published in the third link, the core elements of a totally secure automatic update system are:

  1. Offline Cryptographic Signatures
  2. Reproducible Builds
  3. Decentralized Authenticity / Userbase Consistency Verification
  4. Transport-Layer Security
  5. Mirrors and Other Availability Concerns
  6. Separation of Privileges

However, I'm mostly interested in 1, 2, and 3. I believe 4 is already implemented (if not, this just became a lot scarier).

Proposed Solution

We're going to have to roll this out in phases, rather than all at once.

  1. Offline Cryptographic Signatures
    1. Decide on a digital signature algorithm and/or cryptography library to use.
    2. Generate a keypair for the release managers to use.
    3. Pin the public key in a major release (e.g. 4.8 or 4.9).
    4. Add signature verification to the update process, but for the first release or two, don't enforce it. Just collect data until we're sure it works for everyone.
    5. Enforce digital signatures. Then this is satisfied.
  2. Reproducible Builds.
    1. The update file should be easily reproduced by any end user.
    2. The update file and update served by api.wordpress.org should be easily verifiable.
    3. We wrote Pharaoh for auditing PHP Archives; something similar may be useful for WP updates: https://paragonie.com/project/pharaoh
  3. Decentralized Authenticity / Userbase Consistency Verification
    • See below.
  4. Make plugin/theme updates secure.

Once core updates are secure, the next step is to allow plugin/theme developers to upload their own public keys which can be used to sign their own extensions.

If you want a reference implementation, we already have a working secure automatic update system built into CMS Airship (which is GPL 3):

Decentralized Authenticity

In CMS Airship, we're totally decentralized: Every Airship maintains its own record of every update file or new/revoked public key since its inception. (This is because CMS Airship aims for maximum security.)

For WordPress, I'm recommending a federated model instead, but the concepts are mostly the same:

  1. Notaries (WordPress blogs or other services that opt in to hosting/verifying the updates) will mirror a Merkle tree which contains (with timestamps and signatures):
    • Any new public keys
    • Any public key revocations
    • Cryptographic hashes of any core/extension updates
  2. WordPress blogs will have a pool of notaries they trust explicitly. (This can be provided by your hosting provider, who runs the single source of truth for all their clients, so long as they themselves practice due diligence.)
  3. When an update is received from the server, after checking the signature against the WP core's public key, they will poll at least one trusted Notary (send a challenge nonce, current timestamp, a checksum of the update file, and any other useful identifying metadata e.g. "wp-core version 4.9.2"). The Notary will verify that the update exists and matches the checksum on file, and respond with a signed message containing:
    • The challenge nonce
    • The response timestamp
    • Whether or not the update was valid

This will be useful in the event that the WP.org's signing key is ever compromised by a sophisticated adversary: If they attempt to issue a silent, targeted update to a machine of interest, they cannot do so reliably: To pull off their attack, they have to allow the Merkle tree (that is mirrored by every Notary) to record/broadcast evidence of their attack in order for it to succeed. So while targeted attacks may still be theoretically possible, it will no longer be possible to do them silently.

In addition to a security layer, it's a deterrent against the most sophisticated threats.

Securing Plugins and Themes

This will probably be the last piece tackled. Basically: Offer the same signing capabilities to theme/plugin developers that will already be in the hands of the core team.

This can be done piecemeal (i.e. optional field on WP.org that allows them to upload their public key, generated by some tooling we provide developers). We should incentivize packages that provide their own signature by, for instance, placing them higher in the listings and/or giving them an attractive and desirable UI element that says "we're secure".

If we one day reach near-100% coverage of the WP ecosystem with digital signing, we can discuss making it mandatory.

Implementation Recommendations

Okay, this section is going to be technical so feel free to skip most of this if you're not into cryptography.

TL;DR - We need a libsodium polyfill, which Paragon Initiative Enterprises is willing to write for free if (and only if) the cost of an independent third party audit is covered by the community and/or the community's corporate sponsors.

Digital signatures

PHP, out of the box, only supports RSA signatures (via the OpenSSL extension), but doesn't support RSASSA-PSS+MGF1SHA256. PKCS1v1.5 padding is unacceptable.

It may be tempting to move towards something like ECDSA, but a mix of security concerns (the Sony ECDSA k-value reuse incident, invalid curve attacks against Weierstrass curves) makes us wary even of RFC 6979 (deterministic ECDSA).

We propose a standardized digital signature algorithm based on twisted Edwards curves. Namely, Ed25519 or Ed448 (EdDSA over the RFC 7748 curves).

Merkle Trees

The TrimmedMerkleTree in Halite is probably the best starting point: https://github.com/paragonie/halite/blob/master/src/Structure/TrimmedMerkleTree.php

Halite's Merkle tree implementations are based on the BLAKE2b hash function (a SHA3 finalist with great performance in software based on the ChaCha20 round function).

Checksums

One of the following algorithms should be used where ever a checksum is required:

  • BLAKE2b
  • SHA-512/256
  • SHA-512/224
  • SHA-384

At no point should MD5 or SHA1 be considered. SHA-256 and SHA-512 are vulnerable to length-extension attacks and are not recommended.

Action Plan

First, if this plan is agreeable by WordPress's security team, we'll get to work on a libsodium polyfill that works as far back as PHP 5.2.4 (in the spirit of WordPress's backwards compatibility tradition).

Once that's finished, independently audited by cryptography experts, and released to the public, we'll work on getting the core cryptographically signed. This will require some additional tooling; the release managers will need to run a command to produce a valid signature of the update file before releasing it.

After core updates are signed and signatures are being verified, we'll build the decentralized verification layer.

Then, we can move forward with making everyone's plugins and extensions securely delivered.

---

Edit Jan 3, 2019: As noted in comment #50, adding the sodium_compat library has been split out to a separate ticket, #45806, as it can serve a wider purpose than protecting against infrastructure attacks. -- @peterwilsoncc

Attachments (21)

39309.patch (441.3 KB) - added by paragoninitiativeenterprises 2 years ago.
First round of Ed25519 verification
39309.2.patch (441.4 KB) - added by paragoninitiativeenterprises 2 years ago.
Fix a small mistake in the previous patch. Not security-affecting.
39309-sodium_compat.patch (1.0 MB) - added by paragoninitiativeenterprises 17 months ago.
Add sodium_compat v1.4.0
39309-compat-update.patch (701 bytes) - added by paragoninitiativeenterprises 17 months ago.
39309-upgrader-sign.patch (3.6 KB) - added by paragoninitiativeenterprises 17 months ago.
39309_-_Updated_Patch_3.patch (3.5 KB) - added by paragoninitiativeenterprises 17 months ago.
Replace the most recent patch with this one.
hackerone-wordpress.png (68.4 KB) - added by paragoninitiativeenterprises 16 months ago.
HackerOne report from last year
39309-sodium_compat_v1_6_0.patch (1.1 MB) - added by paragoninitiativeenterprises 15 months ago.
paragonie/sodium_compat v1.6.0
39309.diff (8.5 KB) - added by dd32 2 months ago.
39309.2.diff (8.5 KB) - added by dd32 2 months ago.
39309.diff with minor coding standards whitespace violations fixed
39309.3.diff (8.6 KB) - added by dd32 2 months ago.
39309.2.diff with the addition of "Expire Key 1 in two years time" https://github.com/dd32/wordpress-develop/commit/1f1ff65eb78a6a5c9c6297965c2c7419830f9dfe
39309-single-header.diff (949 bytes) - added by dd32 2 months ago.
39309-extra-debugging.diff (2.3 KB) - added by dd32 7 weeks ago.
39309-tests.diff (13.8 KB) - added by dd32 7 weeks ago.
39309-preemptive-softfail.patch (1.3 KB) - added by paragoninitiativeenterprises 5 weeks ago.
Prevent PHP-FPM 7.2.0-7.2.2 without ext/sodium failures
39309-phpbug.diff (1.4 KB) - added by dd32 5 weeks ago.
Alternative to 39309-preemptive-softfail.patch
39309-phpbug.2.diff (1.3 KB) - added by dd32 5 weeks ago.
39309-signature-urls.diff (1.7 KB) - added by dd32 5 weeks ago.
39309-phpbug.3.diff (1.3 KB) - added by dd32 4 weeks ago.
39309-signature-urls.2.diff (1.7 KB) - added by dd32 4 weeks ago.
refresh of 39309-signature-urls with a function typo corrected
39309.disable-no-warnings-notice.diff (1.5 KB) - added by dd32 4 weeks ago.
Disable the 'No Signatures found' error unless in Debug mode, at 5.2's launch we're unlikely to have plugin/theme/translation signatures and displaying this isn't going to be helpful to end-users

Change History (109)

This ticket was mentioned in Slack in #core-passwords by paragonie. View the logs.


2 years ago

#2 @paragoninitiativeenterprises
2 years ago

https://github.com/paragonie/sodium_compat

We aren't ready even to tag the first alpha release, but the mainline features work.

This ticket was mentioned in Slack in #core by paragonie. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by paragonie. View the logs.


2 years ago

@paragoninitiativeenterprises
2 years ago

First round of Ed25519 verification

#5 @paragoninitiativeenterprises
2 years ago

  • Keywords has-patch added

Example keypairs to go along with this patch.

<?php

$keypair = hex2bin('1887e3dec051b02d61ae2e2e2e2c1ffde69ab8ced3f69b1681dcb59ab36b138f4d6236cc44829b2f96a26d905aec92162077ef5aa7e0a4e2a6d251258dc83bd14d6236cc44829b2f96a26d905aec92162077ef5aa7e0a4e2a6d251258dc83bd1');
$secret_key = hex2in('1887e3dec051b02d61ae2e2e2e2c1ffde69ab8ced3f69b1681dcb59ab36b138f4d6236cc44829b2f96a26d905aec92162077ef5aa7e0a4e2a6d251258dc83bd1');
$public_key = hex2bin('4d6236cc44829b2f96a26d905aec92162077ef5aa7e0a4e2a6d251258dc83bd1');

$publish['Content-Ed25519'] = bin2hex(
    ParagonIE_Sodium_Compat::crypto_sign_detached($fileContents, $secret_key)
);

The signing should take place offline. Then you just need to store the Content-Ed25519 data on the server and serve it with each file.

This ticket was mentioned in Slack in #core by paragonie. View the logs.


2 years ago

@paragoninitiativeenterprises
2 years ago

Fix a small mistake in the previous patch. Not security-affecting.

#7 @rmccue
2 years ago

I haven't reviewed the content of the patch, but in terms of structure: we don't usually bring in the entire library source, just the useful bits. In this case, it looks like we only need sodium_compat/src, not the tests and Composer file. Likewise, the license should be incorporated into the credits page (wp-admin/credits.php) instead.

That's probably more of a thing for a later patch as it seems this is still under development, but something to be aware of for a final patch.

(See also the other external libraries we include: Requests, SimplePie, Text_Diff, random_compat)

#8 @paragoninitiativeenterprises
2 years ago

@rmccue I appreciate the feedback. I was unaware of this nuance, as @dd32 usually handled the random_compat upstream synchronizing.

When I get v0.2.0 released (probably tonight) with crypto_aead_chacha20poly1305_* (for Noise), I'll send an updated patch.

#9 @georgestephanis
2 years ago

It looks like in some of your changes -- notably wp-admin/includes/file.php and wp-admin/includes/class-wp-upgrader.php -- you're using spaces instead of tabs I think? The indentation is looking a bit wonky as I skim the patch.

Minor pedantism, but in the wp-admin/includes/class-wp-upgrader.php changes there's some code style whitespace issues as well. WordPress has a lot of legacy code style issues, and we just fix them as lines get changed -- it helps preserve the blame history. So when you're changing old line 277/new line 283, it should have spaces wrapped around the variable inside the parentheses.

Also, re: whether or not the sodium_compat uses autoloader stuff -- relevant: #36335

If it's possible for the next version of the patch to be split into two files -- one adding the library, the other containing the core file changes -- that may be easier to skim for differing code styles for each, as opposed to having to find the core files in the patch amidst the new library files. Were there just the aforementioned two core files changed?

---

For themes and plugins, I'm assuming that we're most concerned about the authenticity of upgrades. While installations can be important, it strikes me that upgrades -- specifically automatic ones -- are more apt to be the risk area as the user isn't explicitly choosing it.

As such, distributing public keys would likely be simplest if it were part of the theme or plugin file header. At that point it would be a case of adding a UI into the themes/plugins repository to enable the author to get the hash of the generated zip/tarball/whatever from the .org side, verify it independently, and then paste the signature in to the wp.org ui.

(granted, this would also mean that anything with local filesystem write access could change the public key, but if they have local filesystem write access it's game over anyways, so *shrug*)

This can get tricky in several relatively common edge cases, but most notably --

Changing a release after shipping it. A lot of authors do this, even if it's bad practice. Maybe it's tweaking a changelog, maybe it's just fixing some code that is generating fatal errors. If the files in the package change, so would the signature. I'm not sure what the ideal circumstance here would be -- perhaps to have WP.org append a .1 to the end of the svn-specified version number, and serve it as a new version, with the old version still archived (or at least the svn path and revision saved so it could be rebuilt)?

---

Anyway, just my musings on this so far. I'm glad we're all already in agreement that the sodium_compat library would require third-party verification before it is considered for inclusion into WordPress Core. I have no idea if this is already in discussion up the chain or not, but has there been any discussion / recommendation about individuals or organizations with sufficient relevant expertise to actually do the auditing in question?

#10 @georgestephanis
2 years ago

I also worry that we may be conflating too many different aspects brought up here in one ticket. Is there a reason that the "decentralized authenticity" is being pushed here, rather than keeping this focused more on the signing / verifying of packages and moving the decentralized aspect onto another ticket? I'd think they could be undertaken separately, but I could easily be mistaken.

#11 @paragoninitiativeenterprises
2 years ago

The original intent was to split this into several sub-tickets that address one of the items on this list, and close the parent ticket if and only if all of the items on the checklist are resolved.

I can still do that, if that's easier to reason about.

#12 follow-up: @dd32
2 years ago

  • Milestone changed from Awaiting Review to Future Release

I don't have time to delve deeply into this right now, however, a few very rough notes:

  • Due to our old PHP support, sometimes ext/hash isn't available. #29518 We may be able to require the extension, or simply skip signature verification for those cases. I don't think shipping a PHP-based SHA512 implementation is worth anyones time here.
  • Once again, ext/spl can be disabled under PHP 5.2. So no form of autoloading is possible on those systems.
  • Based on the above, supporting back as far as possible is good, but IMHO PHP 5.3+ is acceptable. PHP 5.2 support here isn't critical. PHP 5.3 may still be able to disable ext/hash though, but once again, I'm fine not supporting that.

Testing against https://wordpress.org/wordpress-latest.zip with signature verification based on 39309.2.patch & comment 5 above

  • Under PHP 7.0 local VM performance was ~240ms per sign, ~255ms per verify.
  • Under PHP 5.3.3 local VM performance was ~380ms per sign, ~330ms per verify.
    • PHP 5.3.3 with CPU throttled to 20%, performance suffered obviously with ~830ms per sign, ~950ms per verify.
  • edit: Note: I didn't look at memory usage closely. We already have issues with memory limits during unzip; It looks like verification uses an extra ~2M of memory for an 8M file.

Not super-fast, but not out of the question. I'm not sure why signing was slower than verifying in the initial PHP 5.3.3 run, I've run it over 50 iterations twice on each setup with similar results.

  • It should be expected that signature verification may take up to a second in the wild, hosts are likely going to be under increased load during updates and that will affect signature time. Not all WordPress sites are hosted with adequate hardware resources.
  • It should also be expected that core would have a minimum of 2 valid signing keys authorised, to allow for secure revocation and replacement.
    • For example; primary and secondary backup, OR one primary for Core, one primary for plugins/themes/translations/etc, single secondary to authorise key replacements, OR something else that made sense.
    • If multiple "primary" keys are possible, a way to differentiate which keys are options for which packages would be needed.
    • Secondary keys would add a overhead for failed verifications, but failed signature verification appears to be rather fast, as it aborts much earlier.
    • This does not affect initial implementation of patches, nor is an immediate blocker to be solved, it would require discussion on the w.org systems side to determine what route we would take here.

A few more random thoughts and notes:

  • All signatures should be transparent to a user, a typical user should never be prompted to enter hashes, nor see them.
  • Individual w.org authors having their own keys/etc, should be kept out of scope for now and focused on later, don't complicate matters further than needed for now, focus on the W.org => W link.
  • Focusing on Ed25519 seems fine to me, however, I've reached out to a few others to ask if anyone has other preferences we should look at here.
  • 39309.2.patch doesn't actually use the libsodium extension, correct? due to it's reliance upon ParagonIE_Sodium_Compat::*()
  • @paragoninitiativeenterprises I assume it wouldn't be possible for us to only ship the the Ed25519 crypto (&dependancies) with WordPress, we'd have to pull the full libsodium compat in, correct?

Finally; Marking this as Future Release as it's accepted we'll add something here, but no firm timeline for completion at this point in time.

Last edited 2 years ago by dd32 (previous) (diff)

#13 in reply to: ↑ 12 @rmccue
2 years ago

Replying to dd32:

  • Once again, ext/spl can be disabled under PHP 5.2. So no form of autoloading is possible on those systems.

SPL can be disabled, but autoloading itself cannot be (PHP's core engine contains support for __autoload). #36926 (in 4.7) introduced an spl_autoload_register shim, so it can be used here fine.

#14 follow-ups: @paragoninitiativeenterprises
2 years ago

I don't think shipping a PHP-based SHA512 implementation is worth anyones time here.

Given that hash() is supposed to be in PHP 5.1.2 and newer, anyone using something as old as 5.2.4 should still have it: http://php.net/manual/en/function.hash.php

Anyone who claims not to is either lying about their actual PHP version or running a dangerous configuration, probably by having compiled it themselves once upon a time.

Either way, I'd agree with not supporting systems without ext/hash.

All signatures should be transparent to a user, a typical user should never be prompted to enter hashes, nor see them.

Indeed, the only person who will ever need to do anything is the release managers.

Individual w.org authors having their own keys/etc, should be kept out of scope for now and focused on later, don't complicate matters further than needed for now, focus on the W.org => W link.

Yep. That's why the patch has a hard-coded Ed25519 public key (the exact keypair given is in my comment above).

It should also be expected that core would have a minimum of 2 valid signing keys authorised, to allow for secure revocation and replacement.

This is similar to Airship's requirements, where everyone has at least two keys: One master key, and one signing key. The master key can revoke or mint new (master, signing) keys. The signing keys are the ones actually used for package signing. (This is true for Paragon as it is for anyone who builds an Airship extension.)

This is a more robust model should the signing key ever be compromised.

Focusing on Ed25519 seems fine to me, however, I've reached out to a few others to ask if anyone has other preferences we should look at here.

Please do.

39309.2.patch​ doesn't actually use the libsodium extension, correct? due to it's reliance upon ParagonIE_Sodium_Compat::*()

It uses sodium_compat. If ext/libsodium is installed, sodium_compat will just kick off to the extension. If it's not installed, then sodium_compat uses the pure-PHP implemenation. This is a reasonable trade-off:

  • If you can install PHP extensions from PECL, you get greater performance and less risk of side-channels introduced by the PHP interpreter
  • If you cannot install PHP extensions from PECL, you can still verify signatures etc.

@paragoninitiativeenterprises I assume it wouldn't be possible for us to only ship the the Ed25519 crypto (&dependancies) with WordPress, we'd have to pull the full libsodium compat in, correct?

Strictly speaking, it is possible to isolate Ed25519 and its dependencies.

However, that's a hack job that I don't think anyone is going to want to perform every time we release an update. Given the delicate nature of security code, I wouldn't recommend doing so. Additionally, if someone wanted to use sodium_compat in a WordPress project (e.g. an extension that communicates via the Noise protocol framework, using our ChaCha20Poly1305 implementation) and you only pulled in part of it, they'd probably have a bad time.

Last edited 2 years ago by paragoninitiativeenterprises (previous) (diff)

#15 in reply to: ↑ 14 @rmccue
2 years ago

Replying to paragoninitiativeenterprises:

Given that hash() is supposed to be in PHP 5.1.2 and newer, anyone using something as old as 5.2.4 should still have it: http://php.net/manual/en/function.hash.php

Note that it can be disabled, which is the bigger concern: http://php.net/manual/en/hash.installation.php

That said...

Either way, I'd agree with not supporting systems without ext/hash.

...I also agree. --disable-hash seems like enough of an edge case that we need not bother thinking about it.

#16 in reply to: ↑ 14 @dd32
2 years ago

Replying to paragoninitiativeenterprises:

I don't think shipping a PHP-based SHA512 implementation is worth anyones time here.

Given that hash() is supposed to be in PHP 5.1.2 and newer, anyone using something as old as 5.2.4 should still have it: http://php.net/manual/en/function.hash.php

Put bluntly, --disable-all is standard in many linux distro's (annoyingly) and disables this sort of stuff if you don't also install the extra packages (and not all sysadmins do), later versions of PHP disable the ability to separate some of the core extensions such as ext/hash and ext/spl.

It should also be expected that core would have a minimum of 2 valid signing keys authorised, to allow for secure revocation and replacement.

This is similar to Airship's requirements, where everyone has at least two keys: One master key, and one signing key. The master key can revoke or mint new (master, signing) keys. The signing keys are the ones actually used for package signing. (This is true for Paragon as it is for anyone who builds an Airship extension.)

Makes sense, thanks for confirming that to be a good method.

39309.2.patch​ doesn't actually use the libsodium extension, correct? due to it's reliance upon ParagonIE_Sodium_Compat::*()

It uses sodium_compat. If ext/libsodium is installed, sodium_compat will just kick off to the extension. If it's not installed, then sodium_compat uses the pure-PHP implemenation. This is a reasonable trade-off:

Right, gotcha, it's not a polyfill as such, rather a pass-through library, that makes sense. I hadn't reviewed the code other than the public interface.

#17 @paragoninitiativeenterprises
2 years ago

  • Resolution set to maybelater
  • Status changed from new to closed

Word from @matt is that this is not a priority for 2017. I'm marking as maybelater; it can be picked up again when priorities change.

#18 @netweb
2 years ago

  • Milestone Future Release deleted

This ticket was mentioned in Slack in #core by dd32. View the logs.


2 years ago

#20 @quackquacker
2 years ago

This is higly problematic. It highlights what most of us fear. That security is down prioritized and making more features and keeping BC upvoted.

Security should and must comes first. Wordpress is a large entity and should own up to the responsibility of having such large of an attack surface.

Last edited 2 years ago by quackquacker (previous) (diff)

@paragoninitiativeenterprises
17 months ago

Add sodium_compat v1.4.0

#21 @paragoninitiativeenterprises
17 months ago

  • Resolution maybelater deleted
  • Status changed from closed to reopened

Now that ext/sodium support has landed in PHP 7.2, and RFC 8032 (governing Ed25519) has been standardized by the IETF, I'm resuscitating this ticket. I'm including two patches.

The first and second patch adds sodium_compat 1.4.0 to WordPress.

The third patch updates the auto-updater to verify Ed25519 signatures for two example Ed25519 public keys. (Important: Replace these public keys with one owned by the WordPress core after applying the second patch! Unless you want Paragon to be able to sign updates for you, which might not be a bad idea, but I suspect your users won't be happy about us having that power.)

This is step 1 in making all WordPress updates secure.

#22 @paragoninitiativeenterprises
17 months ago

A couple of notes about the implementation of the latest batch of patches:

  • This imports sodium_compat v1.4.0, which includes 32-bit support (i.e. Windows + PHP5).
  • We're using ParagonIE_Sodium_File which allows the verification of large files on systems with very low memory (megabytes not gigabytes). In particular, it tries to only buffer 8 KB of data at a given time.
  • This allows an arbitrarily long list of public keys to be tested (the order of the elements in WP_Upgrader::getPublicKeys() matters; new keys should be prepended not appended as time goes on).

This patch set does not include Userbase Consistency Verification. If Chronicle (https://github.com/paragonie/chronicle) is deemed an acceptable solution for this requirement, sodium_compat is a pre-requisite.

This patch set does not include update reproducibility, which will require a separate project entirely.

This patch set does not include theme/plugin signing, but is a step in the right direction.

This ticket was mentioned in Slack in #core by paragonie. View the logs.


17 months ago

@paragoninitiativeenterprises
17 months ago

Replace the most recent patch with this one.

#24 @paragoninitiativeenterprises
17 months ago

The patch I just updated should replace the third patch from my previous comment.

That is to say, these are the 3 patches that should be considered:

  1. 39309-sodium_compat.patch (1.0 MB)
  2. 39309-compat-update.patch​ (701 bytes)
  3. 39309_-_Updated_Patch_3.patch​ (3.5 KB)

#25 @paragoninitiativeenterprises
16 months ago

Note: sodium_compat v1.5.5 is out today, which mostly offers a huge performance boost over the version included in the patch provided above. If this discussion ever moves forward, I'll replace the "import sodium_compat" patch with the latest version.

Last edited 16 months ago by paragoninitiativeenterprises (previous) (diff)

This ticket was mentioned in Slack in #core by paragonie. View the logs.


16 months ago

#28 @paragoninitiativeenterprises
16 months ago

  • Severity changed from normal to critical

It's been over a year. Nobody from the WordPress security team has expressed any indication that this will ever be fixed.

It's been total radio silence outside of this ticket (which is mostly me reminding folks that security is kind of important). There has been no backchannel discussion. There has been no steps forward.

Silence and stagnation.

I'm upgrading the severity to critical in a last-ditch effort to get eyeballs on this ticket. It's probably an exercise in futility, because as demonstrated by the timescale even when the patches have been provided (and I can't exactly review/critique my own patches for consideration of inclusion in the WordPress core, one of the core developers has to do that!), this isn't a priority for WordPress.

Please show me something more than lip service, for a change.

This ticket was mentioned in Slack in #core by paragonie. View the logs.


16 months ago

#30 @williampatton
16 months ago

@paragoninitiativeenterprises Try to reach out at https://hackerone.com/wordpress to get in touch with someone on the security team if you feel like your ticket here isn't getting the attention it needs.

#31 @paragoninitiativeenterprises
16 months ago

@williampatton HackerOne report 228854 opened on 2017-05-16. Closed as N/A by @aaroncampbell 5 days later.

@paragoninitiativeenterprises
16 months ago

HackerOne report from last year

This ticket was mentioned in Slack in #core by benlk. View the logs.


16 months ago

#33 @johnbillion
16 months ago

  • Keywords reporter-feedback added

What sort of peer review has the sodium_compat library had? In the repo description it says:

This cryptography library has not been formally audited by an independent third party that specializes in cryptography or cryptanalysis.

Is this still the case? You mentioned in Slack that Joomla now uses this library. Has it therefore been audited? Are you aware of any other projects using the library?

Thanks.

#34 follow-up: @paragoninitiativeenterprises
16 months ago

  • Keywords reporter-feedback removed

This cryptography library has not been formally audited by an independent third party that specializes in cryptography or cryptanalysis.

Is this still the case?

Yes, that is still the case.

I have not suddenly had enough of a financial windfall to be able to pay NCC Group, Kudelski Security, Least Authority, or another trusted firm $2,000-$4,000 per day for a N-week engagement (where N >= 2) to audit sodium_compat.

I started discussions with Mozilla about covering such an audit last year. It never went anywhere.

Outside of Joomla, sodium_compat has been covered by the security experts that contribute to php[architect] magazine:

https://www.phparch.com/wp-content/uploads/2017/12/php-72-sodium-december-2017.pdf
https://www.phparch.com/wp-content/uploads/2018/02/2018-feb-educ-station-phparch.pdf

You mentioned in Slack that Joomla now uses this library. Has it therefore been audited?

An audit is a formal (often paid) third-party application security assessment. As of 2018-02-06, this has not happened.

Joomla's security team lead, Michael Babker, reviewed sodium_compat and was confident enough in its security to add it to Joomla.

What sort of peer review has the sodium_compat library had?

Aside from Michael Babker, a lot of security/cryptography experts have looked at it on some capacity.

However, none of them have given public statements of endorsement. I'll ask some of them to comment on whether or not they would recommend it.

#35 in reply to: ↑ 34 @ericmann
16 months ago

Replying to paragoninitiativeenterprises:

I have not suddenly had enough of a financial windfall to be able to pay NCC Group, Kudelski Security, Least Authority, or another trusted firm $2,000-$4,000 per day for a N-week engagement (where N >= 2) to audit sodium_compat.

I started discussions with Mozilla about covering such an audit last year. It never went anywhere.

I would absolutely love if an organization with the necessary financial resources would contribute to such an audit. Sodium is now in PHP as a core extension and is fast becoming the standard used for secure crypto in our community. It's fast, secure, and well-supported in a variety of languages. Even projects like GPG (https://www.gniibe.org/memo/software/gpg/keygen-25519.html) are moving to the crypto primitives exposed by Sodium.

Even without a formal audit, this is a well-established, well-known library. It's baked into Joomla, CodeIgniter, and many other projects - just take a look at Packagist! Some modern projects will just push devs towards using the native PHP 7.2 support for Sodium or the Pecl extension for PHP7+ ... WordPress can't do either of those because of our support for even older versions of PHP. sodium_compat literally exists to allow devs who can't use 7.2 or the Pecl package to still use secure crypto.

What sort of peer review has the sodium_compat library had?

Aside from Michael Babker, a lot of security/cryptography experts have looked at it on some capacity.

However, none of them have given public statements of endorsement. I'll ask some of them to comment on whether or not they would recommend it.

I've written extensively about both Sodium itself and the sodium_compat module as an efficient polyfill for developers who can't use the modern extensions available in PHP >= 7.0. By "extensively" I mean several references in publications like php[architect] and even an book on secure PHP application development.

I work on cryptographically-secure tools for a living. I write PHP code for a living. I wouldn't recommend sodium_compat unless I was confident in it. My job literally depends on the quality of this library. I've reviewed several Sodium compatibility libraries while building out our team's products (in multiple languages, including Go, Java, and Ruby). sodium_compat is head and shoulders about the rest in terms not just of quality but also coverage of the Sodium library itself. Many others merely implement a handful of functions for a specific project; sodium_compat provides _full_ support for all of Sodium's functionality, meaning developers aren't limited to just one part of the library.

Whenever PHP and WordPress developers ask me about crypto, the first thing I tell them to do is upgrade to PHP 7.2 so they can use Sodium. Even then I encourage the use of sodium_compat merely so their code is more portable - it will use the native extension if available, fall back to the Pecl module if needed, then leverage a PHP-based implementation as a last resort.

Has the library undergone peer review? Yes.
Is it something other devs in the crypto world recommend? Yes.
Is this something we should have in WordPress so WP devs can be using quality, industry-standard best practices when it comes to crypto? YES!

#36 @pcarvalho
16 months ago

its just me thinking its crazy wp isn't coming forward to sponsor the audit themselves?

does all the libs that gets included have this requirement? like any js lib that got included so far?

#37 @aaroncampbell
16 months ago

First of all, thank you @ericmann for your input here. It's super helpful.

Replying to pcarvalho:

its just me thinking its crazy wp isn't coming forward to sponsor the audit themselves?

The cost isn't a small ask, but it's not just the audit that is holding things up. More on this below.

does all the libs that gets included have this requirement? like any js lib that got included so far?

Not all libs are required to have a heavy security audit before being used (although we audit them internally), but those libs also wouldn't be a bedrock piece of our security strategy.

Almost a year ago, Matt wrote WordPress and Update Signing on Medium. I think it still represents where we're at pretty accurately. That's not to say that no progress has been made in a year. Overall, WordPress has made a lot of progress in the last year – including on the security front and even on the infrastructure front. Just not on this specific issue. It’s on the list, but it’s far enough down that in a year we didn’t make it to it.

The library itself seems to be in a much better place now than it was a year ago. It's seeing some use, it has some peer review (thank you @ericmann for your input here, it's super helpful), and it's had numerous improvements to performance, etc. Yes, I would still like to see it get an audit, but it's not like that's the only hurdle. As Matt said in that article, there is a significant amount of work required on the systems side and it needs to be prioritized in with all the other projects that also need to be done.

I hope that helps.

#38 @mbabker
15 months ago

👋

As @paragoninitiativeenterprises pointed out, we in Joomla did review the sodium_compat library to integrate it into our cryptography toolchain (and to a lesser degree the sodium API's password hashing capabilities as it relates to future Argon2i support), and even unaudited we are comfortable including and distributing the library in Joomla in part given Scott's expertise in these matters and IMO an inherent trust he and his work have built over the last few years.

Regarding the lack of an audit or the funding to do so, unfortunately last budget cycle I could get get a line item approved to help raise the funding needed coming from Open Source Matters, Inc. (Joomla's not-for-profit), but those of us involved in steering the project's development are willing to contribute to making this happen and if there is a real effort to raise funds please ping me and I will do what I need to in order to allocate funding on behalf of Joomla and Open Source Matters.

@paragoninitiativeenterprises
15 months ago

paragonie/sodium_compat v1.6.0

#39 @SergeyBiryukov
15 months ago

  • Milestone set to Future Release

#40 @paragoninitiativeenterprises
15 months ago

In case the powers that be are waiting for an HSM to support Ed25519, apparently Yubico's newest HSM does.

https://www.yubico.com/products/yubihsm/

#41 @paragoninitiativeenterprises
8 months ago

We released sodium_compat v1.6.6 tonight, which introduced a significant speedup on 32-bit platforms (i.e. PHP_INT_SIZE === 4).

I believe it is now reasonably practical to consider Ed25519 signature verification on future WordPress updates.

Questions:

  1. What are the specific infrastructure blockers on the WordPress.org side that need to be resolved before my patches can be considered for merging?
    • To be preemptive, demanding HSM support is making perfect the enemy of good. But even if that demand is still on the table, YubiHSM supports Ed25519 so this problem is solved. Surely the companies that depend on WordPress can swing for one?
  2. What additional code changes need to be made to accommodate this patch?
    • e.g. making sure update servers push the Ed25519 header alongside the update file
  3. What are the social (and/or office-political) objections to this change?

In the event of a security concern, I'd like to remind everyone that libsodium is in the PHP standard library as of 7.2, so any concerns over the security of Ed25519 need to be raised in a lot of places (and probably published on the IACR if you've discovered a new attack).


As far as code audits go, I'm not sure what to suggest.

The quotes I received ranged from $2k to $4k per person-day, for a minimum of two weeks. One firm issued a flat rate quote in the $20-30k range. This isn't the sort of thing you can solve with Fiverr.

But more pertinently:

  • None of the firms who specialize in cryptography (save ours) also specializes in PHP.
  • None of the software security firms specializing in PHP that we're aware of have cryptography experts on staff.

A first-party cryptography audit is useless; only through third party verification can we hope to gain confidence that a specific implementation is secure.

This presents a unique challenge for us: Who else has the prerequisite experience and specialized domain knowledge to check our work? Furthermore, an audit of sodium_compat would also require auditing the various PHP versions that WordPress supports. To wit:

  • PHP 5.2.4
  • PHP 5.2.5
  • PHP 5.2.6
  • PHP 5.2.7
  • PHP 5.2.8
  • PHP 5.2.9
  • PHP 5.2.10
  • PHP 5.2.11
  • PHP 5.2.12
  • PHP 5.2.13
  • PHP 5.2.14
  • PHP 5.2.15
  • PHP 5.2.16
  • PHP 5.3.0
  • PHP 5.3.1
  • PHP 5.3.2
  • PHP 5.3.3
  • PHP 5.3.4
  • PHP 5.3.5
  • PHP 5.3.6
  • PHP 5.3.7
  • PHP 5.3.8
  • PHP 5.3.9
  • PHP 5.3.10
  • PHP 5.3.11
  • PHP 5.3.12
  • PHP 5.3.13
  • PHP 5.3.14
  • PHP 5.3.15
  • PHP 5.3.16
  • PHP 5.3.17
  • PHP 5.3.18
  • PHP 5.3.19
  • PHP 5.3.20
  • PHP 5.3.21
  • PHP 5.3.22
  • PHP 5.3.23
  • PHP 5.3.24
  • PHP 5.3.25
  • PHP 5.3.26
  • PHP 5.3.27
  • PHP 5.3.28
  • PHP 5.3.29
  • PHP 5.4.0
  • PHP 5.4.1
  • PHP 5.4.2
  • PHP 5.4.3
  • PHP 5.4.4
  • PHP 5.4.5
  • PHP 5.4.6
  • PHP 5.4.7
  • PHP 5.4.8
  • PHP 5.4.9
  • PHP 5.4.10
  • PHP 5.4.11
  • PHP 5.4.12
  • PHP 5.4.13
  • PHP 5.4.14
  • PHP 5.4.15
  • PHP 5.4.16
  • PHP 5.4.17
  • PHP 5.4.18
  • PHP 5.4.19
  • PHP 5.4.20
  • PHP 5.4.21
  • PHP 5.4.22
  • PHP 5.4.23
  • PHP 5.4.24
  • PHP 5.4.25
  • PHP 5.4.26
  • PHP 5.4.27
  • PHP 5.4.28
  • PHP 5.4.29
  • PHP 5.4.30
  • PHP 5.4.31
  • PHP 5.5.0
  • PHP 5.5.1
  • PHP 5.5.2
  • PHP 5.5.3
  • PHP 5.5.4
  • PHP 5.5.5
  • PHP 5.5.6
  • PHP 5.5.7
  • PHP 5.5.8
  • PHP 5.5.9
  • PHP 5.5.10
  • PHP 5.5.11
  • PHP 5.5.12
  • PHP 5.5.13
  • PHP 5.5.14
  • PHP 5.5.15
  • PHP 5.5.16
  • PHP 5.5.17
  • PHP 5.5.18
  • PHP 5.5.19
  • PHP 5.5.20
  • PHP 5.5.21
  • PHP 5.5.22
  • PHP 5.5.23
  • PHP 5.5.24
  • PHP 5.5.25
  • PHP 5.5.26
  • PHP 5.5.27
  • PHP 5.5.28
  • PHP 5.5.29
  • PHP 5.5.30
  • PHP 5.5.31
  • PHP 5.5.32
  • PHP 5.5.33
  • PHP 5.5.34
  • PHP 5.5.35
  • PHP 5.5.36
  • PHP 5.5.37
  • PHP 5.5.38

... and those are just the versions of PHP that are no longer supported by the PHP team!

So if "we need an audit" is the big blocker issue, as Matt Mullenweg as expressed in the past, then it bears emphasizing the level of effort being demanded here.

An auditor would be required to:

  1. Verify that there are no security bugs in sodium_compat itself. This is the easy part.
  2. Verify that there are no security-affecting side-channel attacks made possible by the PHP interpreter (and/or OpCache).
  3. Verify that any null findings in step 2 hold for every PHP version I listed above, and also for all the versions that are still supported by the developers of PHP.

A team of three security experts (consisting of at least one cryptographer) would likely burn months of their lives on making sure this is done right. And that's just for one audit; a single data point.

Speaking of points, I hope mine is clear: Please tell me what the specific obstacles are to getting this merged, and I will do what I can to solve them.

#42 @paragoninitiativeenterprises
8 months ago

sodium_compat 1.7.0 was subsequently released with further performance gains and some dead code cleanup that changed an internal-only API.

Can we get this ticket onto a newer milestone?

This ticket was mentioned in Slack in #core by paragonie. View the logs.


8 months ago

#44 @FPCSJames
8 months ago

I'm just going to chime in here to say that I find it absurd that the core team isn't giving this the attention that it deserves. With the massive popularity of WP.org, a compromise of the core infrastructure without this patch in place could have devastating results.

@paragoninitiativeenterprises has delivered this on a silver platter, ensuring its compatibility with even the oldest, slowest, most absolutely pathetic systems on which a copy of WordPress could conceivably run. As @ericmann rightly pointed out, sodium_compat is widely used among some of the top packages in the PHP ecosystem. Much as an audit would be nice to have, perfect is the mortal enemy of good here (as noted in comment 41 about another issue). We should not let the lack of one get in the way.

@paragoninitiativeenterprises put in a significant amount of work here, expecting nothing in return except that work actually being used for the good of WP and the Web as a whole. He's made it clear in deed and word (comment 41 again) that he will do whatever it takes from his side to make this as easy as possible for the WP team to implement. As a client-serving WP developer myself, it frustrates me to no end to see this continue to be stonewalled.

Folks, you can do better.

#45 @paragoninitiativeenterprises
7 months ago

I've received confirmation that the ClassicPress fork of WordPress wants to implement code signing in their fork.

In the interest of helping the community become more secure, until radio silence is broken on the WP.org core team's side, all of our efforts are going to be refocused onto making ClassicPress secure.

We're open to resuming our development once we're not screaming into the void, and there's a clear action plan for getting this ticket resolved by the WP.org core team.

#46 @mnelson4
7 months ago

FYI WordPress.org has been hacked in the past.

Matt Mullenweg himself said:

As a weblogging community we should be heading away from centralization as a rule, not flocking to every free or low-cost centralized service that pops up.
Realistically, anyone can be hacked...

That was a different situation and a different time, but it's similar enough to mention.

This ticket was mentioned in Slack in #core by paragonie. View the logs.


7 months ago

This ticket was mentioned in Slack in #core by paragonie. View the logs.


7 months ago

#49 @pento
7 months ago

  • Milestone changed from Future Release to 5.1
  • Owner set to pento
  • Status changed from reopened to assigned

#50 @paragoninitiativeenterprises
5 months ago

The inclusion of sodium_compat has been moved to a separate ticket, #45806, since it's more generally useful than this specific use case. In particular, having sodium_compat installed is necessary for a reasonable transition strategy between min-5.2 and min-7, as it enabled plugin developers to do their thing without waiting for an upstream BC break.

Thus, my future patches in this ticket will only pertain to the auto-updater. The sodium_compat work lives in #45806, and must be closed before this ticket can be closed.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


5 months ago

#52 @peterwilsoncc
5 months ago

  • Description modified (diff)

#53 @pento
4 months ago

Thanks for your patience on this, @paragoninitiativeenterprises. I've been thinking about this a bit, and I believe it will fit nicely into several initiatives planned for 2019.

First off, there's the priorities for 2019: https://make.wordpress.org/core/2018/12/08/9-priorities-for-2019/

Auto updates feature relatively heavily, it's going to be important to do them right, and ensuring the site has downloaded the correct, uncorrupted update file is part of that. As you've mentioned previously, the hash checks for update file downloads are currently... inadequate. 🙂 While there are a myriad of options available, something cryptographically secure makes the most sense from both security and future-proofing perspectives.

Secondly, we're looking at bumping the minimum PHP version pretty aggressively: https://make.wordpress.org/core/2018/12/08/updating-the-minimum-php-version/

The April 2019 date (where we increase the minimum to PHP 5.6) will probably coincide with WordPress 5.2. As you mentioned in #45806, we could avoid committing masses of PHP by bumping our minimum to PHP 5.3+.

So, with those points in in mind, this is the (tentative) list that I'm looking at for WordPress 5.2:

  • Bump WordPress' minimum PHP version to 5.6.
  • Include sodium_compat as a composer dependency.
  • Add experimental package signing for Core updates: a failing signature wouldn't prevent an update, but it would report error information to WordPress.org, so we can determine if there are significant real-world factors that we need to account for.
  • Stretch goal: do the same, but for plugins and themes, too.

Depending on the results from WordPress 5.2, as well as the state of the other auto update work that will need to be done, package signatures would be enforced in a subsequent WordPress 5.x release.

Does this seem like a reasonable set of steps to you? All of it is 100% open for suggestions, feedback, and questions.

#54 @paragoninitiativeenterprises
4 months ago

I have no criticism for your well thought-out plan. The only caveat I would propose is to use the patch in #45806 if (and only if) Composer support doesn't land in time for WordPress 5.2.

#55 @pento
4 months ago

  • Milestone changed from 5.1 to 5.2

Sounds good, thanks @paragoninitiativeenterprises! I'll move this ticket and #45806 to the 5.2 milestone.

#56 @dd32
3 months ago

Hey @paragoninitiativeenterprises

I've been experimenting with some approaches for integrating signature verification

  • I've got a demo plugin which does signing/verification that I've been using to refamiliarise myself with it, there's a few basic integration tests that confirm signing works as intended.
  • I've got a branch which adds support to WordPress, based off of the above plugin, but not aligning 1:1. We should consider porting some of the tests from above over too.

A few things I've noticed:
I'm concerned about Memory Usage, with some plugins and core packages hitting 10+MB

  • sodium_crypto_sign_verify_detached() requires that the entire package is held within memory, due to the algorithm requirements that makes sense.
  • Sodium_Compat's implementation of sodium_crypto_sign_verify_detached() requires twice the memory, so 20+MB. Given it's being used for compat purposes, it seems highly likely that these will also be instances where less memory is going to be available (Even if it is only PHP 5.6+ if the PHP requirement bump goes through).

I've instead looked into hashing the file and signing that hash via sodium_crypto_sign_verify_detached() instead

  • The above plugin/patch uses SHA384 via hash_file(), which appears to be the best hashing option available from your list and compiled into PHP.
  • I looked into using Blake2b as implemented as generichash in Sodium, unfortunately the Sodium_Compat implementation is incredibly slow in my testing (5+ minutes verses 0.2s on 10MB) which rules that out for now.
  • SHA-512/256 and SHA-512/224 are only available in PHP 7.1+ and didn't immediately appear to have a PHP 5.6 compatible option.

Are there any other options which might be better suited here?

Other Implementation Notes:

  • The signature is implemented as being provided in a HTTP Header (If server supports it) and also being available as a separate file for fallback in the cases where setting a HTTP Header isn't viable.
  • The implementation allows for multiple public keys AND multiple signatures to be presented/attempted, That's for a future use where we change the hashing, signing algorithm, or are transitioning between signing keys.
  • It's tied to WordPress.org domains where we could possibly route files, plugins can filter that and also register their own keys. It's reasonable to suggest that this shouldn't be filterable, but if code can filter it, it could re-reroute or alter signature verification anyway.
  • It's implemented in soft-fail mode currently, reporting failure reasons to WordPress.org for debug during development (Data is not yet collected). I would anticipate running in that mode for several months until no more failure reports happen or are mitigated.
  • I've chosen to base64 encode signatures and public keys, hex encoding is also an option, no particular reason.
  • I'm also looking at this from a WordPress.org-signing perspective, since the implementation affects both systems.

#57 follow-up: @paragoninitiativeenterprises
3 months ago

@dd32 Looking at this patch and your comment above: https://github.com/WordPress/wordpress-develop/compare/da4f8d0ffbab9699d940a4bde6839def27956021...9f472e100f8da6d11eb818f99510d74f3dfcd06a

Using hash_file() makes a lot of sense for keeping memory usage low. We wrote ParagonIE_Sodium_File to accomplish the same overall goal, but it's not foolproof.

Additionally, SHA384 is the best possible hash function for PHP <7.1 support. (Composer uses this hash function too.) If anyone is concerned about SHA384 being used in other protocols, adding domain separation (as simple as using hash_hmac_file() with the HMAC key being a constant specific to WordPress that bears no secrecy requirements) would ensure that the hashes used in the file verification are distinct to WordPress.

Base64 is fine here. All of the keys and signatures involved in the patch are public information anyway.

Overall, I think your patch makes sense and the main operational overhead remaining would be to generate/manage the Ed25519 keys on the WP.org side.

#58 in reply to: ↑ 57 ; follow-up: @dd32
3 months ago

Thanks @paragoninitiativeenterprises!

Replying to paragoninitiativeenterprises:

Using hash_file() makes a lot of sense for keeping memory usage low. We wrote ParagonIE_Sodium_File to accomplish the same overall goal, but it's not foolproof.

Glad I'm not totally off-base by suggesting it be used here then :) I can't imagine hosts will want to see the auto-update processes use significantly more memory either

Additionally, SHA384 is the best possible hash function for PHP <7.1 support. (Composer uses this hash function too.) If anyone is concerned about SHA384 being used in other protocols, adding domain separation (as simple as using hash_hmac_file() with the HMAC key being a constant specific to WordPress that bears no secrecy requirements) would ensure that the hashes used in the file verification are distinct to WordPress.

I hadn't considered the option of using hmac to have WordPress-specific hashes, it would certainly make sense if it adds any real additional difficulty in theoretical attacks.

For others, I went and looked up the Composer implementation which uses openssl_verify() and sha384 as the $algo.

Overall, I think your patch makes sense and the main operational overhead remaining would be to generate/manage the Ed25519 keys on the WP.org side.

Great to hear, thank you!
The WordPress.org side is going to require much more additional work obviously, but should be operational soon for testing.

#59 in reply to: ↑ 58 @dd32
3 months ago

Replying to dd32:

Replying to paragoninitiativeenterprises:

Additionally, SHA384 is the best possible hash function for PHP <7.1 support.

Just as a quick mention for that, the implementation supports WordPress.org presenting multiple signatures, It could in the future present a sha384 signature (for back-compat) and a sha512/224 signature (for newer WordPress's).

It doesn't currently have any field in the signature to specify the algorithm of the signature, it'd just attempt to verify Signature New against Old Algo hash and then attempt to verify Signature Old against Old Algo hash.

I think there's a case to be made that the provided signatures should identify the hash algorithm it's for, but realistically I don't believe we'd have a reason to unless there was a serious vulnerability found in the hash. As it accepts multiple signatures we'd just switch newer WordPress to use the updated hash algorigthm, and have WordPress.org providing the older hash signatures for a time-period.

#60 @paragoninitiativeenterprises
3 months ago

It's incredibly unlikely that the SHA2 family of hash function is going to be broken anytime soon.

That being said, if it does, I don't suspect SHA512/224 (collision resistance of 112 bits) will fare much better than SHA384 (collision resistance of 192 bits).

https://3v4l.org/l9TVZ

For PHP 7.1 and newer, sha3-256 (or sha3-384 for consistency) would make a better alternative, due to how different SHA3 is than SHA2.

But otherwise, yes, having the machinery in-place to upgrade the hash function used (e.g. via multisig) is a good idea. I don't anticipate SHA384 having a security reduction to less than 100 bits until practical quantum computers are developed.

@dd32
2 months ago

#61 @dd32
2 months ago

39309.diff is the Diff version of my git branch as of right now.

There were some logic changes since the above review by @paragoninitiativeenterprises, namely that I moved the fetch-signature-from-file functionality directly into download_file() rather than embedding it into wp_verify_signature(). I also added a test-key so that others could experiment with the patch.

To test this, I've enabled signature generation for WordPress.org Themes using the above key, so you can experiment with installing themes once patched

$ curl https://downloads.wordpress.org/theme/twentynineteen.1.3.zip -Is | grep 'content-signature'
x-content-signature: Wn9umvWuU2IpqPqWqbccQMmAqeHlIy49HAK8p5FjhWaZ5xeclIt/zBOMvIwKliRmSbzxsiaekom0vowEGV4aDw==

$ curl https://downloads.wordpress.org/theme/twentynineteen.1.3.zip.sig
Wn9umvWuU2IpqPqWqbccQMmAqeHlIy49HAK8p5FjhWaZ5xeclIt/zBOMvIwKliRmSbzxsiaekom0vowEGV4aDw==

(edit: Just to note - The header will only be present if the server serving it is aware of the signature. A request to the .sig variant will cause it to get generated and used for future file serves if it didn't exist.)

Currently the patch does not output any data in the event the signature is validated, or validation isn't attempted. I would suggest adding debugging around line 1064 here if you wish to confirm.

In the event that signature verification is attempted (default, for all downloads from WordPress.org domains) and signature verification fails, the failure message will be included in the Update/Install verbose output, but won't prevent the installation/update of the theme.

When testing, if you copy the link attached to the 'install' button in the Themes page, you can skip the ajaxification and view the update pages directly and constantly reloading that page will cause it to re-download the theme and perform the verifications before failing at "theme is already installed".

If anyone is concerned about SHA384 being used in other protocols, adding domain separation (as simple as using hash_hmac_file() with the HMAC key being a constant specific to WordPress that bears no secrecy requirements) would ensure that the hashes used in the file verification are distinct to WordPress.

The above patch, and the WordPress.org implementation has proceeded just using hash_file(), I don't feel greatly swayed either way here, adding a public hmac key seemingly wouldn't protect us against a brute-force collision attack and skipping it opens up a wider range of CLI tools for hashing.

If we do change that hashing mechanism, it should be done at the same time as we switch from using the current test key to a production key.

Last edited 2 months ago by dd32 (previous) (diff)

@dd32
2 months ago

39309.diff with minor coding standards whitespace violations fixed

@dd32
2 months ago

39309.2.diff with the addition of "Expire Key 1 in two years time" https://github.com/dd32/wordpress-develop/commit/1f1ff65eb78a6a5c9c6297965c2c7419830f9dfe

#62 @tellyworth
2 months ago

Just adding a thumbs up here, the 2 year expiration on the temporary key covers my only concern (ie that code signed with the temp key could otherwise in theory be valid forever).

Last edited 2 months ago by tellyworth (previous) (diff)

This ticket was mentioned in Slack in #core by tellyworth. View the logs.


2 months ago

#64 @tellyworth
2 months ago

In 44953:

General: Add sodium_compat library for crypto APIs in PHP < 7.2

This adds a pure PHP implementation of the cryptographic functions supported in PHP 7.2+. It provides the necessary backwards compatibility required to support signature verification and other security features going forward across all supported PHP versions.

Props paragoninitiativeenterprises
Fixes #45806. See #39309.

#65 @tellyworth
2 months ago

In 44954:

Upgrade/Install: Add experimental package signing to some updates.

This adds code for soft verification of signatures for theme and plugin installs and updates, when provided by the update server. This experimental version does not reject unverified packages or failed signatures; it simply reports anonymous errors so we can evaluate its feasibility and detect incompatibilities.

This code relies on the new sodium_compat library for PHP versions prior to 7.2.

Props dd32, paragoninitiativeenterprises.
See #39309, #45806.

#66 @pento
2 months ago

  • Type changed from enhancement to task (blessed)

As the feature has been committing to trunk, I'm transforming this to a task, ongoing work can continue during the beta period.

#67 @dd32
2 months ago

The implementation has the ability to accept multiple signatures from the remote server, which is built around having multiple header values returned.
The HTTP protocol however allows servers and proxies to combine multiple values into a single header, for example: X-Content-Signature: SigOne, SigTwo. For maximum compatibility we should take that info consideration.

39309-single-header.diff adds support for that. Note that Whitespace and commas are not part of the character set of base64 encoded values, and are safe to split by here.

#68 @dd32
7 weeks ago

After reviewing the error debugging included, it looks like we've got a few clients failing to verify signatures, but the reason isn't jumping out at me straight away.

While it could be developers debugging the functionality, 39309-extra-debugging.diff adds some extra debugging data to the payload to identify the PHP version, and Sodium Version (or sodium_compat version) in use which should help narrow down any incompatibilities.

@dd32
7 weeks ago

#69 @dd32
7 weeks ago

39309-tests.diff is a first-stab at some PHPunit-based tests.

They're not at all full coverage, and while I don't think they represent real-life cases, they cover some of the functionality of the innards of upgrades.

#43395 is related, and I'll pop a comment over there for completeness.

#70 follow-up: @paragoninitiativeenterprises
7 weeks ago

After reviewing the error debugging included, it looks like we've got a few clients failing to verify signatures, but the reason isn't jumping out at me straight away.

Could you forward some details about this to security@… at your earliest convenience? If there's a platform-specific bug affecting Ed25519 signature verification, it probably needs to be fixed inside sodium_compat.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


7 weeks ago

#72 in reply to: ↑ 70 @dd32
7 weeks ago

Replying to paragoninitiativeenterprises:

After reviewing the error debugging included, it looks like we've got a few clients failing to verify signatures, but the reason isn't jumping out at me straight away.

Could you forward some details about this to security@… at your earliest convenience? If there's a platform-specific bug affecting Ed25519 signature verification, it probably needs to be fixed inside sodium_compat.

We'll most definitely forward anything on, unfortunately at present there's no context other than (paraphrased) Signature X of Hash Y failed to be verified against Key Z on an Unknown Environment on an Unknown Host (where X, Y, and Z are correct).

We don't have any details of if it's ext/sodium, sodium_compat, if the signature or key length were rejected, etc. 39309-extra-debugging.diff aims to give some context there, but it's not going to pinpoint the problematic environment, if that doesn't provide enough details to reproduce, we'll look at other ways to identify a pattern.

#73 @tellyworth
7 weeks ago

In 45112:

Upgrade/Install: Add more context in signature verify failures.

This includes version numbers and signature counts in error reports, to help diagnose isolated failures that have no apparent cause.

Props dd32.
See #39309.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


5 weeks ago

#75 @paragoninitiativeenterprises
5 weeks ago

@dd32 deduced that https://bugs.php.net/bug.php?id=75938 was the source of the corner case failures we were seeing.

Unfortunately, this is a very pernicious bug that makes all PHP 7.2.0, PHP 7.2.1, and PHP 7.2.2 installs with unstable. We might want to separately warn users about this.

Proposed patch incoming to side-step sig failures.

@paragoninitiativeenterprises
5 weeks ago

Prevent PHP-FPM 7.2.0-7.2.2 without ext/sodium failures

@dd32
5 weeks ago

Alternative to 39309-preemptive-softfail.patch

#76 follow-up: @dd32
5 weeks ago

Looking at 39309-preemptive-softfail.patch I agree it's the correct way to go, except I don't think we want a new string here.

39309-phpbug.diff uses the same error/string "unavailable on this system" and adds an extra conditional of "the opcache must be enabled" - Turns out it wasn't on my original test system, which is why it took me longer than i'd have liked to track down the failure.
If we want to add specific "Don't use this version of PHP" we should do that in the Health check functionality, apparently the early versions of PHP 7.3 also had issue with WordPress and popular plugins causing segfaults/etc.

#77 @paragoninitiativeenterprises
5 weeks ago

Yes, PHP 7.3.1 contains the fix for the SodiumException segfault.

#78 in reply to: ↑ 76 ; follow-up: @dd32
5 weeks ago

Replying to dd32:

39309-phpbug.diff uses the same error/string "unavailable on this system" and adds an extra conditional of "the opcache must be enabled" - Turns out it wasn't on my original test system, which is why it took me longer than i'd have liked to track down the failure.

After more consideration, I'm not sure that conditional is best, it's trying to be very specific, where as I think we should go with a pretty broad conditional - Other SAPI's than fpm are affected if opcache is enabled, and while we can detect the exact conditionals, it's a bit wack-a-mole, lets just blacklist those PHP's instead for the very small portion of users who are using it.

I'd suggest we simply reduce the test to this to completely disable the functionality on potentially affected installs (and in the future, disable verification checking requirements on an affected install)

if ( 
   ! extension_loaded('sodium') &&
   in_array( PHP_VERSION_ID, [70200, 70201, 70202] )
) {

We could also blacklist 7.3.0 for the SodiumException segfault if that's actually required @paragoninitiativeenterprises , for reference ~15% of current PHP 7.3 users are on PHP 7.3.0.

if ( 
   (
      ! extension_loaded('sodium') &&
      in_array( PHP_VERSION_ID, [70200, 70201, 70202] )
   ) ||
   (
       extension_loaded( 'sodium' ) &&
       PHP_VERSION_ID === 70300
   )
) {

#79 in reply to: ↑ 78 @dd32
5 weeks ago

Replying to dd32:

I'd suggest we simply reduce the test to this to completely disable the functionality on potentially affected installs (and in the future, disable verification checking requirements on an affected install)

if ( 
   ! extension_loaded('sodium') &&
   in_array( PHP_VERSION_ID, [70200, 70201, 70202] )
) {

39309-phpbug.2.diff implements this, but it also checks for the existence of opcache_get_status() which can also be used to determine if the opcache is enabled or not.

@dd32
5 weeks ago

#80 @dd32
5 weeks ago

Another failure case that's being reported appears to be where the Signature being validated is the raw contents of a ZIP file, in these cases however there's no signature available.

It appears to be a case where the download url has a query argument added, for example https://wordpress.org/plugins/hello-dolly.zip?nostats=1
The original code was just suffixing .sig to the URL, so it was then requesting http://...hello-dolly.zip?nostats=1.sig which then results in it double-downloading the ZIP file.

39309-signature-urls.diff corrects that by only suffixing to the path (It keeps any Query arguments in place) and only affecting download urls which end in .zip or .tar.gz. Urls such as https://api.../download.php?slug=my-private-plugin&auth=123123123 will therefor not trigger the extra download, but a new filter wp_signature_url is present to allow the plugin to specify where to find it.
Additionally, it limits the download size to 10KB (which is enough for 100+ signatures) to hopefully limit cases where it does unfortunately download a ZIP. We can probably safely increase this to 100KB to never have a problem, but also prevent run-away requests that affect overall timeouts.

I'm in two minds on the filter, I don't think it's needed as most implementations (including WordPress.org) will hopefully include the signature as a HTTP header, but if we're going to request a url, we might as well request the correct one.
@tellyworth what's your thoughts on adding a filter here?

#81 follow-up: @johnbillion
5 weeks ago

@dd32 FYI opcache_get_status() isn't reliable as it's commonly blocked by the restrict_api directive which means its return value can be false when the opcode cache is in use, and it also spits out a PHP warning.

Refs:

#82 in reply to: ↑ 81 ; follow-up: @dd32
5 weeks ago

Replying to johnbillion:

@dd32 FYI opcache_get_status() isn't reliable as it's commonly blocked by the restrict_api directive which means its return value can be false when the opcode cache is in use, and it also spits out a PHP warning.

That's the kind of thing I was expecting :(

In that case, an updated patch using extension_loaded( 'opcache' ) will be uploaded tomorrow, even if the cache is disabled, that small segment of users has to miss out.

@dd32
4 weeks ago

#83 in reply to: ↑ 82 @dd32
4 weeks ago

Replying to dd32:

In that case, an updated patch using extension_loaded( 'opcache' ) will be uploaded tomorrow, even if the cache is disabled, that small segment of users has to miss out.

That's 39309-phpbug.3.diff, I've also clarified the comment that it's opcache and not PHP that causes the issue.

#84 @paragoninitiativeenterprises
4 weeks ago

39309-phpbug.3.diff looks good: The comments accurately reflect the current state of affairs, and it correctly disables the signature mechanism only on platforms that would be affected.

@dd32
4 weeks ago

refresh of 39309-signature-urls with a function typo corrected

@dd32
4 weeks ago

Disable the 'No Signatures found' error unless in Debug mode, at 5.2's launch we're unlikely to have plugin/theme/translation signatures and displaying this isn't going to be helpful to end-users

#85 @dd32
4 weeks ago

Current State of this ticket:

The following patches need review and/or commit:

  • 39309-phpbug.3.diff to disable this for incompatible PHP's.
    Best way to test this is to just verify it's not triggered on a 'good' system.
  • 39309-signature-urls.2.diff to prevent WordPress downloading incorrect URLs when searching for a signature file (Review needed, seems no-one reviewed 39309-signature-urls.diff as the patch file was incomplete).
    Best way to test this is to call download_url( "https://downloads.wordpress.org/plugin/hello-dolly.1.6.zip?nostats=1", 300, true ); and verify you get the signature_verification_no_signature error instead of the signature_verification_failed error code.
  • 39309.disable-no-warnings-notice.diff to disable the "No signature found" warning when installing Plugins, Themes, and other items.
    Best way to test this is to install a plugin while viewing it's output, verify that you don't see the "No signature found" message (No plugins have signatures currently)
  • https://core.trac.wordpress.org/ticket/46615#comment:14 also needs review and commit, the patch there improves Backwards compatibility with 3rd party update scripts and renames the $signature_softfail variable to be an on/off switch for signatures.
    Best way to test this is to call download_url( "https://downloads.wordpress.org/plugin/hello-dolly.1.6.zip?nostats=1" ); and verify you get a non-WP_Error object.
  • We'll be updating wp_trusted_keys() with a new public key before 5.2's release - the existing key will be no longer used.

Unfortunately at 5.2's release we're only going to have Signatures for Core Updates packages ready, with themes/plugins/translations to come later, which is why 39309.disable-no-warnings-notice.diff is needed.

It's also likely that we'll change wp_trusted_keys() in 5.2.x to have separate keys for Core Releases and Plugins/Themes/Translations/etc to allow us to apply more fine-grained control, that'll likely also require us to add a $context = core|plugin|theme|translation parameter or similar to switch between different trusted keys and likely also to consider revoked keys.
Some of those improvements might be put in 5.3 instead, as what we currently have in trunk can support improvements being made in a future release without compromising on security or risking a case where updates fail.

#86 @tellyworth
4 weeks ago

In 45262:

Upgrade/install: fix verification bugs and scale back signature checks.

This fixes several bugs in the signature verification code:
Disables signature checks on certain incompatible PHP versions that cause math errors when opcache is enabled;
Prevents a spurious URL and subsequent error when downloading a zip file with query arguments;
Prevents errors triggered by third-party upgrade scripts as per #46615;
Disables signature tests for Plugins, Themes, and Translations, leaving only core updates.

At the 5.2 release the API servers will only provide signatures for core update packages, which is why messages are suppressed for plugins and other package types. Signatures for those other items will become available later.

Props dd32.
See #39309, #46615

#87 @pento
4 weeks ago

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

Closing this ticket as fixed, as this is as much as we're going to get done in 5.2. New tickets can be opened for future iterations.

#88 @SergeyBiryukov
5 days ago

#25052 was marked as a duplicate.

Note: See TracTickets for help on using tickets.