Skip to content

Commit

Permalink
fix(mfa): Prevent reuse of TOTP codes
Browse files Browse the repository at this point in the history
  • Loading branch information
pennersr committed Feb 7, 2024
1 parent ad89388 commit 48a661a
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 1 deletion.
8 changes: 8 additions & 0 deletions ChangeLog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@ Note worthy changes
originated, will be emailed.


Security notice
---------------

- MFA: It was possible to reuse a valid TOTP code within its time window. This
has now been addressed. As a result, a user can now only login once per 30
seconds (``MFA_TOTP_PERIOD``).


Backwards incompatible changes
------------------------------

Expand Down
43 changes: 43 additions & 0 deletions allauth/mfa/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import django
from django.conf import settings
from django.core.cache import cache
from django.test import Client
from django.urls import reverse

import pytest
Expand Down Expand Up @@ -301,3 +303,44 @@ def test_cannot_deactivate_totp(auth_client, user_with_totp, user_password):
assert resp.context["form"].errors == {
"__all__": [get_adapter().error_messages["cannot_delete_authenticator"]],
}


def test_totp_code_reuse(
user_with_totp, user_password, totp_validation_bypass, enable_cache
):
for code, time_lapse, expect_success in [
# First use of code, SUCCESS
("123", False, True),
# Second use, no time elapsed: FAIL
("123", False, False),
# Different code, no time elapsed: SUCCESS
("456", False, True),
# Again, previous code, no time elapsed: FAIL
("123", False, False),
# Previous code, but time elapsed: SUCCESS
("123", True, True),
]:
if time_lapse:
cache.clear()
client = Client()
resp = client.post(
reverse("account_login"),
{"login": user_with_totp.username, "password": user_password},
)
assert resp.status_code == 302
assert resp["location"] == reverse("mfa_authenticate")
# Note that this bypass only bypasses the actual code check, not the
# re-use check we're testing here.
with totp_validation_bypass():
resp = client.post(
reverse("mfa_authenticate"),
{"code": code},
)
if expect_success:
assert resp.status_code == 302
assert resp["location"] == settings.LOGIN_REDIRECT_URL
else:
assert resp.status_code == 200
assert resp.context["form"].errors == {
"code": [get_adapter().error_messages["incorrect_code"]]
}
18 changes: 17 additions & 1 deletion allauth/mfa/totp.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from io import BytesIO
from urllib.parse import quote

from django.core.cache import cache
from django.utils.http import urlencode

import qrcode
Expand Down Expand Up @@ -104,5 +105,20 @@ def deactivate(self):
self.instance.delete()

def validate_code(self, code):
if self._is_code_used(code):
return False

secret = decrypt(self.instance.data["secret"])
return validate_totp_code(secret, code)
valid = validate_totp_code(secret, code)
if valid:
self._mark_code_used(code)
return valid

def _get_used_cache_key(self, code):
return f"allauth.mfa.totp.used?user={self.instance.user_id}&code={code}"

def _is_code_used(self, code):
return cache.get(self._get_used_cache_key(code)) == "y"

def _mark_code_used(self, code):
cache.set(self._get_used_cache_key(code), "y", timeout=app_settings.TOTP_PERIOD)

0 comments on commit 48a661a

Please sign in to comment.