Prevent replay attacks with the confirmation token.

11 files changed, 96 insertions, 20 deletions

diff --git a/TODO.rst b/TODO.rst
index 1f8481c0e..76f633d35 100644
--- a/TODO.rst
+++ b/TODO.rst
@@ -1,5 +1,4 @@
 * TO DO:
- - make sure a user can't double confirm to bypass moderator approval
  - get rid of hold_subscription
  - subsume handle_subscription
  - workflow for unsubscription
diff --git a/src/mailman/app/registrar.py b/src/mailman/app/registrar.py
index cc8e42a33..ae4322d22 100644
--- a/src/mailman/app/registrar.py
+++ b/src/mailman/app/registrar.py
@@ -71,7 +71,7 @@ class Registrar:
         workflow.token = token
         workflow.restore()
         list(workflow)
-        return workflow.token is None
+        return workflow.token
 
     def discard(self, token):
         """See `IRegistrar`."""
diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py
index 7b46aee84..3138c513b 100644
--- a/src/mailman/app/subscriptions.py
+++ b/src/mailman/app/subscriptions.py
@@ -290,6 +290,14 @@ class SubscriptionWorkflow(Workflow):
         else:
             assert self.which is WhichSubscriber.user
             self.subscriber = self.user
+        # Create a new token to prevent replay attacks.  It seems like this
+        # should produce the same token, but it won't because the pending adds
+        # a bit of randomization.
+        pendable = Pendable(
+            list_id=self.mlist.list_id,
+            address=self.address.email,
+            )
+        self.token = getUtility(IPendings).add(pendable, timedelta(days=3650))
         # The user has confirmed their subscription request, and also verified
         # their email address if necessary.  This latter needs to be set on the
         # IAddress, but there's nothing more to do about the confirmation step.
diff --git a/src/mailman/app/tests/test_registrar.py b/src/mailman/app/tests/test_registrar.py
index 3e3c30590..06c22386a 100644
--- a/src/mailman/app/tests/test_registrar.py
+++ b/src/mailman/app/tests/test_registrar.py
@@ -146,15 +146,48 @@ class TestRegistrar(unittest.TestCase):
         self.assertIsNone(member)
         # Now confirm the subscription, and wait for the moderator to approve
         # the subscription.  She is still not subscribed.
-        status = self._registrar.confirm(token)
-        # The status is not true because the user has not yet been subscribed
-        # to the mailing list.
-        self.assertFalse(status)
+        new_token = self._registrar.confirm(token)
+        # The new token, used for the moderator to approve the message, is not
+        # the same as the old token.
+        self.assertNotEqual(new_token, token)
         member = self._mlist.regular_members.get_member('anne@example.com')
         self.assertIsNone(member)
         # Confirm once more, this time as the moderator approving the
         # subscription.  Now she's a member.
-        self._registrar.confirm(token)
+        self._registrar.confirm(new_token)
+        member = self._mlist.regular_members.get_member('anne@example.com')
+        self.assertEqual(member.address, self._anne)
+
+    def test_confirm_then_moderate_with_different_tokens(self):
+        # Ensure that the confirmation token the user sees when they have to
+        # confirm their subscription is different than the token the moderator
+        # sees when they approve the subscription.  This prevents the user
+        # from using a replay attack to subvert moderator approval.
+        self._mlist.subscription_policy = \
+          SubscriptionPolicy.confirm_then_moderate
+        self._anne.verified_on = now()
+        # Runs until subscription confirmation.
+        token = self._registrar.register(self._anne)
+        self.assertIsNotNone(token)
+        member = self._mlist.regular_members.get_member('anne@example.com')
+        self.assertIsNone(member)
+        # Now confirm the subscription, and wait for the moderator to approve
+        # the subscription.  She is still not subscribed.
+        new_token = self._registrar.confirm(token)
+        # The status is not true because the user has not yet been subscribed
+        # to the mailing list.
+        self.assertIsNotNone(new_token)
+        member = self._mlist.regular_members.get_member('anne@example.com')
+        self.assertIsNone(member)
+        # The new token is different than the old token.
+        self.assertNotEqual(token, new_token)
+        # Trying to confirm with the old token does not work.
+        self.assertRaises(LookupError, self._registrar.confirm, token)
+        # Confirm once more, this time with the new token, as the moderator
+        # approving the subscription.  Now she's a member.
+        done_token = self._registrar.confirm(new_token)
+        # The token is None, signifying that the member has been subscribed.
+        self.assertIsNone(done_token)
         member = self._mlist.regular_members.get_member('anne@example.com')
         self.assertEqual(member.address, self._anne)
 
diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py
index 478c7e33b..a4971d793 100644
--- a/src/mailman/app/tests/test_subscriptions.py
+++ b/src/mailman/app/tests/test_subscriptions.py
@@ -537,3 +537,44 @@ approval:
         self.assertIsNotNone(anne.verified_on)
         self.assertEqual(
             self._mlist.regular_members.get_member(self._anne).address, anne)
+
+    def test_prevent_confirmation_replay_attacks(self):
+        # Ensure that if the workflow requires two confirmations, e.g. first
+        # the user confirming their subscription, and then the moderator
+        # approving it, that different tokens are used in these two cases.
+        self._mlist.subscription_policy = \
+          SubscriptionPolicy.confirm_then_moderate
+        anne = self._user_manager.create_address(self._anne)
+        workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True)
+        # Run the state machine up to the first confirmation, and cache the
+        # confirmation token.
+        list(workflow)
+        token = workflow.token
+        # Anne is not yet a member of the mailing list.
+        member = self._mlist.regular_members.get_member(self._anne)
+        self.assertIsNone(member)
+        # The old token will not work for moderator approval.
+        moderator_workflow = SubscriptionWorkflow(self._mlist)
+        moderator_workflow.token = token
+        moderator_workflow.restore()
+        list(moderator_workflow)
+        # While we wait for the moderator to approve the subscription, note
+        # that there's a new token for the next steps.
+        self.assertNotEqual(token, moderator_workflow.token)
+        # The old token won't work.
+        final_workflow = SubscriptionWorkflow(self._mlist)
+        final_workflow.token = token
+        self.assertRaises(LookupError, final_workflow.restore)
+        # Running this workflow will fail.
+        self.assertRaises(AssertionError, list, final_workflow)
+        # Anne is still not subscribed.
+        member = self._mlist.regular_members.get_member(self._anne)
+        self.assertIsNone(member)
+        # However, if we use the new token, her subscription request will be
+        # approved by the moderator.
+        final_workflow.token = moderator_workflow.token
+        final_workflow.restore()
+        list(final_workflow)
+        # And now Anne is a member.
+        member = self._mlist.regular_members.get_member(self._anne)
+        self.assertEqual(member.address.email, self._anne)
diff --git a/src/mailman/commands/eml_confirm.py b/src/mailman/commands/eml_confirm.py
index 2ee48e938..077fab9a6 100644
--- a/src/mailman/commands/eml_confirm.py
+++ b/src/mailman/commands/eml_confirm.py
@@ -53,7 +53,7 @@ class Confirm:
         tokens.add(token)
         results.confirms = tokens
         try:
-            succeeded = IRegistrar(mlist).confirm(token)
+            succeeded = (IRegistrar(mlist).confirm(token) is None)
         except LookupError:
             # The token must not exist in the database.
             succeeded = False
diff --git a/src/mailman/commands/tests/test_confirm.py b/src/mailman/commands/tests/test_confirm.py
index 2f6a8088f..98a26bf7d 100644
--- a/src/mailman/commands/tests/test_confirm.py
+++ b/src/mailman/commands/tests/test_confirm.py
@@ -31,7 +31,7 @@ from mailman.interfaces.command import ContinueProcessing
 from mailman.interfaces.registrar import IRegistrar
 from mailman.interfaces.usermanager import IUserManager
 from mailman.runners.command import Results
-from mailman.testing.helpers import get_queue_messages, reset_the_world
+from mailman.testing.helpers import get_queue_messages
 from mailman.testing.layers import ConfigLayer
 from zope.component import getUtility
 
@@ -51,9 +51,6 @@ class TestConfirm(unittest.TestCase):
         # Clear the virgin queue.
         get_queue_messages('virgin')
 
-    def tearDown(self):
-        reset_the_world()
-
     def test_welcome_message(self):
         # A confirmation causes a welcome message to be sent to the member, if
         # enabled by the mailing list.
diff --git a/src/mailman/interfaces/registrar.py b/src/mailman/interfaces/registrar.py
index 95c8fcd88..ff3f26898 100644
--- a/src/mailman/interfaces/registrar.py
+++ b/src/mailman/interfaces/registrar.py
@@ -91,9 +91,9 @@ class IRegistrar(Interface):
 
         :param token: A token matching a workflow.
         :type token: string
-        :return: A flag indicating whether the confirmation succeeded in
-            subscribing the user or not.
-        :rtype: bool
+        :return: The new token for any follow up confirmation, or None if the
+            user was subscribed.
+        :rtype: str or None
         :raises LookupError: when no workflow is associated with the token.
         """
 
diff --git a/src/mailman/model/docs/registration.rst b/src/mailman/model/docs/registration.rst
index 5a4935355..fc7ad6f1a 100644
--- a/src/mailman/model/docs/registration.rst
+++ b/src/mailman/model/docs/registration.rst
@@ -48,7 +48,6 @@ list.  In this case, verifying implies that she also confirms her wish to join
 the mailing list.
 
     >>> registrar.confirm(token)
-    True
     >>> mlist.members.get_member('anne@example.com')
     <Member: Anne Person <anne@example.com> on ant@example.com
         as MemberRole.member>
@@ -86,7 +85,6 @@ subscribed to the mailing list.
 When the moderator confirms Bart's subscription, he joins the mailing list.
 
     >>> registrar.confirm(token)
-    True
     >>> mlist.members.get_member('bart@example.com')
     <Member: Bart Person <bart@example.com> on ant@example.com
         as MemberRole.member>
diff --git a/src/mailman/runners/docs/command.rst b/src/mailman/runners/docs/command.rst
index 8fb5c6816..19d772b00 100644
--- a/src/mailman/runners/docs/command.rst
+++ b/src/mailman/runners/docs/command.rst
@@ -147,8 +147,8 @@ address, and the other is the results of his email command.
     ...     print('Subject:', subject)
     ...     if 'confirm' in str(subject):
     ...         token = str(subject).split()[1].strip()
-    ...         status = registrar.confirm(token)
-    ...         assert status, 'Confirmation failed'
+    ...         new_token = registrar.confirm(token)
+    ...         assert new_token is None, 'Confirmation failed'
     Subject: The results of your email commands
     Subject: confirm ...
 
diff --git a/src/mailman/runners/tests/test_join.py b/src/mailman/runners/tests/test_join.py
index 5742800c2..c3b52cf0c 100644
--- a/src/mailman/runners/tests/test_join.py
+++ b/src/mailman/runners/tests/test_join.py
@@ -161,7 +161,7 @@ class TestJoinWithDigests(unittest.TestCase):
         self.assertEqual(subject_words[0], 'confirm')
         token = subject_words[1]
         status = IRegistrar(self._mlist).confirm(token)
-        self.assertTrue(status, 'Confirmation failed')
+        self.assertIsNone(status, 'Confirmation failed')
         # Now, make sure that Anne is a member of the list and is receiving
         # digest deliveries.
         members = getUtility(ISubscriptionService).find_members(