fix: Only send password reset email to known, active addresses (#5061)

* fix: Only send password reset email to known, active addresses

Limits password reset to Users with a Person and at least one active
address on file. Avoids the possibility of sending a password reset to
a spoofed address as in CVE-2019-19844.

* test: Use factory instead of explicit construction

* test: Test that a User with no Person cannot reset password

* fix: Fix handling of User.person field when it's null

* test: Test that reset emails are sent to known, active addresses

ietf/ietfauth/forms.py

@@ -21,6 +21,7 @@
 from ietf.mailinglists.models import Allowlisted
 from ietf.utils.text import isascii
 
+
 class RegistrationForm(forms.Form):
     email = forms.EmailField(label="Your email (lowercase)")
 
@@ -193,10 +194,18 @@ class ResetPasswordForm(forms.Form):
     username = forms.EmailField(label="Your email (lowercase)")
 
     def clean_username(self):
-        import ietf.ietfauth.views
+        """Verify that the username is valid
+
+        In addition to EmailField's checks, verifies that a User matching the username exists.
+        """
         username = self.cleaned_data["username"]
         if not User.objects.filter(username=username).exists():
-            raise forms.ValidationError(mark_safe("Didn't find a matching account. If you don't have an account yet, you can <a href=\"{}\">create one</a>.".format(urlreverse(ietf.ietfauth.views.create_account))))
+            raise forms.ValidationError(mark_safe(
+                "Didn't find a matching account. "
+                "If you don't have an account yet, you can <a href=\"{}\">create one</a>.".format(
+                    urlreverse('ietf.ietfauth.views.create_account')
+                )
+            ))
         return username
 
 

ietf/ietfauth/tests.py

@@ -414,12 +414,10 @@ def test_reset_password(self):
         email = '[email protected]'
         password = 'foobar'
 
-        user = User.objects.create(username=email, email=email)
+        user = PersonFactory(user__email=email).user
         user.set_password(password)
         user.save()
-        p = Person.objects.create(name="Some One", ascii="Some One", user=user)
-        Email.objects.create(address=user.username, person=p, origin=user.username)
-
+
         # get
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)
@@ -497,6 +495,39 @@ def test_reset_password(self):
         r = self.client.get(confirm_url)
         self.assertEqual(r.status_code, 404)
 
+    def test_reset_password_without_person(self):
+        """No password reset for account without a person"""
+        url = urlreverse('ietf.ietfauth.views.password_reset')
+        user = UserFactory()
+        user.set_password('some password')
+        user.save()
+        empty_outbox()
+        r = self.client.post(url, { 'username': user.username})
+        self.assertContains(r, 'No known active email addresses', status_code=200)
+        q = PyQuery(r.content)
+        self.assertTrue(len(q("form .is-invalid")) > 0)
+        self.assertEqual(len(outbox), 0)
+
+    def test_reset_password_address_handling(self):
+        """Reset password links are only sent to known, active addresses"""
+        url = urlreverse('ietf.ietfauth.views.password_reset')
+        person = PersonFactory()
+        person.email_set.update(active=False)
+        empty_outbox()
+        r = self.client.post(url, { 'username': person.user.username})
+        self.assertContains(r, 'No known active email addresses', status_code=200)
+        q = PyQuery(r.content)
+        self.assertTrue(len(q("form .is-invalid")) > 0)
+        self.assertEqual(len(outbox), 0)
+
+        active_address = EmailFactory(person=person).address
+        r = self.client.post(url, {'username': person.user.username})
+        self.assertNotContains(r, 'No known active email addresses', status_code=200)
+        self.assertEqual(len(outbox), 1)
+        to = outbox[0].get('To')
+        self.assertIn(active_address, to)
+        self.assertNotIn(person.user.username, to)
+
     def test_review_overview(self):
         review_req = ReviewRequestFactory()
         assignment = ReviewAssignmentFactory(review_request=review_req,reviewer=EmailFactory(person__user__username='reviewer'))

ietf/ietfauth/views.py

@@ -413,32 +413,39 @@ def password_reset(request):
     if request.method == 'POST':
         form = ResetPasswordForm(request.POST)
         if form.is_valid():
-            username = form.cleaned_data['username']
-
-            data = { 'username': username }
-            if User.objects.filter(username=username).exists():
-                user = User.objects.get(username=username)
-                data['password'] = user.password and user.password[-4:]
-                if user.last_login:
-                    data['last_login'] = user.last_login.timestamp()
-                else:
-                    data['last_login'] = None
-
-            auth = django.core.signing.dumps(data, salt="password_reset")
-
-            domain = Site.objects.get_current().domain
-            subject = 'Confirm password reset at %s' % domain
-            from_email = settings.DEFAULT_FROM_EMAIL
-            to_email = username # form validation makes sure that this is an email address
-
-            send_mail(request, to_email, from_email, subject, 'registration/password_reset_email.txt', {
-                'domain': domain,
-                'auth': auth,
-                'username': username,
-                'expire': settings.MINUTES_TO_EXPIRE_RESET_PASSWORD_LINK,
-            })
+            submitted_username = form.cleaned_data['username']
+            # The form validation checks that a matching User exists. Add the person__isnull check
+            # because the OneToOne field does not gracefully handle checks for user.person is Null.
+            # If we don't get a User here, we know it's because there's no related Person.
+            user = User.objects.filter(username=submitted_username, person__isnull=False).first()
+            if not (user and user.person.email_set.filter(active=True).exists()):
+                form.add_error(
+                    'username',
+                    'No known active email addresses are associated with this account. '
+                    'Please contact the secretariat for assistance.',
+                )
+            else:
+                data = {
+                    'username': user.username,
+                    'password': user.password and user.password[-4:],
+                    'last_login': user.last_login.timestamp() if user.last_login else None,
+                }
+                auth = django.core.signing.dumps(data, salt="password_reset")
 
-            success = True
+                domain = Site.objects.get_current().domain
+                subject = 'Confirm password reset at %s' % domain
+                from_email = settings.DEFAULT_FROM_EMAIL
+                # Send email to addresses from the database, NOT to the address from the form.
+                # This prevents unicode spoofing tricks (https://nvd.nist.gov/vuln/detail/CVE-2019-19844).
+                to_emails = list(set(email.address for email in user.person.email_set.filter(active=True)))
+                to_emails.sort()
+                send_mail(request, to_emails, from_email, subject, 'registration/password_reset_email.txt', {
+                    'domain': domain,
+                    'auth': auth,
+                    'username': submitted_username,
+                    'expire': settings.MINUTES_TO_EXPIRE_RESET_PASSWORD_LINK,
+                })
+                success = True
     else:
         form = ResetPasswordForm()
     return render(request, 'registration/password_reset.html', {