Merge lp:~abentley/launchpad/duplicate-msgid into lp:launchpad

[Aaron Bentley]

Skip duplicate incoming messages.

= Summary =
Fix bug #595166: IntegrityError raised filing a bug using the email interface

== Proposed fix ==
Skip all incoming messages whose message-ids duplicate messages already in the database.

== Pre-implementation notes ==
Discussed with sinzui

== Implementation details ==
The IntegrityError comes from trying to link the same message to two different bugs. This can happen if a message with identical contents is received twice. While there may be times we want to permit duplicate message-ids, but not when processing incoming commands.

== Tests ==
bin/test -t duplicate_message_id test_incoming

== Demo and Q/A ==
Send an email to launchpad to create a bug. Then re-send that message with the same message-id. The second message should be ignored.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/mail/incoming.py
  lib/lp/services/mail/tests/test_incoming.py
  lib/lp/services/mail/tests/__init__.py

[Abel Deuring (adeuring)]

review: Approve (code)

[Robert Collins (lifeless)]

Unfortunately this has to be rolled back - it has a security hole and a functional hole. The functional hole will prevent the security hole being an issue - but fixing the functional hole would expose the security hole - both need to be addressed.

The security hole is this: using a crafted or predicted msgid an attacker can cause Launchpad to generate a link to a mail that was sent to a private bug or mailing list when they have no such permission. We *must* check that the body of the mail is identical before treating an existing row as the same thing.

The functional hole is that a new message with an existing id will not have its mail handler called, which means that a mail sent to two bugs, or to two mailing lists, or any such pair of objects will only be added to one of the objects.

Lastly MessageSet.fromEmail will take care of deduping legitimate duplicate message ids - it checks for identical content and so forth.

What is causing the OOPS is that fromEmail *is* handling the duplicate id and returning the original message primary key. That then fails when linking to a bug because bugmessage has a (bug,message) unique constraints (as does bugnotification). So basically this needs to be handled in each incoming mail handler, not systematically, because we don't have an introspectable model for how things are linked to messages (which would let handle_one_mail call fromEmail and then check for (target, message) not existing.

The message links I see are:

    TABLE "bountymessage" CONSTRAINT "bountymessage_message_fk" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "bugattachment" CONSTRAINT "bugattachment_message_fk" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "bugmessage" CONSTRAINT "bugmessage_message_fk" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "bugnotification" CONSTRAINT "bugnotification_message_fkey" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "bugnotificationarchive" CONSTRAINT "bugnotificationarchive__message__fk" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "bugnotificationattachment" CONSTRAINT "bugnotificationattachment_message_fkey" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "codereviewmessage" CONSTRAINT "codereviewmessage_message_fkey" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "distroseriesdifferencemessage" CONSTRAINT "distroseriesdifferencemessage__message__fk" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "messageapproval" CONSTRAINT "messageapproval_message_fkey" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "questionmessage" CONSTRAINT "questionmessage__message__fk" FOREIGN KEY (message) REFERENCES message(id)
    TABLE "specificationmessage" CONSTRAINT "specificationmessage__message__fk" FOREIGN KEY (message) REFERENCES message(id)

review: Needs Fixing

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-05-30 07:54 PM, Robert Collins wrote:
> Review: Needs Fixing
> Unfortunately this has to be rolled back - it has a security hole and a functional hole. The functional hole will prevent the security hole being an issue - but fixing the functional hole would expose the security hole - both need to be addressed.
>
> The security hole is this: using a crafted or predicted msgid an attacker can cause Launchpad to generate a link to a mail that was sent to a private bug or mailing list when they have no such permission. We *must* check that the body of the mail is identical before treating an existing row as the same thing.

My change causes it to dump messages with duplicate msgids on the floor.
 I don't see how it can cause a any link to be created.

> The functional hole is that a new message with an existing id will not have its mail handler called, which means that a mail sent to two bugs, or to two mailing lists, or any such pair of objects will only be added to one of the objects.

Surely we process each message only once, no matter how many bugs it's
associated with?

> Lastly MessageSet.fromEmail will take care of deduping legitimate duplicate message ids

I know.

> - it checks for identical content and so forth.
>
> What is causing the OOPS is that fromEmail *is* handling the duplicate id and returning the original message primary key.

I know.

> That then fails when linking to a bug because bugmessage has a (bug,message) unique constraints (as does bugnotification).

I know.

> So basically this needs to be handled in each incoming mail handler

I discussed this with sinzui, and he couldn't think of any circumstance
in which we would want to process the same message twice, so we agreed
to handle it this way.

> not systematically, because we don't have an introspectable model for how things are linked to messages

We don't know whether it will violate constraints, but we do know that
messages are generally idempotent, and it doesn't make sense to process
them more than once.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3kV4wACgkQ0F+nu1YWqI09SQCcDYD3XNvdYvDm89p0YWP/9Ijk
K0oAn3ETjlCRR03xhrVy/Hw4UfznoJjo
=mKD4
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

On Tue, May 31, 2011 at 2:52 PM, Aaron Bentley <email address hidden> wrote:
>
> My change causes it to dump messages with duplicate msgids on the floor.
>  I don't see how it can cause a any link to be created.

I was unclear; sorry. If the patch were extended to call a handler
when messageset.get(rfc822id) returned a message, it would do this.

>> The functional hole is that a new message with an existing id will not have its mail handler called, which means that a mail sent to two bugs, or to two mailing lists, or any such pair of objects will only be added to one of the objects.
>
> Surely we process each message only once, no matter how many bugs it's
> associated with?

No. We process it once per address. We receive the same message
multiple times with different envelope-to headers.

-Rob

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11-05-30 11:04 PM, Robert Collins wrote:
>> Surely we process each message only once, no matter how many bugs it's
>> associated with?
>
> No. We process it once per address. We receive the same message
> multiple times with different envelope-to headers.

Ah. Glad to see that's been fixed. For quite a while, we were just
using the To and Cc headers.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3k6JAACgkQ0F+nu1YWqI3/zQCcDwfvR4RK4u6rIeTdcNEEKsd1
b4oAn198R8hArzXaNgQQOe5vSTmZaIld
=3J2S
-----END PGP SIGNATURE-----