Bug 7785 - DKIM fails with spamass-milter (CRLF problems)
Summary: DKIM fails with spamass-milter (CRLF problems)
Status: REOPENED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Plugins (show other bugs)
Version: 3.4.3
Hardware: All All
: P2 major
Target Milestone: Future
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-18 07:10 UTC by Henrik Krohns
Modified: 2022-04-11 12:57 UTC (History)
3 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Henrik Krohns 2020-01-18 07:10:13 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..
Comment 1 Henrik Krohns 2020-01-18 07:34:10 UTC 
(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
Comment 2 Henrik Krohns 2020-01-18 08:48:03 UTC 
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.
Comment 3 Henrik Krohns 2020-01-18 10:20:41 UTC 
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.
Comment 4 Henrik Krohns 2020-01-18 14:11:02 UTC 
Confirmed bug, wrote a patch for spamass-milter:

https://savannah.nongnu.org/bugs/index.php?57626
https://github.com/andybalholm/spamass-milter/issues/7
Comment 5 Kevin A. McGrail 2020-01-18 15:31:29 UTC 
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.
Comment 6 Henrik Krohns 2020-01-27 12:48:50 UTC 
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.
Comment 7 Henrik Krohns 2020-02-05 06:52:52 UTC 
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?
Comment 8 Henrik Krohns 2022-04-11 12:57:27 UTC 
Postponing into future, not important enough to delay 4.0.0.