This was initially posted as a private security issue (#179138) by @cmlara → , but it's been agreed that it can be a public security hardening.
---------------
The TFA Basic plugins module has multiple vulnerabilities related to its handling of previously used tokens leading to possible access bypass through a replay attack.
Vulnerability #1:
The TfaTotp::alreadyAcceptedCode() method is coupled to the site hash salt through drupal_get_hash_salt(), should the site hash_salt change the function is no longer able to determine if a code has been previously utilized. The hash_salt is a value that is likely to be changed if a site believes a compromise may have occurred rendering the alreadyAcceptedCode() less effective when it is most needed.
Steps to reproduce:
Setup tfa_basic per the instructions, ensure the TOTP plugin is enabled.
Create a new test user, ensure they have a role with the 'Set up TFA for account' permission:
Login as the test user, setup the TOTP plugin and logout
Login with test user and enter the TOTP code, retain the code value.
Logout as the test user.
Adjust the $drupal_hash_salt and clear all caches
Login as the test user using the previously used code. Note adjusting tfa_basic_time_skew may be helpful to allow the tokens to be valid longer.
Notice that login is allowed.
Logout and login again with the same code, login is rejected this time because the previous login saved it under the new salt.
Mitigating factors:
An attacker must obtain a valid first factor credential.
An attacker must obtain a previously used second factor credential.
A site must have changed its hash salt after the second factor was used.
It appears only the TfaTotp plugin is vulnerable.
Vulnerability #2:
tfa_recovery and tfa_totp, can be subject to a race condition between the time a code is validated and the time the code is marked used where a second use of the same code may be allowed.
I haven't fully reviewed the sms plugin, however it appears to me it may be safe since it may be per form load for code generation.
This vulnerability requires more visual inspection as writing a test case is hard to do as it is a race condition dependent upon execution between two checks of the existing code being done before the list of valid codes is updated.
Mitigating factors:
An attacker must obtain a valid first factor credential.
An attacker must obtain a second factor credential as it is being used and replay prior to the original request reaching completion.
-----------------------
Comment #7 (@poker10)
-----------------------
Uploading an initial patch (without tests).
I have backported these changes for the Vulnerability #1:
1) Added a new setting to be used for holding older salt hashes
3) Moved to storing new used codes without utilizing the site salt + added a check for historical hash keys
I have backported changes for the Vulnerability #2 - added locking to the tfa_recovery and tfa_totp.
There is one difference with the D10 TFA patch and that is that in tfa_recovery I needed to save used code directly in the TfaBasicRecoveryCode::validate() method, because otherwise the lock would not have any effect, as by default the code was saved only in tfa_form_submit() later (where the $tfa->finalize() call is). See the addition:
@@ -118,9 +123,13 @@ protected function validate($code) {
$this->usedCode = $id;
+ $this->finalize();
+ lock_release($recovery_validation_lock_id);
+ $this->usedCode = NULL;
--------
I have not backported this change for the Vulnerability #1:
2) Modify the TOTP plugin to store the last accepted timestamp and validate that the code utilized is not older than the last accepted key.
This change is using $this->auth->otp->checkHotpResync() in D10 version of the patch and the library christian-riesen/otp. Not sure if we should add all this and use a similar solution, or we just skip this part in the tfa_basic module? Any suggestions here?
-----------------------
Comment #8 (@cmlara)
-----------------------
Not sure if we should add all this and use a similar solution, or we just skip this part in the tfa_basic module? Any suggestions here?
To my knowledge I've always seen this enforced as a presented TOTP code couldn't be older than the last accepted code though I haven't tested against Google.com or similar recently to validate if they enforce this.
That said this is indeed one of the least damaging portions of the vulnerability tree as a true 'replay' attack doesn't occur and it generally has a very limited attack window.
The RFC's are a bit vague when it comes to this, HOTP says the counter must always be increasing, and TOTP says one must use the the HOTP formula as the base. Arguably that means that this should be implemented to comply with the RFC itself. yet ironically none of the PHP libraries I've looked at seem to provide an easy method to enforce this (though I've seen several feature requests for the libraries to do so)
Glancing the library tfa_basic uses, it appears similar can be done with the $currentTimeSlice parameter.
This would look something like:
$this->isValid = FALSE;
$seed = $this->getSeed();
if (!$seed) {
return FALSE;
}
// Get the time when we start to avoid the counter moving.
$start_slice = floor($time / 30 );
for ($i = -$this->timeSkew; $i <= $this->timeSkew; ++$i) {
$time_slice = $start_slice + $i;
// Validate only a single code at a specific time_slice so we know where in the window the token is.
if ($this->ga->verifyCode($seed, $code, 0, $time_slice)) {
$last_used_time_slice = function_to_get_last_used_time_slice();
if ($last_used_time_slice >= $time_slice) {
// Code is valid, however it is older than the last code we accepted.
return FALSE;
}
function_to_store_last_used_time_slice($time_slice);
$this->isValid = TRUE;
}
return $this->isValid;
}
a bit of that is basically a copy from the GoogleAuthenticator library merged with the tfa_basic modules existing logic.
I suppose a large part of this comes down to balance. It appears doable, with the biggest question being how significant an impact is it to store/retrieve the codes which I'm not sure I'm the best to opine for that regarding D7 install base, especially for sites like D.O. which are using the module. I'm assuming this needs another Database table which means an install update hook which I know is tended to be avoided where possible.
While the security engineer in me would like to see this make it into the security fix, if you deem it as 'too significant an impact' I won't object to it being pushed to a public followup issue.
Needs review
1.0
Code
It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.