Re: pf: honor quick on anchor rules
I tested the diff and while im not qualified to judge the code, it now behaves
like i would expect.
Thanks for working on this,
Regards.
> Klemens Nanni hat am 16. Oktober 2018 um 13:34 geschrieben:
>
>
> On Tue, Oct 16, 2018 at 10:03:01AM +0200, Alexandr Nedvedicky wrote:
> > I also wonder if we should apply same change to wildcard anchors here:
> They need fixing as well, but I have yet to look into it.
>
> > diff --git a/sys/net/pf.c b/sys/net/pf.c
> > index 0bdf90a8d13..5a5c739773a 100644
> > --- a/sys/net/pf.c
> > +++ b/sys/net/pf.c
> > @@ -3129,10 +3129,32 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
> > pf_rule *r)
> > } else {
> > rv = pf_match_rule(ctx, &r->anchor->ruleset);
> This comment is way too much, imho. How about this:
>
> Unless errors occured, stop iff any rule matched
> within quick anchors.
>
> It's dense but we're inside pf_step_into_anchor() so context is given
> while the condition speaks for itself as well. I'd like to leave
> detailed explanations to the manual.
>
> > - if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
> > + if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK &&
> > + *ctx->am == r)
> > rv = PF_TEST_QUICK;
> > }
> That makes sense and works as expected with *all* examples and real use
> cases I could come up with.
>
> Index: net/pf.c
> ===
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1076
> diff -u -p -r1.1076 pf.c
> --- net/pf.c 4 Oct 2018 20:25:59 - 1.1076
> +++ net/pf.c 16 Oct 2018 11:32:10 -
> @@ -3129,10 +3129,11 @@ pf_step_into_anchor(struct pf_test_ctx *
> } else {
> rv = pf_match_rule(ctx, &r->anchor->ruleset);
> /*
> - * Unless there was an error inside the anchor,
> - * retain its quick state.
> + * Unless errors occured, stop iff any rule matched
> + * within quick anchors.
>*/
> - if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
> + if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK &&
> + *ctx->am == r)
> rv = PF_TEST_QUICK;
> }
>
>
Re: pf: honor quick on anchor rules
Hello, > This comment is way too much, imho. How about this: > > Unless errors occured, stop iff any rule matched > within quick anchors. I'm fine with your proposal. You have my OK to commit the fix. thanks and regards sashan
Re: pf: honor quick on anchor rules
On Tue, Oct 16, 2018 at 10:03:01AM +0200, Alexandr Nedvedicky wrote:
> I also wonder if we should apply same change to wildcard anchors here:
They need fixing as well, but I have yet to look into it.
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 0bdf90a8d13..5a5c739773a 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -3129,10 +3129,32 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
> pf_rule *r)
> } else {
> rv = pf_match_rule(ctx, &r->anchor->ruleset);
This comment is way too much, imho. How about this:
Unless errors occured, stop iff any rule matched
within quick anchors.
It's dense but we're inside pf_step_into_anchor() so context is given
while the condition speaks for itself as well. I'd like to leave
detailed explanations to the manual.
> - if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
> + if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK &&
> + *ctx->am == r)
> rv = PF_TEST_QUICK;
> }
That makes sense and works as expected with *all* examples and real use
cases I could come up with.
Index: net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1076
diff -u -p -r1.1076 pf.c
--- net/pf.c4 Oct 2018 20:25:59 - 1.1076
+++ net/pf.c16 Oct 2018 11:32:10 -
@@ -3129,10 +3129,11 @@ pf_step_into_anchor(struct pf_test_ctx *
} else {
rv = pf_match_rule(ctx, &r->anchor->ruleset);
/*
-* Unless there was an error inside the anchor,
-* retain its quick state.
+* Unless errors occured, stop iff any rule matched
+* within quick anchors.
*/
- if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
+ if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK &&
+ *ctx->am == r)
rv = PF_TEST_QUICK;
}
Re: pf: honor quick on anchor rules
Hello,
>
> In my understanding they do, but you are missing the point. Given the
> following ruleset:
>
> pass
> anchor quick {
> block inet6
> }
> block
>
> With the current fix ruleset-evaluation terminates after the anchor,
> before the last block even *if no ipv6-traffic was encountered*. That is,
> no rule inside the (not empty) anchor matched, but evaluation is stopped
> nevertheless.
>
> On 6.1 the anchor is ignored if no rule inside matches and evaluation
> continues, and that is what the original commit-log states:
>
now I got it (I think). Klemens also made similar point off-list.
So with your help I could come to diff below. Which should hopefully
fix the bug you found.
I also wonder if we should apply same change to wildcard anchors here:
3103 enum pf_test_status
3104 pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_rule *r)
3105 {
3118 RB_FOREACH(child, pf_anchor_node, &r->anchor->children) {
3119 rv = pf_match_rule(ctx, &child->ruleset);
3120 if ((rv == PF_TEST_QUICK) || (rv == PF_TEST_FAIL)) {
3121 /*
3122 * we either hit a rule with quick action
3123 * (more likely), or hit some runtime
3124 * error (e.g. pool_get() failure).
3125 */
3126 break;
3127 }
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 0bdf90a8d13..5a5c739773a 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -3129,10 +3129,32 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
pf_rule *r)
} else {
rv = pf_match_rule(ctx, &r->anchor->ruleset);
/*
-* Unless there was an error inside the anchor,
-* retain its quick state.
+* Handle the 'anchor quick' case. Consider rules as follows:
+*
+* pass all
+* anchor quick {
+* pass inet6
+* }
+* block all
+*
+* According to specification in pf.conf(5), the rules above
+* should block everything except IPv6 traffic. Rules above
+* should be read as follows:
+*
+* if packet does not match any rule in anchor, then PF
+* continues to process rules. The next rule after anchor
+* is block, thus packet, which does not match 'pass
+* inet6', gets blocked. On the other hand if particular
+* packet matches 'pass inet6' rule, the 'quick' option
+* kicks in and 'block all' is never reached. Such packet
+* is allowed.
+*
+* If there was no error and we deal with 'anchor quick' and
+* there was a rule, which matched the packet, then we must
+* honor the 'quick' option to terminate rule processing.
*/
- if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
+ if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK &&
+ *ctx->am == r)
rv = PF_TEST_QUICK;
}
Re: pf: honor quick on anchor rules
On 18-10-15 23:45:58, Alexandr Nedvedicky wrote:
> Hello,
>
> > That is still different from the original (2006) behaviour which
> > would terminate ruleset-evaluation IFF any rules inside the anchor
> > actually match.
>
> Perhaps I still misunderstand the whole anchor thing.
>
> let me clarify the proposed change by two examples. Ruleset below allows
> new connection because pass rule matches in 'anchor quick' block (the
> 'block all' rule is never reached).
>
> pass all
> anchor quick {
> pass all
> }
> block all
>
> ruleset below does not allow connection, because the 'anchor quick' is
> empty. As such it does not match packet, therefore the whole anchor does
> not match, thus 'quick' does not count. PF continues to apply rules. It
> finds 'block all' for packet in question. Packet is blocked and game
> is over, the connection attempt times out.
>
> pass all
> anchor quick {
> }
> block all
>
> Do those two examples follow the behavior described in pf.conf(5) or not?
> if not what is the expected behavior for those two simple rulesets?
In my understanding they do, but you are missing the point. Given the
following ruleset:
pass
anchor quick {
block inet6
}
block
With the current fix ruleset-evaluation terminates after the anchor,
before the last block even *if no ipv6-traffic was encountered*. That is,
no rule inside the (not empty) anchor matched, but evaluation is stopped
nevertheless.
On 6.1 the anchor is ignored if no rule inside matches and evaluation
continues, and that is what the original commit-log states:
>> Allow the 'quick' keyword on an anchor. IFF there is a matching
>> rule inside
>> the anchor, terminate ruleset evaluation when stepping out of the
>> anchor.
>>
>> This means that if you absolutely want the anchor to be terminal,
>> you
>> probably want to use a 'block all' or 'pass all' rule at the start
>> of the
>> anchor.
>
> thanks a lot for your help
> regards
> sashan
Regards
Fabian
Re: pf: honor quick on anchor rules
Hello,
> That is still different from the original (2006) behaviour which
> would terminate ruleset-evaluation IFF any rules inside the anchor
> actually match.
Perhaps I still misunderstand the whole anchor thing.
let me clarify the proposed change by two examples. Ruleset below allows
new connection because pass rule matches in 'anchor quick' block (the
'block all' rule is never reached).
pass all
anchor quick {
pass all
}
block all
ruleset below does not allow connection, because the 'anchor quick' is
empty. As such it does not match packet, therefore the whole anchor does
not match, thus 'quick' does not count. PF continues to apply rules. It
finds 'block all' for packet in question. Packet is blocked and game
is over, the connection attempt times out.
pass all
anchor quick {
}
block all
Do those two examples follow the behavior described in pf.conf(5) or not?
if not what is the expected behavior for those two simple rulesets?
thanks a lot for your help
regards
sashan
Re: pf: honor quick on anchor rules
On 18-10-15 13:38:32, Alexandr Nedvedicky wrote:
> Hello,
>
> I just got back on-line after a week. Thank you all for detailed analysis.
> This particular bug, which Klemens tries to fix is caused by my commit 1.1024:
>
> - percpu anchor stacks
> we actually don't need to pre-allocate per_anchor_stack[], if we use
> a 'natural' recursion, when doing anchor tree traversal.
>
> Diff below fixes the case reported by Fabian:
>
> > snap# uname -a
> > OpenBSD snap.my.domain 6.4 GENERIC#333 amd64
> > snap# pfctl -sr
> >
> > pass all flags S/SA
> > anchor quick all {
> > }
> > block drop all
> >
>
> The idea is to override 'anchor quick' for empty rulesets. If the particular
> anchor, which pf_step_into_anchor() is about to enter, is empty, then the
> pf_step_into_anchor() bails out immediately with PF_TEST_OK result to keep
> PF processing the rules. I think this was the missing piece in mosaic.
That is still different from the original (2006) behaviour which
would terminate ruleset-evaluation IFF any rules inside the anchor
actually match.
>
> OK ?
>
> sorry for inconveniences
> regards
> sashan
>
> 8<---8<---8<--8<
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 0bdf90a8d13..72841c9b8f0 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -3126,7 +3126,7 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
> pf_rule *r)
> break;
> }
> }
> - } else {
> + } else if (!TAILQ_EMPTY(r->anchor->ruleset.rules.active.ptr)) {
> rv = pf_match_rule(ctx, &r->anchor->ruleset);
> /*
>* Unless there was an error inside the anchor,
> @@ -3134,7 +3134,8 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
> pf_rule *r)
>*/
> if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
> rv = PF_TEST_QUICK;
> - }
> + } else
> + rv = PF_TEST_OK;
>
> ctx->depth--;
>
Re: pf: honor quick on anchor rules
Hello,
I just got back on-line after a week. Thank you all for detailed analysis.
This particular bug, which Klemens tries to fix is caused by my commit 1.1024:
- percpu anchor stacks
we actually don't need to pre-allocate per_anchor_stack[], if we use
a 'natural' recursion, when doing anchor tree traversal.
Diff below fixes the case reported by Fabian:
> snap# uname -a
> OpenBSD snap.my.domain 6.4 GENERIC#333 amd64
> snap# pfctl -sr
>
> pass all flags S/SA
> anchor quick all {
> }
> block drop all
>
The idea is to override 'anchor quick' for empty rulesets. If the particular
anchor, which pf_step_into_anchor() is about to enter, is empty, then the
pf_step_into_anchor() bails out immediately with PF_TEST_OK result to keep
PF processing the rules. I think this was the missing piece in mosaic.
OK ?
sorry for inconveniences
regards
sashan
8<---8<---8<--8<
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 0bdf90a8d13..72841c9b8f0 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -3126,7 +3126,7 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
pf_rule *r)
break;
}
}
- } else {
+ } else if (!TAILQ_EMPTY(r->anchor->ruleset.rules.active.ptr)) {
rv = pf_match_rule(ctx, &r->anchor->ruleset);
/*
* Unless there was an error inside the anchor,
@@ -3134,7 +3134,8 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct
pf_rule *r)
*/
if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
rv = PF_TEST_QUICK;
- }
+ } else
+ rv = PF_TEST_OK;
ctx->depth--;
Re: pf: honor quick on anchor rules
Henning Brauer(hb-openbsdt...@ml.bsws.de) on 2018.10.08 13:56:20 +0200: > * Sebastian Benoit [2018-10-08 10:50]: > > The quick in the anchor does not do anything by itself, it should just > > "behave like all the rules inside the anchor had the quick keyword". > > no, that is *not* the intended semantic. > the intent is that ruleset evaluation is aborted if any rule inside > the anchor matched. note that this is very different from "any rule > inside treated like it had quick", since that would abort evaluation > *inside* the anchor immediately as well. yes, i simplified it too much, thanks
Re: pf: honor quick on anchor rules
* Sebastian Benoit [2018-10-08 10:50]: > The quick in the anchor does not do anything by itself, it should just > "behave like all the rules inside the anchor had the quick keyword". no, that is *not* the intended semantic. the intent is that ruleset evaluation is aborted if any rule inside the anchor matched. note that this is very different from "any rule inside treated like it had quick", since that would abort evaluation *inside* the anchor immediately as well. -- Henning Brauer, h...@bsws.de, henn...@openbsd.org BS Web Services GmbH, http://bsws.de, Full-Service ISP Secure Hosting, Mail and DNS. Virtual & Dedicated Servers, Root to Fully Managed Henning Brauer Consulting, http://henningbrauer.com/
Re: pf: honor quick on anchor rules
Klemens Nanni(k...@openbsd.org) on 2018.10.07 19:41:22 +0200:
> On Fri, Oct 05, 2018 at 11:53:08PM +0200, Klemens Nanni wrote:
> > On Fri, Oct 05, 2018 at 10:38:48PM +0200, Fabian Mueller-Knapp wrote:
> > > If i read man correctly it means "evaluate the rules inside and stop if
> > > any rule within matched".
> > While it's own description is quite clear and unambigious:
> >
> > quick If a packet matches a rule which has the quick option set, this
> > rule is considered the last matching rule, and evaluation of
> > subsequent rules is skipped.
> >
> > the anchor specific addition can be improved indeed:
> >
> > If the anchor itself is marked with the quick option, ruleset
> > evaluation will terminate when the anchor is exited if the packet is
> > matched by any rule within the anchor.
> >
> > Does an empty ruleset match a packet? If so, and our code technically
> > does so, -current including my latest fix behaves correctly according
> > to the documentation.
> Admittedly, I should have read the second paragraph more carefully.
>
> > > With the current fix, rule evaluation stops after the first block,
> > > regardless of matching. Therefore the following currently always passes:
> > >
> > > snap# uname -a
> > > OpenBSD snap.my.domain 6.4 GENERIC#333 amd64
> > > snap# pfctl -sr
> > >
> > > pass all flags S/SA
> > > anchor quick all {
> > > }
> > > block drop all
> > >
> > > 6.1 behaves as i would expect, it drops all. Instead of the empty block,
> > > any non-matching rule has the same effect.
> > While rules within the anchor do not match, the anchor itself does.
> > After all, it is a rule itself just like `pass' or `block', hence your
> > ruleset's second rule `anchor quick' matches every packet and therefore
> > stops evaluation.
> When mcbride introduced `anchor quick' back in 2006 it was indeed bound
> to matches within the ruleset:
>
> commit a81eb4e4f7ac27afb4708cf434aee361f1ef8b19
> Author: mcbride
> Date: Wed Oct 11 08:42:31 2006 +
>
> Allow the 'quick' keyword on an anchor. IFF there is a matching
> rule inside
> the anchor, terminate ruleset evaluation when stepping out of the
> anchor.
>
> This means that if you absolutely want the anchor to be terminal,
> you
> probably want to use a 'block all' or 'pass all' rule at the start
> of the
> anchor.
>
> ok dhartmei@ henning@ deraadt@
>
> Digging through commits and the history of PF already takes more of my
> time than I anticipated. Some time after 2006, or more specifically
> after 6.1 being released, this behaviour got broken^Wchanged - most
> likely unintionally.
>
> Since our code now treats `anchor quick' like any other normal rule
> (see my other reply to benno in this thread), I suggest we adjust our
> documentation so users will not fall for it again.
>
> Fixing^Wchaning this behaviour is debatable but should probably not
> happen before 6.4 comes out anyway, so I suggest we better ship with
> a correct manual.
>
> Feedback? OK?
I think the current behaviour is wrong and the original from 2006 correct.
I think it should do what the above commit message (and the manpage) says.
The quick in the anchor does not do anything by itself, it should just
"behave like all the rules inside the anchor had the quick keyword".
> We might as well remove the entire sentence to not special case anchors
> at all anymore.
>
> Index: pf.conf.5
> ===
> RCS file: /cvs/src/share/man/man5/pf.conf.5,v
> retrieving revision 1.577
> diff -u -p -r1.577 pf.conf.5
> --- pf.conf.5 12 Jul 2018 05:54:49 - 1.577
> +++ pf.conf.5 7 Oct 2018 17:40:07 -
> @@ -1865,8 +1865,7 @@ anchors and the main ruleset.
> If the anchor itself is marked with the
> .Cm quick
> option,
> -ruleset evaluation will terminate when the anchor is exited if the packet is
> -matched by any rule within the anchor.
> +ruleset evaluation will terminate when the anchor is exited.
> .Pp
> An anchor references other anchor attachment points
> using the following syntax:
>
Re: pf: honor quick on anchor rules
On Fri, Oct 05, 2018 at 11:53:08PM +0200, Klemens Nanni wrote:
> On Fri, Oct 05, 2018 at 10:38:48PM +0200, Fabian Mueller-Knapp wrote:
> > If i read man correctly it means "evaluate the rules inside and stop if
> > any rule within matched".
> While it's own description is quite clear and unambigious:
>
> quick If a packet matches a rule which has the quick option set, this
> rule is considered the last matching rule, and evaluation of
> subsequent rules is skipped.
>
> the anchor specific addition can be improved indeed:
>
> If the anchor itself is marked with the quick option, ruleset
> evaluation will terminate when the anchor is exited if the packet is
> matched by any rule within the anchor.
>
> Does an empty ruleset match a packet? If so, and our code technically
> does so, -current including my latest fix behaves correctly according
> to the documentation.
Admittedly, I should have read the second paragraph more carefully.
> > With the current fix, rule evaluation stops after the first block,
> > regardless of matching. Therefore the following currently always passes:
> >
> > snap# uname -a
> > OpenBSD snap.my.domain 6.4 GENERIC#333 amd64
> > snap# pfctl -sr
> >
> > pass all flags S/SA
> > anchor quick all {
> > }
> > block drop all
> >
> > 6.1 behaves as i would expect, it drops all. Instead of the empty block,
> > any non-matching rule has the same effect.
> While rules within the anchor do not match, the anchor itself does.
> After all, it is a rule itself just like `pass' or `block', hence your
> ruleset's second rule `anchor quick' matches every packet and therefore
> stops evaluation.
When mcbride introduced `anchor quick' back in 2006 it was indeed bound
to matches within the ruleset:
commit a81eb4e4f7ac27afb4708cf434aee361f1ef8b19
Author: mcbride
Date: Wed Oct 11 08:42:31 2006 +
Allow the 'quick' keyword on an anchor. IFF there is a matching
rule inside
the anchor, terminate ruleset evaluation when stepping out of the
anchor.
This means that if you absolutely want the anchor to be terminal,
you
probably want to use a 'block all' or 'pass all' rule at the start
of the
anchor.
ok dhartmei@ henning@ deraadt@
Digging through commits and the history of PF already takes more of my
time than I anticipated. Some time after 2006, or more specifically
after 6.1 being released, this behaviour got broken^Wchanged - most
likely unintionally.
Since our code now treats `anchor quick' like any other normal rule
(see my other reply to benno in this thread), I suggest we adjust our
documentation so users will not fall for it again.
Fixing^Wchaning this behaviour is debatable but should probably not
happen before 6.4 comes out anyway, so I suggest we better ship with
a correct manual.
Feedback? OK?
We might as well remove the entire sentence to not special case anchors
at all anymore.
Index: pf.conf.5
===
RCS file: /cvs/src/share/man/man5/pf.conf.5,v
retrieving revision 1.577
diff -u -p -r1.577 pf.conf.5
--- pf.conf.5 12 Jul 2018 05:54:49 - 1.577
+++ pf.conf.5 7 Oct 2018 17:40:07 -
@@ -1865,8 +1865,7 @@ anchors and the main ruleset.
If the anchor itself is marked with the
.Cm quick
option,
-ruleset evaluation will terminate when the anchor is exited if the packet is
-matched by any rule within the anchor.
+ruleset evaluation will terminate when the anchor is exited.
.Pp
An anchor references other anchor attachment points
using the following syntax:
Re: pf: honor quick on anchor rules
On Sat, Oct 06, 2018 at 01:07:33PM +0200, Sebastian Benoit wrote:
> I would find it surprising if an empty anchor matches anything.
> And i find the wording above clear in this regard:
>
> ... ruleset evaluation will terminate when the anchor is exited if
> the packet is matched by any rule within the anchor.
>
> If there is no rule in the anchor, then there is no match, so the quick
> should do nothing.
You are wrong. Matches *within* are different from matching on the
anchor itself:
# echo anchor a quick | pfctl -vf-
anchor "a" quick all
All packets match this anchor, as made clear by `all'. Think
s/anchor "a"/pass/ here if you will, doesn't make a difference. Further
adding rules to the anchor "a" can change it's outcome, though:
# echo block inet | pfctl -aa -f-
# pfctl -sr -a\*
anchor "a" quick all {
block drop inet all
}
So now you just block IPv4, but still pass everything else.
This is perfectly valid. Thinking about it once more, we really cannot
special-case anchors (with empty rulesets). Taking above example into
account, should the anchor disappear and magically change your loaded
ruleset once it's cleared? It does not make any sense and would break
lots of real use cases.
Again: daemons rely on this behaviour, see relayd(8) which must interact
with PF through a previously defined anchor:
anchor "relayd/*"
Where this anchor is placed is up to the user and so is it's content.
There is nothing that stops you from doing
anchor "relayd/*" inet6
to ignore IPv4 traffic through relayd or other crazy things. Messing
with how anchors are treated (they are really just normal rules) is a
bad idea.
This should somewhat answer your question as well, Theo.
Re: pf: honor quick on anchor rules
Klemens Nanni(k...@openbsd.org) on 2018.10.05 23:53:08 +0200:
> On Fri, Oct 05, 2018 at 10:38:48PM +0200, Fabian Mueller-Knapp wrote:
> > If i read man correctly it means "evaluate the rules inside and stop if
> > any rule within matched".
> While it's own description is quite clear and unambigious:
>
> quick If a packet matches a rule which has the quick option set, this
> rule is considered the last matching rule, and evaluation of
> subsequent rules is skipped.
>
> the anchor specific addition can be improved indeed:
>
> If the anchor itself is marked with the quick option, ruleset
> evaluation will terminate when the anchor is exited if the packet is
> matched by any rule within the anchor.
>
> Does an empty ruleset match a packet? If so, and our code technically
I would find it surprising if an empty anchor matches anything.
And i find the wording above clear in this regard:
... ruleset evaluation will terminate when the anchor is exited if
the packet is matched by any rule within the anchor.
If there is no rule in the anchor, then there is no match, so the quick
should do nothing.
> does so, -current including my latest fix behaves correctly according
> to the documentation.
>
> > With the current fix, rule evaluation stops after the first block,
> > regardless of matching. Therefore the following currently always passes:
> >
> > snap# uname -a
> > OpenBSD snap.my.domain 6.4 GENERIC#333 amd64
> > snap# pfctl -sr
> >
> > pass all flags S/SA
> > anchor quick all {
> > }
> > block drop all
> >
> > 6.1 behaves as i would expect, it drops all. Instead of the empty block,
> > any non-matching rule has the same effect.
> While rules within the anchor do not match, the anchor itself does.
> After all, it is a rule itself just like `pass' or `block', hence your
> ruleset's second rule `anchor quick' matches every packet and therefore
> stops evaluation.
>
Re: pf: honor quick on anchor rules
Klemens Nanni wrote:
> On Fri, Oct 05, 2018 at 11:53:08PM +0200, Klemens Nanni wrote:
> > While rules within the anchor do not match, the anchor itself does.
> > After all, it is a rule itself just like `pass' or `block', hence your
> > ruleset's second rule `anchor quick' matches every packet and therefore
> > stops evaluation.
> To illustrate further:
>
> Block everything except IPv4 traffic:
>
> pass
> anchor quick inet {
> }
> block
>
> This very bad example is stupid but works as expected. It's especially
> bad as it uses an anonymous anchor, thus you cannot modify it later on
> using `pfctl -a ...'.
>
> Maybe we should disallow anonymous anchors with empty rulesets for above
> mentioned reasons?
I don't understand why to disallow it. It is quirky, but does as
expected. Why does one need to be able to modify it? anchors grew
into what you see now, the initial design was an attempt to beat skip-step
performance by giving the rule-writer more control. But then the optimizer
showed up...
Re: pf: honor quick on anchor rules
On Fri, Oct 05, 2018 at 11:53:08PM +0200, Klemens Nanni wrote:
> While rules within the anchor do not match, the anchor itself does.
> After all, it is a rule itself just like `pass' or `block', hence your
> ruleset's second rule `anchor quick' matches every packet and therefore
> stops evaluation.
To illustrate further:
Block everything except IPv4 traffic:
pass
anchor quick inet {
}
block
This very bad example is stupid but works as expected. It's especially
bad as it uses an anonymous anchor, thus you cannot modify it later on
using `pfctl -a ...'.
Maybe we should disallow anonymous anchors with empty rulesets for above
mentioned reasons?
Do note that named but empty anchors as fine and even neccessary for
some daemons such as relayd(8):
anchor "relayd/*"
Re: pf: honor quick on anchor rules
On Fri, Oct 05, 2018 at 10:38:48PM +0200, Fabian Mueller-Knapp wrote:
> If i read man correctly it means "evaluate the rules inside and stop if
> any rule within matched".
While it's own description is quite clear and unambigious:
quick If a packet matches a rule which has the quick option set, this
rule is considered the last matching rule, and evaluation of
subsequent rules is skipped.
the anchor specific addition can be improved indeed:
If the anchor itself is marked with the quick option, ruleset
evaluation will terminate when the anchor is exited if the packet is
matched by any rule within the anchor.
Does an empty ruleset match a packet? If so, and our code technically
does so, -current including my latest fix behaves correctly according
to the documentation.
> With the current fix, rule evaluation stops after the first block,
> regardless of matching. Therefore the following currently always passes:
>
> snap# uname -a
> OpenBSD snap.my.domain 6.4 GENERIC#333 amd64
> snap# pfctl -sr
>
> pass all flags S/SA
> anchor quick all {
> }
> block drop all
>
> 6.1 behaves as i would expect, it drops all. Instead of the empty block,
> any non-matching rule has the same effect.
While rules within the anchor do not match, the anchor itself does.
After all, it is a rule itself just like `pass' or `block', hence your
ruleset's second rule `anchor quick' matches every packet and therefore
stops evaluation.
Re: pf: honor quick on anchor rules
Hello,
Sorry for the late reply. I just tested the fix with the latest snapshot
and think the behaviour is still not correct:
On 18-10-04 01:25:09, Klemens Nanni wrote:
> On Sat, Sep 29, 2018 at 10:44:41PM +0200, Klemens Nanni wrote:
>
> `anchor quick' means "evaluate the rules inside but stop after that".
If i read man correctly it means "evaluate the rules inside and stop if
any rule within matched".
With the current fix, rule evaluation stops after the first block,
regardless of matching. Therefore the following currently always passes:
snap# uname -a
OpenBSD snap.my.domain 6.4 GENERIC#333 amd64
snap# pfctl -sr
pass all flags S/SA
anchor quick all {
}
block drop all
6.1 behaves as i would expect, it drops all. Instead of the empty block,
any non-matching rule has the same effect.
Regards,
Fabian
Re: pf: honor quick on anchor rules
I just committed the fix, thanks.
Re: pf: honor quick on anchor rules
Hi Klemens, > > > Do i misread the manpage somehow? > > No, this is a bug. > Allow me a bit of rubber ducking to explain this bug and ease review: > > The kernel evaluates the ruleset pretty much like we read it: Down to > bottom until quick appears or an error occurs in which case we stop > evaluating. That is, packets are tested against each rule which yields > either of OK, QUICK, FAIL. > > `anchor quick' means "evaluate the rules inside but stop after that". > According to the procedure explained above, an anchor rule's test status > is the result of its contained ruleset: > > sys/net/pf.c > 3130 rv = pf_match_rule(ctx, &r->anchor->ruleset); > > While this approach is valid for other type of rules, it overwrites the > anchor rule's *own* QUICK test result such that it has no effect at all. > To fix this, simply pass it along except when there was an error so we > do not clobber it (and make the same mistake again). > > > Feedback? OK? thank you for cleaning up the mess I did create long time ago. I agree with your change. OK sashan
pf: honor quick on anchor rules
On Sat, Sep 29, 2018 at 10:44:41PM +0200, Klemens Nanni wrote:
> On Sat, Sep 29, 2018 at 06:17:05PM +0200, Fabian Mueller-Knapp wrote:
> > I have the following pf.conf:
> >
> > anchor quick {
> > pass
> > }
> > block
> >
> > # pfctl -sr
> > anchor quick all {
> > pass all flags S/SA
> > }
> > block drop all
> >
> > Because of the 'quick' i assumed, that 'block' is never reached, but it
> > is since 6.2.
> Indeed, `pfctl -s rules -v' clearly shows how every packet goes through
> all three rules.
>
> > man pf.conf(5) states:
> >
> > "If the anchor itself is marked with the quick option, ruleset
> > evaluation will terminate when the anchor is exited if the packet is
> > matched by any rule within the anchor."
> >
> > I tested with fresh installs of 6.1, 6.2, 6.3 and current via vmd and
> > 6.1 does in fact behave as i would accept (that is, all packets
> > pass). From 6.2 on however, all packets are dropped.
> Thanks for your report.
>
> > Do i misread the manpage somehow?
> No, this is a bug.
Allow me a bit of rubber ducking to explain this bug and ease review:
The kernel evaluates the ruleset pretty much like we read it: Down to
bottom until quick appears or an error occurs in which case we stop
evaluating. That is, packets are tested against each rule which yields
either of OK, QUICK, FAIL.
`anchor quick' means "evaluate the rules inside but stop after that".
According to the procedure explained above, an anchor rule's test status
is the result of its contained ruleset:
sys/net/pf.c
3130rv = pf_match_rule(ctx, &r->anchor->ruleset);
While this approach is valid for other type of rules, it overwrites the
anchor rule's *own* QUICK test result such that it has no effect at all.
To fix this, simply pass it along except when there was an error so we
do not clobber it (and make the same mistake again).
Feedback? OK?
Index: net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1075
diff -u -p -r1.1075 pf.c
--- net/pf.c13 Sep 2018 19:53:58 - 1.1075
+++ net/pf.c3 Oct 2018 22:22:32 -
@@ -3128,6 +3128,12 @@ pf_step_into_anchor(struct pf_test_ctx *
}
} else {
rv = pf_match_rule(ctx, &r->anchor->ruleset);
+ /*
+* Unless there was an error inside the anchor,
+* retain its quick state.
+*/
+ if (rv != PF_TEST_FAIL && r->quick == PF_TEST_QUICK)
+ rv = PF_TEST_QUICK;
}
ctx->depth--;
