Re: pf label $nr macro expand reproducable bug

On 22 Sep 2021, at 11:47, Özkan KIRIK wrote:
> Hi Kristof,
>
> I tried many things and I found the real problem to reproduce the bug.
> Tested with the latest stable/12.
> And also tested with Live CD without installing
> (https://download.freebsd.org/ftp/snapshots/ISO-IMAGES/12.2/FreeBSD-12.2-STABLE-amd64-20210916-r370608-disc1.iso).
> The result is same.
>
> My determination is the problem in the rule optimizer of pf. You can
> see the difference with / without ruleset optimization.
> Without ruleset optimization, $nr macro expanding is true. otherwise 
> false.
>
> if the interface used in the rule, have multiple IP addresses that
> rule optimizer removes lines then the rule number expanding fails. ie:
>
Yeah, that makes sense. It looks like the problem is that we expand the 
$nr tag before the ruleset optimisation, so we end up using the wrong 
rule number.

As a first attempt this also seems to work:

	diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
	index 89d5f330da47..eb38fd9e344e 100644
	--- a/sbin/pfctl/parse.y
	+++ b/sbin/pfctl/parse.y
	@@ -330,14 +330,12 @@ int                filter_consistent(struct 
pfctl_rule *, int);
	 int             nat_consistent(struct pfctl_rule *);
	 int             rdr_consistent(struct pfctl_rule *);
	 int             process_tabledef(char *, struct table_opts *);
	-void            expand_label_str(char *, size_t, const char *, const 
char *);
	 void            expand_label_if(const char *, char *, size_t, const 
char *);
	 void            expand_label_addr(const char *, char *, size_t, 
u_int8_t,
	                    struct node_host *);
	 void            expand_label_port(const char *, char *, size_t,
	                    struct node_port *);
	 void            expand_label_proto(const char *, char *, size_t, 
u_int8_t);
	-void            expand_label_nr(const char *, char *, size_t);
	 void            expand_label(char *, size_t, const char *, u_int8_t,
	                    struct node_host *, struct node_port *, struct 
node_host *,
	                    struct node_port *, u_int8_t);
	@@ -5125,17 +5123,6 @@ expand_label_proto(const char *name, char 
*label, size_t len, u_int8_t proto)
	        }
	 }

	-void
	-expand_label_nr(const char *name, char *label, size_t len)
	-{
	-       char n[11];
	-
	-       if (strstr(label, name) != NULL) {
	-               snprintf(n, sizeof(n), "%u", pf->anchor->match);
	-               expand_label_str(label, len, name, n);
	-       }
	-}
	-
	 void
	 expand_label(char *label, size_t len, const char *ifname, sa_family_t 
af,
	     struct node_host *src_host, struct node_port *src_port,
	@@ -5148,7 +5135,6 @@ expand_label(char *label, size_t len, const char 
*ifname, sa_family_t af,
	        expand_label_port("$srcport", label, len, src_port);
	        expand_label_port("$dstport", label, len, dst_port);
	        expand_label_proto("$proto", label, len, proto);
	-       expand_label_nr("$nr", label, len);
	 }

	 int
	diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
	index d7bde0012e9b..749d73705f45 100644
	--- a/sbin/pfctl/pfctl.c
	+++ b/sbin/pfctl/pfctl.c
	@@ -1528,6 +1528,15 @@ pfctl_load_ruleset(struct pfctl *pf, char *path, 
struct pfctl_ruleset *rs,

	        while ((r = TAILQ_FIRST(rs->rules[rs_num].active.ptr)) != NULL) 
{
	                TAILQ_REMOVE(rs->rules[rs_num].active.ptr, r, entries);
	+               char n[11];
	+               snprintf(n, sizeof(n), "%u", r->nr);
	+               for (int i = 0; i < PF_RULE_MAX_LABEL_COUNT; i++) {
	+                       expand_label_str(r->label[i], 
PF_RULE_LABEL_SIZE,
	+                           "$nr", n);
	+               }
	+               expand_label_str(r->tagname, PF_TAG_NAME_SIZE, "$nr", 
n);
	+               expand_label_str(r->match_tagname, PF_TAG_NAME_SIZE, 
"$nr", n);
	+
	                if ((error = pfctl_load_rule(pf, path, r, depth)))
	                        goto error;
	                if (r->anchor) {
	diff --git a/sbin/pfctl/pfctl.h b/sbin/pfctl/pfctl.h
	index 80ef184fa90f..d725341d1cd7 100644
	--- a/sbin/pfctl/pfctl.h
	+++ b/sbin/pfctl/pfctl.h
	@@ -139,5 +139,6 @@ struct pfctl_ruleset        *pf_find_ruleset(const 
char *);
	 struct pfctl_ruleset   *pf_find_or_create_ruleset(const char *);

	 const char *pfctl_proto2name(int);
	+void expand_label_str(char *, size_t, const char *, const char *);

	 #endif /* _PFCTL_H_ */

I think I want to restructure that a bit, but this should already work.

Best regards,
Kristof