Bug 7831

Thank you.  That's a good find.  Can you work on a patch to resolve the issue?

Just to be clear, which of these two have you seen fail:

A.  DKIM-Signature: ... i=john@foo.example.com ... d=example.com


B   DKIM-Signature: ... d=example.com 
    From: john@foo.example.com


My understanding is that the DKIM RFC passage is referring to A only.

I should elaborate a bit. As I understand it section 3.10 is about the validity of signature headers like the one in A. It's not about alignment with RFC5322.From. However, if a combination like this:

  DKIM-Signature: ... i=john@foo.example.com ... d=example.com
  From: john@foo.example.com

hits DKIM_VALID but not DKIM_VALID_AU then that's clearly a bug.

The case of B is either handled by DMARC or, in the case of DKIM_VALID_AU, it's a design decision - DKIM_VALID_AU is equivalent to DMARC's strict DKIM alignment.

IMO SA could benefit from a relaxed version of  DKIM_VALID_AU, as well as a relaxed author aligned version of SPF_PASS

The bug is here, as it only checks author domain against d=

https://github.com/apache/spamassassin/blob/df83e64408fe1eb76d4a4fd7e0c6a730187ce2f8/lib/Mail/SpamAssassin/Plugin/DKIM.pm#L917


  DKIM-Signature: ... i=john@foo.example.com ... d=example.com
  From: john@foo.example.com

This would only hit DKIM_VALID, and not DKIM_VALID_AU

I'll put a patch together when I get a few minutes

As long as i= is a subdomain of d=, it should check the message From: against i= unless t=s is present in the DKIM domain record

It's a simple patch but it took a bit of research as I'm not familiar with any of these code bases.  Some review would be good.

Some notes...

In Mail::DKIM, the identity will not validate if t=s is set and the identity is a subdomain of the domain.  This is already handled correctly so I don't think we need to add any additional logic.

If an identity is not specified, it is filled in with @domain, so it should be safe to use in place in those cases.

https://metacpan.org/release/Mail-DKIM/source/lib/Mail/DKIM/Signature.pm#L458

check_dkim_valid_envelopefrom also wasn't checking the full return path domain, and just the base domain.

Test from valid subdomain
X-Spam-Status: No, score=1.4 required=4.0 tests=BAYES_00,BODY_SINGLE_WORD,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FSL_BULK_SIG,
	PYZOR_CHECK,SPF_HELO_PASS,SPF_PASS autolearn=no autolearn_force=no
	version=3.4.2

Test from the base domain
X-Spam-Status: No, score=1.4 required=4.0 tests=BAYES_00,BODY_SINGLE_WORD,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FSL_BULK_SIG,
	PYZOR_CHECK,SPF_HELO_PASS,SPF_PASS autolearn=no autolearn_force=no
	version=3.4.2

Test mailing from subdomain with t=s set in domain record
X-Spam-Status: No, score=1.8 required=4.0 tests=BAYES_00,BODY_SINGLE_WORD,
	DKIM_INVALID,DKIM_SIGNED,FSL_BULK_SIG,PYZOR_CHECK,SPF_HELO_PASS,
	SPF_PASS autolearn=no autolearn_force=no version=3.4.2


--- a/lib/Mail/SpamAssassin/Plugin/DKIM.pm
+++ b/lib/Mail/SpamAssassin/Plugin/DKIM.pm
@@ -561,9 +561,10 @@ sub check_dkim_valid_author_sig {
 sub check_dkim_valid_envelopefrom {
   my ($self, $pms, $full_ref) = @_;
   my $result = 0;
-  my $envfrom=$self->{'main'}->{'registryboundaries'}->uri_to_domain($pms->get("EnvelopeFrom"));
+  my ( $envfrom ) = $pms->get("EnvelopeFrom") =~ /@([a-z0-9\-\.]*)/i;
   # if no envelopeFrom, it cannot be valid
   return $result if !$envfrom;
+  $envfrom = lc $envfrom;
   $self->_check_dkim_signature($pms)  if !$pms->{dkim_checked_signature};
   if (!$pms->{dkim_valid}) {
     # don't bother
@@ -720,7 +721,7 @@ sub _check_dkim_signed_by {
       next if $minimum_key_bits && $sig->{_spamassassin_key_size} &&
               $sig->{_spamassassin_key_size} < $minimum_key_bits;
     }
-    my $sdid = $sig->domain;
+    my ( $sdid ) = $sig->identity =~ /@(.*)/;
     next if !defined $sdid;  # a signature with a missing required tag 'd' ?
     $sdid = lc $sdid;
     if ($must_be_author_domain_signature) {
@@ -909,7 +910,7 @@ sub _check_dkim_signature {
       push(@valid_signatures, $signature)  if $valid && !$expired;

       # check if we have a potential Author Domain Signature, valid or not
-      my $d = $signature->domain;
+      my ( $d ) = $signature->identity =~ /@(.*)/;
       if (!defined $d) {
         # can be undefined on a broken signature with missing required tags
       } else {
@@ -1261,7 +1262,7 @@ sub _wlcheck_list {
       }
     }

-    my $sdid = $signature->domain;
+    my ( $sdid ) = $signature->identity =~ /@(.*)/;
     $sdid = lc $sdid  if defined $sdid;

     my %tried_authors;

Created attachment 5705 [details]
Patch to verify DKIM identities properly

patch attached as file

also, PerMsgStatus->_get("EnvelopeFrom") is broken when there are multiple Return-Path headers specified.

I should be using get("EnvelopeFrom:addr") in this patch, but it wasn't working right in my tests.  I'll update this patch to use it properly and include a patch to fix PerMsgStatus.pm

I've also found some bugs in SPF.pm, some of that due it returning junk when there are multiple Return-Path headers found.  Others due to not checking for empty strings.  I'll open up a bug report for all of these.

Testing:

    $sender = $scanner->get("EnvelopeFrom:addr");
    dbg("spf: pms:from:addr " . $sender);
    dbg("spf: pms:from:raw " . $scanner->get("EnvelopeFrom:raw"));
    dbg("spf: pms:from " . $scanner->get("EnvelopeFrom"));
    dbg("spf: pms:get:rp " . $scanner->get("Return-Path"));

$ grep Return-Path: mail2.txt
Return-Path: <user@domain.com>
Return-Path: <user@domain.com>

dbg: spf: pms:from:addr user@domain.com> <user@domain.com
dbg: spf: pms:from:raw user@domain.com>
dbg: spf: pms:from user@domain.com>
dbg: spf: pms:get:rp <user@domain.com>

But with only one Return-Path:
dbg: spf: pms:from:addr user@domain.com
dbg: spf: pms:from:raw user@domain.com
dbg: spf: pms:from user@domain.com
dbg: spf: pms:get:rp <user@domain.com>

adjusted... 

Here's the trunk patch

--- a/lib/Mail/SpamAssassin/Plugin/DKIM.pm
+++ b/lib/Mail/SpamAssassin/Plugin/DKIM.pm
@@ -561,9 +561,10 @@ sub check_dkim_valid_author_sig {
 sub check_dkim_valid_envelopefrom {
   my ($self, $pms, $full_ref) = @_;
   my $result = 0;
-  my $envfrom=$self->{'main'}->{'registryboundaries'}->uri_to_domain($pms->get("EnvelopeFrom"));
+  my ( $envfrom ) = $pms->get("EnvelopeFrom:addr") =~ /@(\S*)/i;
   # if no envelopeFrom, it cannot be valid
   return $result if !$envfrom;
+  $envfrom = lc $envfrom;
   $self->_check_dkim_signature($pms)  if !$pms->{dkim_checked_signature};
   if (!$pms->{dkim_valid}) {
     # don't bother
@@ -720,7 +721,7 @@ sub _check_dkim_signed_by {
       next if $minimum_key_bits && $sig->{_spamassassin_key_size} &&
               $sig->{_spamassassin_key_size} < $minimum_key_bits;
     }
-    my $sdid = $sig->domain;
+    my ( $sdid ) = $sig->identity =~ /@(\S*)/;
     next if !defined $sdid;  # a signature with a missing required tag 'd' ?
     $sdid = lc $sdid;
     if ($must_be_author_domain_signature) {
@@ -909,7 +910,7 @@ sub _check_dkim_signature {
       push(@valid_signatures, $signature)  if $valid && !$expired;

       # check if we have a potential Author Domain Signature, valid or not
-      my $d = $signature->domain;
+      my ( $d ) = $signature->identity =~ /@(\S*)/;
       if (!defined $d) {
         # can be undefined on a broken signature with missing required tags
       } else {
@@ -1261,7 +1262,7 @@ sub _wlcheck_list {
       }
     }

-    my $sdid = $signature->domain;
+    my ( $sdid ) = $signature->identity =~ /@(\S+)/;
     $sdid = lc $sdid  if defined $sdid;

     my %tried_authors;

And here's the 3.4 patch

--- spamassassin-3.4.4/lib/Mail/SpamAssassin/Plugin/DKIM.pm     2020-01-18 03:44:49.000000000 -0500
+++ /usr/share/perl5/Mail/SpamAssassin/Plugin/DKIM.pm   2020-07-05 21:55:06.897221239 -0400
@@ -560,9 +560,10 @@ sub check_dkim_valid_author_sig {
 sub check_dkim_valid_envelopefrom {
   my ($self, $pms, $full_ref) = @_;
   my $result = 0;
-  my $envfrom=$self->{'main'}->{'registryboundaries'}->uri_to_domain($pms->get("EnvelopeFrom"));
+  my ( $envfrom ) = $pms->get("EnvelopeFrom:addr") =~ /@(\S*)/i;
   # if no envelopeFrom, it cannot be valid
   return $result if !$envfrom;
+  $envfrom = lc $envfrom;
   $self->_check_dkim_signature($pms)  if !$pms->{dkim_checked_signature};
   if (!$pms->{dkim_valid}) {
     # don't bother
@@ -719,7 +720,7 @@ sub _check_dkim_signed_by {
       next if $minimum_key_bits && $sig->{_spamassassin_key_size} &&
               $sig->{_spamassassin_key_size} < $minimum_key_bits;
     }
-    my $sdid = $sig->domain;
+    my ( $sdid ) = $sig->identity =~ /@(\S*)/;
     next if !defined $sdid;  # a signature with a missing required tag 'd' ?
     $sdid = lc $sdid;
     if ($must_be_author_domain_signature) {
@@ -902,7 +903,7 @@ sub _check_dkim_signature {
       push(@valid_signatures, $signature)  if $valid && !$expired;

       # check if we have a potential Author Domain Signature, valid or not
-      my $d = $signature->domain;
+      my ( $d ) = $signature->identity =~ /@(\S*)/;
       if (!defined $d) {
         # can be undefined on a broken signature with missing required tags
       } else {
@@ -1254,7 +1255,7 @@ sub _wlcheck_list {
       }
     }

-    my $sdid = $signature->domain;
+    my ( $sdid ) = $signature->identity =~ /@(\S+)/;
     $sdid = lc $sdid  if defined $sdid;

     my %tried_authors;

Created attachment 5707 [details]
v2 of Patch to verify DKIM identities properly (trunk)

Created attachment 5708 [details]
v2 of Patch to verify DKIM identities properly (3.4)

Looks reasonable, committing to trunk for testing..

Sending        trunk/lib/Mail/SpamAssassin/Plugin/DKIM.pm
Transmitting file data .done
Committing transaction...
Committed revision 1879692.

I'd like to make a test case for this too, will look later..

I'm getting "uninitialized value in pattern match" errors with this patch, where it's trying to extract the domain from a signature identity:

Jan 23 10:35:40.102 [19982] dbg: dkim: performing public key lookup and signature verification
Jan 23 10:35:40.104 [19982] dbg: dkim: FAILED DKIM, i=transfer-your-credit-balance@3harmfullfoods.com, d=3harmfullfoods.com, s=dkim, a=rsa-sha1, c=relaxed/relaxed, unknown key size, invalid, matches author domain
Use of uninitialized value in pattern match (m//) at /home/jhardin/develop/spamassassin/svn/trunk/masses/../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 913.
Jan 23 10:35:40.104 [19982] dbg: dkim: FAILED DK, i=(undef), d=(undef), s=dkim, a=rsa-sha1, c=nofws, unknown key size, invalid, does not match author domain
Jan 23 10:35:40.104 [19982] dbg: dkim: signature verification result: INVALID (PUBLIC KEY: NOT AVAILABLE)
Jan 23 10:35:40.104 [19982] dbg: dkim: FAILED signature by 3harmfullfoods.com, author transfer-your-credit-balance@3harmfullfoods.com, no valid matches
Use of uninitialized value in pattern match (m//) at /home/jhardin/develop/spamassassin/svn/trunk/masses/../blib/lib/Mail/SpamAssassin/Plugin/DKIM.pm line 1265.
Jan 23 10:35:40.104 [19982] dbg: dkim: FAILED signature by (undef), author transfer-your-credit-balance@3harmfullfoods.com, no valid matches
Jan 23 10:35:40.104 [19982] dbg: dkim: author transfer-your-credit-balance@3harmfullfoods.com, not in any dkim whitelist

A DK header referring a domain that does not exist/does not publish any DKIM record seems to be a failure case.


> If an identity is not specified, it is filled in with @domain, so it should be safe to use in place in those cases.

This doesn't actually appear to be happening for the DK header check.

The DK signature check is *not* setting the identity from the domain (which *is* present - the "d=" value in the log above is the domain extracted from the identity value, not $signature->domain) and the SA code can't update that value to repair it even though the Mail::DKIM documentation suggests that is possible.

This appears to be a bug in Mail::DKIM - it's occurring on the latest version, 1.20200907. I have not filed an upstream bug.


Here are the DKIM/DK headers from that message:

DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; s=dkim; d=3harmfullfoods.com;
 h=Date:From:To:Subject:MIME-Version:Content-Type:List-Unsubscribe:Message-ID; i=transfer-your-credit-balance@3harmfullfoods.com;
 bh=6Md+1tEoP1V8A8eusTw2Aml04jw=;
 b=Rp6RdJadb6WCcr3WQRh4ArRFaX+SZERqDJfbBhUFc5cUPZeBXNjfoFxRZ+cnSF9sMbcK5GhJ6FyU
   rgTcnZxOiMtABwizp+94SVa3i3oSi5wf9H7kl25rZy/yydPOMdd1Gq1xx2xI3HjmqkUFFZDnt4YY
   C8KEIiqJ1jX2agM4atU=
DomainKey-Signature: a=rsa-sha1; c=nofws; q=dns; s=dkim; d=3harmfullfoods.com;
 b=DW+bNtslRBdaAIIoQlwVJTbdj13CQ06RVB/bhG+hWucu3JZz2rMHPN3r1vr6j0Q9UrZVdyy+X5iy
   4RxwkXnx2Kb6Wj96v24TuyLkN+IS3S64g9xD/8eehFqkkBgXlfBPpBySjXOjCRLcP9KVv6Ite6QN
   ujl/lQsqYxoBS7AyoaI=;

The DKIM header processes cleanly, the DK one blows up. I have multiple messages from various (apparently bogus) domains in my corpus that exhibit this behavior.


Fixing the code to react gracefully to missing identity, and adding a bit more logging...

Modified: trunk/lib/Mail/SpamAssassin/Plugin/DKIM.pm
Committed revision 1885854.

https://bugs.gentoo.org/758935

This looks finished in trunk.