Bug 7831 - DKIM_VALID_AU does not get set properly when mailing from a subdomain
Summary: DKIM_VALID_AU does not get set properly when mailing from a subdomain
Status: NEW
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Plugins (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: All All
: P2 major
Target Milestone: Undefined
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-27 01:07 UTC by Rob Mosher
Modified: 2020-11-24 09:21 UTC (History)
5 users (show)



Attachment Type Modified Status Actions Submitter/CLA Status
Patch to verify DKIM identities properly patch None Rob Mosher [NoCLA]
v2 of Patch to verify DKIM identities properly (trunk) patch None Rob Mosher [NoCLA]
v2 of Patch to verify DKIM identities properly (3.4) patch None Rob Mosher [NoCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Mosher 2020-06-27 01:07:35 UTC
RFC6376[1] states that mail authored from a subdomain of the domain specified by d= in the DKIM signature are valid unless t=s is set in the DKIM DNS record.

Spamassassin's DKIM plugin fails to handle subdomains correctly, only checking the sender against the domain specified in the DKIM mail header.  This causes DKIM_VALID_AU not to be set when there is a valid signed DKIM message.

[1] https://tools.ietf.org/html/rfc6376#section-3.10
Comment 1 Kevin A. McGrail 2020-06-27 15:05:00 UTC
Thank you.  That's a good find.  Can you work on a patch to resolve the issue?
Comment 2 RW 2020-06-28 14:15:59 UTC
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.
Comment 3 RW 2020-06-28 16:19:51 UTC
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
Comment 4 Rob Mosher 2020-06-28 17:19:10 UTC
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
Comment 5 Rob Mosher 2020-06-28 17:42:47 UTC
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
Comment 6 Rob Mosher 2020-06-30 05:56:22 UTC
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;
Comment 7 Rob Mosher 2020-06-30 05:58:14 UTC
Created attachment 5705 [details]
Patch to verify DKIM identities properly

patch attached as file
Comment 8 Rob Mosher 2020-07-06 00:18:36 UTC
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>
Comment 9 Rob Mosher 2020-07-06 02:04:08 UTC
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;
Comment 10 Rob Mosher 2020-07-06 02:04:49 UTC
Created attachment 5707 [details]
v2 of Patch to verify DKIM identities properly (trunk)
Comment 11 Rob Mosher 2020-07-06 02:05:21 UTC
Created attachment 5708 [details]
v2 of Patch to verify DKIM identities properly (3.4)
Comment 12 Henrik Krohns 2020-07-09 09:53:20 UTC
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..