Hello,
patch below fixes various glitches I've found in pfctl. All issues are related
to anchors. I did test the patch using regress/sbin/pfctl. Although all tests
have passed, I still would like to ask for testing in field, especially if you
use some fancy configuration with 'load anchor'. The patch is just intended for
testing. I'll break the patch to smaller chunks to get some OKs.
the list of bugfixes is as follows:
- pfctl_optimize.c is not able to create radix table. This bug was reported
by Leonardo Guardati. It's yet another occurrence of 'name vs. path' mix
up [1]
- use after free, when pfctl_optimize fails to create radix table. We are
just (un)lucky enough the Leonardo hit [1] in pfctl_optimize.c
- the use after free is triggered in error path of
pfctl_optimize_ruleset(). I gave a closer look to function and spot a
memory leak. Fortunately the memory leak is not critical, because the
pfctl is just one-shot application: load rules and quit anyway.
- when I implemented a test case using the rules from Leonardo I found
yet another oddity in processing of 'load anchor'. The 'load anchor"
is handled differently when the rules are being loaded to root
anchor (pfctl -f pf-load-anchor.conf) and when the same rules are being
loaded to non-root anchor (pfctl -a regression -f pf-load-anchor.conf).
The test case I've got from Leonardo is simple configuration using nested
anchors. There are just two nesting levels. The pf.conf file, which loads
level one looks as follows:
--------8<---------------8<---------------8<------------------8<--------
anchor "one"
load anchor "one" from "/home/sashan/leonardo/pf-optimize.one"
--------8<---------------8<---------------8<------------------8<--------
the pf-optimize.one file contains just simple anchor, which loads anchor
two:
--------8<---------------8<---------------8<------------------8<--------
anchor "two"
load anchor "two" from "/home/sashan/leonardo/pf-optimize.two"
--------8<---------------8<---------------8<------------------8<--------
finally the pf-optimize.two contains a ruleset, which triggers the
bugs found in pfctl_optimize.c:
--------8<---------------8<---------------8<------------------8<--------
addrs = "{ 1.2.3.4,
10.20.30.40,
2.4.6.8,
20.40.60.80,
4.8.12.16,
40.80.120.160,
5.6.7.8,
50.60.70.80,
10.12.14.16,
100.120.140.160
}"
pass from $addrs
--------8<---------------8<---------------8<------------------8<--------
As you can see the optimizer is supposed to fold the list of addresses 'addrs'
to radix table. This currently fails because of 'name vs. path' mix up
in call to pfctl_define_table().
Assuming the 'mix up' is either fixed (or pf-optimize.two is altered to simple
'pass all') we can load the pf.conf to driver using 'pfctl -f pf.conf' command:
--------8<---------------8<---------------8<------------------8<--------
pfctl -f pf.conf
pfctl -vsA
one
one/two
--------8<---------------8<---------------8<------------------8<--------
The output of 'pfctl -vsA' is something we expect. Now let's try to load
the pf.conf using 'pfctl -a regress -f pf.conf' we get something like this:
--------8<---------------8<---------------8<------------------8<--------
pfctl -a regress -f pf.conf
pfctl -vsA
regress
regress/one
--------8<---------------8<---------------8<------------------8<--------
we are missing 'regress/one/two' anchor in output above. With patch applied we
get expected results:
--------8<---------------8<---------------8<------------------8<--------
pfctl -a regress -f /home/sashan/leonardo/pf-optimize.conf
pfctl -vsA
regress
regress/one
regress/one/two
--------8<---------------8<---------------8<------------------8<--------
I'll send individual fixes for review later on Friday.
Thank you for testing of patch below.
regards
sasha
[1] https://marc.info/?l=openbsd-tech&m=147292142630058&w=2
--------8<---------------8<---------------8<------------------8<--------
diff --git a/regress/sbin/pfctl/Makefile b/regress/sbin/pfctl/Makefile
index d42bb446e68..5572785b121 100644
--- a/regress/sbin/pfctl/Makefile
+++ b/regress/sbin/pfctl/Makefile
@@ -30,6 +30,7 @@ PFIF2IP=1 2 3
PFCHKSUM=1 2 3
PFCMD=1
PFCMDFAIL=1
+PFLOADANCHORS=112 113
MAKEOBJDIRPREFIX=
@@ -328,6 +329,20 @@ pfchksum-update: ${PFCHKSUM_UPDATES}
NODEFAULT_TARGETS+=pfchksum
REGRESS_ROOT_TARGETS+=pfchksum
+.for n in ${PFLOADANCHORS}
+PFLOADANCHORS_TARGETS+=pfloadanchors${n}
+
+pfloadanchors${n}:
+ ${SUDO} pfctl -Fa > /dev/null 2>&1
+ ${SUDO} pfctl -a regress -f - < ${.CURDIR}/pf${n}.in
+ ${SUDO} pfctl -Fa > /dev/null 2>&1
+
+.endfor
+
+pfloadanchors: ${PFLOADANCHORS_TARGETS}
+
+REGRESS_TARGETS+=pfloadanchors
+
update: ${UPDATE_TARGETS}
alltests: ${REGRESS_TARGETS} ${NODEFAULT_TARGETS}
diff --git a/regress/sbin/pfctl/pf112.in b/regress/sbin/pfctl/pf112.in
new file mode 100644
index 00000000000..5b40dc0e69d
--- /dev/null
+++ b/regress/sbin/pfctl/pf112.in
@@ -0,0 +1,2 @@
+anchor "one"
+load anchor "one" from "pf112.one"
diff --git a/regress/sbin/pfctl/pf112.one b/regress/sbin/pfctl/pf112.one
new file mode 100644
index 00000000000..68e20033087
--- /dev/null
+++ b/regress/sbin/pfctl/pf112.one
@@ -0,0 +1,2 @@
+anchor "two"
+load anchor "two" from "pf112.two"
diff --git a/regress/sbin/pfctl/pf112.two b/regress/sbin/pfctl/pf112.two
new file mode 100644
index 00000000000..84e5f759569
--- /dev/null
+++ b/regress/sbin/pfctl/pf112.two
@@ -0,0 +1,2 @@
+table <foo> { 10.0.0.1 }
+pass from <foo>
diff --git a/regress/sbin/pfctl/pf113.in b/regress/sbin/pfctl/pf113.in
new file mode 100644
index 00000000000..f62fa7ab84a
--- /dev/null
+++ b/regress/sbin/pfctl/pf113.in
@@ -0,0 +1,2 @@
+anchor "one"
+load anchor "one" from "pf113.one"
diff --git a/regress/sbin/pfctl/pf113.one b/regress/sbin/pfctl/pf113.one
new file mode 100644
index 00000000000..4e4b63316b4
--- /dev/null
+++ b/regress/sbin/pfctl/pf113.one
@@ -0,0 +1,2 @@
+anchor "two"
+load anchor "two" from "pf113.two"
diff --git a/regress/sbin/pfctl/pf113.two b/regress/sbin/pfctl/pf113.two
new file mode 100644
index 00000000000..a99c64f94dc
--- /dev/null
+++ b/regress/sbin/pfctl/pf113.two
@@ -0,0 +1,12 @@
+addrs = "{ 1.2.3.4,
+ 10.20.30.40,
+ 2.4.6.8,
+ 20.40.60.80,
+ 4.8.12.16,
+ 40.80.120.160,
+ 5.6.7.8,
+ 50.60.70.80,
+ 10.12.14.16,
+ 100.120.140.160
+ }"
+pass from $addrs
diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index e1dcfbc382f..1016c909ccc 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -939,7 +939,7 @@ anchorrule : ANCHOR anchorname dir quick interface af
proto fromto
loadrule : LOAD ANCHOR string FROM string {
struct loadanchors *loadanchor;
- if (strlen(pf->anchor->name) + 1 +
+ if (strlen(pf->anchor->path) + 1 +
strlen($3) >= PATH_MAX) {
yyerror("anchorname %s too long, max %u\n",
$3, PATH_MAX - 1);
@@ -954,7 +954,7 @@ loadrule : LOAD ANCHOR string FROM string {
err(1, "loadrule: malloc");
if (pf->anchor->name[0])
snprintf(loadanchor->anchorname, PATH_MAX,
- "%s/%s", pf->anchor->name, $3);
+ "%s/%s", pf->anchor->path, $3);
else
strlcpy(loadanchor->anchorname, $3, PATH_MAX);
if ((loadanchor->filename = strdup($5)) == NULL)
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index d6cfc132c0e..10d34c3ffa2 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -1614,7 +1614,6 @@ pfctl_rules(int dev, char *filename, int opts, int
optimize,
sizeof(pf.anchor->name)) >= sizeof(pf.anchor->name))
errx(1, "pfctl_add_rule: strlcpy");
-
pf.astack[0] = pf.anchor;
pf.asd = 0;
pf.trans = t;
@@ -1658,20 +1657,21 @@ pfctl_rules(int dev, char *filename, int opts, int
optimize,
free(path);
path = NULL;
- /* process "load anchor" directives that might have used queues */
- if (!anchorname[0]) {
+ if (trans == NULL) {
+ /*
+ * process "load anchor" directives that might have used queues
+ */
if (pfctl_load_anchors(dev, &pf, t) == -1)
ERRX("load anchors");
pfctl_clear_queues(&qspecs);
pfctl_clear_queues(&rootqs);
- }
- if (trans == NULL && (opts & PF_OPT_NOACTION) == 0) {
- if (!anchorname[0])
- if (pfctl_load_options(&pf))
+ if ((opts & PF_OPT_NOACTION) == 0) {
+ if (!anchorname[0] && pfctl_load_options(&pf))
goto _error;
- if (pfctl_trans(dev, t, DIOCXCOMMIT, osize))
- ERR("DIOCXCOMMIT");
+ if (pfctl_trans(dev, t, DIOCXCOMMIT, osize))
+ ERR("DIOCXCOMMIT");
+ }
}
return (0);
diff --git a/sbin/pfctl/pfctl_optimize.c b/sbin/pfctl/pfctl_optimize.c
index a1b19781756..6ec429f2e06 100644
--- a/sbin/pfctl/pfctl_optimize.c
+++ b/sbin/pfctl/pfctl_optimize.c
@@ -238,6 +238,8 @@ int skip_cmp_src_addr(struct pf_rule *, struct pf_rule *);
int skip_cmp_src_port(struct pf_rule *, struct pf_rule *);
int superblock_inclusive(struct superblock *, struct pf_opt_rule *);
void superblock_free(struct pfctl *, struct superblock *);
+struct pf_opt_tbl *pf_opt_table_ref(struct pf_opt_tbl *);
+void pf_opt_table_unref(struct pf_opt_tbl *);
int (*skip_comparitors[PF_SKIP_COUNT])(struct pf_rule *, struct pf_rule *);
@@ -315,9 +317,11 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct pf_ruleset
*rs)
err(1, "calloc");
memcpy(r, &por->por_rule, sizeof(*r));
TAILQ_INSERT_TAIL(rs->rules.active.ptr, r, entries);
+ pf_opt_table_unref(por->por_src_tbl);
+ pf_opt_table_unref(por->por_dst_tbl);
free(por);
}
- free(block);
+ superblock_free(pf, block);
}
return (0);
@@ -325,16 +329,10 @@ pfctl_optimize_ruleset(struct pfctl *pf, struct
pf_ruleset *rs)
error:
while ((por = TAILQ_FIRST(&opt_queue))) {
TAILQ_REMOVE(&opt_queue, por, por_entry);
- if (por->por_src_tbl) {
- pfr_buf_clear(por->por_src_tbl->pt_buf);
- free(por->por_src_tbl->pt_buf);
- free(por->por_src_tbl);
- }
- if (por->por_dst_tbl) {
- pfr_buf_clear(por->por_dst_tbl->pt_buf);
- free(por->por_dst_tbl->pt_buf);
- free(por->por_dst_tbl);
- }
+ if (por->por_src_tbl)
+ pf_opt_table_unref(por->por_src_tbl);
+ if (por->por_dst_tbl)
+ pf_opt_table_unref(por->por_dst_tbl);
free(por);
}
while ((block = TAILQ_FIRST(&superblocks))) {
@@ -502,13 +500,14 @@ combine_rules(struct pfctl *pf, struct superblock *block)
if (add_opt_table(pf, &p1->por_dst_tbl,
p1->por_rule.af, &p2->por_rule.dst, NULL))
return (1);
- p2->por_dst_tbl = p1->por_dst_tbl;
if (p1->por_dst_tbl->pt_rulecount >=
TABLE_THRESHOLD) {
TAILQ_REMOVE(&block->sb_rules, p2,
por_entry);
free(p2);
- }
+ } else
+ p2->por_dst_tbl =
+ pf_opt_table_ref(p1->por_dst_tbl);
} else if (!src_eq && dst_eq && p1->por_dst_tbl == NULL
&& p2->por_src_tbl == NULL &&
p2->por_dst_tbl == NULL &&
@@ -524,13 +523,14 @@ combine_rules(struct pfctl *pf, struct superblock *block)
if (add_opt_table(pf, &p1->por_src_tbl,
p1->por_rule.af, &p2->por_rule.src, NULL))
return (1);
- p2->por_src_tbl = p1->por_src_tbl;
if (p1->por_src_tbl->pt_rulecount >=
TABLE_THRESHOLD) {
TAILQ_REMOVE(&block->sb_rules, p2,
por_entry);
free(p2);
- }
+ } else
+ p2->por_src_tbl =
+ pf_opt_table_ref(p1->por_src_tbl);
}
}
}
@@ -1218,6 +1218,7 @@ add_opt_table(struct pfctl *pf, struct pf_opt_tbl **tbl,
sa_family_t af,
((*tbl)->pt_buf = calloc(1, sizeof(*(*tbl)->pt_buf))) ==
NULL)
err(1, "calloc");
+ (*tbl)->pt_refcnt = 1;
(*tbl)->pt_buf->pfrb_type = PFRB_ADDRS;
SIMPLEQ_INIT(&(*tbl)->pt_nodes);
@@ -1307,7 +1308,7 @@ again:
tablenum++;
if (pfctl_define_table(tbl->pt_name, PFR_TFLAG_CONST | tbl->pt_flags, 1,
- pf->astack[0]->name, tbl->pt_buf, pf->astack[0]->ruleset.tticket)) {
+ pf->astack[0]->path, tbl->pt_buf, pf->astack[0]->ruleset.tticket)) {
warn("failed to create table %s in %s",
tbl->pt_name, pf->astack[0]->name);
return (1);
@@ -1617,20 +1618,12 @@ superblock_free(struct pfctl *pf, struct superblock
*block)
struct pf_opt_rule *por;
while ((por = TAILQ_FIRST(&block->sb_rules))) {
TAILQ_REMOVE(&block->sb_rules, por, por_entry);
- if (por->por_src_tbl) {
- if (por->por_src_tbl->pt_buf) {
- pfr_buf_clear(por->por_src_tbl->pt_buf);
- free(por->por_src_tbl->pt_buf);
- }
- free(por->por_src_tbl);
- }
- if (por->por_dst_tbl) {
- if (por->por_dst_tbl->pt_buf) {
- pfr_buf_clear(por->por_dst_tbl->pt_buf);
- free(por->por_dst_tbl->pt_buf);
- }
- free(por->por_dst_tbl);
- }
+ if (por->por_src_tbl)
+ pf_opt_table_unref(por->por_src_tbl);
+
+ if (por->por_dst_tbl)
+ pf_opt_table_unref(por->por_dst_tbl);
+
free(por);
}
if (block->sb_profiled_block)
@@ -1638,3 +1631,24 @@ superblock_free(struct pfctl *pf, struct superblock
*block)
free(block);
}
+struct pf_opt_tbl *
+pf_opt_table_ref(struct pf_opt_tbl *pt)
+{
+ /* parser does not run concurrently, we don't need atomic ops. */
+ if (pt != NULL)
+ pt->pt_refcnt++;
+
+ return (pt);
+}
+
+void
+pf_opt_table_unref(struct pf_opt_tbl *pt)
+{
+ if ((pt != NULL) && ((--pt->pt_refcnt) == 0)) {
+ if (pt->pt_buf != NULL) {
+ pfr_buf_clear(pt->pt_buf);
+ free(pt->pt_buf);
+ }
+ free(pt);
+ }
+}
diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
index 5963b6bdffd..de544da4172 100644
--- a/sbin/pfctl/pfctl_parser.c
+++ b/sbin/pfctl/pfctl_parser.c
@@ -62,15 +62,17 @@
#include "pfctl_parser.h"
#include "pfctl.h"
-void print_op (u_int8_t, const char *, const char *);
-void print_port (u_int8_t, u_int16_t, u_int16_t, const char *, int);
-void print_ugid (u_int8_t, unsigned, unsigned, const char *,
unsigned);
-void print_flags (u_int8_t);
-void print_fromto(struct pf_rule_addr *, pf_osfp_t,
- struct pf_rule_addr *, u_int8_t, u_int8_t, int);
-void print_bwspec(const char *index, struct pf_queue_bwspec *);
-void print_scspec(const char *, struct pf_queue_scspec *);
-int ifa_skip_if(const char *filter, struct node_host *p);
+void print_op (u_int8_t, const char *, const char *);
+void print_port (u_int8_t, u_int16_t, u_int16_t, const char *, int);
+void print_ugid (u_int8_t, unsigned, unsigned, const char *, unsigned);
+void print_flags (u_int8_t);
+void print_fromto(struct pf_rule_addr *, pf_osfp_t,
+ struct pf_rule_addr *, u_int8_t, u_int8_t, int);
+void print_bwspec(const char *index, struct pf_queue_bwspec *);
+void print_scspec(const char *, struct pf_queue_scspec *);
+int ifa_skip_if(const char *filter, struct node_host *p);
+struct pfioc_trans_e *
+ pfctl_find_trans(struct pfr_buffer *, int, const char *);
struct node_host *ifa_grouplookup(const char *, int);
struct node_host *host_if(const char *, int);
@@ -1973,18 +1975,34 @@ append_addr_host(struct pfr_buffer *b, struct node_host
*n, int test, int not)
return (0);
}
+struct pfioc_trans_e *
+pfctl_find_trans(struct pfr_buffer *buf, int type, const char *anchor)
+{
+ struct pfioc_trans_e *p = NULL;
+
+ PFRB_FOREACH(p, buf)
+ if (type == p->type && !strcmp(anchor, p->anchor))
+ break;
+
+ return (p);
+}
+
int
pfctl_add_trans(struct pfr_buffer *buf, int type, const char *anchor)
{
struct pfioc_trans_e trans;
+ int rv = 0;
+
+ if (pfctl_find_trans(buf, type, anchor) == NULL) {
+ bzero(&trans, sizeof(trans));
+ trans.type = type;
+ if (strlcpy(trans.anchor, anchor,
+ sizeof(trans.anchor)) >= sizeof(trans.anchor))
+ errx(1, "pfctl_add_trans: strlcpy");
+ rv = pfr_buf_add(buf, &trans);
+ }
- bzero(&trans, sizeof(trans));
- trans.type = type;
- if (strlcpy(trans.anchor, anchor,
- sizeof(trans.anchor)) >= sizeof(trans.anchor))
- errx(1, "pfctl_add_trans: strlcpy");
-
- return pfr_buf_add(buf, &trans);
+ return (rv);
}
u_int32_t
diff --git a/sbin/pfctl/pfctl_parser.h b/sbin/pfctl/pfctl_parser.h
index 7dacc34de1c..b83e2878c8a 100644
--- a/sbin/pfctl/pfctl_parser.h
+++ b/sbin/pfctl/pfctl_parser.h
@@ -175,6 +175,7 @@ struct pf_opt_tbl {
int pt_rulecount;
int pt_generated;
u_int32_t pt_flags;
+ u_int32_t pt_refcnt;
struct node_tinithead pt_nodes;
struct pfr_buffer *pt_buf;
};
