commit 5e6258041d492c0748e03079f366bbb0d06d53f9 Author: Isis Lovecruft isis@torproject.org Date: Wed May 28 23:05:20 2014 +0000

Fix #12086; disallow emails not to our domain.

At the SMTP layer, we previously only checked that the email address within the SMTP 'RCPT TO:' command (after being stripped of any '+' aliases) as well as the email address in the 'To:' header both had a username equal to the configured EMAIL_USERNAME. It didn't check that these email addresses were the same, nor did it check the domain at all.

This refactors the ``bridgedb.email.server.MailDelivery.validateTo()`` method to check that the domain name in the SMTP 'RCPT TO:' command is either our domain or a subdomain of it.

At the email layer, we now check that the email was sent to a domain which is either our domain or a subdomain of it. We also check that the username matches our configured username (after '+' aliases have been removed).

* FIXES #12086, a bug where BridgeDB would accept emails addressed to addresses such as 'givemebridges@serious.ly'. * CHANGES MailDelivery.validateTo() to check domain names and usernames in the SMTP 'RCPT TO' command that an incoming email was received for. * RENAME MailMessage.getRecipient() → MailMessage.getMailFrom() in bridgedb.email.server module. * CHANGES MailMessage.getMailFrom() to check domain names and usernames in the 'To:' header of an incoming email. --- lib/bridgedb/email/server.py | 71 ++++++++++++++++++++++++-------- lib/bridgedb/test/test_email_server.py | 18 ++++---- 2 files changed, 62 insertions(+), 27 deletions(-)

diff --git a/lib/bridgedb/email/server.py b/lib/bridgedb/email/server.py index 79b4a59..593713b 100644 --- a/lib/bridgedb/email/server.py +++ b/lib/bridgedb/email/server.py @@ -506,7 +506,7 @@ class MailMessage(object): else: return client

- def getRecipient(self, incoming): + def getMailFrom(self, incoming): """Find our address in the recipients list of the **incoming** message.

:type incoming: :api:`twisted.mail.smtp.rfc822.Message` @@ -516,21 +516,42 @@ class MailMessage(object): :return: Our address from the recipients list. If we can't find it return our default ``SMTP_FROM_ADDRESS`` from the config file. """ - ourAddress = self.context.fromAddr + logging.debug("Searching for our email address in 'To:' header...") + + ours = None

try: - ourAddress = smtp.Address(ourAddress) + ourAddress = smtp.Address(self.context.fromAddr) allRecipients = incoming.getaddrlist("To") + for _, addr in allRecipients: recipient = smtp.Address(addr) - # See if the user looks familiar. We do a 'find' instead of - # compare because we might have a '+' address here. - if recipient.local.find(ourAddress.local) != -1: - return '@'.join([recipient.local, recipient.domain]) - except smtp.AddressError as error: - logging.warn(error) + if not (ourAddress.domain in recipient.domain): + logging.debug(("Not our domain (%s) or subdomain, skipping" + " email address: %s") + % (ourAddress.domain, str(recipient))) + continue + # The recipient's username should at least start with ours, + # but it still might be a '+' address. + if not recipient.local.startswith(ourAddress.local): + logging.debug(("Username doesn't begin with ours, skipping" + " email address: %s") % str(recipient)) + continue + # Ignore everything after the first '+', if there is one. + beforePlus = recipient.local.split('+', 1)[0] + if beforePlus == ourAddress.local: + ours = str(recipient) + if not ours: + raise BadEmail(allRecipients)

- return ourAddress + except Exception as error: + logging.error(("Couldn't find our email address in incoming email " + "headers: %r" % error)) + # Just return the email address that we're configured to use: + ours = self.context.fromAddr + + logging.debug("Found our email address: %s." % ours) + return ours

def getCanonicalDomain(self, domain): try: @@ -571,7 +592,7 @@ class MailMessage(object): d.addErrback(_replyEB)

incoming = self.getIncomingMessage() - recipient = self.getRecipient(incoming) + recipient = self.getMailFrom(incoming) client = self.getClientAddress(incoming)

if not client: @@ -693,13 +714,27 @@ class MailDelivery(object): :returns: A parameterless function which returns an instance of :class:`SMTPMessage`. """ - u = user.dest.local - # Hasplus? If yes, strip '+foo' - idx = u.find('+') - if idx != -1: - u = u[:idx] - if u != self.context.username: - raise smtp.SMTPBadRcpt(user) + logging.debug("Validating SMTP 'RCPT TO:' email address...") + + recipient = user.dest + ourAddress = smtp.Address(self.context.smtpFromAddr) + + if not (ourAddress.domain in recipient.domain): + logging.debug(("Not our domain (%s) or subdomain, skipping" + " SMTP 'RCPT TO' address: %s") + % (ourAddress.domain, str(recipient))) + raise smtp.SMTPBadRcpt(str(recipient)) + # The recipient's username should at least start with ours, + # but it still might be a '+' address. + if not recipient.local.startswith(ourAddress.local): + logging.debug(("Username doesn't begin with ours, skipping" + " SMTP 'RCPT TO' address: %s") % str(recipient)) + raise smtp.SMTPBadRcpt(str(recipient)) + # Ignore everything after the first '+', if there is one. + beforePlus = recipient.local.split('+', 1)[0] + if beforePlus != ourAddress.local: + raise smtp.SMTPBadRcpt(str(recipient)) + return lambda: MailMessage(self.context, self.fromCanonical)

diff --git a/lib/bridgedb/test/test_email_server.py b/lib/bridgedb/test/test_email_server.py index 7015339..7aff08e 100644 --- a/lib/bridgedb/test/test_email_server.py +++ b/lib/bridgedb/test/test_email_server.py @@ -316,37 +316,37 @@ class MailMessageTests(unittest.TestCase): ] self.message.lines = lines

- def test_MailMessage_getRecipient_notbridgedb_at_yikezors_dot_net(self): - """MailMessage.getRecipient() for an incoming email sent to any email + def test_MailMessage_getMailFrom_notbridgedb_at_yikezors_dot_net(self): + """MailMessage.getMailFrom() for an incoming email sent to any email address other than the one we're listening for should return our configured address, not the one in the incoming email. """ self._getIncomingLines() self.message.lines[1] = 'To: notbridgedb@yikezors.net' incoming = self.message.getIncomingMessage() - recipient = self.message.getRecipient(incoming) + recipient = str(self.message.getMailFrom(incoming)) self.assertEqual(recipient, self.context.fromAddr)

- def test_MailMessage_getRecipient_givemebridges_at_seriously(self): - """MailMessage.getRecipient() for an incoming email sent to any email + def test_MailMessage_getMailFrom_givemebridges_at_seriously(self): + """MailMessage.getMailFrom() for an incoming email sent to any email address other than the one we're listening for should return our configured address, not the one in the incoming email. """ self._getIncomingLines() self.message.lines[1] = 'To: givemebridges@serious.ly' incoming = self.message.getIncomingMessage() - recipient = self.message.getRecipient(incoming) + recipient = str(self.message.getMailFrom(incoming)) self.assertEqual(recipient, self.context.fromAddr)

- def test_MailMessage_getRecipient_bad_address(self): - """MailMessage.getRecipient() for an incoming email sent to a malformed + def test_MailMessage_getMailFrom_bad_address(self): + """MailMessage.getMailFrom() for an incoming email sent to a malformed email address should log an smtp.AddressError and then return our configured email address. """ self._getIncomingLines() self.message.lines[1] = 'To: ><@><<<>>.foo' incoming = self.message.getIncomingMessage() - recipient = self.message.getRecipient(incoming) + recipient = str(self.message.getMailFrom(incoming)) self.assertEqual(recipient, self.context.fromAddr)

def test_MailMessage_reply_noFrom(self):