Skip to content

Commit

Permalink
fix(account): Account enumeration timing attack
Browse files Browse the repository at this point in the history
  • Loading branch information
pennersr committed Nov 30, 2024
1 parent 0482b62 commit e01cc5f
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 7 deletions.
7 changes: 7 additions & 0 deletions ChangeLog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ Note worthy changes
- Added support for TOTP code tolerance (see ``MFA_TOTP_TOLERANCE``).


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

- Authentication by email/password was vulnerable to account enumeration by
means of a timing attack. Thanks to Julie Rymer for the report and the patch.


65.2.0 (2024-11-08)
*******************

Expand Down
19 changes: 14 additions & 5 deletions allauth/account/auth_backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ def authenticate(self, request, **credentials):
if app_settings.AUTHENTICATION_METHOD == AuthenticationMethod.EMAIL:
ret = self._authenticate_by_email(**credentials)
elif app_settings.AUTHENTICATION_METHOD == AuthenticationMethod.USERNAME_EMAIL:
ret = self._authenticate_by_email(**credentials)
ret = self._authenticate_by_email(
**credentials, time_attack_mitigation=False
)
if not ret:
ret = self._authenticate_by_username(**credentials)
else:
Expand Down Expand Up @@ -45,17 +47,24 @@ def _authenticate_by_username(self, **credentials):
if self._check_password(user, password):
return user

def _authenticate_by_email(self, **credentials):
def _authenticate_by_email(
self, time_attack_mitigation: bool = True, **credentials
):
# Even though allauth will pass along `email`, other apps may
# not respect this setting. For example, when using
# django-tastypie basic authentication, the login is always
# passed as `username`. So let's play nice with other apps
# and use username as fallback
email = credentials.get("email", credentials.get("username"))
if email:
for user in filter_users_by_email(email, prefer_verified=True):
if self._check_password(user, credentials["password"]):
return user
password = credentials["password"]
users = filter_users_by_email(email, prefer_verified=True)
if users:
for user in users:
if self._check_password(user, password):
return user
elif time_attack_mitigation:
get_user_model()().set_password(password)
return None

def _check_password(self, user, password):
Expand Down
33 changes: 33 additions & 0 deletions allauth/account/tests/test_auth_backends.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
from unittest.mock import patch

from django.contrib.auth import get_user_model
from django.test.utils import override_settings

import pytest

from allauth.account import app_settings
from allauth.account.auth_backends import AuthenticationBackend
from allauth.tests import TestCase
Expand Down Expand Up @@ -71,3 +75,32 @@ def test_auth_by_username_or_email(self):
).pk,
user.pk,
)


@pytest.mark.parametrize(
"auth_method",
[
app_settings.AuthenticationMethod.EMAIL,
app_settings.AuthenticationMethod.USERNAME,
app_settings.AuthenticationMethod.USERNAME_EMAIL,
],
)
def test_account_enumeration_timing_attack(user, db, rf, settings, auth_method):
settings.ACCOUNT_AUTHENTICATION_METHOD = auth_method
with (
patch("django.contrib.auth.models.User.set_password") as set_password_mock,
patch("django.contrib.auth.models.User.check_password", new=set_password_mock),
):
backend = AuthenticationBackend()
backend.authenticate(
rf.get("/"), email="not@known.org", username="not-known", password="secret"
)
set_password_mock.assert_called_once()
set_password_mock.reset_mock()
backend.authenticate(rf.get("/"), username=user.username, password="secret")
set_password_mock.assert_called_once()
set_password_mock.reset_mock()
backend.authenticate(
rf.get("/"), email=user.email, username="not-known", password="secret"
)
set_password_mock.assert_called_once()
4 changes: 2 additions & 2 deletions allauth/account/utils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import unicodedata
from collections import OrderedDict
from typing import Optional
from typing import List, Optional

from django.conf import settings
from django.contrib.auth import REDIRECT_FIELD_NAME, get_user_model
Expand Down Expand Up @@ -326,7 +326,7 @@ def filter_users_by_username(*username):

def filter_users_by_email(
email: str, is_active: Optional[bool] = None, prefer_verified: bool = False
):
) -> List:
"""Return list of users by email address
Typically one, at most just a few in length. First we look through
Expand Down

0 comments on commit e01cc5f

Please sign in to comment.