Milter-greylist

Hello, 

Currently, priv_rcptcount variable is only used for comparison against the 
'rcptcount' settings specified by the user, and not to store the count 
of 'accepted' rcpt entries in the priv_rcpt list structure.

% fgrep priv_rcptcount *
acl.c:	if (acl_opnum_cmp(priv->priv_rcptcount, ad->opnum.op, ad->opnum.num))
milter-greylist.c:	priv->priv_rcptcount = 0;
milter-greylist.c:	priv->priv_rcptcount++;
milter-greylist.h:	int priv_rcptcount;

However, priv_rcptcount is only incremented in add_recipient(), 
which is only called from real_envrcpt() upon an "exit_accept:" 
condition.

Battlefield experience shows that my 'rcptcount' 'flushaddr' rules are 
successfully bypassed, contributing to an increased greylist.db, and 
I think that this logic is to blame.

The patch below is supposed to address this behaviour, although it 
hasn't been tested in production as of yet.  Does it look all right?

Cheers,
Constantine A. Murenin,
University of Waterloo.

Index: milter-greylist.c
===================================================================
RCS file: /milter-greylist/milter-greylist/milter-greylist.c,v
retrieving revision 1.202
diff -u -d -p -4 -r1.202 milter-greylist.c
--- milter-greylist.c	29 Dec 2007 19:06:49 -0000	1.202
+++ milter-greylist.c	27 Jul 2008 21:58:47 -0000
@@ -496,8 +496,9 @@ real_envrcpt(ctx, envrcpt)
 	if ((priv = (struct mlfi_priv *) smfi_getpriv(ctx)) == NULL) {
 		mg_log(LOG_ERR, "Internal error: smfi_getpriv() returns NULL");
 		return SMFIS_TEMPFAIL;
 	}
+	priv->priv_rcptcount++;
 
 	if (!iptostring(SA(&priv->priv_addr), priv->priv_addrlen, addrstr,
 	    sizeof(addrstr)))
 		goto exit_accept;
@@ -1989,9 +1990,8 @@ add_recipient(priv, rcpt)
 	strncpy(nr->r_addr, rcpt, sizeof(nr->r_addr));
 	nr->r_addr[ADDRLEN] = '\0';
 
 	LIST_INSERT_HEAD(&priv->priv_rcpt, nr, r_list);
-	priv->priv_rcptcount++;
 	return;
 }
 
 static void

Constantine A. Murenin <mureninc@...> wrote:

> The patch below is supposed to address this behaviour, although it 
> hasn't been tested in production as of yet.  Does it look all right?

There is one small problem: if real_rcpt returns tempfail or reject, you
counted an addicitonnal recipient that does not exist.

So your fix may improve the situation, but still contain a bug. I fail
to understand what logic is to blame: in what situation the counter
increasse in add_recipient hurts?

-- 
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
manu@...

On Mon, Jul 28, 2008 at 04:49:51AM +0200, manu@... wrote:
> Constantine A. Murenin <mureninc@...> wrote:
> 
> > The patch below is supposed to address this behaviour, although it 
> > hasn't been tested in production as of yet.  Does it look all right?
> 
> There is one small problem: if real_rcpt returns tempfail or reject, you
> counted an addicitonnal recipient that does not exist.

Well, that's more of an intended behaviour -- we want to use rcptcount 
to blacklist spammers that send messages to possibly non-existing addresses, 
and not valid cases where the recipients are actually good to go, right? :)

> So your fix may improve the situation, but still contain a bug. I fail
> to understand what logic is to blame: in what situation the counter
> increasse in add_recipient hurts?

Right after the initial set of "acl whitelist" rules, I have 
the following rule:

	acl blacklist rcptcount >= 3 flushaddr msg "sorry, you're flushed"

Yet fgrepping through /var/log/maillog, which averages 30MB a day, 
returns no signs that this rather strict rule has ever matched. 
Looking closer at certain entries in greylist.db and then tracking 
them through /var/log/maillog, or just looking at maillog alone, 
shows that some spambots do indeed try more than 2 addresses 
in a single transaction, yet the rule never clicks.

Example from maillog:

Jul 27 00:06:36 ???? sm-mta[92561]: ruleset=check_relay, arg1=[211.94.125.139], arg2=211.94.125.139, relay=[211.94.125.139], reject=421 4.3.2 Connection rate limit exceeded.
Jul 27 00:06:42 ???? milter-greylist: m6R46eU1092574: addr [77.31.231.216][77.31.231.216] from <yahvarc@...> to <patcassel@???????????.??> delayed for 01:00:00 (ACL 286)
Jul 27 00:06:42 ???? sm-mta[92574]: m6R46eU1092574: Milter: to=<patcassel@???????????.??>, reject=451 4.7.1 Temporary failure, please try again later. (ESMTP spamd IP-based SPAM blocker)
Jul 27 00:06:42 ???? milter-greylist: m6R46eU1092574: addr [77.31.231.216][77.31.231.216] from <yahvarc@...> to <parque-jones@???????????.??> delayed for 01:00:00 (ACL 286)
Jul 27 00:06:42 ???? sm-mta[92574]: m6R46eU1092574: Milter: to=<parque-jones@???????????.??>, reject=451 4.7.1 Temporary failure, please try again later. (ESMTP spamd IP-based SPAM blocker)
Jul 27 00:06:42 ???? milter-greylist: m6R46eU1092574: addr [77.31.231.216][77.31.231.216] from <yahvarc@...> to <parraco@???????????.??> delayed for 01:00:00 (ACL 286)
Jul 27 00:06:42 ???? sm-mta[92574]: m6R46eU1092574: Milter: to=<parraco@???????????.??>, reject=451 4.7.1 Temporary failure, please try again later. (ESMTP spamd IP-based SPAM blocker)
Jul 27 00:06:42 ???? milter-greylist: m6R46eU1092574: addr [77.31.231.216][77.31.231.216] from <yahvarc@...> to <pault2@???????????.??> delayed for 01:00:00 (ACL 286)
Jul 27 00:06:42 ???? sm-mta[92574]: m6R46eU1092574: Milter: to=<pault2@???????????.??>, reject=451 4.7.1 Temporary failure, please try again later. (ESMTP spamd IP-based SPAM blocker)
Jul 27 00:06:42 ???? sm-mta[92574]: m6R46eU1092574: lost input channel from [77.31.231.216] to IPv4 after data
Jul 27 00:06:42 ???? sm-mta[92574]: m6R46eU1092574: from=<yahvarc@...>, size=0, class=0, nrcpts=0, proto=ESMTP, daemon=IPv4, relay=[77.31.231.216]
Jul 27 00:06:59 ???? milter-greylist: m6R46vcS092618: addr bzq-84-109-52-26.red.bezeqint.net[84.109.52.26] from <trnygo@...> to <one4jettn@???????????.??> delayed for 01:00:00 (ACL 286)

Yes, in this example sendmail itself mentions nrcpts=0, but that has 
a different meaning and name than an 'rcptcount' feature in a greylisting 
software, where tempfail is the norm, not the exception.

I do think there's something wrong if I have a rule to flush all the 
tuples from a given IP-address when more than 2 recipients per envelope 
are ever presented, yet milter-greylist wastes my memory by greylisting 
even more tuples from the spambot in question, and I do think that 
my patch indeed solves this problem.

Best regards,
Constantine A. Murenin,
University of Waterloo.

On Mon, Jul 28, 2008 at 06:48:55AM +0100, Constantine A. Murenin wrote:
> Well, that's more of an intended behaviour -- we want to use rcptcount 
> to blacklist spammers that send messages to possibly non-existing addresses, 
> and not valid cases where the recipients are actually good to go, right? :)

Ok, so we want two counters: one for valid recipients, and one for
attempted recipient. I wonder if we should also have tempfailed and
rejected recipient count available as well. Opinions?

It seems rcptcount will go deprecated and we will introduce new
counter names. In order to avoid longer lines, I thought about that:
ircount: input recipient count
arcount: accepted recipient count (like old rcptcount)
trcount: tempfail'ed recipient count
rrcount: rejected recipient count
nrcount: not accepted recipient count (trcount+rrcount)

-- 
Emmanuel Dreyfus
manu@...

Sound good to me, but PLEASE drop some explanation comment in the
sources as well :-)
Ondrej

Emmanuel Dreyfus wrote:

>
> On Mon, Jul 28, 2008 at 06:48:55AM +0100, Constantine A. Murenin wrote:
> > Well, that's more of an intended behaviour -- we want to use rcptcount
> > to blacklist spammers that send messages to possibly non-existing
> addresses,
> > and not valid cases where the recipients are actually good to go,
> right? :)
>
> Ok, so we want two counters: one for valid recipients, and one for
> attempted recipient. I wonder if we should also have tempfailed and
> rejected recipient count available as well. Opinions?
>
> It seems rcptcount will go deprecated and we will introduce new
> counter names. In order to avoid longer lines, I thought about that:
> ircount: input recipient count
> arcount: accepted recipient count (like old rcptcount)
> trcount: tempfail'ed recipient count
> rrcount: rejected recipient count
> nrcount: not accepted recipient count (trcount+rrcount)
>
> -- 
> Emmanuel Dreyfus
> manu@... <mailto:manu%40netbsd.org>
>
>

On 28/07/2008, Emmanuel Dreyfus <manu@...> wrote:
> On Mon, Jul 28, 2008 at 06:48:55AM +0100, Constantine A. Murenin wrote:
>  > Well, that's more of an intended behaviour -- we want to use rcptcount
>  > to blacklist spammers that send messages to possibly non-existing addresses,
>  > and not valid cases where the recipients are actually good to go, right? :)
>
>
> Ok, so we want two counters: one for valid recipients, and one for
>  attempted recipient. I wonder if we should also have tempfailed and
>  rejected recipient count available as well. Opinions?
>
>  It seems rcptcount will go deprecated and we will introduce new
>  counter names. In order to avoid longer lines, I thought about that:
>  ircount: input recipient count
>  arcount: accepted recipient count (like old rcptcount)
>  trcount: tempfail'ed recipient count
>  rrcount: rejected recipient count
>  nrcount: not accepted recipient count (trcount+rrcount)

Sounds a bit too excessive. :) I think rcptcount, counting the number
of RCPT statements, whether successful or not, should be quite enough.
These "ircount" statements are not saying much by themselves, and More
is Less (tm), so I'd rather prefer an 'rcptcount' keyword with the
obvious semantics that is not meaningless for greylisting.

C.

What about using structures?
struct t_count {
int input_rcpt, accept_rcpt,....;
} counters;

That's the most elegant solution to me.
....
small note: well, if it was completely up to me, I would go for classes
and C++, but that's a different story :-)

Ondrej
Constantine A. Murenin wrote:

>
> On 28/07/2008, Emmanuel Dreyfus <manu@...
> <mailto:manu%40netbsd.org>> wrote:
> > On Mon, Jul 28, 2008 at 06:48:55AM +0100, Constantine A. Murenin wrote:
> > > Well, that's more of an intended behaviour -- we want to use rcptcount
> > > to blacklist spammers that send messages to possibly non-existing
> addresses,
> > > and not valid cases where the recipients are actually good to go,
> right? :)
> >
> >
> > Ok, so we want two counters: one for valid recipients, and one for
> > attempted recipient. I wonder if we should also have tempfailed and
> > rejected recipient count available as well. Opinions?
> >
> > It seems rcptcount will go deprecated and we will introduce new
> > counter names. In order to avoid longer lines, I thought about that:
> > ircount: input recipient count
> > arcount: accepted recipient count (like old rcptcount)
> > trcount: tempfail'ed recipient count
> > rrcount: rejected recipient count
> > nrcount: not accepted recipient count (trcount+rrcount)
>
> Sounds a bit too excessive. :) I think rcptcount, counting the number
> of RCPT statements, whether successful or not, should be quite enough.
> These "ircount" statements are not saying much by themselves, and More
> is Less (tm), so I'd rather prefer an 'rcptcount' keyword with the
> obvious semantics that is not meaningless for greylisting.
>
> C.
>
>

On Mon, Jul 28, 2008 at 04:12:17PM +0200, Ondrej Valousek wrote:
> What about using structures?
> struct t_count {
> int input_rcpt, accept_rcpt,....;
> } counters;
> 
> That's the most elegant solution to me.
> ....

Sure, but I was thinking about the config file...

> small note: well, if it was completely up to me, I would go for classes
> and C++, but that's a different story :-)

And then you would have to find another maintainer for this software :-)

-- 
Emmanuel Dreyfus
manu@...