From: Alexandr Nedvedicky Subject: Re: wrong reference to anchor/rule may appear in pflog (or state) To: tech@openbsd.org Date: Mon, 17 Jun 2024 10:43:18 +0200 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; >