Hello,
One of the teams in Oracle Solaris uses sophisticated naming scheme for PF
rulesets. The anchor (ruleset) is identified by something like that:
root/whatever:component:name/some-virtual-instance-long-name/inbound
That particular team hit a bug in pfctl, when they were trying to load rule to
ruleset specified by anchor above. pfctl(8) on OpenBSD suffers from same
problem:
echo 'pass'|pfctl -a
root/whatever:component:name/some-virtual-instance-long-name/inbound -f -
pfctl: pfctl_add_rule: strlcpy
the command above bails out in pfctl_rules() function at line 1488:
1481 pf_init_ruleset(rs);
1482 rs->anchor = pf.anchor;
1483 if (strlcpy(pf.anchor->path, anchorname,
1484 sizeof(pf.anchor->path)) >= sizeof(pf.anchor->path))
1485 errx(1, "pfctl_add_rule: strlcpy");
1486 if (strlcpy(pf.anchor->name, anchorname,
1487 sizeof(pf.anchor->name)) >= sizeof(pf.anchor->name))
1488 errx(1, "pfctl_add_rule: strlcpy");
1489
Looks like pfctl confuses anchorname with anchorpath. The anchorname uses 64B
buffer. Anchorname is a leaf-path component. If we stick to example above, then
anchorname should be '/inbound'. The snippet above has been fixed by change
below:
+
+ if ((p = strrchr(anchorname, '/')) != NULL) {
+ if (!strlen(p))
+ err(1, "pfctl_add_rule: bad anchor name %s",
+ anchorname);
+ } else
+ p = anchorname;
+
+ if (strlcpy(pf.anchor->name, p,
same code already exists in pfctl_add_rule(). After giving a try I hit
a different error:
pfctl: pfctl_get_ticket: assertion failed
This time game over happened at line 1505:
1496 if ((opts & PF_OPT_NOACTION) == 0) {
1497 /*
1498 * XXX For the time being we need to open transactions
for
1499 * the main ruleset before parsing, because tables are
still
1500 * loaded at parse time.
1501 */
1502 if (pfctl_ruleset_trans(&pf, anchorname, pf.anchor))
1503 ERRX("pfctl_rules");
1504 pf.astack[0]->ruleset.tticket =
1505 pfctl_get_ticket(t, PF_TRANS_TABLE, anchorname);
1506 }
After some more debugging I've arrived to pfctl_load_ruleset():
1340 pf->anchor = rs->anchor;
1341
1342 if (path[0])
1343 snprintf(&path[len], PATH_MAX - len, "/%s",
pf->anchor->name);
1344 else
1345 snprintf(&path[len], PATH_MAX - len, "%s",
pf->anchor->name);
I think the else branch should be using ->path instead of ->name.
Complete patch is further below. The patch works fine for me (Solaris),
still I'm not quite sure it's 100% correct.
thanks and
regards
sasha
--------8<---------------8<---------------8<------------------8<--------
diff -r 2f5f0295677c src/sbin/pfctl/pfctl.c
--- a/src/sbin/pfctl/pfctl.c Fri Sep 02 15:53:45 2016 +0200
+++ b/src/sbin/pfctl/pfctl.c Fri Sep 02 17:31:05 2016 +0200
@@ -1342,7 +1342,7 @@ pfctl_load_ruleset(struct pfctl *pf, cha
if (path[0])
snprintf(&path[len], PATH_MAX - len, "/%s", pf->anchor->name);
else
- snprintf(&path[len], PATH_MAX - len, "%s", pf->anchor->name);
+ snprintf(&path[len], PATH_MAX - len, "%s", pf->anchor->path);
if (depth) {
if (TAILQ_FIRST(rs->rules.active.ptr) != NULL) {
@@ -1447,6 +1447,7 @@ pfctl_rules(int dev, char *filename, int
struct pfr_table trs;
char *path = NULL;
int osize;
+ char *p;
bzero(&pf, sizeof(pf));
RB_INIT(&pf_anchors);
@@ -1483,7 +1484,15 @@ pfctl_rules(int dev, char *filename, int
if (strlcpy(pf.anchor->path, anchorname,
sizeof(pf.anchor->path)) >= sizeof(pf.anchor->path))
errx(1, "pfctl_add_rule: strlcpy");
- if (strlcpy(pf.anchor->name, anchorname,
+
+ if ((p = strrchr(anchorname, '/')) != NULL) {
+ if (!strlen(p))
+ err(1, "pfctl_add_rule: bad anchor name %s",
+ anchorname);
+ } else
+ p = anchorname;
+
+ if (strlcpy(pf.anchor->name, p,
sizeof(pf.anchor->name)) >= sizeof(pf.anchor->name))
errx(1, "pfctl_add_rule: strlcpy");
