Index | Thread | Search

From:
Alexandr Nedvedicky <sashan@fastmail.net>
Subject:
wrong reference to anchor/rule may appear in pflog (or state)
To:
tech@openbsd.org
Date:
Fri, 24 May 2024 09:23:54 +0200

Download raw body.

Thread
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;