#47186 closed defect (bug) (fixed)
At least one function in /wp-includes/sodium_compat/src/Core32 times out on 32 bit servers
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (62)
#2
@
6 years 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
@
6 years 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...)
This ticket was mentioned in Slack in #core by clorith. View the logs.
6 years ago
#5
@
6 years 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
@
6 years 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
@
6 years 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:
↓ 12
↓ 13
@
6 years 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
@
6 years 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
@
6 years ago
- Keywords has-patch added; needs-patch removed
@JeffPaul Please see: https://core.trac.wordpress.org/attachment/ticket/47186/47186.patch
This ticket was mentioned in Slack in #forums by t-p. View the logs.
6 years ago
#12
in reply to:
↑ 8
@
6 years 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 andini_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
@
6 years 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
@
6 years 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:
↓ 19
@
6 years 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
@
6 years 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).
@
6 years 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
@
6 years 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.
6 years ago
#19
in reply to:
↑ 15
@
6 years 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 in5.2.1
.
If you'd like to help test this in the meantime, simply copy
src/Core32/Int64.php
from the officialv1.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:
↓ 23
@
6 years 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
@
6 years ago
- Resolution invalid deleted
- Status changed from closed to reopened
Wait what. Why did it get set to invalid?
#23
in reply to:
↑ 20
@
6 years 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!
#25
@
6 years 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.
#26
@
6 years 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.
#27
@
6 years 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.
#28
follow-up:
↓ 30
@
6 years 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.
#30
in reply to:
↑ 28
@
6 years 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.
6 years ago
#32
follow-up:
↓ 33
@
6 years 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.
Platform | ext/sodium | Sodium_Compat as included with WP 5.2 | Sodium_Compat at git HEAD |
32bit Raspberry Pi Zero W | 0.006s | ~310s | ~50s |
32bit DigitalOcean VM | 0.0005s | ~30s | ~2.5s |
64bit VPS | 0.0003s | 0.7s | 0.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
@
6 years 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/sParagonIE_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
#34
@
6 years ago
The latest two patches (https://core.trac.wordpress.org/attachment/ticket/47186/47186-sodium_compat-v1.10.2-fixed.patch and https://core.trac.wordpress.org/attachment/ticket/47186/47186-final-timeout.patch) should address all of the performance concerns raised in this ticket.
#35
@
6 years 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
.
#37
@
6 years 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.
@
6 years ago
Lowers parameters, uses mulInt64Fast via static property on ParagonIE_Sodium_Compat as per @dd32's suggestion
#38
@
6 years ago
Thanks for the direction @paragoninitiativeenterprises :)
I've been testing the actual patches and here's what I'm going with:
- Update library: https://github.com/dd32/wordpress-develop/commit/c416af5aaf21c02180711d1d2eef1e9d80fd6c51
- Don't use it on slow systems: https://github.com/dd32/wordpress-develop/commit/0d00d59dbef14d0f139e3ef8b26d43a291523d96
I'll get this squared away tomorrow.
#39
@
6 years ago
- Priority changed from normal to high
Marking this high priority because it is something we should not punt.
#40
@
6 years 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.
@
6 years ago
Guard the runtime test check with method_exists() to prevent compatibility issues, cc @wfmattr
@
6 years ago
Cautious patch approach, Verifications only available if things are running fast. Props paragoninitiativeenterprises, dd32, wfmattr, lovingboth
@
6 years 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
@
6 years 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.
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.