broken dkim key on causes mail to be dropped

Bug #881237 reported by Martin Pool on 2011-10-25
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
Martin Pool
Martin Pool

Bug Description

OOPS-2121INBOUNDEMAIL1 shows an incoming mail (saved to which fails in dkim verification:

Traceback (most recent call last):
  Module, line 398, in handleMail
  Module, line 472, in handle_one_mail
    mail, signature_timestamp_checker)
  Module, line 203, in authenticateEmail
    dkim_trusted_addr = _authenticateDkim(mail)
  Module, 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;; s=s0907;
 t=1319283879; bh=YNYe6JlvFtLnPIyBgQ/s3tPBunXKtwylEzBLrkWuD1E=;

This points to which has 806 IN TXT "k=rsa" 806 IN TXT " p=MHwwDQYJKoZIhvcNAQEBBQADawAwaAJhAJJepXlnnJe5bFumOeyG8v2W8LFpudv3kOkV3eMapHGkl1TU6hgVCyNSOJgYgQkvozdZRJ5b9d7FJsrEhHbeR+klTcLYCe1u1HP7HbxdUfDU34szV/Tc0rE125WENSGXlwIDAQAB" 806 IN TXT " t=y"

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

Related branches

Martin Pool (mbp) wrote :

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

Changed in pydkim:
status: New → Confirmed
importance: Undecided → High
Martin Pool (mbp) on 2011-10-25
description: updated
Martin Pool (mbp) wrote :

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.

summary: - incoming mail lacking required DKIM 'p' field causes KeyError oops
+ broken dkim key on causes mail to be dropped
Martin Pool (mbp) wrote :
Changed in launchpad:
assignee: nobody → Martin Pool (mbp)
Changed in pydkim:
assignee: nobody → Martin Pool (mbp)
status: Confirmed → In Progress
Stuart Gathman (stuart-gathman) wrote :

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) wrote :

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) wrote :

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

Martin Pool (mbp) wrote :

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) wrote :

> 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) wrote :

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

> Where is your fix?

Committed as revisions 71,72 to:

$ cat .bzr/branch/branch.conf
bound_location = bzr+ssh://
bound = True

Martin Pool (mbp) wrote :

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

Martin Pool (mbp) wrote :

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

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) wrote : makes Launchpad just tolerate errors coming out of pydkim.

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

Changed in launchpad:
status: Triaged → In Progress
Martin Pool (mbp) wrote :

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

tags: added: qa-untestable
Scott Kitterman (kitterman) wrote :

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

Changed in pydkim:
status: In Progress → Fix Released
Martin Pool (mbp) wrote :

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

Changed in launchpad:
status: In Progress → Fix Committed
Changed in launchpad:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers