> On 31 Aug 2015, at 22:57, Alexandr Nedvedicky
> <[email protected]> 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.y 21 Apr 2015 16:34:59 -0000 1.648
> +++ sbin/pfctl/parse.y 31 Aug 2015 20:30:56 -0000
> @@ -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 -0000 1.936
> +++ sys/net/pf.c 31 Aug 2015 20:31:02 -0000
> @@ -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) {
>
