From af33fb250e9847f1ca8c0ba0d72671d76659704f Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Tue, 23 Jan 2018 13:20:18 -0500 Subject: [PATCH] Fixed CVE-2018-6188 -- Fixed information leakage in AuthenticationForm. Reverted 359370a8b8ca0efe99b1d4630b291ec060b69225 (refs #28645). This is a security fix. --- django/contrib/auth/forms.py | 9 --------- docs/releases/1.11.10.txt | 23 +++++++++++++++++++++-- docs/releases/2.0.2.txt | 23 +++++++++++++++++++++-- tests/admin_views/test_forms.py | 5 ++++- tests/auth_tests/test_forms.py | 21 +++++++++++++++++++++ 5 files changed, 67 insertions(+), 14 deletions(-) diff --git a/django/contrib/auth/forms.py b/django/contrib/auth/forms.py index dfceccb2ec..2f67dc15ec 100644 --- a/django/contrib/auth/forms.py +++ b/django/contrib/auth/forms.py @@ -191,15 +191,6 @@ class AuthenticationForm(forms.Form): if username is not None and password: self.user_cache = authenticate(self.request, username=username, password=password) if self.user_cache is None: - # An authentication backend may reject inactive users. Check - # if the user exists and is inactive, and raise the 'inactive' - # error if so. - try: - self.user_cache = UserModel._default_manager.get_by_natural_key(username) - except UserModel.DoesNotExist: - pass - else: - self.confirm_login_allowed(self.user_cache) raise self.get_invalid_login_error() else: self.confirm_login_allowed(self.user_cache) diff --git a/docs/releases/1.11.10.txt b/docs/releases/1.11.10.txt index cfa8fc2070..96f07920a3 100644 --- a/docs/releases/1.11.10.txt +++ b/docs/releases/1.11.10.txt @@ -2,9 +2,28 @@ Django 1.11.10 release notes ============================ -*Expected February 1, 2018* +*February 1, 2018* -Django 1.11.10 fixes several bugs in 1.11.9. +Django 1.11.10 fixes a security issue and several bugs in 1.11.9. + +CVE-2018-6188: Information leakage in ``AuthenticationForm`` +============================================================ + +A regression in Django 1.11.8 made +:class:`~django.contrib.auth.forms.AuthenticationForm` run its +``confirm_login_allowed()`` method even if an incorrect password is entered. +This can leak information about a user, depending on what messages +``confirm_login_allowed()`` raises. If ``confirm_login_allowed()`` isn't +overridden, an attacker enter an arbitrary username and see if that user has +been set to ``is_active=False``. If ``confirm_login_allowed()`` is overridden, +more sensitive details could be leaked. + +This issue is fixed with the caveat that ``AuthenticationForm`` can no longer +raise the "This account is inactive." error if the authentication backend +rejects inactive users (the default authentication backend, ``ModelBackend``, +has done that since Django 1.10). This issue will be revisited for Django 2.1 +as a fix to address the caveat will likely be too invasive for inclusion in +older versions. Bugfixes ======== diff --git a/docs/releases/2.0.2.txt b/docs/releases/2.0.2.txt index 562f30995f..475ddfb23a 100644 --- a/docs/releases/2.0.2.txt +++ b/docs/releases/2.0.2.txt @@ -2,9 +2,28 @@ Django 2.0.2 release notes ========================== -*Expected February 1, 2018* +*February 1, 2018* -Django 2.0.2 fixes several bugs in 2.0.1. +Django 2.0.2 fixes a security issue and several bugs in 2.0.1. + +CVE-2018-6188: Information leakage in ``AuthenticationForm`` +============================================================ + +A regression in Django 1.11.8 made +:class:`~django.contrib.auth.forms.AuthenticationForm` run its +``confirm_login_allowed()`` method even if an incorrect password is entered. +This can leak information about a user, depending on what messages +``confirm_login_allowed()`` raises. If ``confirm_login_allowed()`` isn't +overridden, an attacker enter an arbitrary username and see if that user has +been set to ``is_active=False``. If ``confirm_login_allowed()`` is overridden, +more sensitive details could be leaked. + +This issue is fixed with the caveat that ``AuthenticationForm`` can no longer +raise the "This account is inactive." error if the authentication backend +rejects inactive users (the default authentication backend, ``ModelBackend``, +has done that since Django 1.10). This issue will be revisited for Django 2.1 +as a fix to address the caveat will likely be too invasive for inclusion in +older versions. Bugfixes ======== diff --git a/tests/admin_views/test_forms.py b/tests/admin_views/test_forms.py index 8c58fe7eae..d4eecf48aa 100644 --- a/tests/admin_views/test_forms.py +++ b/tests/admin_views/test_forms.py @@ -1,8 +1,11 @@ from django.contrib.admin.forms import AdminAuthenticationForm from django.contrib.auth.models import User -from django.test import TestCase +from django.test import TestCase, override_settings +# To verify that the login form rejects inactive users, use an authentication +# backend that allows them. +@override_settings(AUTHENTICATION_BACKENDS=['django.contrib.auth.backends.AllowAllUsersModelBackend']) class AdminAuthenticationFormTests(TestCase): @classmethod def setUpTestData(cls): diff --git a/tests/auth_tests/test_forms.py b/tests/auth_tests/test_forms.py index 497d385fce..b1d55c9749 100644 --- a/tests/auth_tests/test_forms.py +++ b/tests/auth_tests/test_forms.py @@ -313,6 +313,9 @@ class UserCreationFormTest(ReloadFormsMixin, TestDataMixin, TestCase): self.assertTrue(form.is_valid()) +# To verify that the login form rejects inactive users, use an authentication +# backend that allows them. +@override_settings(AUTHENTICATION_BACKENDS=['django.contrib.auth.backends.AllowAllUsersModelBackend']) class AuthenticationFormTest(TestDataMixin, TestCase): def test_invalid_username(self): @@ -342,6 +345,24 @@ class AuthenticationFormTest(TestDataMixin, TestCase): self.assertFalse(form.is_valid()) self.assertEqual(form.non_field_errors(), [str(form.error_messages['inactive'])]) + # Use an authentication backend that rejects inactive users. + @override_settings(AUTHENTICATION_BACKENDS=['django.contrib.auth.backends.ModelBackend']) + def test_inactive_user_incorrect_password(self): + """An invalid login doesn't leak the inactive status of a user.""" + data = { + 'username': 'inactive', + 'password': 'incorrect', + } + form = AuthenticationForm(None, data) + self.assertFalse(form.is_valid()) + self.assertEqual( + form.non_field_errors(), [ + form.error_messages['invalid_login'] % { + 'username': User._meta.get_field('username').verbose_name + } + ] + ) + def test_login_failed(self): signal_calls = []