WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#47186 closed defect (bug) (fixed)

At least one function in /wp-includes/sodium_compat/src/Core32 times out on 32 bit servers

Reported by: lovingboth Owned by: paragoninitiativeenterprises
Milestone: 5.2.1 Priority: high
Severity: normal Version: 5.2
Component: Upgrade/Install Keywords: needs-testing has-patch commit
Focuses: Cc:

Description

The sodium_compat library was added in 5.2

When attempting to update themes in WP 5.2 on a 32-bit Debian Stretch server (uses PHP 7.0.33) the process fails with errors like the following in the Apache error log:

[Wed May 08 14:55:36.159261 2019] [proxy_fcgi:error] [pid 27002:tid 2923363136] [client 1.2.3.4:54280] AH01071: Got error 'PHP message: PHP Fatal error: Maximum execution time of 30 seconds exceeded in /home/user/public_html/example/wp-includes/sodium_compat/src/Core32/Int64.php on line 274\n', referer: https://example.com/wp-admin/update-core.php?action=do-theme-upgrade

That's at the end of the function to multiply two 64-bit integers, and seems to be wanted by the Poly1305/State.php, X25519.php and Curve25519.php files.

Whatever it's doing, it's not working correctly.

Attachments (15)

47186.patch (1.4 KB) - added by paragoninitiativeenterprises 4 months ago.
47186-update_sodium_compat_to_v1_9_2.patch (11.7 KB) - added by paragoninitiativeenterprises 4 months ago.
Updates sodium_compat to v1.9.2 which contains a speedup for 32-bit platforms
47186-update_sodium_compat_to_v1_9_3.patch (11.8 KB) - added by paragoninitiativeenterprises 4 months ago.
Same as the previous patch, except this one also addresses a false positive type error when testing with Psalm and other static analysis tools.
47186-combined.patch (30.9 KB) - added by paragoninitiativeenterprises 4 months ago.
All-in-one patch that should be ready to commit.
47186-update_sodium_compat_to_v1_9_4.patch (29.3 KB) - added by paragoninitiativeenterprises 4 months ago.
47186-timeout-30-seconds.patch (1.4 KB) - added by paragoninitiativeenterprises 4 months ago.
47186-timeout-30-seconds.2.patch (1.4 KB) - added by paragoninitiativeenterprises 4 months ago.
Now with corrected whitespace
47186-sodium_compat-v1_10_0.patch (34.6 KB) - added by paragoninitiativeenterprises 4 months ago.
sodium_compat v1.10.0
47186-sodium_compat-v1.10.2-fixed.patch (41.6 KB) - added by paragoninitiativeenterprises 4 months ago.
47186-final-timeout.patch (1.6 KB) - added by paragoninitiativeenterprises 4 months ago.
47186-bikeshed.patch (1.8 KB) - added by paragoninitiativeenterprises 4 months ago.
Lowers parameters, uses mulInt64Fast via static property on ParagonIE_Sodium_Compat as per @dd32's suggestion
47186-support-old-wordfence.patch (1.9 KB) - added by paragoninitiativeenterprises 4 months ago.
Guard the runtime test check with method_exists() to prevent compatibility issues, cc @wfmattr
47186.update-sodium.diff (44.2 KB) - added by dd32 4 months ago.
Update of Sodium_Compat to 1.10.0
47186.diff (2.1 KB) - added by dd32 4 months ago.
Cautious patch approach, Verifications only available if things are running fast. Props paragoninitiativeenterprises, dd32, wfmattr, lovingboth
47186.tests.diff (2.5 KB) - added by dd32 4 months ago.
Unit tests which verify verify_file_signature() operates and works within a reasonable timeframe, and that it fails to validate an invalid signature.

Download all attachments as: .zip

Change History (62)

#1 @lovingboth
4 months ago

If I increase PHP's max_execution_time to 60 seconds from the default of 30 seconds, it completes, but this is not

a) something everyone can do or
b) acceptable.

It's running on a DigitalOcean VPS, so it's not the fastest it could possibly be, but if you're demanding more CPU power than that has, the code is the problem, not the server.

#2 @lovingboth
4 months ago

If I run

# sysbench --test=cpu run
sysbench 0.4.12:  multi-threaded system evaluation benchmark

Running the test with following options:
Number of threads: 1

Doing CPU performance benchmark

Threads started!
Done.

Maximum prime number checked in CPU test: 10000

Test execution summary:
    total time:                          15.9531s
    total number of events:              10000
    total time taken by event execution: 15.9465
    per-request statistics:
         min:                                  1.18ms
         avg:                                  1.59ms
         max:                                  9.27ms
         approx.  95 percentile:               2.08ms

Threads fairness:
    events (avg/stddev):           10000.0000/0.00
    execution time (avg/stddev):   15.9465/0.00

For comparison, running it with a single thread on this PC with a 3.4GHz Ryzen CPU, it takes 10 seconds, 2/3rds of the time.

lscpu tells me the VPS has a Intel(R) Xeon(R) CPU E5-2630L v2 @ 2.40GHz, and a BogoMIPS: of 4800. This PC has one of 6800.

It's not a lack of CPU power.

#3 @lovingboth
4 months ago

On another VPS it's happening on, lscpu says it's a Intel(R) Xeon(R) CPU E5-2630L 0 @ 2.00GHz with a BogoMIPS of 4000.

Setting max_execution_time = 60 on this one is just sufficient to avoid problems with larger themes.

In the updates page of the dashboard, if I update Twenty Ten, it takes 30 seconds from the appearance of 'Update Themes' to the 'The update process is starting. This process may take a while on some hosts, so please be patient...' lines.

I know the cryptographic functions in the sodium_compat library are designed to have a constant execution time for any particular task on any particular hardware, but it's clearly taking too long.

(Unless someone here is paying for me to have faster VPSes...)

Last edited 4 months ago by lovingboth (previous) (diff)

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


4 months ago

#5 @ocean90
4 months ago

  • Component changed from Administration to Upgrade/Install
  • Keywords needs-testing added
  • Owner set to paragoninitiativeenterprises
  • Status changed from new to reviewing

@paragoninitiativeenterprises Could you review this please?

#6 @lovingboth
4 months ago

Someone else on the forum reports the same problem when using Debian Stretch on a Raspberry Pi 2 - that will also be 32-bit, and I wonder if having to do lots of 64-bit multiply operations via 32-bit maths is an element of this (and why it wasn't picked up when testing...)

"Installing new themes is possible via uploading a zip-file, but not via download from wordpress.org."

#7 @lovingboth
4 months ago

I don't necessarily think it's relevant, but as the source of verify_file_signature in wp-admin/includes/files.php mentions that it doesn't work with PHP 7.2.0 to 7.2.2 if opcache is enabled...

.. I have opcache enabled on these PHP 7.0.33 servers too.

#8 follow-ups: @paragoninitiativeenterprises
4 months ago

This is a problem with 32-bit platforms. See: https://github.com/paragonie/sodium_compat#help-sodium_compat-is-slow-how-can-i-make-it-fast

The ParagonIE_Sodium_Compat::polyfill_is_fast() method was created to detect when performance might be a problem. The typical workaround most people employed was: pecl install sodium.

However, that's not a universal solution. Some people cannot install PECL extensions. So a lot of the changes between v1.6.0 and v1.9.1 (the latest) have been attempts to optimize the 32-bit implementation of these functions.

@lovingboth

I don't necessarily think it's relevant, but as the source of verify_file_signature in wp-admin/includes/files.php mentions that it doesn't work with PHP 7.2.0 to 7.2.2 if opcache is enabled...

The verification is disabled on those versions because it generates incorrect results, not for performance reasons.

The fix here, if there is one, will land in sodium_compat. It's not a bug in WordPress. We do have a few optimization ideas that we haven't tried yet (which are nontrivial to implement).

A short-term workaround would be to, if ParagonIE_Sodium_Compat::polyfill_is_fast() returns false and ini_get('max_execution_time') is greater than 1 but less than some reasonable value (30 is too small... maybe 40?), don't check signatures.

#9 @JeffPaul
4 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.2.1

@paragoninitiativeenterprises while we await that upstream fix, would you be able to help submit a patch for that workaround you noted?

#10 @paragoninitiativeenterprises
4 months ago

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #forums by t-p. View the logs.


4 months ago

#12 in reply to: ↑ 8 @lovingboth
4 months ago

Apologies if this sounds somewhat pointed, but the number of hours I have had to spend over this in the last couple of days is not small.

Replying to paragoninitiativeenterprises:

This is a problem with 32-bit platforms. See: https://github.com/paragonie/sodium_compat#help-sodium_compat-is-slow-how-can-i-make-it-fast

Ah, so it's a known problem with the library that was added to WordPress core without, as far as I can see, ever seeing how many WP users are running on 32-bit systems.

The ParagonIE_Sodium_Compat::polyfill_is_fast() method was created to detect when performance might be a problem. The typical workaround most people employed was: pecl install sodium.

However, that's not a universal solution. Some people cannot install PECL extensions. So a lot of the changes between v1.6.0 and v1.9.1 (the latest) have been attempts to optimize the 32-bit implementation of these functions.

Or you could use a faster, albeit theoretically slightly less secure, algorithm in the first place. Was using SHA512 hashes tested? That's native in PHP for all versions WordPress supports.

@lovingboth

I don't necessarily think it's relevant, but as the source of verify_file_signature in wp-admin/includes/files.php mentions that it doesn't work with PHP 7.2.0 to 7.2.2 if opcache is enabled...

The verification is disabled on those versions because it generates incorrect results, not for performance reasons.

Ah, thanks.

The fix here, if there is one, will land in sodium_compat. It's not a bug in WordPress. We do have a few optimization ideas that we haven't tried yet (which are nontrivial to implement).

The bug would be including a library in core that was known - see https://core.trac.wordpress.org/ticket/45806, "If PHP_INT_SIZE === 4 its public-key cryptography implementations are much slower than PHP_INT_SIZE === 8, but there's no safe way to get around this limitation" - not to work effectively on an unknown number of systems.

Fan as I am of verifying stuff, if https:api.wordpress.org is compromised, it's a) not unlikely that the signing key is too and b) there are worse security problems for WordPress than not having a signature on files downloaded from it.

(See, for example, the way that in 2019 I think you can still make infinite failed attempts to login without core doing anything about it - even notifying the owner - and continue to have that bruteforce attempt amplified by the 'feature' of xmlrpc that allows hundreds of simultaneous attempts in a single call, something for which there are literally zero valid use cases.)

A short-term workaround would be to, if ParagonIE_Sodium_Compat::polyfill_is_fast() returns false and ini_get('max_execution_time') is greater than 1 but less than some reasonable value (30 is too small... maybe 40?), don't check signatures.

30 seconds is, of course, the default for PHP installations and a large chunk of WP users will have no way to change that.

#13 in reply to: ↑ 8 @lovingboth
4 months ago

Replying to paragoninitiativeenterprises:

I've just realised who I am talking to, argh, blush, sorry.

Double apologies for the tone, given you're the source of the library and not the people who chose to add it to core despite its acknowledged issues with a unknown sized chunk of the installed base. (It's not on at https://wordpress.org/about/stats/ but what is there says that less than half the user base are on any 5.x version of WordPress, and less than one in three are using a version of PHP that WordPress counts as "secure".)

Triple apologies for asking you about whether SHA512 was considered.

#14 @lovingboth
4 months ago

Just checking...

I can reinstall WordPress 5.2 via the Dashboard relatively quickly.

Does this mean that the push of 5.2.1 with this patch, when it happens, will not be checked by this library?

As the execution speed seems to depend on the size of the file being downloaded, if it is checked, people affected are going to need a max_execution_time setting that's a lot larger than 60.

(And if it is not checked, isn't that a big security issue in the event of api.wordpress.org being compromised?)

#15 follow-up: @paragoninitiativeenterprises
4 months ago

@lovingboth:

Or you could use a faster, albeit theoretically slightly less secure, algorithm in the first place. Was using SHA512 hashes tested? That's native in PHP for all versions WordPress supports.

SHA512 isn't even in the same universe as Ed25519. See https://paragonie.com/blog/2015/08/you-wouldnt-base64-a-password-cryptography-decoded for more information. We tried to make the basic classifications of cryptographic algorithms easy to understand with that article.

To be explicit:

  • SHA512 is a hash function, that can be almost-trivially used as a building block for symmetric authentication protocols (i.e. HMAC)
  • Ed25519 is an asymmetric authentication digital signature algorithm

They have completely different security properties. In fact, Ed25519 uses SHA512 internally for different steps.

You can't just replace Ed25519 with the SHA512. That would be like trying to build a house with bacon. I like bacon, but I'm not about to trust my family's life in its structural integrity.

@lovingboth:

Apologies if this sounds somewhat pointed, but the number of hours I have had to spend over this in the last couple of days is not small.

I understand your frustration. You're not the only one expending hours on this problem. It took me a month and a half of almost non-stop development to get Curve25519 field arithmetic to work on 32-bit at all', and that was just the up-front development time.

I had been taking great efforts over the past year to make it faster, but it clearly wasn't adequate.

@lovingboth:

Ah, so it's a known problem with the library that was added to WordPress core without, as far as I can see, ever seeing how many WP users are running on 32-bit systems.

It's a known problem that's extremely challenging to solve, and almost nobody runs PHP in general in production on 32-bit systems where they can't also install PHP extensions via PECL, so it's not one that gets a lot of attention.

In fact, when I raised this issue on Twitter, I had this response: https://twitter.com/_francislavoie/status/1126541694617440256

Is there actually enough people still on 32bit for that to be worth the time?

Until now, the answer to their question was flatly, "No".

Most people who run 32-bit PHP have been perfectly content with pecl install sodium as a solution, to date. I suppose that's the curse of early adopters: They tend to be more technical.


Anyway! Misunderstandings aside, I'm releasing a new version of sodium_compat this evening, which introduces a 9x to 10x speedup when you set ParagonIE_Sodium_Compat::$fastMult = true; on 32-bit systems.

https://github.com/paragonie/sodium_compat/pull/86

For Ed25519 signature verification, we automatically set this (temporarily) to true since there are no cryptographic secrets that can be leaked from integer multiplication in this context.

In other words: You can anticipate a significant speed-up that won't, in this specific context, even theoretically harm security.

A patch for WordPress will be provided as soon as I'm confident the changes are non-breaking and v1.9.2 is tagged. This will be safe to release in 5.2.1.

If you'd like to help test this in the meantime, simply copy src/Core32/Int64.php from the official v1.9.2 release over the one WordPress provides and see if the runtime is acceptable on your machine.

#16 @paragoninitiativeenterprises
4 months ago

@lovingboth:

@dd32 et al. decided to soft-fail signature verification in 5.2.0 just in case something like this came up. That was a wise move on their part.

Soft-fail means it won't block updates, it will just complain loudly and then proceed as if there were no signature verification.

However, it might timeout. Which means updating from 5.2.0 to 5.2.1 will have to be manual for anyone affected by the execution time (which, thankfully, isn't most websites).

@paragoninitiativeenterprises
4 months ago

Updates sodium_compat to v1.9.2 which contains a speedup for 32-bit platforms

@paragoninitiativeenterprises
4 months ago

Same as the previous patch, except this one also addresses a false positive type error when testing with Psalm and other static analysis tools.

#17 @paragoninitiativeenterprises
4 months ago

Okay, the new patch should be ready to test. Please let me know if this adequately addresses your performance woes.

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


4 months ago

#19 in reply to: ↑ 15 @lovingboth
4 months ago

Replying to paragoninitiativeenterprises:

I understand your frustration. You're not the only one expending hours on this problem. It took me a month and a half of almost non-stop development to get Curve25519 field arithmetic to work on 32-bit at all', and that was just the up-front development time.

I had been taking great efforts over the past year to make it faster, but it clearly wasn't adequate.

Thank you, especially as I am old enough to have had to develop 32-bit and floating point maths code on 8-bit CPUs... without having to try to make it have constant runtime as well.

@lovingboth:

Ah, so it's a known problem with the library that was added to WordPress core without, as far as I can see, ever seeing how many WP users are running on 32-bit systems.

It's a known problem that's extremely challenging to solve, and almost nobody runs PHP in general in production on 32-bit systems where they can't also install PHP extensions via PECL

How does anyone know?

If any of the more than one in seven running WP in May 2019 with PHP earlier than 5.5 are on Windows, they are very unlikely to be running 64-bit. Or have the ability to install extensions themselves.

My use case for staying on 32-bit is that with a VPS with 1GiB RAM or less, running 64-bit Linux isn't noticeably faster but does use considerably more memory = you can do considerably less on it before hitting the real speed difference of spending lots of time swapping virtual memory about. Given I am working with organisations that struggle for every penny, doing it for half the monthly price makes a difference. (From trying it on a netbook, it also makes a significant difference with 2GiB RAM, so the saving is probably even larger.)

Other people are running it on Raspberry Pis, where the same thing applies and there either isn't a 64-bit CPU or isn't a widely used 64-bit OS for the newer models.

Most people who run 32-bit PHP have been perfectly content with pecl install sodium as a solution, to date.

How does anyone know?

Anyway! Misunderstandings aside, I'm releasing a new version of sodium_compat this evening, which introduces a 9x to 10x speedup when you set ParagonIE_Sodium_Compat::$fastMult = true; on 32-bit systems.

https://github.com/paragonie/sodium_compat/pull/86

For Ed25519 signature verification, we automatically set this (temporarily) to true since there are no cryptographic secrets that can be leaked from integer multiplication in this context.

In other words: You can anticipate a significant speed-up that won't, in this specific context, even theoretically harm security.

A patch for WordPress will be provided as soon as I'm confident the changes are non-breaking and v1.9.2 is tagged. This will be safe to release in 5.2.1.

If you'd like to help test this in the meantime, simply copy src/Core32/Int64.php from the official v1.9.2 release over the one WordPress provides and see if the runtime is acceptable on your machine.

"Never be the first kid on your block to try.." :)

Having said that, doing that cuts down the time to update Twenty Seventeen on that VPS from over thirty seconds to about seven.

Result!

Thank you. (And again, sorry!)

#20 follow-up: @paragoninitiativeenterprises
4 months ago

  • Resolution set to invalid
  • Status changed from reviewing to closed

How does anyone know?

Informally, based entirely on user feedback.

We could do a telemetry survey (just grab PHP_INT_SIZE from WordPress installs, or through e.g. Packagist for Composer users).

I think it'd probably look something like this, in the end:

  • 98% or more: PHP_INT_SIZE === 8
  • 2% or less: PHP_INT_SIZE === 4

It might even be a generally good thing to do, since there aren't reliable statistics available anywhere for this, but it's way outside the scope of this ticket.

Having said that, doing that cuts down the time to update Twenty Seventeen on that VPS from over thirty seconds to about seven.

That's great to hear!

#21 @paragoninitiativeenterprises
4 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Wait what. Why did it get set to invalid?

#22 @lovingboth
4 months ago

Wasn't me :)

#23 in reply to: ↑ 20 @lovingboth
4 months ago

Replying to paragoninitiativeenterprises:

I think it'd probably look something like this, in the end:

  • 98% or more: PHP_INT_SIZE === 8
  • 2% or less: PHP_INT_SIZE === 4

It would be fascinating to know. If WordPress is behind a third of all websites, 2% of that would be "only" 4.3 million sites. Given it would be only newly installed/updated ones, the actual percentage of WordPress sites out there running on 32-bit PHP is likely to be higher because, as that stats page shows, large numbers of older sites are not kept up to date.

That's great to hear!

It's even greater to see!

Last edited 4 months ago by lovingboth (previous) (diff)

#24 @SergeyBiryukov
4 months ago

  • Keywords commit added

#25 @dd32
4 months ago

I'm still catching up on the changes in Sodium_Compat, but I can say that yes, we were aware it could be slow on some systems (32bit, windows, and lower-powered PHPs) but were also hopeful that bumping the minimum PHP from 5.2 to 5.6 was going to help reduce affected installs - of course when you're operating on the WordPress-scale of things though, you're always bound to run into all of the edge-cases, it's a balancing act of pushing the envelope but also having a fallback and a safe exit to avoid breaking things.

Currently the only two types of packages which we are intentionally allowing signatures to be served for are A) Themes and B) WordPress releases.

During the 5.2-development cycle a few issues came up, and we used that data to gauge problems as they occurred and to find the appropriate way forward.

  • One of the things we did was move from Signing entire ZIPs (10M) to signing Hashes instead (88Bytes?) with the hope that that would bypass any speed concerns - and it did in the test scenario's we worked with.
  • Another was finding out that there's a bug in PHP 7.2.0-7.2.2 which caused PHP Math functions to return incorrect results - something that few would have expected, but one which would cause signature verification (and a whole bunch of other random things) to not work as expected.

Thankfully this issue has been reported prior to us shipping WordPress 5.2.1, which means that the update failures being experienced are limited to a) nightly/development builds and b) theme updates/installs.

Moving forward, there's a few things we're going to need to do IMHO:

  • Stop serving signatures for Themes immediately (which will bypass this timeout issue for existing WP 5.2.0 installs - I've just done this, caches may take a few minutes to clear.
  • Include the updates to Sodium_Compat in 5.2.1
  • Include the signature bypass for likely-to-be-slow systems in 5.2.1 (That will also give us some telemetry for number of affected hosts)
  • Not serve signatures for the WordPress 5.2.1 packages, avoiding any failed updates due to Sodium_Compat timeouts
  • Once 5.2.1 is released, we can re-enable signatures for Themes and see if the timeouts occur again (hopefully they don't)

The reason for not serving WP 5.2.1 signatures is that we don't include enough system-details on the API requests to allow us to conditionally serve signatures to only systems which aren't affected by this.


Looking at 47186.patch Although I recognise that the latest Sodium_Compat will help greatly, What's your opinion on making that overly safe and using 60s instead @paragoninitiativeenterprises?
I'd rather not have to bump the required time up later.. I'm mostly considering lower-processing-power systems right now such as ARM devices, overly full shared hosts and super-low-cpu VMs.

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

#26 @paragoninitiativeenterprises
4 months ago

Both patches (47186.patch and 47186-update_sodium_compat_to_v1_9_3.patch) solve the problem differently.

The first skips when the timeout is less than 40 seconds, because a reported runtime of ~30 seconds was causing timeouts. Looking at this patch in isolation, it makes sense to bump it to 60.

However, the second makes the process much faster. My benchmarks are 4-5x faster for Ed25519 verification and 9-10x faster for the test suites, although others' mileage will certainly vary.

With the reported reduction from ~30 seconds to ~7, I think detecting timeouts shorter than 30 will be safe going forward. However, if you believe 60 to be safer, I have no objections.

I'll issue a superseding, unified patch containing both changes, as soon as I know which direction you want to go with the timeout window.

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

#27 @paragoninitiativeenterprises
4 months ago

Regarding skipping a signature on 5.2.1 so that the fix here can be included to prevent future update lock-outs: This is a sensible course of action to take for the time being.

@paragoninitiativeenterprises
4 months ago

All-in-one patch that should be ready to commit.

#28 follow-up: @dd32
4 months ago

@paragoninitiativeenterprises FWIW Generally we'd commit library updates separately from other fixes, as it allows a better version history. Easier to revert and review as well.

My suggestion of 60s was a super-cautious approach, without having tested anything at all.
I'll try to test on a 32bit low-CPU host (or simulate it) and try to verify your testings, if it seems likely that the Sodium_Compat changes mean that signature validations will work without timeout problems, we can potentially skip that entirely or only kick in if someone is running with a 30s-or-lower execution cap.

#29 @paragoninitiativeenterprises
4 months ago

Understood. Here are two separate patches.

@paragoninitiativeenterprises
4 months ago

Now with corrected whitespace

#30 in reply to: ↑ 28 @lovingboth
4 months ago

Replying to dd32:

I'll try to test on a 32bit low-CPU host (or simulate it) and try to verify your testings,

For 32-bit images, DigitalOcean currently only offers Ubuntu 16.04 that has the PHP7.0 I am seeing this with, and CentOS 6.7, which I don't know the details of.

The $5 and $10/month VPSes there are not a 'low-CPU host' in the sense that ARM could be categorised as. For a single thread benchmark, the results are a very healthy chunk of a current Ryzen CPU, and when it's happening, top and the DigitalOcean graphical displays show no stress on the CPU at all.

This ticket was mentioned in Slack in #forums by t-p. View the logs.


4 months ago

#32 follow-up: @dd32
4 months ago

I've tested on a few platforms to get some rough comparative numbers. In all cases below the VM/device was only running PHP-CLI and nothing more.

Platformext/sodiumSodium_Compat as included with WP 5.2Sodium_Compat at git HEAD
32bit Raspberry Pi Zero W0.006s~310s~50s
32bit DigitalOcean VM0.0005s~30s~2.5s
64bit VPS0.0003s0.7s0.7s

Although the DigitalOcean instance is much faster, it was also at 100% CPU utilisation during the operations above.
Enabling XDebug also slows it down significantly, For example taking the 2.5s to ~15s.

Ignoring the Raspberry Pi result (one of the lowest power CPU cores possible), it seems that updating just the library is probably enough for the majority of sites.
Realistically though we don't want things to timeout on low-end systems either, but simply looking at max_execution_time doesn't give us a reasonable idea on if it's likely to timeout..

Would it perhaps be possible to "guess" the performance of the system when running in 32bit mode? I'm thinking something like "Run 100 Int64 multiplications, if it takes longer than 10ms it's a super-slow system".
Looks like the 32bit DigitalOcean VM above can run ~55k/s ParagonIE_Sodium_Core32_Int64::mulInt64Fast()'s, whereas the pi can only do 4.6k/s.

I think there's a high chance that recent PHP's would optimise it out if done wrong, but without some kind of check I suspect we're either going to result in timeouts on low-spec home-servers (Pi's, NAS boxes, etc) or to have to disable it for all 32bit's.

#33 in reply to: ↑ 32 @paragoninitiativeenterprises
4 months ago

Replying to dd32:

Would it perhaps be possible to "guess" the performance of the system when running in 32bit mode? I'm thinking something like "Run 100 Int64 multiplications, if it takes longer than 10ms it's a super-slow system".
Looks like the 32bit DigitalOcean VM above can run ~55k/s ParagonIE_Sodium_Core32_Int64::mulInt64Fast()'s, whereas the pi can only do 4.6k/s.

A runtime test as a last resort is a good idea. https://github.com/paragonie/sodium_compat/pull/90

#35 @dd32
4 months ago

Thanks @paragoninitiativeenterprises!

Running a few tests, I'm seeing ParagonIE_Sodium_Compat::runtime_speed_test(100, 10) not return true, even on the DigitalOcean VM used above.
100 iterations seems to take ~25ms on the DigitalOcean, and ~150ms on the pi (Both above the 10ms maxTimeout). I would probably suggest lowering it to 25 iterations, which comes in at ~5ms on DO and ~35ms on the Pi.

Alternatively, if the function was switched from mulInt64 to mulInt64Fast (Since that's the function WordPress ends up using) seems to be a good test in this case, as DO is 1ms @ 100iterations, while the Pi is 50ms @ 100iterations. Seems like it's testing a different CPU path that is more constrained at least on this architecture.

To keep the ticket up to date, I've also commented on the PR:
https://github.com/paragonie/sodium_compat/pull/90/files#r283589471

Although this isn't a problem right now, FYI this might have a problem in the future with PHP's Dead-Code Elimination optimiser. I've looked at the optimised opcodes at present and it seems OK for now.

See https://www.youtube.com/watch?v=WJJKZM8bruQ#t=28m10s & http://talks.php.net/etsy18#/dce for some examples.

That fits into my earlier comment of I think there's a high chance that recent PHP's would optimise it out if done wrong.

#36 @desrosj
4 months ago

  • Keywords commit removed

Removing commit as this is still being tested.

#37 @paragoninitiativeenterprises
4 months ago

It's not still being tested. The patches work.

Rather, we're bikeshedding over possible ways to prevent future, hypothetical performance problems on extreme corner case systems.

@paragoninitiativeenterprises
4 months ago

Lowers parameters, uses mulInt64Fast via static property on ParagonIE_Sodium_Compat as per @dd32's suggestion

#38 @dd32
4 months ago

Thanks for the direction @paragoninitiativeenterprises :)

I've been testing the actual patches and here's what I'm going with:

I'll get this squared away tomorrow.

#39 @desrosj
4 months ago

  • Priority changed from normal to high

Marking this high priority because it is something we should not punt.

#40 @WFMattR
4 months ago

@dd32: Is it possible to use method_exists() to check if runtime_speed_test exists before using it (or call_user_func()), in this change? https://github.com/dd32/wordpress-develop/commit/0d00d59dbef14d0f139e3ef8b26d43a291523d96

Wordfence has been bundling sodium_compat for a while, and it's possible for Wordfence's autoloader to be registered before the one in WP core, so that function may not exist in that case.

We'll be releasing an update with the same sodium_compat version included here, and will also look at preventing our copy from loading first, but can't prevent an issue for people who don't have the latest Wordfence version.

@paragoninitiativeenterprises
4 months ago

Guard the runtime test check with method_exists() to prevent compatibility issues, cc @wfmattr

@dd32
4 months ago

Update of Sodium_Compat to 1.10.0

@dd32
4 months ago

Cautious patch approach, Verifications only available if things are running fast. Props paragoninitiativeenterprises, dd32, wfmattr, lovingboth

@dd32
4 months ago

Unit tests which verify verify_file_signature() operates and works within a reasonable timeframe, and that it fails to validate an invalid signature.

#41 @dd32
4 months ago

  • Keywords commit added

47186.diff is a variant upon 47186-support-old-wordfence.patch and the previous which takes a much more conservative approach here. In short, it ignores the max_execution_time and instead focuses on the runtime speed, if an old library is in use then signing is also unavailable.

47186.update-sodium.diff is Sodium_Compat 1.10.0, similar to 47186-sodium_compat-v1.10.2-fixed.patch but updates a few ancillary files as well.

47186.tests.diff is an attempt at covering verify_file_signatures() with a unit test. It also requires that verify_file_signature() returns in under 10s, covering the original cause for concern here, that it could run too long (30-300s as seen in comment:32 If nothing else, it fails on environments that aren't properly covered.

#42 @tellyworth
4 months ago

In 45344:

Upgrade/Install: Update sodium_compat to v1.10.0.

This adds a runtime_speed_test() method for estimating if the 32-bit implementation is fast enough for expensive computations.

Props paragoninitiativeenterprises.
See #47186.

#43 @tellyworth
4 months ago

In 45345:

Upgrade/Install: Don't run signature verify on slow 32-bit systems.

The sodium_compat library can be very slow for certain operations on 32-bit architectures, which can lead to web server timeouts while attempting to verify an update. This adds a runtime speed check to skip signature verification on systems that would otherwise time out. Includes simple unit tests.

Props dd32, paragoninitiativeenterprises.
See #47186.

This ticket was mentioned in Slack in #core-php by nerrad. View the logs.


4 months ago

#45 @desrosj
4 months ago

In 45355:

Upgrade/Install: Update sodium_compat to v1.10.0.

This adds a runtime_speed_test() method for estimating if the 32-bit implementation is fast enough for expensive computations.

Merges [45344] to the 5.2 branch.

Props paragoninitiativeenterprises, tellyworth.
See #47186.

#46 @desrosj
4 months ago

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

In 45356:

Upgrade/Install: Don't run signature verify on slow 32-bit systems.

The sodium_compat library can be very slow for certain operations on 32-bit architectures, which can lead to web server timeouts while attempting to verify an update. This adds a runtime speed check to skip signature verification on systems that would otherwise time out. Includes simple unit tests.

Merges [45345] to the 5.2 branch.

Props dd32, paragoninitiativeenterprises, tellyworth.
Fixes #47186.

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


4 months ago

Note: See TracTickets for help on using tickets.