From: Kapetanakis Giannis Subject: Re: wrong reference to anchor/rule may appear in pflog (or state) To: tech@openbsd.org Date: Sat, 22 Jun 2024 01:05:19 +0300 To my point of view this deserves an errata. Apart from pflog wrong numbering, you also have a broken pfctl -ss -vv You see states belonging to different (rule) numbers than they really are. It puzzled me a lot trying to understand why I had certain states. I though something was wrong with my firewall rules. G On 20/06/2024 14:42, Stuart Henderson wrote: > From my understanding of the description, I think this diff makes sense. > It certainly fixes quite an annoying problem that makes debugging rulesets > very tricky since 7.3 if you use anchors (e.g. for ftp-proxy). > > I don't have full understanding of the anchor code, but OK sthen FWIW. > > On 2024/06/17 10:43, Alexandr Nedvedicky wrote: >> ping! OK from anyone? >> >> thanks and >> regards >> sashan >> >> >> On Fri, May 24, 2024 at 09:23:54AM +0200, Alexandr Nedvedicky wrote: >>> Hello, >>> >>> The issue has been brought up and analyzed by Giannis Kapetanakis here [1]. >>> Giannis went back in history and found a culprit: >>> >>> pf.c (2023/01/05) 1.1169 >>> >>> The change in 1.1169 fixes stack overflow which may be triggered with deeply >>> nested anchors. Unfortunately that fix introduces issue reported by Giannis. >>> To trigger it one has to use rule anchors like for example here: >>> >>> @0 match in all scrub (no-df random-id) >>> @1 pass out log proto tcp from self to any port 12345 >>> @2 anchor "relayd/*" >>> @3 anchor "test" { >>> @0 pass out log proto tcp from self to any port 12346 >>> @1 anchor "foo" { >>> @0 pass out log proto tcp from self to any port 12348 >>> } >>> @2 pass out log proto tcp from self to any port 12349 >>> } >>> @4 pass out log proto tcp from self to any port 12347 >>> >>> Rules above use the same numbering style which is also used by command >>> 'pfctl -sr -v' >>> >>> if packet is sent to remote port 12349 it matches the rule @2 >>> in anchor @3 ('test'). pflog (and also state shown by 'pfctl -ss -vv') >>> should report that in form: >>> anchor 3, rule 2 >>> however the pf in current reports this: >>> anchor 1, rule 2 >>> >>> The diff below is based on idea of simple one-liner tweak >>> proposed by Giannis off-list. His idea is to just forget >>> the reference to anchor as pf backtraces back towards root. >>> Such fix would work if there would be no rule @2 in anchor @3. >>> For general case we need to save/restore reference to anchor >>> as we traverse the rulesets. >>> >>> Note: I think pf never recorded a full path from root to rule, >>> pf just reports leaf anchor where the rule belongs to. To record >>> the full path a follow-up change is needed. >>> >>> OK to commit diff below? >>> >>> thanks and >>> regards >>> sashan >>> >>> [1] https://marc.info/?t=171611737500001&r=1&w=2 >>> >>> --------8<---------------8<---------------8<------------------8<-------- >>> diff --git a/sys/net/pf.c b/sys/net/pf.c >>> index 8591b044e43..007a00c6bc6 100644 >>> --- a/sys/net/pf.c >>> +++ b/sys/net/pf.c >>> @@ -3666,8 +3666,8 @@ pf_anchor_stack_top(void) >>> } >>> >>> int >>> -pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *r, >>> - struct pf_anchor *child, int jump_target) >>> +pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *anchor, >>> + struct pf_rule *r, struct pf_anchor *child, int jump_target) >>> { >>> struct pf_anchor_stackframe *stack; >>> struct pf_anchor_stackframe *top_sf = pf_anchor_stack_top(); >>> @@ -3677,6 +3677,7 @@ pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *r, >>> return (-1); >>> >>> top_sf->sf_rs = rs; >>> + top_sf->sf_anchor = anchor; >>> top_sf->sf_r = r; >>> top_sf->sf_child = child; >>> top_sf->sf_jump_target = jump_target; >>> @@ -3693,8 +3694,8 @@ pf_anchor_stack_push(struct pf_ruleset *rs, struct pf_rule *r, >>> } >>> >>> int >>> -pf_anchor_stack_pop(struct pf_ruleset **rs, struct pf_rule **r, >>> - struct pf_anchor **child, int *jump_target) >>> +pf_anchor_stack_pop(struct pf_ruleset **rs, struct pf_rule **anchor, >>> + struct pf_rule **r, struct pf_anchor **child, int *jump_target) >>> { >>> struct pf_anchor_stackframe *top_sf = pf_anchor_stack_top(); >>> struct pf_anchor_stackframe *stack; >>> @@ -3710,6 +3711,7 @@ pf_anchor_stack_pop(struct pf_ruleset **rs, struct pf_rule **r, >>> __func__); >>> >>> *rs = top_sf->sf_rs; >>> + *anchor = top_sf->sf_anchor; >>> *r = top_sf->sf_r; >>> *child = top_sf->sf_child; >>> *jump_target = top_sf->sf_jump_target; >>> @@ -4306,25 +4308,27 @@ enter_ruleset: >>> if (r->quick) >>> return (PF_TEST_QUICK); >>> } else { >>> - ctx->a = r; >>> ctx->aruleset = &r->anchor->ruleset; >>> if (r->anchor_wildcard) { >>> RB_FOREACH(child, pf_anchor_node, >>> &r->anchor->children) { >>> - if (pf_anchor_stack_push(ruleset, r, >>> - child, PF_NEXT_CHILD) != 0) >>> + if (pf_anchor_stack_push(ruleset, >>> + ctx->a, r, child, >>> + PF_NEXT_CHILD) != 0) >>> return (PF_TEST_FAIL); >>> >>> + ctx->a = r; >>> ruleset = &child->ruleset; >>> goto enter_ruleset; >>> next_child: >>> continue; /* with RB_FOREACH() */ >>> } >>> } else { >>> - if (pf_anchor_stack_push(ruleset, r, child, >>> - PF_NEXT_RULE) != 0) >>> + if (pf_anchor_stack_push(ruleset, ctx->a, >>> + r, child, PF_NEXT_RULE) != 0) >>> return (PF_TEST_FAIL); >>> >>> + ctx->a = r; >>> ruleset = &r->anchor->ruleset; >>> child = NULL; >>> goto enter_ruleset; >>> @@ -4335,7 +4339,9 @@ next_rule: >>> r = TAILQ_NEXT(r, entries); >>> } >>> >>> - if (pf_anchor_stack_pop(&ruleset, &r, &child, &target) == 0) { >>> + if (pf_anchor_stack_pop(&ruleset, &ctx->a, &r, &child, >>> + &target) == 0) { >>> + >>> /* stop if any rule matched within quick anchors. */ >>> if (r->quick == PF_TEST_QUICK && *ctx->am == r) >>> return (PF_TEST_QUICK); >>> diff --git a/sys/net/pfvar_priv.h b/sys/net/pfvar_priv.h >>> index 7a420bfa308..559273046ee 100644 >>> --- a/sys/net/pfvar_priv.h >>> +++ b/sys/net/pfvar_priv.h >>> @@ -321,6 +321,7 @@ struct pf_pdesc { >>> >>> struct pf_anchor_stackframe { >>> struct pf_ruleset *sf_rs; >>> + struct pf_rule *sf_anchor; >>> union { >>> struct pf_rule *u_r; >>> struct pf_anchor_stackframe *u_stack_top; >>>