broken dkim key on qq.com causes mail to be dropped

[Martin Pool]

OOPS-2121INBOUNDEMAIL1 shows an incoming mail (saved to http://launchpadlibrarian.net/83439610/56f7b376-fca3-11e0-9627-001e0bc3957e.txt) which fails in dkim verification:

Traceback (most recent call last):
  Module lp.services.mail.incoming, line 398, in handleMail
    signature_timestamp_checker)
  Module lp.services.mail.incoming, line 472, in handle_one_mail
    mail, signature_timestamp_checker)
  Module lp.services.mail.incoming, line 203, in authenticateEmail
    dkim_trusted_addr = _authenticateDkim(mail)
  Module lp.services.mail.incoming, line 138, in _authenticateDkim
    signed_message.parsed_string, dkim_log, details=signing_details)
  Module dkim, line 573, in verify
    x = asn1_parse(ASN1_Object, base64.b64decode(pub['p']))
KeyError: 'p'

The dkim signature is

DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qq.com; s=s0907;
 t=1319283879; bh=YNYe6JlvFtLnPIyBgQ/s3tPBunXKtwylEzBLrkWuD1E=;
 h=X-QQ-SSF:X-QQ-BUSINESS-ORIGIN:X-Originating-IP:X-QQ-STYLE:
  X-QQ-mid:From:To:Subject:Mime-Version:Content-Type:
  Content-Transfer-Encoding:Date:X-Priority:Message-ID:X-QQ-MIME:
  X-Mailer:X-QQ-Mailer:X-QQ-ReplyHash;
 b=ZQIwng8t1rDum5Rj+/uMDDitTG6wK07zJ46n0P7WBtOfers6Fi1/NRrXvsx0PJ0VE
  XBG3HfWDaT/PDtVqSs6aupHpJh/mRqV+7xPtL8obZWl2dlMzdCV6pctFtY3yXTz

This points to s0907._domainkey.qq.com which has

s0907._domainkey.qq.com. 806 IN TXT "k=rsa"
s0907._domainkey.qq.com. 806 IN TXT " p=MHwwDQYJKoZIhvcNAQEBBQADawAwaAJhAJJepXlnnJe5bFumOeyG8v2W8LFpudv3kOkV3eMapHGkl1TU6hgVCyNSOJgYgQkvozdZRJ5b9d7FJsrEhHbeR+klTcLYCe1u1HP7HbxdUfDU34szV/Tc0rE125WENSGXlwIDAQAB"
s0907._domainkey.qq.com. 806 IN TXT " t=y"

According to <http://www.ietf.org/rfc/rfc4871.txt> the p= field is required in the key record; I'm not sure if having it across several RRs is valid or not.

This does seem to be a real message, not spam, and mail from this user is currently dropped.

[Martin Pool (mbp)]

Probably this ought to be fixed in pydkim and then Launchpad just needs an update to that.

[Martin Pool (mbp)]

Having the key split across multiple RRs is fine: it's allowed by the spec and pydkim concatenates them apparently correctly. However, it should have semicolon separators and they're missing, which is invalid.

[Martin Pool (mbp)]

https://github.com/ghewgill/pydkim/pull/2

[Stuart Gathman (stuart-gathman)]

The current version gets a "KeyFormatError" exception for this and other publisher screwups. An email application can (and should) treat this differently than a signature failure - usually the same as if there were no DKIM sig at all.

For that matter, dropping mail on a DKIM sig failure would be problematic anyway because of persistent problems with MTA modifications. I recently talked to a guy who insisted that "optimizing" attachments by changing the charset and encoding to save a few bytes was a perfectly legit thing to do. If his attitude prevails, then DKIM is dead in the water. (Although that problem could be fixed by decoding all attachments and translating all charsets to UTF-8 before computing hashes - and attachments could be individually signed as well if you go to that trouble.)

[Stuart Gathman (stuart-gathman)]

Looks like Martins fix is competing. His prints a debug message and returns False. Mine throws a KeyFormatError (which other formatting problems will also throw).

[Stuart Gathman (stuart-gathman)]

The top level verify function, however, traps DKIMException (including KeyFormatError) and returns false, just like Martins verion.

[Martin Pool (mbp)]

Hi Stuart,

Where is your fix? I looked at the trunk on github and the version in Ubuntu Oneiric, but perhaps I'm just missing it.

I don't really care whether this is reported in the return code or by a KeyFormatError, as long as it's clearly distinguished from internal errors like a generic KeyError.

> For that matter, dropping mail on a DKIM sig failure would be problematic anyway because of persistent problems with MTA modifications.

I agree, we shouldn't do that. The problem here is that an unexpected exception causes Launchpad's incoming mail script to abort.

Possibly we should just catch exceptions very widely around dkim parsing and just treat any unexpected failure as a negative. Generally a broad catch papers over errors and isn't very attractive but perhaps it's pragmatic here.

[Martin Pool (mbp)]

> Possibly we should just catch exceptions very widely...

I mean, catch things in Launchpad, around it's call to pydkim. That would not be good to do within pydkim itself.

[Stuart Gathman (stuart-gathman)]

I made the fix on Aug 12, but committed to launchpad as revisions 71,72 today. (I'm new to bzr, so I may not have done something correctly.)

[Stuart Gathman (stuart-gathman)]

> Where is your fix?

Committed as revisions 71,72 to:

$ cat .bzr/branch/branch.conf
bound_location = bzr+ssh://bazaar.launchpad.net/%2Bbranch/pydkim/
bound = True

[Martin Pool (mbp)]

ok, i was just confused by there now being apparently two forks of pydkim :/

[Martin Pool (mbp)]

@stuart your patch in lp:pydkim looks fine to me.

[Scott Kitterman (kitterman)]

On 10/25/2011 07:32 PM, Martin Pool wrote:
> Where is your fix? I looked at the trunk on github and the version in
> Ubuntu Oneiric, but perhaps I'm just missing it.

The original pydkim author put his version up on github, but still isn't
responding to email, so we'll probably need to rename this at some
point, but our source is still on LP.

[Martin Pool (mbp)]

https://code.launchpad.net/~mbp/launchpad/881237-dkim-error/+merge/80413 makes Launchpad just tolerate errors coming out of pydkim.

We should also update the egg to get Stuart's fix.

[Martin Pool (mbp)]

qa: I fed the message linked above in to process-one-mail, and it treated it as ok but unverified, as it should.

i didn't see the message about an unexpected error, but rather pydkim detected this as an invalid key format. the problem is that qq.com has their key split across multiple RRs and the order in which they arrive (which is not guaranteed) determines exactly how it will trap. All of them are wrong though.

I don't think lp qastaging will be meaningfully different to my local test, and it's not very representative of lpnet, so qa-untestable.

[Scott Kitterman (kitterman)]

Fix for pydkim is included in pydkim 0.5 (the LP one) just released.

[Martin Pool (mbp)]

this is fixed in lp stable but still waiting to be deployed