benningm / mtpolicyd

a modular policy daemon for postfix
21 stars 3 forks source link

SPF check on null Sender not supported #4

Closed kitterma closed 9 years ago

kitterma commented 9 years ago

In the SPF plugin, there is the code snippet:

my $ip = $r->attr('client_address');
my $sender = $r->attr('sender');
my $helo = $r->attr('helo_name');

if( ! defined $ip || ! defined $sender || ! length($sender) ) {
    $self->log( $r, 'cant check SPF without client_address and sender');
    return;
}

This isn't correct. You can and should check SPF without a sender. RFC 7208 section 2.2 paragraph 2 says (it's predecessor, RFC 4408 had the same requirement):

[RFC5321] allows the reverse-path to be null (see Section 4.5.5 in [RFC5321]). In this case, there is no explicit sender mailbox, and such a message can be assumed to be a notification message from the mail system itself. When the reverse-path is null, this document defines the "MAIL FROM" identity to be the mailbox composed of the local-part "postmaster" and the "HELO" identity (which might or might not have been checked separately before).

So rather than bail out if sender is none, the correct action would be to check using "postmaster@" + helo as the sender.

benningm commented 9 years ago

The openspf.org policyd does also not implement mfrom for empty senders:

http://bazaar.launchpad.net/~kitterman/postfix-policyd-spf-perl/trunk/view/head:/postfix-policyd-spf-perl#L299

May be we should also do the helo check before.

kitterma commented 9 years ago

It does, it just does it wrong. When I did that, I misread the spec to mean if sender is null, you should do a helo check (which Mail::SPF supports). What you should really do is a mfrom check with postmaster@$HELO. I intend to correct that, but haven't gotten to it yet. Except in the case of SPF records with macros (which are very rare), it will get the same result, so I don't consider it a high priority, but since you're implementing for the first time, you may as well do it right.

Doing a HELO check first is something people argue about. Rejecting based on HELO not passing is very safe (no forwarding issues like SPF on Mail From might have), so it's a low risk way to get rid of some junk. OTOH, SPF records for hostnames typically used in HELO are relatively rare, so it's also less likely to help. SPF records for HELO tend to be very simple, so they don't generally cause much in the way of DNS loading.

On balance, I'm in favor of doing them first, but I can see it's also a reasonable design choice to only do them on null sender or after checking sender.

benningm commented 9 years ago

Thank you for help on this.

I just pushed an updated version of the SPF plugin with helo check and the postmaster on null sender change. I'm not sure if it is 100% right but i think i should read the RFC and implement some more test cases.

benningm commented 9 years ago

fixed in release 1.16