To avoid false positives weak algorithms should not be used (see https://www.drupal.org/node/845876)

Affected code:

docroot/modules/contrib/tfa/src/Plugin/TfaBasePlugin.php 209
docroot/modules/contrib/tfa/src/Plugin/TfaBasePlugin.php 226

Solution:

use Drupal’s hashBase64 methods:

\Drupal\Component\Utility\Crypt::hashBase64($data)
\Drupal\Component\Utility\Crypt::hmacBase64($data, $key)

in those files affected (see description)

Create a patch for the files mentioned in the description, share in d.o and use the patch from d.o url.

CommentFileSizeAuthor
#3 weak_algorithms-3069417-3.patch1.29 KBsergiu stici

Comments

alexmoreno created an issue. See original summary.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Thanks, @alexmoreno - could you perhaps provide a patch? Are there any considerations to keep in mind for a site that has TFA installed already and will be upgrading to this new hash?

sergiu stici’s picture

sergiu stici commented
Status: Active » Needs review
StatusFileSize
new weak_algorithms-3069417-3.patch1.29 KB

The old hash will not change, but i don't know if it will affect something.

omkar06’s picture

omkar06 commented
Status: Needs review » Reviewed & tested by the community

Before Applying patch, TFA enabled and configured for the admin user.
Login was working perfectly after submitting Google Authenticator's code.

Patch applying patch, tested same user login. After submitting Google Authenticator's code, the user was able to log in without any issue.
As the patch is working fine, marking issue to RTBC.

alexmoreno’s picture

alexmoreno commented

confirming as well, thanks for the patch @Sergiu Stici

jcnventura’s picture

jcnventura
Primary language English
commented 
  1. +++ b/src/Plugin/TfaBasePlugin.php
@@ -206,7 +207,7 @@ abstract class TfaBasePlugin extends PluginBase {
-    $hash = hash('sha1', Settings::getHashSalt() . $code);
+    $hash = Crypt::hashBase64(Settings::getHashSalt() . $code);

    This is fine. 

  2. +++ b/src/Plugin/TfaBasePlugin.php
@@ -223,7 +224,7 @@ abstract class TfaBasePlugin extends PluginBase {
-    $hash = hash('sha1', Settings::getHashSalt() . $code);
+    $hash = Crypt::hashBase64(Settings::getHashSalt() . $code);

    There is a brief tine window where this code can result in a successful replay attack just after installing the new version of this module.

    On the other hand, this is an alpha version of the module. There's no real guarantee of being secure.

  • Sergiu Stici authored c39e249 on 8.x-1.x 
    Issue #3069417 by Sergiu Stici, alexmoreno, jcnventura, greggles,...
jcnventura’s picture

jcnventura
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.