Mcrypt is required, but it is deprecated in PHP 7.1+

question, can I move to PHP71 even though this deprecated error still exists?

thanks

The patch doesn't work for me. It always uses decryptWithMCrypt even when the mcrypt extension is not loaded. I think the problem could be here:

    $version_prefix = self::CRYPT_VERSION . '|';
    $version_match = strpos($data, $version_prefix) === 0;

Could you please look into that?

Has libsodium been considered?

I was able to produce a patch which uses OpenSSL to decrypt data that was originally encrypted with MCrypt.

Libsodium may be worth considering, I lean towards not using it (but don't have a strong opinion). Presumably, data encrypted by OpenSSL can be decrypted by Sodium down the road if there is a desire to make the switch. I used OpenSSL because:

1. The existing patch uses it
2. OpenSSL is readily available for PHP 5 and 7 implementations.
3. The documentation on php.net for Sodium is incomplete.
4. I am unfamiliar with Sodium.

This patch will decrypt with OpenSSL (instead of MCrypt) if both extensions are available. There is no particular reason why, other than it addresses #13 and may aid in testing/verification. Depending on compatibility with obsolete versions of PHP (e.g. I haven't tested 5.3), it might be worth using MCrypt over OpenSSL when decrypting legacy data.

The MCrypt extension was removed from PHP 7.2. This patch should make the TFA plugin compatible with PHP 7.2 while remaining backwards-compatible with older versions of PHP, as well as sites which upgrade from an older version (and already have encrypted data).

It may or may not be not be a lot of work to re-implement my patch with Sodium - I don't know.

The patch I uploaded for #16 is malformed - the "interdiff" utility won't even work with it. I guess I should have known better than to manually edit it before upload. I've re-generated the patch to try again.

diff tfa-2820710-16.patch tfa-2820710-18.patch

20c20
< index bb9ab82..88f1760 100644
---
> index bb9ab82..a063360 100644
116c116
< @@ -551,6 +609,64 @@ abstract class TfaBasePlugin {
---
> @@ -551,6 +609,65 @@ abstract class TfaBasePlugin {

Hi,

We're having trouble with this patch where random users are not prompted to enter their 2FA details on login. They are logged straight in.

The error displayed is:

Warning: openssl_decrypt(): IV passed is only 13 bytes long, cipher expects an IV of precisely 16 bytes, padding with \0 in TfaBasePlugin->decrypt() (line 581 of /var/www/vm7/web/sites/all/modules/contrib/tfa/tfa.inc

The number 13 varies.

I believe it's down to the iv or formatting of the iv with the data before it's stored.

I believe the iv is being generated with a pipe symbol ('|') and then stored so you end up with

$data = "1|iv|here|data here";
list(, $iv, $data) = explode('|', $data, 3);
$iv == 'iv';
$data == 'here|data here';

Where you should have:

$data = "1|iv here|data here";
list(, $iv, $data) = explode('|', $data, 3);
$iv == 'iv here';
$data == 'data here';

Quick back of a napkin script shows drupal_random_bytes() returns the pipe symbol in ~6% of cases for me.

Perhaps the iv needs to be base64 encoded before storing, something like that. Or perhaps a quick solution would be retry drupal_random_bytes() until one is generated without a pipe symbol?

Thanks for the feedback. I don't think base64 encoding the IV is the right way forward, since existing encryptions don't use it. I've updated my patch to ensure that the IV length is always 16. I've done this by creating additional methods that parse the encrypted storage string by using a regex. Please review / give it a try. I haven't tested it so hopefully everything works :).

Sorry, my patch had the wrong regex.. updated it.

Thanks for taking a look. I'm getting a similar error (but different):

Warning: openssl_decrypt(): IV passed is 32 bytes long which is longer than the 16 expected by selected cipher, truncating in TfaBasePlugin->decrypt() (line 658 of /var/www/bbi.vm7/web/sites/all/modules/contrib/tfa/tfa.inc).

I think the getIvFromEncryptedStorageText() and getDataToDecryptFromEncryptedStorageText() are looking in the wrong positions in the array.

getIvFromEncryptedStorageText() should be looking in $matches[2] and getDataToDecryptFromEncryptedStorageText() in $matches[3] (there is no $matches[4])

Changing this seems to resolve it for me, at least the error disappears and I am prompted for the verification code now.

Regexp does not always work.
I tested with the following snippet using drush:
$iv = drupal_random_bytes(16); $data = drupal_random_bytes(32); $encrypted = "1|" . $iv . "|" . $data; var_dump($encrypted); preg_match("/^\d+\|(.{16})\|(.*)\$/m", $encrypted, $matches); var_dump($matches);
and occasionally it got me no matches.

The following code I did not see fail so far:
$iv = drupal_random_bytes(16); $data = drupal_random_bytes(32); $encrypted = "1|" . $iv . "|" . $data; var_dump($encrypted); $version_cutoff = strpos($encrypted, "|"); $decoded_iv = substr($encrypted, $version_cutoff + 1, 16); var_dump($decoded_iv); $decoded_data = substr($encrypted, $version_cutoff + 18); var_dump($decoded_data);

Good spot, yes I see the same. I believe it's down to drupal_random_bytes() returning newline characters, and by default '.' in the regex does not include them. Adding the 's' modifier e.g. '/^(\d+\|){0,1}(.{16})\|(.*)$/s' and I can't get it to fail after ~1000 attempts.

Having said that, using substr is likely to be quicker though anyway? Only edge case I can think of is *if* http://php.net/manual/en/mbstring.overload.php is in use, it might return the wrong number of chars.

I'm thinking we don't need a nonstandard data format that requires a nontrivial regex. I've rerolled the patch using JSON encoding to simplify. If the JSON parser doesn't return something that matches expectations, then use a decryptLegacyData method. It also makes it easy to extend the format to store additional data fields (should the need arise in the future).

I've modified my patch to use base64 encoding, since I can't json_encode the IV/ciphertext as-is.

#29 worked for me with PHP 7.1.20

patch #29 worked for me with PHP 7.2.

Worked for me also

I can also confirm that #29 works with PHP 7.1.

Thanks!

How do we handle already encrypted seeds with mcrypt?

Patch #39 is generating the following error:

Warning: substr() expects parameter 3 to be integer, string given in TfaBasePlugin->decryptLegacyDataWithOpenSSL() (line 640 of /srv/bindings/dba848bb571549fca85a165e4be9c31f/code/sites/all/modules/tfa/tfa.inc).

Invalid application code. Please try again.

Patch #29 works perfectly, however.

Hi, I've tried patch #29 and it is working (by which I mean that I can set up and use 2FA; I don't have the skill to assess it for security in regard to how it does encryption).

Tested with Drupal 7.63 and PHP 7.2

Patch #42 works great. Thank you @pjcdawkins.

Patch looks good to me and works, thanks a lot!

Since the module completely breaks on PHP 7.2 this is critical.

FYI the ?: syntax requires PHP 5.3, so I adjusted that part to be compatible with PHP 5.2. Otherwise, this patch is identical to #42.

I believe the patch in #51 will not work identically to #42, based on the interdiff. I wrote some quick-and-dirty code to check my hesitation.

function withTernary($value = null) {
  $value = $value ?: "assigned by function";
  return $value;
}
echo "withTernary called null: " . withTernary(null) . PHP_EOL;
echo "withTernary called already set: " . withTernary("assigned by caller") . PHP_EOL;


function withoutTernary($value = null) {
  if (!is_null($value)) {
    $value = "assigned by function";
  }
  return $value;
}
echo "withoutTernary called null: " . withoutTernary(null) . PHP_EOL;
echo "withoutTernary called already set: " . withoutTernary("assigned by caller") . PHP_EOL;

$ php testternary.php
withTernary called null: assigned by function
withTernary called already set: assigned by caller
withoutTernary called null: 
withoutTernary called already set: assigned by function

$ php --version
PHP 7.2.19-0ubuntu0.18.04.1 (cli) (built: Jun  4 2019 14:48:12) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
    with Zend OPcache v7.2.19-0ubuntu0.18.04.1, Copyright (c) 1999-2018, by Zend Technologies
    with Xdebug v2.6.0, Copyright (c) 2002-2018, by Derick Rethans

So would a !empty() be better?

I agree empty is a little better to use in this situation. However, the result with #53 is the same as #51 (and different from #42).

$ php testternary2.php 
withTernary called null: assigned by function
withTernary called already set: assigned by caller
withoutTernary called null: 
withoutTernary called already set: assigned by function

It looks like the negation is throwing off the result. If you delete the "!" in the if statement, the result becomes consistent with #42:

withTernary called null: assigned by function
withTernary called already set: assigned by caller
withoutTernary called null: assigned by function
withoutTernary called already set: assigned by caller

I've attached an updated patch and interdiff (removing the "!" from #53's if statement).

bump!

Acquia is forcing php7.2 on October the first. At that time, mcrypt will no longer be available. It would be great to see this merged in with a release version before then.

I tested in Acquia with PHP 7.2 enabled. We are also using tfa_duo. I confirmed that I was able to use TFA to login and that the status message on admin/reports/status no longer displayed warning about mcrypt.

Hi, I've just upgraded my server to php 7.2 - and moved the tfa module from the release version to the dev version in order to apply this patch. I'm now getting an alert that I need to update to the beta3 version... Am I correct in thinking that this patch will only work with the dev version? Is the dev version secure?

thanks @pjcdawkins. I've patched the 2.0 version and all looks good! The only concern really is why the dev version was being flagged as needing a security update - not a big issue, but I thought you should be aware.

I'm trying the patch at #54 with the TFA Basic authenticators, and getting this error upon trying to validate with Authy:

Error: Undefined class constant 'CRYPT_VERSION' in TfaBasePlugin->encrypt() (line 520 of www/html/docroot/sites/all/modules/tfa/tfa.inc).

Am I missing something? Is #54 an insufficient fix without using prior patches in this thread?

Error: Undefined class constant 'CRYPT_VERSION' in TfaBasePlugin->encrypt() (line 520 of www/html/docroot/sites/all/modules/tfa/tfa.inc).

@Rory: The tfa module of version 2.0 does not have line 520 at all. You must be using a wrong version of the module for patching.

I've just try patching tfa version 2.0 with the patch and it works fine.

Is there anything blocking the merge? PHP 7.4 has been out and PHP 7.1 has just dropped out of support. I think its critical to have the module patch up as soon as possible. Is there anything that needs help here?

+1

For anyone who needs it, you can manually add mcrypt onto PHP 7.x using this pecl package: https://pecl.php.net/package/mcrypt

Same problem with PHP 7.2, will there be an update soon?

Just tested #54 and all seems to be working properly on environment with PHP 7.3 without mcrypt.

1. +++ b/tfa.inc
   @@ -502,12 +504,41 @@ abstract class TfaBasePlugin {
   +  protected function encryptWithMCrypt($text, $iv = null) {
   ...
   +    if (empty($iv)) {
   +      $iv = drupal_random_bytes(mcrypt_enc_get_iv_size($td));
   

   I don't see the need for this $iv parameter. The code only calls this function once, and when it does, it only passes the first parameter.

2. +++ b/tfa.inc
   @@ -528,10 +559,42 @@ abstract class TfaBasePlugin {
   +    $is_legacy = TRUE;
   ...
   +    if (!empty($crypto_data['version']) && !empty($crypto_data['iv_base64']) && !empty($crypto_data['ciphertext_base64'])) {
   +      $is_legacy = FALSE;
   +    }
   +    // Backwards compatibility with the old MCrypt scheme.
   +    if ($is_legacy === TRUE) {
   +      if (extension_loaded('openssl')) {
   +        return $this->decryptLegacyDataWithOpenSSL($data);
   +      }
   +      if (extension_loaded('mcrypt')) {
   +        return $this->decryptLegacyDataWithMCrypt($data);
   +      }
   +      return FALSE;
   +    }
   

   This is so convoluted.

   Why not simply negate the complete if conditions, and move the content of the $is_legacy block inside the if? Way more easy to read the code.

3. +++ b/tfa.inc
   @@ -551,6 +614,36 @@ abstract class TfaBasePlugin {
   +    // Using 3 instead of the constant OPENSSL_NO_PADDING, for PHP 5.3.
   +    $options = 3;
   +    $decrypted_text = openssl_decrypt($data, 'AES-256-CBC', $key, $options, $iv);
   

   No need to set the $options variable. Simply set 4th parameter to 3.

And some PHPDoc nitpicks:

1. +++ b/tfa.inc
   @@ -502,12 +504,41 @@ abstract class TfaBasePlugin {
   +   * @param string $text
   +   *
   +   * @return string
   +   */
   +  protected function encryptWithMCrypt($text, $iv = null) {
   

   Missing the parameter and return descriptions.

2. +++ b/tfa.inc
   @@ -528,10 +559,42 @@ abstract class TfaBasePlugin {
   +   * @param string $data
   +   *
   +   * @return string|boolean
   +   *   The plaintext, or FALSE on failure.
   +   */
   +  protected function decryptLegacyDataWithMCrypt($data) {
   

   Missing the parameter description.

3. +++ b/tfa.inc
   @@ -551,6 +614,36 @@ abstract class TfaBasePlugin {
   +   * @param string $data
   +   *
   +   * @return string|boolean
   +   *   The plaintext, or FALSE on failure.
   +   */
   +  protected function decryptLegacyDataWithOpenSSL($data) {
   

   Missing the parameter description.

Was the patch in #54 ever tested against legacy mcrypt data? I'm adapting this code to the Drupal 8 module, and reduced the decryptLegacyDataWithOpenSsl() function to:

  private function decryptLegacyDataWithOpenSsl($text, $key) {
    $key = substr($key, 0, 32);
    $text = base64_decode($text);

    return openssl_decrypt($text, 'aes-128-cbc', $key, OPENSSL_NO_PADDING);
  }

#54 works fine on legacy mcrypt data. I have patched sites running PHP 7.3 where Google Authenticator was setup while running the un-patched version and the users can still login.

While your fixes might clean the code up, the code in patch 54 has been reviewed and works fine as it has been tested by many in the community. Therefore changing back to "Reviewed and tested by the community"

I would rather have working code that is a little ugly and missing some documentation than code that doesn't work. I have servers running PHP 7.4 now and we are still stuck patching to get PHP 7.1 to work...

#68 to #70 are important. #70 is critical. The code to decode an existing Mcrypt(ed) TFA seed simply doesn't work.

BTW, I'm maintaining this module.. Don't set it back to RTBC in the hope a maintainer will then commit it faster. A maintainer has reviewed it, and found that it needs work.

Working on the suggested improvements.

Improvements per #68, #69.

I think the extra error handling in decryptLegacyDataWithOpenSsl($text, $key) is important for backwards compatibility?

$decrypted_text = openssl_decrypt($data, 'AES-256-CBC', $key, 3, $iv);

This line doesn't decrypt data encrypted with 'rijndael-128', at least not in Drupal 8. It does work if it is like this:

openssl_decrypt($text, 'aes-128-cbc', $key, 3);

Does it matter of the $method passed to openssl_decrypt() is uppercase or lowercase, the PHP documentation wasn't clear about it.

Is there a downside to not passing the $iv?

I think the problem is the 256/128, not the case.

I'm trying the code with some data I have in an archive of a D7 site to figure out why this code seems to have been validated, even if it clearly didn't work on my D8 tests.

OK.. Indeed, it must be 'aes-256-cbc'. Something must be really different between the way that D7's tfa encrypts stuff, and D8's. I'm surprised by that, but this answers my question in #70.

1. +++ b/tfa.inc
   @@ -551,6 +614,37 @@ protected function decrypt($data) {
   +    if ($decrypted_text === FALSE) {
   +      return FALSE;
   +    }
   

   This is not necessary, strpos(FALSE, '|') is FALSE. And this function returns strings, not boolean, so it should return an empty string ("").

2. +++ b/tfa.inc
   @@ -551,6 +614,37 @@ protected function decrypt($data) {
   +    if (strpos($decrypted_text, '|') !== FALSE) {
   +      list($length, $padded_data) = explode('|', $decrypted_text, 2);
   +      $decrypted_text = substr($padded_data, 0, $length);
   +    }
   +
   +    return $decrypted_text;
   

   The original code returns an empty string if $decrypted_text does not contain a '|'. This is returning the full decrypted string. Move the return inside the if, and add a else return "".

OK. Doing the changes from #79, and adapting all decrypt functions to document that an empty string is returned on failure, as per the existing code.

Also, it seems that since OpenSSL 1.1.1, openssl_get_cipher_methods() returns only lowercased versions of the cipher methods, so changing that. Also, Mcrypt's name is not MCrypt (see https://www.php.net/manual/en/book.mcrypt.php)

I'll probably commit this soon, and create a 2.1-rc1 release. It would be highly appreciated if this could be moved back to RTBC before that :)

+++ b/tfa.install
@@ -27,13 +27,19 @@ function tfa_uninstall() {
+        $description = t('The TFA module recommends the PHP OpenSSL extension to be installed on the web server.');
...
+        $description = t('The TFA module requires either the PHP OpenSSL or Mcrypt extensions to be installed on the web server.');

Must be $t()

Adding an interdiff to #54, to be easier to review for those that have been using it during this time.

Patch in #83 applied cleanly to 7.x-2.x-dev but fails for 7.x-2.0 (not unexpected).

I have a local development install working cleanly with the patch in #83.

I've analysed the interdiff between #54 and #83 several times now. Other than the MCrypt->Mcrypt refactors, and changes to the code comments, the only meaningful change is that the decrypt functions now return an empty string instead of a FALSE. As the existing code behaves this way, this improves compatibility with modules providing TFA plugins.

Setting to RTBC as per #55.

I'm setting this issue to "Needs Review" due to warnings @edvanleeuwen reported in a different issue: https://www.drupal.org/project/tfa_basic/issues/2916011

Deprecated function: Function mcrypt_enc_get_iv_size() is deprecated in TfaBasePlugin->decryptLegacyDataWithMcrypt()
Deprecated function: Function mcrypt_module_open() is deprecated in TfaBasePlugin->decryptLegacyDataWithMcrypt()

The patch I originally created used the openssl extension primarily, and only would make use of mcrypt as a last resort. Same with #54.

Was the patch in #54 ever tested against legacy mcrypt data?

On my site, from what I recall, yes. Or maybe it was the patch I provided in #16. It's been a long time.

The code to decode an existing Mcrypt(ed) TFA seed simply doesn't work.

Are you sure that #54 that didn't work for your use case? Or were you testing this use case after modifying the code? It was reported that legacy data was decrypting.

I've analysed the interdiff between #54 and #83 several times now. Other than the MCrypt->Mcrypt refactors, and changes to the code comments, the only meaningful change is that the decrypt functions now return an empty string instead of a FALSE.

From what I see of interdiff_54_83.txt, this comment doesn't mention the change to make mcrypt be preferred over openssl. My intent was for mcrypt to only be used as a last resort.

@nullkernel: The TFA_Basic module also needs a patch to resolves its reliance upon mcrypt when using the Google Authenticator submodule.

@nullkernel, the mcrypt is only used as 'preferred' for decryption if it is installed. OpenSSL is still the preferred way of doing new encryptions. It is a matter of making sure that the code is as compatible as can be with before: if mcrypt is installed, it gets used for decryption, same as before, with less risk of a decryption problem occurring in case OpenSSL does something slightly different. I understand this will make some warnings show up, probably only in PHP 7.0-7.1 (both of them already EOL).

As I said in #79, I then tested #54 and it does work with 'aes-256-cbc', and not with 'aes-128-cbc'. At issue here was that the code in D8 only worked with 128, and I thought the data was being encrypted here the same way. I was wrong, as demonstrated by my tests.

I do not think that this was fixed as when running the PHP Compatibility using Drupal core 7.91, PHP 8.0.21 and TFA dev version I get the following:
FILE: tfa/tfa.inc
-------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 29 ERRORS AFFECTING 15 LINES
-------------------------------------------------------------------------------------------------------------------------------------------------------
647 | ERROR | Function mcrypt_module_open() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
647 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
648 | ERROR | Function mcrypt_enc_get_iv_size() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
648 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
650 | ERROR | Function mcrypt_enc_get_key_size() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
650 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
652 | ERROR | Function mcrypt_generic_init() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
652 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
656 | ERROR | Function mcrypt_generic() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
656 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
658 | ERROR | Function mcrypt_generic_deinit() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
658 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
659 | ERROR | Function mcrypt_module_close() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
659 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
704 | ERROR | Function mcrypt_module_open() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
704 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
705 | ERROR | Function mcrypt_enc_get_iv_size() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
705 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
707 | ERROR | Function mcrypt_enc_get_iv_size() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
707 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
708 | ERROR | Function mcrypt_enc_get_key_size() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
708 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
710 | ERROR | Function mcrypt_generic_init() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
710 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
712 | ERROR | Function mdecrypt_generic() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
717 | ERROR | Function mcrypt_generic_deinit() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
717 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
718 | ERROR | Function mcrypt_module_close() is deprecated since PHP 7.1 and removed since PHP 7.2; Use OpenSSL instead
718 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead

Added an annotation so that PHPCompatibility checks no longer provide false-positive warnings for the code using the optional mcrypt extension.

See https://git.drupalcode.org/project/tfa/-/commit/312cc7c4d742bcbb0c8f2909...

It it normal that I am still getting the following error with Two-factor Authentication (TFA) 7.x-2.2 on Drupal 7.92 and PHP 7.4.32 ?

Warning: substr() expects parameter 3 to be int, string given in TfaBasePlugin->decryptLegacyDataWithOpenSSL() (line 752 of /sites/all/modules/tfa/tfa.inc).

Not sure if it is relevant, but I have only detected this with the admin user (of course admin's TFA was enabled before mcrypt was deprecated) but maybe some other users are affected too (not sure as it does not seem very easy to replicate it since this probably happens only with users with an old encryption key). If it is only the admin who is in this situation, it is obviously not a problem as I suppose that it can be easily fixed by generating new verification codes. But so far I have not done so in order to be capable of tracking the issue just in case I become aware that some user is still affected besides the admin.

Hi @ezilo,

Since this issue is closed, can you please create a new one and copy+paste your message above into it?

Your problem makes sense, and can be trivially fixed with casting the 3rd parameter to a number.

Sure @jcnventura ,

New issue opened:
https://www.drupal.org/project/tfa/issues/3320552

#83 patch doesn't work for me.

patching file tfa.admin.inc
Reversed (or previously applied) patch detected! Assume -R? [n]

Probably have to remove it from the 4 sites we host that use it.

@mchaplin: that means the fix is already included in the version you downloaded, so you don't need to apply it.