SA Bugzilla – Bug 7785
DKIM fails with spamass-milter (CRLF problems)
Last modified: 2022-04-11 12:57:27 UTC
Referring to users-list discussion "Spamassassin always says DKIM_INVALID" It seems spamass-milter removes CR from wrapped headers when passing CRLF data to SpamAssassin: 2006-03-23 16:41 Dan Nelson <dnelson@allantgroup.com> * spamass-milter.cpp: Spamassassin 3.1.1 now emits headers with CRLF if the incoming message has CRLFs. Make sure that we strip the CR from wrapped headers when we parse the returned message. My "optimization" committed in 3.4.3 breaks Mail::DKIM verification, since it expects all lines ending in CRLF? http://svn.apache.org/viewvc/spamassassin/branches/3.4/lib/Mail/SpamAssassin/Plugin/DKIM.pm?diff_format=h&r1=1864870&r2=1864869&pathrev=1864870 Snippet of passed data to Mail::DKIM (CR is shown as ^M): DKIM-Filter: OpenDKIM Filter v2.10.3 mail.bristolweb.net 96BD3320531^M DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linkcheck.co.uk; s=mail; t=1579001879; bh=CxJX/YLOdkbC5W0v1up9JeX5a5WItUgcQd7vfnwSG5I=; h=To:From:Subject:Date:From; b=PXrrNHdBu6H/qOBbRBZR95X0qfxhAkZHyCDjJZcgAOlQtJ4vQCF1CQKCRR0iPCqKw q319iVwREf0DMInvjuCEha1uvEgo5fzf8Iw/sPKAbsx9bc/m+h/urQ76apiTS/uqD5 Xr93YQCSr6gDuqjZHlEjqUkWTAuRLO52JQiGTzKaxqpkc1+eRh7YyinSFRuu9L58+J LOD++Lb0kbOrOIZrkASVo2SnMyYEXnT+XtAS6uJOiueRfekk5dcQ5Co4ret+yUUaGM iKXqt8MGAiGIXYrtBWQKSV9k33eTqsItAS8F3t4zK8cJKfgGfLbMq38OBnRfOS5cpa DS3ZZ46Ie7sqw==^M To: Spamassassin <users@spamassassin.apache.org>^M If I add missing CR back to all the wrapped line endings in DKIM-Signature, the message passes. I guess if any of the specified h= (To:From etc..) headers were wrapped similarly, it would break on them too? So the main question is, should spamass-milter strip CR's from wrapped headers? Is this based on some RFC or why does it do it? The answer above affects whether SpamAssassin should naively apply s/\r?\n/\012\015/gs for data passed to Mail::DKIM, like earlier versions did. In this case, shouldn't the fix be done at the internal pristine message data level then, so get_pristine() returns correct data regardless what uses it? Also this makes me think, if get_pristine() can return CRLF endings, could this affect "full" rules if they assumed something about line endings? Or DCC/Pyzor which use it..
(In reply to Henrik Krohns from comment #0) > > Also this makes me think, if get_pristine() can return CRLF endings, could > this affect "full" rules if they assumed something about line endings? Or > DCC/Pyzor which use it.. Uhh yes indeed it affects rules. Found one.. full __FROM_NAME_IN_MSG /^From:\s+([^<]\S+\s\S+)\s(?=.{1,2048}^\1$)/sm Committing fix for it to accept CR ...^\1\r?$)sm
I think it's best to revert the 3.4 branch DKIM.pm change from Revision 1864870, it works and this won't delay 3.4.4 release anymore. Further work will go into trunk as needed, also the \012 vs \n handling etc need some fixing there. Sending spamassassin-3.4/lib/Mail/SpamAssassin/Plugin/DKIM.pm Transmitting file data .done Committing transaction... Committed revision 1872942.
RFC5322: - "A field body MUST NOT include CR and LF except when used in "folding" and "unfolding", as described in section 2.2.3." - "Unfolding is accomplished by simply removing any CRLF that is immediately followed by WSP." I don't see anything directly allowing standalone use of LF for header folding. So it would seem spamass-milter is stripping the CR for no reason at all. Nothing in SA requires it, atleast in modern versions. Not sure if spamass-milter is maintained anymore, but I can try filing a bug. I think I'll commit a workaround to trunk that fixes these badly folder headers at message parse time.
Confirmed bug, wrote a patch for spamass-milter: https://savannah.nongnu.org/bugs/index.php?57626 https://github.com/andybalholm/spamass-milter/issues/7
I will roll an RC for 3.4.4 this evening. I've confirmed all other issues we desired in this security release are fixed. This is triaged for 3.4 and open for a better fix in 4.0 as I read it.
More extensive fixes committed to trunk Sending trunk/MANIFEST Sending trunk/lib/Mail/SpamAssassin/Conf.pm Sending trunk/lib/Mail/SpamAssassin/Message.pm Sending trunk/lib/Mail/SpamAssassin/Plugin/DKIM.pm Sending trunk/lib/Mail/SpamAssassin.pm Adding trunk/t/data/dkim/test-pass-20.msg Adding trunk/t/data/dkim/test-pass-21.msg Adding trunk/t/data/dkim/test-pass-22.msg Adding trunk/t/data/dkim/test-pass-23.msg Adding trunk/t/data/nice/spf5-received-spf-crlf Adding trunk/t/data/nice/spf6-received-spf-crlf2 Adding trunk/t/data/spam/gtubedcc_crlf.eml Sending trunk/t/dcc.t Sending trunk/t/dkim.t Sending trunk/t/spf.t Transmitting file data ...............done Committing transaction... Committed revision 1873207.
Keeping this open, since milter-spamc seems to be (was) affected too. Lots of old discussion in Bug 5179, too much to digest fully right now. First: header\r\n X-Foo: line1\n line2\r\n \r\n body\r\n So not sure if we should fix wrapped \n -> \r\n at SA parse time, or just simply fix the feed to Mail::DKIM. This will affect how internal pristine message is stored also and outputted to other things. So much ambiguity. Also "full" rules newlines are affected depending on line endings. Should we just normalize the message to one format?
Postponing into future, not important enough to delay 4.0.0.