Re: PF ignores block action when rule contains route-to/dup-to action
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< >
Re: PF ignores block action when rule contains route-to/dup-to action
> 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 " > +"are not supported
Re: PF ignores block action when rule contains route-to/dup-to action
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: /cvs/src/sys/net/pf.
Re: PF ignores block action when rule contains route-to/dup-to action
> On 31 Aug 2015, at 22:57, Alexandr Nedvedicky > wrote: > > 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(&pd)) { > >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? IMHO, yes that would make sense. Some bike shedding inline below, apart from that, ok jung@ > 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.y21 Apr 2015 16:34:59 - 1.648 > +++ sbin/pfctl/parse.y31 Aug 2015 20:30:56 - > @@ -3997,8 +3997,11 @@ rule_consistent(struct pf_rule *r, int a > problems++; > } > > - /* match rules rules */ Sad, I think this comment is funny enough to keep it :) > - if (r->action == PF_MATCH) { > + /* > + * Basic rule sanity check. > + */ A single line comment is enough here, isn’t it? > + 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? > + 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.c 19 Aug 2015 21:22:41 - 1.936 > +++ sys/net/pf.c 31 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) { >
PF ignores block action when rule contains route-to/dup-to action
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(&pd)) { 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) {