Milter-greylist

Hi

I just discovered an obvious bug: when you have multiple recipients, if
RCPT stage ACL decides the first one shall be accepted, that decision is
kept for all next recipients, even if RCPT stage ACL evaluation would
cause them to be rejected.

The problem exist because milter-greylist keeps the status and adds flag
to it for each recpitients. It does that to print a reason in the
X-Greylist header line, which is obviously an impossible task to perform
correctly for multiple recipients since there is a single X-Greylist
line for multiple decisions.

I would call that a bug. I tested code that reset the
accept/reject/greylist decision for each recpipient and keep the other
flags, but of course the X-Greylist header may sometime display
nonsense. I assume some users will prefer the current behavior,
therefore I am about to add a global option to enable the "fixed"
behavior. I found nothing better than "multiracl"

Opinions?

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

Hi,

Are you sure that there is a bug?
I have made test with two ACL's one to reject specific recipient
other to accept an other different recipient and all works as expected.
(Of course in one mail transaction with the two recipients.)
Both the syslog message and the header lines obviously refers only to
the accepted recipient and its ACL line in the configuration file.

Best,
Attila

> -----Original Message-----
> From: milter-greylist@yahoogroups.com [mailto:milter-greylist@yahoogroups.com] On Behalf Of manu@netbsd.org
> Sent: vendredi, 21. juin 2013 02:41
> To: milter-greylist@yahoogroups.com
> Subject: [milter-greylist] racl confusion
> 
> Hi
> 
> I just discovered an obvious bug: when you have multiple recipients, if
> RCPT stage ACL decides the first one shall be accepted, that decision is
> kept for all next recipients, even if RCPT stage ACL evaluation would
> cause them to be rejected.
> 
> The problem exist because milter-greylist keeps the status and adds flag
> to it for each recpitients. It does that to print a reason in the
> X-Greylist header line, which is obviously an impossible task to perform
> correctly for multiple recipients since there is a single X-Greylist
> line for multiple decisions.
> 
> I would call that a bug. I tested code that reset the
> accept/reject/greylist decision for each recpipient and keep the other
> flags, but of course the X-Greylist header may sometime display
> nonsense. I assume some users will prefer the current behavior,
> therefore I am about to add a global option to enable the "fixed"
> behavior. I found nothing better than "multiracl"
> 
> Opinions?
> 
> --
> Emmanuel Dreyfus
> http://hcpnet.free.fr/pubz
> manu@...
>

On Fri, Jun 21, 2013 at 07:51:59AM +0000, Bruncsak, Attila wrote:
> Are you sure that there is a bug?

Almost sure.

> I have made test with two ACL's one to reject specific recipient
> other to accept an other different recipient and all works as expected.

What version do you run?

-- 
Emmanuel Dreyfus
manu@...

> 
> > I have made test with two ACL's one to reject specific recipient
> > other to accept an other different recipient and all works as expected.
> 
> What version do you run?
> 

4.4.3

Actually I made two tests with different order of the recipients in the
SMTP conversation. Only one syslog entry has been printed, and it is
appropriate for the first recipient in the SMTP conversation.
For the second recipient no syslog entry
independently is this recipient which is accepted or rejected.
The mail header always properly refers  to the recipient which is accepted.

> 4.4.3
> 
> Actually I made two tests with different order of the recipients in the
> SMTP conversation. Only one syslog entry has been printed, and it is
> appropriate for the first recipient in the SMTP conversation.
> For the second recipient no syslog entry
> independently is this recipient which is accepted or rejected.
> The mail header always properly refers  to the recipient which is accepted.

I am wrong. Both the two syslog lines are there for the two recipients.
Sorry for the overlook.

Bruncsak, Attila <attila.bruncsak@...> wrote:

> I am wrong. Both the two syslog lines are there for the two recipients.
> Sorry for the overlook. 

Hence you see a bug too?

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

> > I am wrong. Both the two syslog lines are there for the two recipients.
> > Sorry for the overlook.
> 
> Hence you see a bug too?
> 

I do not see any bug.
Everything is working as supposed to be.
The order of the syslog entries match
the order of the recipients
in the SMTP conversation.

Bruncsak, Attila <attila.bruncsak@...> wrote:

> I do not see any bug.

I tracked it down to this minimal example:

racl blacklist rcpt archibald.haddock@... msg "blacklisted"
racl whitelist auth /.*/ report "Authenticated sender"
racl whitelist default

Sending as an authenticated user, I get the following result:
archibald.haddock@... -> blacklisted, as expected
emmanuel.dreyfus@... -> accepted

Now if I start sending with an accepted user first:
emmanuel.dreyfus@... -> accepted
archibald.haddock@... -> wrongly accepted

My understanding is that the offending code is at the begining of
real_envrcpt():
        
        if ((priv->priv_sr.sr_whitelist & EXF_WHITELIST) &&
            (priv->priv_sr.sr_whitelist &
             (EXF_NONIP | EXF_AUTH | EXF_STARTTLS | EXF_SPF)))
                goto exit_accept;

That reuses the status from previous recipient. I made the change below,
which avoids it based on a global configuration parameter, but I wonder
if it makes sense to preserve the original behavior. Is it just a plain
bug, or can it have some merit?

--- milter-greylist-4.5.1/milter-greylist.c 
+++ milter-greylist-4.5.1p1/milter-greylist.c 
@@ -638,8 +638,16 @@
         */
        prop_clear(priv, UP_CLEARPROP);
 #endif
 
+       /*
+        * If we re-evaluate racl for each recipient, forget
+        * about previous decision.
+        */
+       if (conf.c_multiracl)
+               priv->priv_sr.sr_whitelist &=
+                    ~(EXF_WHITELIST|EXF_GREYLIST|EXF_BLACKLIST);
+
        if ((priv->priv_sr.sr_whitelist & EXF_WHITELIST) &&
            (priv->priv_sr.sr_whitelist &
             (EXF_NONIP | EXF_AUTH | EXF_STARTTLS | EXF_SPF)))
                goto exit_accept;


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

No reply. No opinon?

Emmanuel Dreyfus <manu@...> wrote:

> Bruncsak, Attila <attila.bruncsak@...> wrote:
> 
> > I do not see any bug.
> 
> I tracked it down to this minimal example:
> 
> racl blacklist rcpt archibald.haddock@... msg "blacklisted"
> racl whitelist auth /.*/ report "Authenticated sender"
> racl whitelist default
> 
> Sending as an authenticated user, I get the following result:
> archibald.haddock@... -> blacklisted, as expected
> emmanuel.dreyfus@... -> accepted
> 
> Now if I start sending with an accepted user first:
> emmanuel.dreyfus@... -> accepted
> archibald.haddock@... -> wrongly accepted
> 
> My understanding is that the offending code is at the begining of
> real_envrcpt():
>         
>         if ((priv->priv_sr.sr_whitelist & EXF_WHITELIST) &&
>             (priv->priv_sr.sr_whitelist &
>              (EXF_NONIP | EXF_AUTH | EXF_STARTTLS | EXF_SPF)))
>                 goto exit_accept;
> 
> That reuses the status from previous recipient. I made the change below,
> which avoids it based on a global configuration parameter, but I wonder
> if it makes sense to preserve the original behavior. Is it just a plain
> bug, or can it have some merit?
> 
> --- milter-greylist-4.5.1/milter-greylist.c 
> +++ milter-greylist-4.5.1p1/milter-greylist.c 
> @@ -638,8 +638,16 @@
>          */
>         prop_clear(priv, UP_CLEARPROP);
>  #endif
>  
> +       /*
> +        * If we re-evaluate racl for each recipient, forget
> +        * about previous decision.
> +        */
> +       if (conf.c_multiracl)
> +               priv->priv_sr.sr_whitelist &=
> +                    ~(EXF_WHITELIST|EXF_GREYLIST|EXF_BLACKLIST);
> +
>         if ((priv->priv_sr.sr_whitelist & EXF_WHITELIST) &&
>             (priv->priv_sr.sr_whitelist &
>              (EXF_NONIP | EXF_AUTH | EXF_STARTTLS | EXF_SPF)))
>                 goto exit_accept;


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

Emmanuel Dreyfus <manu@...> wrote:

> My understanding is that the offending code is at the begining of
> real_envrcpt():
>         
>         if ((priv->priv_sr.sr_whitelist & EXF_WHITELIST) &&
>             (priv->priv_sr.sr_whitelist &
>              (EXF_NONIP | EXF_AUTH | EXF_STARTTLS | EXF_SPF)))
>                 goto exit_accept;

I finally used the fix below, which should nnot hurt anyone

Index: milter-greylist.c
===================================================================
RCS file: /cvsroot/milter-greylist/milter-greylist.c,v
retrieving revision 1.255
retrieving revision 1.256
diff -U 4 -r1.255 -r1.256
--- milter-greylist.c   19 May 2013 05:53:34 -0000      1.255
+++ milter-greylist.c   30 Jun 2013 04:55:49 -0000      1.256
@@ -638,11 +638,34 @@
         */
        prop_clear(priv, UP_CLEARPROP);
 #endif
 
-       if ((priv->priv_sr.sr_whitelist & EXF_WHITELIST) &&
-           (priv->priv_sr.sr_whitelist &
-            (EXF_NONIP | EXF_AUTH | EXF_STARTTLS | EXF_SPF)))
+       /*
+        * Global authenticated or TLS whitelisting, unless
+        * - noauth global parameter is set, or
+        * - any ACL with auth or tls clause
+        */
+       if (conf.c_noauth == 0) {
+               if (priv->priv_sr.sr_whitelist & EXF_AUTH)
+                       goto exit_accept;
+               if (priv->priv_sr.sr_whitelist & EXF_STARTTLS)
+                       goto exit_accept;
+       }
+
+       /*
+        * Global SPF whitelisting, unless
+        * - nospf global parameter is set, or
+        * - any ACL with spf clause
+        */
+       if (conf.c_nospf == 0) {
+               if (priv->priv_sr.sr_whitelist & EXF_SPF)
+                       goto exit_accept;
+       }
+
+       /*
+        * Mail sent from non IP source is always whitelisted
+        */
+       if (priv->priv_sr.sr_whitelist & EXF_NONIP) 
                goto exit_accept;
 
 #ifdef USE_DRAC
        if ((SA(&priv->priv_addr)->sa_family == AF_INET) && 

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