Stricten password validation to match PCI DSS requirements (#4467)

* Stricten password validation to match PCI DSS requirements * Review fix * Fix a file header
2024-09-17 13:29:17 +02:00
parent aa07533693
commit 32d6ded003
8 changed files with 249 additions and 34 deletions
@@ -32,13 +32,16 @@
 # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 # License for the specific language governing permissions and limitations under the License.

+import string
 from collections import OrderedDict
 from importlib import import_module

 from django import forms
 from django.conf import settings
 from django.contrib.auth import authenticate
-from django.utils.translation import gettext_lazy as _
+from django.contrib.auth.hashers import check_password, make_password
+from django.core.exceptions import ValidationError
+from django.utils.translation import gettext_lazy as _, ngettext


 def get_auth_backends():
@@ -160,3 +163,62 @@ class NativeAuthBackend(BaseAuthBackend):
        u = authenticate(request=request, email=form_data['email'].lower(), password=form_data['password'])
        if u and u.auth_backend == self.identifier:
            return u
+
+
+class NumericAndAlphabeticPasswordValidator:
+
+    def validate(self, password, user=None):
+        has_numeric = any(c in string.digits for c in password)
+        has_alpha = any(c in string.ascii_letters for c in password)
+        if not has_numeric or not has_alpha:
+            raise ValidationError(
+                _(
+                    "Your password must contain both numeric and alphabetic characters.",
+                ),
+                code="password_numeric_and_alphabetic",
+            )
+
+    def get_help_text(self):
+        return _(
+            "Your password must contain both numeric and alphabetic characters.",
+        )
+
+
+class HistoryPasswordValidator:
+
+    def __init__(self, history_length=4):
+        self.history_length = history_length
+
+    def validate(self, password, user=None):
+        from pretix.base.models import User
+
+        if not user or not user.pk or not isinstance(user, User):
+            return
+
+        for hp in user.historic_passwords.order_by("-created")[:self.history_length]:
+            if check_password(password, hp.password):
+                raise ValidationError(
+                    ngettext(
+                        "Your password may not be the same as your previous password.",
+                        "Your password may not be the same as one of your %(history_length)s previous passwords.",
+                        self.history_length,
+                    ),
+                    code="password_history",
+                    params={"history_length": self.history_length},
+                )
+
+    def get_help_text(self):
+        return ngettext(
+            "Your password may not be the same as your previous password.",
+            "Your password may not be the same as one of your %(history_length)s previous passwords.",
+            self.history_length,
+        ) % {"history_length": self.history_length}
+
+    def password_changed(self, password, user=None):
+        if not user:
+            pass
+
+        user.historic_passwords.create(password=make_password(password))
+        user.historic_passwords.filter(
+            pk__in=user.historic_passwords.order_by("-created")[self.history_length:].values_list("pk", flat=True),
+        ).delete()
@@ -0,0 +1,36 @@
+# Generated by Django 4.2.15 on 2024-09-16 15:10
+
+import django.db.models.deletion
+from django.conf import settings
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("pretixbase", "0269_order_api_meta"),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name="HistoricPassword",
+            fields=[
+                (
+                    "id",
+                    models.BigAutoField(
+                        auto_created=True, primary_key=True, serialize=False
+                    ),
+                ),
+                ("created", models.DateTimeField(auto_now_add=True)),
+                ("password", models.CharField(max_length=128)),
+                (
+                    "user",
+                    models.ForeignKey(
+                        on_delete=django.db.models.deletion.CASCADE,
+                        related_name="historic_passwords",
+                        to=settings.AUTH_USER_MODEL,
+                    ),
+                ),
+            ],
+        ),
+    ]
@@ -654,3 +654,9 @@ class WebAuthnDevice(Device):
    @property
    def webauthnpubkey(self):
        return websafe_decode(self.pub_key)
+
+
+class HistoricPassword(models.Model):
+    user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="historic_passwords")
+    created = models.DateTimeField(auto_now_add=True)
+    password = models.CharField(verbose_name=_("Password"), max_length=128)
@@ -19,6 +19,7 @@
 # You should have received a copy of the GNU Affero General Public License along with this program.  If not, see
 # <https://www.gnu.org/licenses/>.
 #
+import functools
 import hashlib
 import ipaddress
 import random
@@ -27,7 +28,7 @@ from django import forms
 from django.conf import settings
 from django.contrib.auth.hashers import check_password
 from django.contrib.auth.password_validation import (
-    password_validators_help_texts, validate_password,
+    get_password_validators, password_validators_help_texts, validate_password,
 )
 from django.contrib.auth.tokens import PasswordResetTokenGenerator
 from django.core import signing
@@ -271,6 +272,11 @@ class RegistrationForm(forms.Form):
        return customer


+@functools.lru_cache(maxsize=None)
+def get_customer_password_validators():
+    return get_password_validators(settings.CUSTOMER_AUTH_PASSWORD_VALIDATORS)
+
+
 class SetPasswordForm(forms.Form):
    required_css_class = 'required'
    error_messages = {
@@ -311,7 +317,7 @@ class SetPasswordForm(forms.Form):

    def clean_password(self):
        password1 = self.cleaned_data.get('password', '')
-        if validate_password(password1, user=self.customer) is not None:
+        if validate_password(password1, user=self.customer, password_validators=get_customer_password_validators()) is not None:
            raise forms.ValidationError(_(password_validators_help_texts()), code='pw_invalid')
        return password1

@@ -405,7 +411,7 @@ class ChangePasswordForm(forms.Form):

    def clean_password(self):
        password1 = self.cleaned_data.get('password', '')
-        if validate_password(password1, user=self.customer) is not None:
+        if validate_password(password1, user=self.customer, password_validators=get_customer_password_validators()) is not None:
            raise forms.ValidationError(_(password_validators_help_texts()), code='pw_invalid')
        return password1

@@ -714,6 +714,10 @@ BOOTSTRAP3 = {
 }

 PASSWORD_HASHERS = [
+    # Note that when updating this, all user passwords will be re-hashed on next login, however,
+    # the HistoricPassword model will not be changed automatically. In case a serious issue with a hasher
+    # comes to light, dropping the contents of the HistoricPassword table might be the more risk-adequate
+    # decision.
    "django.contrib.auth.hashers.Argon2PasswordHasher",
    "django.contrib.auth.hashers.PBKDF2PasswordHasher",
    "django.contrib.auth.hashers.PBKDF2SHA1PasswordHasher",
@@ -725,7 +729,44 @@ AUTH_PASSWORD_VALIDATORS = [
        'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator',
    },
    {
-        'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator',
+        "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator",
+        "OPTIONS": {
+            # To fulfill per PCI DSS requirement 8.3.6
+            "min_length": 12,
+        },
+    },
+    {
+        # To fulfill per PCI DSS requirement 8.3.6
+        'NAME': 'pretix.base.auth.NumericAndAlphabeticPasswordValidator',
+    },
+    {
+        "NAME": "pretix.base.auth.HistoryPasswordValidator",
+        "OPTIONS": {
+            # To fulfill per PCI DSS requirement 8.3.7
+            "history_length": 4,
+        },
+    },
+    {
+        'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator',
+    },
+    {
+        'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator',
+    },
+]
+CUSTOMER_AUTH_PASSWORD_VALIDATORS = [
+    # For customer accounts, we apply a little less strict requirements to provide a risk-adequate
+    # user experience.
+    {
+        'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator',
+    },
+    {
+        "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator",
+        "OPTIONS": {
+            "min_length": 8,
+        },
+    },
+    {
+        'NAME': 'pretix.base.auth.NumericAndAlphabeticPasswordValidator',
    },
    {
        'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator',
@@ -337,7 +337,7 @@ class RegistrationFormTest(TestCase):

        response = self.client.post('/control/register', {
            'email': 'dummy@dummy.dummy',
-            'password': 'foobarbar',
+            'password': 'f00barbarbar',
            'password_repeat': ''
        })
        self.assertEqual(response.status_code, 200)
@@ -347,8 +347,8 @@ class RegistrationFormTest(TestCase):
        self.user = User.objects.create_user('dummy@dummy.dummy', 'dummy')
        response = self.client.post('/control/register', {
            'email': 'dummy@dummy.dummy',
-            'password': 'foobarbar',
-            'password_repeat': 'foobarbar'
+            'password': 'f00barbarbar',
+            'password_repeat': 'f00barbarbar'
        })
        self.assertEqual(response.status_code, 200)

@@ -356,8 +356,8 @@ class RegistrationFormTest(TestCase):
    def test_success(self):
        response = self.client.post('/control/register', {
            'email': 'dummy@dummy.dummy',
-            'password': 'foobarbar',
-            'password_repeat': 'foobarbar'
+            'password': 'f00barbarbar',
+            'password_repeat': 'f00barbarbar'
        })
        self.assertEqual(response.status_code, 302)
        assert time.time() - self.client.session['pretix_auth_login_time'] < 60
@@ -367,8 +367,8 @@ class RegistrationFormTest(TestCase):
    def test_disabled(self):
        response = self.client.post('/control/register', {
            'email': 'dummy@dummy.dummy',
-            'password': 'foobarbar',
-            'password_repeat': 'foobarbar'
+            'password': 'f00barbarbar',
+            'password_repeat': 'f00barbarbar'
        })
        self.assertEqual(response.status_code, 403)

@@ -376,8 +376,8 @@ class RegistrationFormTest(TestCase):
    def test_no_native_auth(self):
        response = self.client.post('/control/register', {
            'email': 'dummy@dummy.dummy',
-            'password': 'foobarbar',
-            'password_repeat': 'foobarbar'
+            'password': 'f00barbarbar',
+            'password_repeat': 'f00barbarbar'
        })
        self.assertEqual(response.status_code, 403)

@@ -593,8 +593,8 @@ class PasswordRecoveryFormTest(TestCase):
        response = self.client.post(
            '/control/forgot/recover?id=%d&token=foo' % self.user.id,
            {
-                'password': 'foobarbar',
-                'password_repeat': 'foobarbar'
+                'password': 'f00barbarbar',
+                'password_repeat': 'f00barbarbar'
            }
        )
        self.assertEqual(response.status_code, 302)
@@ -615,8 +615,8 @@ class PasswordRecoveryFormTest(TestCase):
        response = self.client.post(
            '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token),
            {
-                'password': 'foobarbar',
-                'password_repeat': 'foobarbar'
+                'password': 'f00barbarbar',
+                'password_repeat': 'f00barbarbar'
            }
        )
        self.assertEqual(response.status_code, 302)
@@ -630,13 +630,13 @@ class PasswordRecoveryFormTest(TestCase):
        response = self.client.post(
            '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token),
            {
-                'password': 'foobarbar',
-                'password_repeat': 'foobarbar'
+                'password': 'f00barbarbar',
+                'password_repeat': 'f00barbarbar'
            }
        )
        self.assertEqual(response.status_code, 302)
        self.user = User.objects.get(id=self.user.id)
-        self.assertTrue(self.user.check_password('foobarbar'))
+        self.assertTrue(self.user.check_password('f00barbarbar'))

    def test_recovery_valid_token_empty_passwords(self):
        token = default_token_generator.make_token(self.user)
@@ -645,7 +645,7 @@ class PasswordRecoveryFormTest(TestCase):
        response = self.client.post(
            '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token),
            {
-                'password': 'foobarbar',
+                'password': 'f00barbarbar',
                'password_repeat': ''
            }
        )
@@ -660,7 +660,7 @@ class PasswordRecoveryFormTest(TestCase):
            '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token),
            {
                'password': '',
-                'password_repeat': 'foobarbar'
+                'password_repeat': 'f00barbarbar'
            }
        )
        self.assertEqual(response.status_code, 200)
@@ -697,6 +697,48 @@ class PasswordRecoveryFormTest(TestCase):
        self.user = User.objects.get(id=self.user.id)
        self.assertTrue(self.user.check_password('demo'))

+    def test_recovery_valid_token_password_reuse(self):
+        self.user.set_password("GsvdU4gGZDb4J9WgIhLNcZT9PO7CZ3")
+        self.user.save()
+        self.user.set_password("hLPqPpuZIjouGBk9xTLu1aXYqjpRYS")
+        self.user.save()
+        self.user.set_password("Jn2nQSa25ZJAc5GUI1HblrneWCXotD")
+        self.user.save()
+        self.user.set_password("cboaBj3yIfgnQeKClDgvKNvWC69cV1")
+        self.user.save()
+        self.user.set_password("Kkj8f3kGXbXmbgcwHBgf3WKmzkUOhM")
+        self.user.save()
+
+        assert self.user.historic_passwords.count() == 4
+
+        token = default_token_generator.make_token(self.user)
+        response = self.client.get('/control/forgot/recover?id=%d&token=%s' % (self.user.id, token))
+        self.assertEqual(response.status_code, 200)
+        response = self.client.post(
+            '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token),
+            {
+                'password': 'cboaBj3yIfgnQeKClDgvKNvWC69cV1',
+                'password_repeat': 'cboaBj3yIfgnQeKClDgvKNvWC69cV1'
+            }
+        )
+        self.assertEqual(response.status_code, 200)
+        self.user = User.objects.get(id=self.user.id)
+        self.assertTrue(self.user.check_password('Kkj8f3kGXbXmbgcwHBgf3WKmzkUOhM'))
+
+        token = default_token_generator.make_token(self.user)
+        response = self.client.get('/control/forgot/recover?id=%d&token=%s' % (self.user.id, token))
+        self.assertEqual(response.status_code, 200)
+        response = self.client.post(
+            '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token),
+            {
+                'password': 'GsvdU4gGZDb4J9WgIhLNcZT9PO7CZ3',
+                'password_repeat': 'GsvdU4gGZDb4J9WgIhLNcZT9PO7CZ3'
+            }
+        )
+        self.assertEqual(response.status_code, 302)
+        self.user = User.objects.get(id=self.user.id)
+        self.assertTrue(self.user.check_password('GsvdU4gGZDb4J9WgIhLNcZT9PO7CZ3'))
+
    def test_recovery_valid_token_short_passwords(self):
        token = default_token_generator.make_token(self.user)
        response = self.client.get('/control/forgot/recover?id=%d&token=%s' % (self.user.id, token))
@@ -704,8 +746,8 @@ class PasswordRecoveryFormTest(TestCase):
        response = self.client.post(
            '/control/forgot/recover?id=%d&token=%s' % (self.user.id, token),
            {
-                'password': 'foobar',
-                'password_repeat': 'foobar'
+                'password': 'foobarfooba',
+                'password_repeat': 'foobarfooba'
            }
        )
        self.assertEqual(response.status_code, 200)
@@ -349,8 +349,8 @@ def test_invite_new_user(event, admin_team, client):
    assert b'<form' in resp.content
    resp = client.post('/control/invite/{}'.format(i.token), {
        'email': 'dummy@example.org',
-        'password': 'asdsdgfgjh',
-        'password_repeat': 'asdsdgfgjh'
+        'password': 'asdsdgfgjh1234567',
+        'password_repeat': 'asdsdgfgjh1234567'
    }, follow=True)

    assert b'alert-success' in resp.content
@@ -112,8 +112,8 @@ class UserSettingsTest(SoupTest):
        self.user.auth_backend = 'test_request'
        self.user.save()
        self.save({
-            'new_pw': 'foobarbar',
-            'new_pw_repeat': 'foobarbar',
+            'new_pw': 'f00barbarbar',
+            'new_pw_repeat': 'f00barbarbar',
            'old_pw': 'barfoofoo',
        })
        pw = self.user.password
@@ -122,13 +122,13 @@ class UserSettingsTest(SoupTest):

    def test_change_password_success(self):
        doc = self.save({
-            'new_pw': 'foobarbar',
-            'new_pw_repeat': 'foobarbar',
+            'new_pw': 'f00barbarbar',
+            'new_pw_repeat': 'f00barbarbar',
            'old_pw': 'barfoofoo',
        })
        assert doc.select(".alert-success")
        self.user = User.objects.get(pk=self.user.pk)
-        assert self.user.check_password("foobarbar")
+        assert self.user.check_password("f00barbarbar")

    def test_change_password_short(self):
        doc = self.save({
@@ -171,6 +171,28 @@ class UserSettingsTest(SoupTest):
        })
        assert doc.select(".alert-danger")

+    def test_change_password_history(self):
+        doc = self.save({
+            'new_pw': 'qvuSpukdKWUV7m7PoRrWwpCd2Taij9',
+            'new_pw_repeat': 'qvuSpukdKWUV7m7PoRrWwpCd2Taij9',
+            'old_pw': 'barfoofoo',
+        })
+        assert doc.select(".alert-success")
+
+        doc = self.save({
+            'new_pw': '9UQl4lSwHLMVUIMgw0L1X8XEFmyvdn',
+            'new_pw_repeat': '9UQl4lSwHLMVUIMgw0L1X8XEFmyvdn',
+            'old_pw': 'qvuSpukdKWUV7m7PoRrWwpCd2Taij9',
+        })
+        assert doc.select(".alert-success")
+
+        doc = self.save({
+            'new_pw': 'qvuSpukdKWUV7m7PoRrWwpCd2Taij9',
+            'new_pw_repeat': 'qvuSpukdKWUV7m7PoRrWwpCd2Taij9',
+            'old_pw': '9UQl4lSwHLMVUIMgw0L1X8XEFmyvdn',
+        })
+        assert doc.select(".alert-danger")
+
    def test_needs_password_change(self):
        self.user.needs_password_change = True
        self.user.save()
@@ -187,8 +209,8 @@ class UserSettingsTest(SoupTest):
        self.user.needs_password_change = True
        self.user.save()
        self.save({
-            'new_pw': 'foobarbar',
-            'new_pw_repeat': 'foobarbar',
+            'new_pw': 'f00barbarbar',
+            'new_pw_repeat': 'f00barbarbar',
            'old_pw': 'barfoofoo'
        })
        self.user.refresh_from_db()