Fix mcrypt fatal - Removed on PHP 7.2 #159

vaurdan · 2021-07-05T14:54:56Z

mcrypt library has been deprecated since PHP 7.1, and removed on PHP 7.2. This PR aims to replace the mcrypt implementation with the alternative openssl library. It also aims to keep backwards compatibility when mcrypt is still available on the system.

There will be issues decrypting old data, however: if the original data was encrypted with mcrypt, it will not be compatible with openssl, as the Rijndael AES is not available on openssl and it's not compatible with aes-256-cbc (source). If there is a way to make the old encrypted data retro compatible, I'm open to suggestions.

I also replaced the usage of serialize with wp_json_encode for PHPCS compliance.

Testing

I did a quick test on my PHP environment (PHP 7.4.21), and the encrypt/decrypt functions are working as intended:

$simple_data = "this is a simple example";
$complex_data = [ 
    'test' => 100, 
    'test_array' => [ 
        'other' => 'test', 
        123
     ]
];

Testing the simple string:

wp> $data = push_syndicate_encrypt( $simple_data );
=> string(60) "b2REUStDTGJISTY1Q0FwRXBJWWNKTkJ1R3Q5ekNSeUl3VUtOQUN5UGJHcz0="
wp> push_syndicate_decrypt ( $data );
=> string(24) "this is a simple example"

Testing with a complex array:

wp> $data = push_syndicate_encrypt( $complex_data );
=> string(120) "amRrU1l0Z1dWakdvZ1ByTng2KzNMYWo1QUNjdm5sREVRQmlHQWdIZ0Y2a0ZYcktLNUFwNW5NcDAzblU5d0c2ZzVCdThGOTFLeVMycFJ5YVNMZ0Q1UWc9PQ=="
wp> push_syndicate_decrypt( $data );
=> object(stdClass)#2883 (2) {
  ["test"]=>
  int(100)
  ["test_array"]=>
  object(stdClass)#2884 (2) {
    ["other"]=>
    string(4) "test"
    ["0"]=>
    int(123)
  }
}

TODO/Questions

If there is no openssl implementation (or cipher) available, the push_syndicate_get_cipher returns false. We should add a warning on the dashboard disclaiming that the access tokens will not be encrypted.
Is aes-256-cbc the best cipher for this? I believe this is the closest to the original MCRYPT_RIJNDAEL_256.
How can we handle the potential backwards compatibility issues? Or is it a non-issue?

This PR will address and close #80, #144, and #150.

GaryJones

I think this is going to need unit and / integration tests to demonstrate that the encrypt/decrypt functions work on a range of strings, on a range of PHP versions.

How can we handle the potential backwards compatibility issues?

The plugin's readme.txt currently says it requires WP 3.4 - and that version of WP supports down to PHP 5.2, so ideally all of those versions would be tested in a GitHub Action workflow for the integration tests.

…ation into vip/deprecated-mcrypto � Conflicts: � tests/test-encryption.php

vaurdan · 2021-07-09T15:09:45Z

Thanks, @GaryJones! Actually writing a unit test for this functionality is a great idea, and I just added the first draft with 3 tests and 13 assertions. Let me know your thoughts.

My concern with backward compatibility is also on the plugin update action. When a user running PHP 7.0 updates to the new version of Push Syndication, they will have the data encrypted with the old cipher, and if they update PHP to 7.2 or later, it will be just garbage. We might add some code to the upgrade() method on the WP_Push_Syndication_Server class, but this migration will only be possible if the server is running PHP 7.1 or older, as it will need mcrypt to do the upgrade, as the MCRYPT_RIJNDAEL_256 cipher is not available on OpenSSL. ( my idea was to decrypt the option with mcrypt, encrypt it again with openssl, and save )

tests/test-encryption.php

…ions.

includes/push-syndicate-encryption.php

tests/test-encryption.php

vaurdan · 2021-07-19T15:29:46Z

Thanks for the feedback @GaryJones! I just went ahead and refactored the code with your suggestions. It's possible that I might have overengineered it a little bit, but I'm open to more suggestions.

I have added a few more test cases, but I was wondering if I should simplify it with smaller tests but with more test cases. What are your thoughts on that?

GaryJones

I like where it's heading, but I've left a few more comments.

re tests - smaller is better, but more focused is even better - instead of only testing push_syndicate_encrypt, test the public methods of each individual class instead to test their functionality and responses - and then test push_syndicate_encrypt() to test the logic which decides which strategy is inserted in which context.

includes/class-syndication-encryptor-mcrypt.php

includes/class-syndication-encryptor-openssl.php

includes/class-syndication-encryption.php

includes/class-syndication-encryptor.php

includes/push-syndicate-encryption.php

includes/class-syndication-encryption.php

includes/interface-syndication-encryptor.php

…class

vaurdan · 2021-07-21T14:29:11Z

@GaryJones thanks! I was a bit conflicted if I should make it not static, so your input was decisive into doing that change. For now, it's using a global variable to store the object instance ( $push_syndication_encryption ), which is not the best approach. But I added a TODO so when we refactor the code we can store it as an attribute of the WP_Push_Syndication_Server object, for example.

Regarding the filename, I haven't change it from interface-syndication-encryptor.php to class-syndication-encryptor.php for a couple of reasons: there's already an interface (Syndication_Client ) using the same name scheme, and WPCS sniff for the class filename will only run on files with the class keyword, so the interface file doesn't trigger any CS warning.

Thanks for the PHP_VERSION_ID tip, way simpler than using version_compare!

includes/class-syndication-encryption.php

tests/test-encryption.php

vaurdan · 2021-07-23T15:42:30Z

@GaryJones take a look at the changes. I changed the EncryptionTest to stop using the global. However, I left an inline question on your original comment on that matter, if you can take a look. 🙂

Thank you!

GaryJones

I think we're getting there.

tests/test-encryption.php

tests/test-encryptor-openssl.php

tests/class-encryptor-test-case.php

vaurdan · 2022-08-16T14:08:50Z

@GaryJones is there anything else missing on this one?
i think the @depends annotation was already added here.

GaryJones

One minor copypasta, and I'd otherwise love to see namespaces used for the new code, but these don't have to be a blocker.

The only consideration would be what happens if someone has data encrypted with mcrypt under PHP 7.1, then updates to PHP 7.2, and finds they can't decrypt using the OpenSSL?

includes/class-syndication-encryptor-mcrypt.php

.

GaryJones · 2024-02-11T13:53:10Z

There will be issues decrypting old data, however: if the original data was encrypted with mcrypt, it will not be compatible with openssl, as the Rijndael AES is not available on openssl and it's not compatible with aes-256-cbc (source). If there is a way to make the old encrypted data retro compatible, I'm open to suggestions.

This is still an issue.

Fix mcrypt fatal on data encryption

a844294

vaurdan requested a review from trepmal July 5, 2021 14:56

This was linked to issues Jul 5, 2021

PHP 7.2 Compat - mcrypt_decrypt() is REMOVED #144

Open

Fatal error on php 7.2 #150

Open

vaurdan mentioned this pull request Jul 6, 2021

PHPCS Fixes #160

Draft

GaryJones requested changes Jul 6, 2021

View reviewed changes

vaurdan added 9 commits July 8, 2021 17:17

Fix error with unset push_syndicate_settings

e89712c

Add initial version of a test suite for the encryption functions.

2997500

Make push_syndicate_decrypt return an array by default

671576d

Use is_string if assertIsString is not available.

f3ad7a1

Use is_string if assertIsString is not available.

4d5d0f4

Merge branch 'vip/deprecated-mcrypto' of github.com:Automattic/syndic…

9927d42

…ation into vip/deprecated-mcrypto � Conflicts: � tests/test-encryption.php

Use is_array if assertIsArray is not available.

22ae798

Only test with mcrypt if the PHP version is < 7.1

ffa394f

Extend the PHP 7.1 validation to the encrypt and decrypt functions.

1f67318

vaurdan force-pushed the vip/deprecated-mcrypto branch from 08527eb to 1f67318 Compare July 8, 2021 16:47

vaurdan requested a review from GaryJones July 9, 2021 15:09

GaryJones reviewed Jul 12, 2021

View reviewed changes

tests/test-encryption.php Outdated Show resolved Hide resolved

Use yoast/wp-test-utils for compatibility with different phpunit vers…

5024eaa

…ions.

vaurdan requested a review from GaryJones July 12, 2021 15:09

GaryJones requested changes Jul 13, 2021

View reviewed changes

includes/push-syndicate-encryption.php Outdated Show resolved Hide resolved

includes/push-syndicate-encryption.php Show resolved Hide resolved

includes/push-syndicate-encryption.php Outdated Show resolved Hide resolved

tests/test-encryption.php Outdated Show resolved Hide resolved

vaurdan added 2 commits July 19, 2021 16:04

Refactor encryption to use Syndication_Encryption class

b1690a9

Fix tests failing on PHP 7.1

02f0aa8

vaurdan force-pushed the vip/deprecated-mcrypto branch from 990143f to 02f0aa8 Compare July 19, 2021 15:22

vaurdan requested a review from GaryJones July 19, 2021 15:29

GaryJones requested changes Jul 19, 2021

View reviewed changes

vaurdan added 2 commits July 20, 2021 15:32

Move imports and initialization to plugin root file

8a4d2e7

Refactor tests to have individual tests for each encryptor

6f09b04

vaurdan requested a review from GaryJones July 20, 2021 15:48

GaryJones reviewed Jul 21, 2021

View reviewed changes

includes/class-syndication-encryption.php Outdated Show resolved Hide resolved

GaryJones reviewed Jul 21, 2021

View reviewed changes

includes/interface-syndication-encryptor.php Show resolved Hide resolved

vaurdan added 2 commits July 21, 2021 15:15

Change Syndication_Encryption from a static class to an instantiable …

fdb506a

…class

Add extra validation for older PHP versions (<5.2.7), just in case.

d418b9c

vaurdan requested a review from GaryJones July 21, 2021 14:29

GaryJones requested changes Jul 21, 2021

View reviewed changes

vaurdan added 3 commits July 23, 2021 16:16

Remove encryptor strategy getters and setters

4e3873b

Address feedback on Encryption_Test

16ec5b8

Correction on the tests PHPDOC

c9f684e

vaurdan force-pushed the vip/deprecated-mcrypto branch from 75085c9 to c9f684e Compare July 23, 2021 15:39

vaurdan requested a review from GaryJones July 23, 2021 15:42

GaryJones previously requested changes Jul 24, 2021

View reviewed changes

tests/test-encryption.php Outdated Show resolved Hide resolved

Change EncryptionTest to test the encrypt and decrypt methods.

1f6b87c

vaurdan force-pushed the vip/deprecated-mcrypto branch from 4bd2285 to 1f6b87c Compare July 28, 2021 16:06

vaurdan requested a review from GaryJones July 28, 2021 16:11

GaryJones reviewed Jul 29, 2021

View reviewed changes

tests/test-encryptor-openssl.php Show resolved Hide resolved

tests/class-encryptor-test-case.php Show resolved Hide resolved

vaurdan requested a review from GaryJones August 16, 2022 14:08

GaryJones reviewed Aug 16, 2022

View reviewed changes

includes/class-syndication-encryptor-mcrypt.php Outdated Show resolved Hide resolved

Fix DocBlock

9a3434d

GaryJones merged commit 3739533 into master Feb 11, 2024
0 of 40 checks passed

GaryJones deleted the vip/deprecated-mcrypto branch February 11, 2024 11:43

GaryJones restored the vip/deprecated-mcrypto branch February 11, 2024 11:45

GaryJones mentioned this pull request Feb 11, 2024

Fix mcrypt fatal (on develop branch) #164

Merged

GaryJones deleted the vip/deprecated-mcrypto branch February 11, 2024 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mcrypt fatal - Removed on PHP 7.2 #159

Fix mcrypt fatal - Removed on PHP 7.2 #159

vaurdan commented Jul 5, 2021 •

edited

Loading

GaryJones left a comment

vaurdan commented Jul 9, 2021 •

edited

Loading

vaurdan commented Jul 19, 2021

GaryJones left a comment

vaurdan commented Jul 21, 2021

vaurdan commented Jul 23, 2021 •

edited

Loading

GaryJones left a comment

vaurdan commented Aug 16, 2022

GaryJones left a comment

GaryJones commented Feb 11, 2024

Fix mcrypt fatal - Removed on PHP 7.2 #159

Fix mcrypt fatal - Removed on PHP 7.2 #159

Conversation

vaurdan commented Jul 5, 2021 • edited Loading

Testing

TODO/Questions

GaryJones left a comment

Choose a reason for hiding this comment

vaurdan commented Jul 9, 2021 • edited Loading

vaurdan commented Jul 19, 2021

GaryJones left a comment

Choose a reason for hiding this comment

vaurdan commented Jul 21, 2021

vaurdan commented Jul 23, 2021 • edited Loading

GaryJones left a comment

Choose a reason for hiding this comment

vaurdan commented Aug 16, 2022

GaryJones left a comment

Choose a reason for hiding this comment

GaryJones commented Feb 11, 2024

vaurdan commented Jul 5, 2021 •

edited

Loading

vaurdan commented Jul 9, 2021 •

edited

Loading

vaurdan commented Jul 23, 2021 •

edited

Loading