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

Revision history for this message
Robert Collins (lifeless) wrote :

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

« Back to merge proposal