Hello,
> I’ve had a bug report against FreeBSD’s pfctl which I think also applies to
> OpenBSD.
>
> The gist of it is that the macro expansion in labels/tags is done prior to
> the rule optimisation, which means that at least the $nr expansion can be
> wrong.
I agree OpenBSD suffers from the same issue. Below is a diff for OpenBSD.
The FreeBSD diff, which we got from Kristof, merged with rejects. While
dealing with them, I came with slightly different version of the fix, which
minimizes diff.
Instead of changing the whole expand_label() and its friends, I've decided
to make rule number a special case. The rule number will get expanded after
optimizer will be done.
I used a sample configuration sent by Kristof few days back [1].
running 'pfctl -nvf /tmp/kp-pf.conf' using those rules I'm getting
following output:
table <__automatic_0> const { ... 127.15.0.0/16 }
pass in on lo0 inet from <__automatic_0> to any flags S/SA
block return all
pass all flags S/SA label "ruleNo:17"
^^^^^^^^^^^^^
17 is wrong here, should be 2, we start
counting from 0
block return in on ! lo0 proto tcp from any to any port 6000:6010
block return out log proto tcp all user = 55
block return out log proto udp all user = 55
the pfctl, which runs with diff below shows rules as follows:
table <__automatic_0> const { ... 127.15.0.0/16 }
pass in on lo0 inet from <__automatic_0> to any flags S/SA
block return all
pass all flags S/SA label "ruleNo:2"
block return in on ! lo0 proto tcp from any to any port 6000:6010
block return out log proto tcp all user = 55
block return out log proto udp all user = 55
the regress tests pass with my change.
thanks and
regards
sashan
[1] https://marc.info/?l=openbsd-bugs&m=163431381023475&w=2
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index 7eb43c38e87..50a0b8a328e 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -358,7 +358,6 @@ void expand_label_addr(const char *, char
*, size_t, u_int8_t,
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);
@@ -4196,14 +4195,20 @@ 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)
+pfctl_expand_label_nr(struct pf_rule *r, unsigned int rno)
{
char n[11];
- if (strstr(label, name) != NULL) {
- snprintf(n, sizeof(n), "%u", pf->anchor->match);
- expand_label_str(label, len, name, n);
- }
+ snprintf(n, sizeof(n), "%u", rno);
+
+ if (strstr(r->label, "$nr") != NULL)
+ expand_label_str(r->label, PF_RULE_LABEL_SIZE, "$nr", n);
+
+ if (strstr(r->tagname, "$nr") != NULL)
+ expand_label_str(r->tagname, PF_TAG_NAME_SIZE, "$nr", n);
+
+ if (strstr(r->match_tagname, "$nr") != NULL)
+ expand_label_str(r->match_tagname, PF_TAG_NAME_SIZE, "$nr", n);
}
void
@@ -4218,7 +4223,7 @@ 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);
+ /* rule number, '$nr', gets expanded after optimizer */
}
int
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 3441d47aaca..923daaa6937 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -1425,6 +1425,7 @@ pfctl_load_ruleset(struct pfctl *pf, char *path, struct
pf_ruleset *rs,
struct pf_rule *r;
int error, len = strlen(path);
int brace = 0;
+ unsigned int rno = 0;
pf->anchor = rs->anchor;
@@ -1454,6 +1455,8 @@ pfctl_load_ruleset(struct pfctl *pf, char *path, struct
pf_ruleset *rs,
while ((r = TAILQ_FIRST(rs->rules.active.ptr)) != NULL) {
TAILQ_REMOVE(rs->rules.active.ptr, r, entries);
+ pfctl_expand_label_nr(r, rno);
+ rno++;
if ((error = pfctl_load_rule(pf, path, r, depth)))
goto error;
if (r->anchor) {
diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h
index a82854a0fea..6179b95cb63 100644
--- a/sbin/pfctl/pfctl_parser.h
+++ b/sbin/pfctl/pfctl_parser.h
@@ -248,6 +248,7 @@ void print_queuespec(struct pf_queuespec *);
int pfctl_define_table(char *, int, int, const char *, struct pfr_buffer *,
u_int32_t);
+void pfctl_expand_label_nr(struct pf_rule *, unsigned int);
void pfctl_clear_fingerprints(int, int);
int pfctl_file_fingerprints(int, int, const char *);
