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
