Re: PF ignores block action when rule contains route-to/dup-to action

[Alexandr Nedvedicky]

Hello,


> > As a side effect the patch breaks block rules with dup-to action. dup-to
> > action as a part of block rule might make some sense... So if there is
> > someone, who really needs block ... dup-to he should opt for equivalent
> > rule using pass ... route-to 
> > 
> > Also there is one more question:
> > 
> >shall we implement similar sanity checks for nat-to/rdr-to/... actions?
> 
> IMHO, yes that would make sense.
> 

I'll try to keep it on my todo list...

> Some bike shedding inline below, apart from that, ok jung@

I love bike shedding, so let's go an pick up some colors ;-)

> 
> > -   if (r->action == PF_MATCH) {
> > +   /* 
> > +* Basic rule sanity check.
> > +*/
> 
> A single line comment is enough here, isn’t it?
> 

O.K. new patch has single line comment.

> > +   switch (r->action) {
> > +   case PF_MATCH:
> > if (r->divert.port) {
> > yyerror("divert is not supported on match rules");
> > problems++;
> > @@ -4016,6 +4019,15 @@ rule_consistent(struct pf_rule *r, int a
> > yyerror("af-to is not supported on match rules");
> > problems++;
> > }
> > +   break;
> > +   case PF_DROP:
> > +   if (r->rt) {
> > +   yyerror("route-to, reply-to and dup-to "
> > +  "must not be used on block rules”);
> 
> The other error messages say “is not supported” instead of “must no be used”.
> I do not care which wording you chose, but maybe take the chance and unify 
> it, 
> to be more consistent here? 
> 

the question is which consistency do you want. The patch does not show them,
but there are two options, actually three. Let me show the current code without
patch applied:

4000 /* match rules rules */
4001 if (r->action == PF_MATCH) {
4002 if (r->divert.port) {
4003 yyerror("divert is not supported on match 
rules");
4004 problems++;
4005 }
4006 if (r->divert_packet.port) {
4007 yyerror("divert is not supported on match 
rules");
4008 problems++;
4009 }
4010 if (r->rt) {
4011 yyerror("route-to, reply-to and dup-to "
4012"must not be used on match rules");
4013 problems++;
4014 }
4015 if (r->rule_flag & PFRULE_AFTO) {
4016 yyerror("af-to is not supported on match 
rules");
4017 problems++;
4018 }
4019 }

as you can see there are two colors to chose from:

color A:
... is not supported on ... rules (used at 4003, 4007, 4016

color B:
... must not be used on match rules (used at line 4911)

we have three options:

1) leave it as it is (both colors will be used)

2) use color A

3) use color B

IMO consistency is good here. I prefer color A as it sounds more polite. 

updated patch is further below.

regards
sasha

8<---8<---8<--8<

Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.648
diff -u -p -r1.648 parse.y
--- sbin/pfctl/parse.y  21 Apr 2015 16:34:59 -  1.648
+++ sbin/pfctl/parse.y  1 Sep 2015 12:29:41 -
@@ -3997,8 +3997,9 @@ rule_consistent(struct pf_rule *r, int a
problems++;
}
 
-   /* match rules rules */
-   if (r->action == PF_MATCH) {
+   /* Basic rule sanity check. */
+   switch (r->action) {
+   case PF_MATCH:
if (r->divert.port) {
yyerror("divert is not supported on match rules");
problems++;
@@ -4009,13 +4010,22 @@ rule_consistent(struct pf_rule *r, int a
}
if (r->rt) {
yyerror("route-to, reply-to and dup-to "
-  "must not be used on match rules");
+  "are not supported on match rules");
problems++;
}
if (r->rule_flag & PFRULE_AFTO) {
yyerror("af-to is not supported on match rules");
problems++;
}
+   break;
+   case PF_DROP:
+   if (r->rt) {
+   yyerror("route-to, reply-to and dup-to "
+  "are not supported on block rules");
+   problems++;
+   }
+   break;
+   default:;
}
return (-problems);
 }
Index: sys/net/pf.c
===
RCS file: 

Re: PF ignores block action when rule contains route-to/dup-to action

[Joerg Jung]

> On 01 Sep 2015, at 14:31, Alexandr Nedvedicky 
>  wrote:
> 
>>> As a side effect the patch breaks block rules with dup-to action. dup-to
>>> action as a part of block rule might make some sense... So if there is
>>> someone, who really needs block ... dup-to he should opt for equivalent
>>> rule using pass ... route-to 
>>> 
>>> Also there is one more question:
>>> 
>>>   shall we implement similar sanity checks for nat-to/rdr-to/... actions?
>> 
>> IMHO, yes that would make sense.
>> 
> 
> I'll try to keep it on my todo list...
> 
>> Some bike shedding inline below, apart from that, ok jung@
> 
> I love bike shedding, so let's go an pick up some colors ;-)

:)

>> 
>>> -   if (r->action == PF_MATCH) {
>>> +   /* 
>>> +* Basic rule sanity check.
>>> +*/
>> 
>> A single line comment is enough here, isn’t it?
>> 
> 
> O.K. new patch has single line comment.
> 
>>> +   switch (r->action) {
>>> +   case PF_MATCH:
>>> if (r->divert.port) {
>>> yyerror("divert is not supported on match rules");
>>> problems++;
>>> @@ -4016,6 +4019,15 @@ rule_consistent(struct pf_rule *r, int a
>>> yyerror("af-to is not supported on match rules");
>>> problems++;
>>> }
>>> +   break;
>>> +   case PF_DROP:
>>> +   if (r->rt) {
>>> +   yyerror("route-to, reply-to and dup-to "
>>> +  "must not be used on block rules”);
>> 
>> The other error messages say “is not supported” instead of “must no be used”.
>> I do not care which wording you chose, but maybe take the chance and unify 
>> it, 
>> to be more consistent here? 
>> 
> 
> the question is which consistency do you want.

As written above: I do not really care.

> The patch does not show them,
> but there are two options, actually three. Let me show the current code 
> without
> patch applied:
> 
>4000 /* match rules rules */
>4001 if (r->action == PF_MATCH) {
>4002 if (r->divert.port) {
>4003 yyerror("divert is not supported on match 
> rules");
>4004 problems++;
>4005 }
>4006 if (r->divert_packet.port) {
>4007 yyerror("divert is not supported on match 
> rules");
>4008 problems++;
>4009 }
>4010 if (r->rt) {
>4011 yyerror("route-to, reply-to and dup-to "
>4012"must not be used on match rules");
>4013 problems++;
>4014 }
>4015 if (r->rule_flag & PFRULE_AFTO) {
>4016 yyerror("af-to is not supported on match 
> rules");
>4017 problems++;
>4018 }
>4019 }
> 
> as you can see there are two colors to chose from:
> 
>color A:
>   ... is not supported on ... rules (used at 4003, 4007, 4016
> 
>color B:
>   ... must not be used on match rules (used at line 4911)
> 
> we have three options:
> 
>1) leave it as it is (both colors will be used)
> 
>2) use color A
> 
>3) use color B
> 
> IMO consistency is good here. I prefer color A as it sounds more polite. 

Yes, good choice. :)

> updated patch is further below.

ok jung@

> regards
> sasha
> 
> 8<---8<---8<--8<
> 
> Index: sbin/pfctl/parse.y
> ===
> RCS file: /cvs/src/sbin/pfctl/parse.y,v
> retrieving revision 1.648
> diff -u -p -r1.648 parse.y
> --- sbin/pfctl/parse.y21 Apr 2015 16:34:59 -  1.648
> +++ sbin/pfctl/parse.y1 Sep 2015 12:29:41 -
> @@ -3997,8 +3997,9 @@ rule_consistent(struct pf_rule *r, int a
>   problems++;
>   }
> 
> - /* match rules rules */
> - if (r->action == PF_MATCH) {
> + /* Basic rule sanity check. */
> + switch (r->action) {
> + case PF_MATCH:
>   if (r->divert.port) {
>   yyerror("divert is not supported on match rules");
>   problems++;
> @@ -4009,13 +4010,22 @@ rule_consistent(struct pf_rule *r, int a
>   }
>   if (r->rt) {
>   yyerror("route-to, reply-to and dup-to "
> -"must not be used on match rules");
> +"are not supported on match rules");
>   problems++;
>   }
>   if (r->rule_flag & PFRULE_AFTO) {
>   yyerror("af-to is not supported on match rules");
>   problems++;
>   }
> + break;
> + case PF_DROP:
> + if (r->rt) {
> + yyerror("route-to, reply-to and dup-to "
> +   

Re: PF ignores block action when rule contains route-to/dup-to action

[Mike Belopuhov]

On 1 September 2015 at 14:31, Alexandr Nedvedicky
 wrote:
> Hello,
>
>
>> > As a side effect the patch breaks block rules with dup-to action. dup-to
>> > action as a part of block rule might make some sense... So if there is
>> > someone, who really needs block ... dup-to he should opt for equivalent
>> > rule using pass ... route-to
>> >
>> > Also there is one more question:
>> >
>> >shall we implement similar sanity checks for nat-to/rdr-to/... actions?
>>
>> IMHO, yes that would make sense.
>>
>
> I'll try to keep it on my todo list...
>
>> Some bike shedding inline below, apart from that, ok jung@
>
> I love bike shedding, so let's go an pick up some colors ;-)
>
[snip]
> as you can see there are two colors to chose from:
>
> color A:
> ... is not supported on ... rules (used at 4003, 4007, 4016
>
> color B:
> ... must not be used on match rules (used at line 4911)
>
> we have three options:
>
> 1) leave it as it is (both colors will be used)
>
> 2) use color A
>
> 3) use color B
>
> IMO consistency is good here. I prefer color A as it sounds more polite.
>

Me too.

> updated patch is further below.
>

Looks OK to me.

> regards
> sasha
>
> 8<---8<---8<--8<
>

PF ignores block action when rule contains route-to/dup-to action

[Alexandr Nedvedicky]

Hello,

Dilli Paudel in Oracle was playing with PF enough to find funny glitch.
He used rule as follows:

block in on vnic4 from 192.168.1.0/24 to any route-to 172.16.1.1@vnic5

Many people expect the route-to action is somewhat futile as 'block' action
takes precedence here, so packet gets always dropped. Well the reality is
very different (and still makes a sort of sense) from PF point of view.
The snippet comes from pf_test():

6586 switch (action) {
6587 case PF_SYNPROXY_DROP:

6593 case PF_DIVERT:
6594 switch (pd.af) {
6595 case AF_INET:

6610 case PF_AFRT:
6611 if (pf_translate_af()) {

6624 #endif /* INET6 */
6625 default:
6626 /* pf_route can free the mbuf causing *m0 to become 
NULL */
6627 if (r->rt) {
6628 switch (pd.af) {
6629 case AF_INET:
6630 pf_route(m0, r, pd.dir, 
pd.kif->pfik_ifp, s);
6631 break;

the action comes from matching rule. It's PF_DROP in case of Dilli's rule.  As
you can see there is no case-branch for PF_DROP in switch statement at line
6586, so a default: is executed. For route-to action the r->rt is set and PF
executes the route_to*().

Dilli  suggests to introduce PF_DROP case branch to switch() at line 6586 Issue
has been also discussed on Friday over icb, where Mr. bluhm further suggested
we should try to add some sanity check to parse.y.

As a side effect the patch breaks block rules with dup-to action. dup-to
action as a part of block rule might make some sense... So if there is
someone, who really needs block ... dup-to he should opt for equivalent
rule using pass ... route-to 

Also there is one more question:

shall we implement similar sanity checks for nat-to/rdr-to/... actions?

no one should expect those in block rule, so making pfctl to refuse such rules
loudly sounds like a right thing to do...

regards
sasha

8<---8<---8<--8<

Index: sbin/pfctl/parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.648
diff -u -p -r1.648 parse.y
--- sbin/pfctl/parse.y  21 Apr 2015 16:34:59 -  1.648
+++ sbin/pfctl/parse.y  31 Aug 2015 20:30:56 -
@@ -3997,8 +3997,11 @@ rule_consistent(struct pf_rule *r, int a
problems++;
}
 
-   /* match rules rules */
-   if (r->action == PF_MATCH) {
+   /* 
+* Basic rule sanity check.
+*/
+   switch (r->action) {
+   case PF_MATCH:
if (r->divert.port) {
yyerror("divert is not supported on match rules");
problems++;
@@ -4016,6 +4019,15 @@ rule_consistent(struct pf_rule *r, int a
yyerror("af-to is not supported on match rules");
problems++;
}
+   break;
+   case PF_DROP:
+   if (r->rt) {
+   yyerror("route-to, reply-to and dup-to "
+  "must not be used on block rules");
+   problems++;
+   }
+   break;
+   default:;
}
return (-problems);
 }
Index: sys/net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.936
diff -u -p -r1.936 pf.c
--- sys/net/pf.c19 Aug 2015 21:22:41 -  1.936
+++ sys/net/pf.c31 Aug 2015 20:31:02 -
@@ -6622,6 +6622,10 @@ done:
action = PF_PASS;
break;
 #endif /* INET6 */
+   case PF_DROP:
+   m_freem(*m0);
+   *m0 = NULL;
+   break;
default:
/* pf_route can free the mbuf causing *m0 to become NULL */
if (r->rt) {