Launchpad itself

Code review comment for lp:~abentley/launchpad/duplicate-msgid

duplicate-msgid
Merge into devel

Revision history for this message

Aaron Bentley (abentley) wrote on 2011-05-31:

-----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-----

-----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.

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-----

« Back to merge proposal