Milter-greylist

I made three changes against milter-greylist. They are conceptually
independent from each other, but patches can only be applied in the
right order.

1. may_be_forged ACL condition
http://www.j10n.org/files/milter-greylist-3.0a2-step1.patch

When a client has bogus reverse DNS, that is, IP -> PTR -> A != IP,
sendmail sets {client_resolve} macro to FORGED. This patch implements
ACL condition to take advantage of it. You need to add
{client_resolve} into Milter.macros.connect. Example:
acl blacklist domain /.*\.info/ may_be_forged
acl greylist may_be_forged

2. Threading bug fix
http://www.j10n.org/files/milter-greylist-3.0a2-step2.patch

This patch resolvs number of race conditions in threading
code. It also includes some minor modifications. Since my changes
affect almost everywhere, code reviews are welcome.

3. Binary search tree
http://www.j10n.org/files/milter-greylist-3.0a2-step3.patch

This patch introduces red-black tree structure to manage the greylist
and autowhitelist. It is just a pedantic change. No realistic
improvement is expected.

AIDA Shinra <shinra@...> wrote:

> 1. may_be_forged ACL condition
> http://www.j10n.org/files/milter-greylist-3.0a2-step1.patch
> 
> When a client has bogus reverse DNS, that is, IP -> PTR -> A != IP,
> sendmail sets {client_resolve} macro to FORGED. This patch implements
> ACL condition to take advantage of it. You need to add
> {client_resolve} into Milter.macros.connect. Example:
> acl blacklist domain /.*\.info/ may_be_forged
> acl greylist may_be_forged

Some thoughts:
- may_be_forged name is a bit long to read in ACL. What about just
forged?
- Changes to documentation for the new feature would be nice :-)
- Do you catch a lot of spam with that?

> 2. Threading bug fix
> http://www.j10n.org/files/milter-greylist-3.0a2-step2.patch
> 
> This patch resolvs number of race conditions in threading
> code. It also includes some minor modifications. Since my changes
> affect almost everywhere, code reviews are welcome.

There are huge changes. Could you explain what was wrong?

> 3. Binary search tree
> http://www.j10n.org/files/milter-greylist-3.0a2-step3.patch
> 
> This patch introduces red-black tree structure to manage the greylist
> and autowhitelist. It is just a pedantic change. No realistic
> improvement is expected.

I'm a bit reluctant to make changes that buy nothing. What's the
advantage of adding this?

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

At 09:23 PM 8.21.2006 +0200, manu@... wrote:
>AIDA Shinra <shinra@...> wrote:
>
>> 1. may_be_forged ACL condition
>> http://www.j10n.org/files/milter-greylist-3.0a2-step1.patch
>> 
>> When a client has bogus reverse DNS, that is, IP -> PTR -> A != IP,
>> sendmail sets {client_resolve} macro to FORGED. This patch implements
>> ACL condition to take advantage of it. You need to add
>> {client_resolve} into Milter.macros.connect. Example:
>> acl blacklist domain /.*\.info/ may_be_forged
>> acl greylist may_be_forged
>
>Some thoughts:
>- may_be_forged name is a bit long to read in ACL. What about just
>forged?
>- Changes to documentation for the new feature would be nice :-)
>- Do you catch a lot of spam with that?
>

This "may be forged" thing would have a lot of FPs methinks......

(^_^)
Happy trails,
Jack L. Stone

System Admin
Sage-american

Jack L. Stone wrote:
> At 09:23 PM 8.21.2006 +0200, manu@... wrote:
>> AIDA Shinra <shinra@...> wrote:
>>
>>> 1. may_be_forged ACL condition
>>> http://www.j10n.org/files/milter-greylist-3.0a2-step1.patch
>>>
>>> When a client has bogus reverse DNS, that is, IP -> PTR -> A != IP,
>>> sendmail sets {client_resolve} macro to FORGED. This patch implements
>>> ACL condition to take advantage of it. You need to add
>>> {client_resolve} into Milter.macros.connect. Example:
>>> acl blacklist domain /.*\.info/ may_be_forged
>>> acl greylist may_be_forged
>> Some thoughts:
>> - may_be_forged name is a bit long to read in ACL. What about just
>> forged?
>> - Changes to documentation for the new feature would be nice :-)
>> - Do you catch a lot of spam with that?
>>
> 
> This "may be forged" thing would have a lot of FPs methinks......
> 
> 

True, but it's still likely useful if you also use the per-acl greylist delay
time, and use it as a criteria for a longer greylist period.

Or, if you're like me and have your default set to whitelist, you could use this
as one of your greylist conditions. Right now I greylist no RDNS, APNIC, LACNIC,
and dialup-looking hosts, then default to whitelist. Eventually I'd like to
greylist based on spamcop and NJABL-DUL lookups, as well as my no-RDNS, apnic
and lacnic rules. If this feature gets added, I'll probably use it as another
criteria to trigger greylisting.

However, right now I'm having trouble building the 3.0a series and haven't had
time to play with fixing it, but I've seen a lot of discussion of it. (undefined
reference to `__ns_init parse')

Matt Kettler <mkettler@...> wrote:

> Or, if you're like me and have your default set to whitelist, you could
> use this as one of your greylist conditions. Right now I greylist no RDNS,
> APNIC, LACNIC, and dialup-looking hosts, then default to whitelist.
> Eventually I'd like to greylist based on spamcop and NJABL-DUL lookups, as
> well as my no-RDNS, apnic and lacnic rules. If this feature gets added,
> I'll probably use it as another criteria to trigger greylisting.

What DNSRBL do you use to identify which RIR allocated an IP? 

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

> > 1. may_be_forged ACL condition
> > http://www.j10n.org/files/milter-greylist-3.0a2-step1.patch
> > 
> > When a client has bogus reverse DNS, that is, IP -> PTR -> A != IP,
> > sendmail sets {client_resolve} macro to FORGED. This patch implements
> > ACL condition to take advantage of it. You need to add
> > {client_resolve} into Milter.macros.connect. Example:
> > acl blacklist domain /.*\.info/ may_be_forged
> > acl greylist may_be_forged
> 
> Some thoughts:
> - may_be_forged name is a bit long to read in ACL. What about just
> forged?

First, most of their reverse DNS are not malicious forgeries but their
ISP's misconfigurations. I feel "forged" misleading. Second, sendmail
records them as "(may be forged)" to its log. It is a good hint for
users.

> - Changes to documentation for the new feature would be nice :-)

http://www.j10n.org/files/milter-greylist-3.0a2-doc.patch

> - Do you catch a lot of spam with that?

I get 5% to 10% of spam from such zombie hosts. Approximately an half
of them are not registered to SORBS DNSBL, and part of them have
static IP addresses. Conclusion: may_be_forged test improves zombie
probe in some percent.

False positives will be not significant as long as it is applied to
greylisting rather than blacklisting, because legitimate MTA runners
will probably find and report their ISP's DNS misconfiguration. My
test can also become positive due to a temporary lookup failure, but
temporary false positives do not harm in greylisting.

> > 2. Threading bug fix
> > http://www.j10n.org/files/milter-greylist-3.0a2-step2.patch
> > 
> > This patch resolvs number of race conditions in threading
> > code. It also includes some minor modifications. Since my changes
> > affect almost everywhere, code reviews are welcome.
> 
> There are huge changes. Could you explain what was wrong?

The conf and dump_dirty are clear examples. They are not protected by
any lock. Another example is sync_sender(). If something is added to
sync queue exactly before pthread_cond_wait(), it will not be sent to
peers until another item is added.

But now I understand nothing is wrong in the autowhite.c. *I* was
wrong. The pending.c is also correct except that:
 - pending_del_addr() needs writelocking.
 - the pending list should be really sorted or pending_timeout()
   should tranverse to the end.

> > 3. Binary search tree
> > http://www.j10n.org/files/milter-greylist-3.0a2-step3.patch
> > 
> > This patch introduces red-black tree structure to manage the greylist
> > and autowhitelist. It is just a pedantic change. No realistic
> > improvement is expected.
> 
> I'm a bit reluctant to make changes that buy nothing. What's the
> advantage of adding this?

Ignore it if you don't like.

On Mon, Aug 21, 2006 at 11:37:13PM +0900, AIDA Shinra wrote:
> 1. may_be_forged ACL condition
> http://www.j10n.org/files/milter-greylist-3.0a2-step1.patch
> 
> When a client has bogus reverse DNS, that is, IP -> PTR -> A != IP,
> sendmail sets {client_resolve} macro to FORGED. This patch implements
> ACL condition to take advantage of it. You need to add
> {client_resolve} into Milter.macros.connect. Example:
> acl blacklist domain /.*\.info/ may_be_forged
> acl greylist may_be_forged

I had an idea about this: what about allowing random macros to be checked,
instead of just this particular one? I think about such a syntax:
sm_macro "may_be_forged" "{client_resolve}" "FORGED"
acl blacklist domain /.*\.info/ sm_macro "may_be_forged"

It would bring much more flexibility, as any sendmail setting could be 
used in the ACL. For instance, someone asked for a sendmail bound to 
multiple IP with a different milter-greylist configuration for each IP.
Using the if_addr macro in the ACL would allow that:

sm_macro "ip1" "{if_addr}" "192.0.2.3"
sm_macro "ip2" "{if_addr}" "192.0.2.4"
acl whitelist sm_macro "ip1"
acl greylist sm_macro "ip2" delay 15m autowhite 3d

Likewise, SMTP AUTH status could be used in the ACL just by checking
{auth_authen}.

I wonder if going further is of any interest: should we support regexp for
the macro value? lists of sm_macros? Anyone sees an usage for that? Here is an 
example of macro + list that we could support:

sm_macro "may_be_forged" "{client_resolve}" "FORGED"
sm_macro "blacklist" "{blacklist}" "BLACK"
list "bad_macros" sm_macro { "may_be_forged" "blacklist" }
acl blacklist domain /.*\.info/ list "bad_macros"

Any comment?

-- 
Emmanuel Dreyfus
manu@...

On Thu, Aug 24, 2006 at 08:20:55AM +0000, Emmanuel Dreyfus wrote:
> I had an idea about this: what about allowing random macros to be checked,
> instead of just this particular one? I think about such a syntax:
> sm_macro "may_be_forged" "{client_resolve}" "FORGED"
> acl blacklist domain /.*\.info/ sm_macro "may_be_forged"

No comment, anyone?

-- 
Emmanuel Dreyfus
manu@...

> I had an idea about this: what about allowing random macros to be checked,
> instead of just this particular one? I think about such a syntax:
> sm_macro "may_be_forged" "{client_resolve}" "FORGED"
> acl blacklist domain /.*\.info/ sm_macro "may_be_forged"
> 
> It would bring much more flexibility, as any sendmail setting could be 
> used in the ACL. For instance, someone asked for a sendmail bound to 
> multiple IP with a different milter-greylist configuration for each IP.
> Using the if_addr macro in the ACL would allow that:
> 
> sm_macro "ip1" "{if_addr}" "192.0.2.3"
> sm_macro "ip2" "{if_addr}" "192.0.2.4"
> acl whitelist sm_macro "ip1"
> acl greylist sm_macro "ip2" delay 15m autowhite 3d
> 
> Likewise, SMTP AUTH status could be used in the ACL just by checking
> {auth_authen}.
> 
> I wonder if going further is of any interest: should we support regexp for
> the macro value? lists of sm_macros? Anyone sees an usage for that? Here is an 
> example of macro + list that we could support:
> 
> sm_macro "may_be_forged" "{client_resolve}" "FORGED"
> sm_macro "blacklist" "{blacklist}" "BLACK"
> list "bad_macros" sm_macro { "may_be_forged" "blacklist" }
> acl blacklist domain /.*\.info/ list "bad_macros"
> 
> Any comment?

1. What is the advantage of giving names to macro conditions rather
than the following syntax? Line length?

acl greylist macro "{client_resolve}" "FORGED"
list "bad_macros" macro "{foobar}" { "foo" "bar" }

2. If we introduce your sm_macro syntax, we need to consider the
sm_macro as a part of ACL rather than config. The sm_macro must be
protected by the ACL lock and may be inconsistent with the config.

On Fri, Aug 25, 2006 at 11:37:06PM +0900, AIDA Shinra wrote:
> 1. What is the advantage of giving names to macro conditions rather
> than the following syntax? Line length?

Yes, line length, and avoid replicating the same macro value in 
multiple rules.

> acl greylist macro "{client_resolve}" "FORGED"
> list "bad_macros" macro "{foobar}" { "foo" "bar" }

That list has a somewhat different semantic than the other ones: we
would have a list of macro values instead of having a list of 
(macro name, macro value) tuples. It would be a bit confusing.

> 2. If we introduce your sm_macro syntax, we need to consider the
> sm_macro as a part of ACL rather than config. The sm_macro must be
> protected by the ACL lock and may be inconsistent with the config.

Yes, it can be directly derived from what is currently done for dnsrbl.

-- 
Emmanuel Dreyfus
manu@...