FreeBSD: use libpfctl

[kprovost]

FreeBSD 15 has removed several ioctl calls (such as DIOCGETSTATUS and DIOCGETRULE) and replaced them. The easiest way to cope with that is to use the provided libpfctl library.

NOTE: This version of the patch will break use on OpenBSD or NetBSD and is intended to serve as a basis for discussing the best approach to cope with these differences.

Sponsored by: Rubicon Communications, LLC ("Netgate")

[miniupnp]

I think we should #define USE_LIBPFCTL in configure and use some #ifdef in the code...

[kprovost]

Something like this then.

I've added an 'r' pointer in most functions because it avoids a lot of ifdef USE_LIBPFCTL then rule.... #else pr.rule.... #endif all over the place.

[miniupnp]

It looks great !

[miniupnp]

@kprovost libpfctl will only be available starting with FreeBSD 15, right ?

[kprovost]

@kprovost libpfctl will only be available starting with FreeBSD 15, right ?

No, it's available on all support FreeBSD versions, 12.4, 13.2 and 14.0. The DIOCGETRULE ioctl no longer exists in 15, so there you either use libpfctl or implement the new nvlist-based ioctl (don't do that, it's likely to go away and be replaced as well).

[miniupnp]

shouldn't pfctl_get_rules_info() be used instead of DIOCGETRULES ?

[kprovost]

We can, and in due course should, but it's not required on FreeBSD 15.
The DIOCGETRULES ioctl hasn't been removed (yet).

I think I'd prefer to keep this patch as small as possible to fix the immediate problem (of not actually working with FreeBSD 15). We can continue other cleanup work in separate patches.

[miniupnp]

@kprovost could you test my branch libpfctl2 please ?

[kprovost]

That appears to still be using DIOCGETRULE in ./pf/pfpinhole.c

[miniupnp]

@kprovost I updated pfpinhole.c on the branch libpfctl2

[kprovost]

Okay, I can confirm that builds, and at first glance that looks good.

I'll do a better review and try to test this as well early next week.

[kprovost]

There's still at least one issue here. I can create pinholes (rdr rules in pf parlance), but not list them. Using 'upnpc -l' I see 'ioctl(dev, DIOCGETADDR, ...):1523 Device busy' from miniupnpd. That appears to be because in get_redirect_rule_by_index() we take the ticket number from pr.ticket, however in the LIBPFCTL case we get that from ri.ticket. With that adjustment I can enumerate the rdr rules.

I'm also still seeing a problem where rules that have an expired leaseTime (i.e. 0) don't appear to be cleaned up. There's no associated error message in the miniupnpd log, so I've not worked out why that is yet.

[miniupnp]

I think the delete doesn't work...

[miniupnp]

There's still at least one issue here. I can create pinholes (rdr rules in pf parlance), but not list them. Using 'upnpc -l' I see 'ioctl(dev, DIOCGETADDR, ...):1523 Device busy' from miniupnpd. That appears to be because in get_redirect_rule_by_index() we take the ticket number from pr.ticket, however in the LIBPFCTL case we get that from ri.ticket. With that adjustment I can enumerate the rdr rules.

@kprovost what is that adjustment you made ?

[kprovost]

Essentially this, everywhere we call pfctl_get_rules_info() (and also use a struct pfioc_rule pr):

--- /home/kp/tmp/miniupnp/miniupnpd/pf/obsdrdr.c	2024-06-11 11:28:09.530049000 +0200
+++ pf/obsdrdr.c	2024-06-11 09:28:41.113074000 +0200
@@ -1144,6 +1144,7 @@
 		syslog(LOG_ERR, "pfctl_get_rules_info: %m");
 		return -1;
 	}
+	pr.ticket = ri.ticket;
 	n = ri.nr;
 #else /* USE_LIBPFCTL */
 	if(ioctl(dev, DIOCGETRULES, &pr) < 0)
@@ -1437,6 +1438,7 @@
 		syslog(LOG_ERR, "pfctl_get_rules_info: %m");
 		return -1;
 	}
+	pr.ticket = ri.ticket;
 	n = ri.nr;
 #else /* USE_LIBPFCTL */
 	memset(&pr, 0, sizeof(pr));
@@ -1506,7 +1510,7 @@
 	strlcpy(pp.anchor, anchor_name, MAXPATHLEN);
 	pp.r_action = PF_RDR;
 	pp.r_num = index;
-	pp.ticket = pr.ticket;
+	pp.ticket = ri.ticket;
 	if(ioctl(dev, DIOCGETADDRS, &pp) < 0)
 	{
 		syslog(LOG_ERR, "ioctl(dev, DIOCGETADDRS, ...): %m");
@@ -1767,6 +1776,7 @@
 	if (pfctl_get_rules_info(dev, &ri, PF_RDR, anchor_name) < 0)
 		perror("pfctl_get_rules_info");
 	printf("ticket = %d, nr = %d\n", ri.ticket, ri.nr);
+	pr.ticket = ri.ticket;
 	n = ri.nr;
 #else /* USE_LIBPFCTL */
 	memset(&pr, 0, sizeof(pr));

[miniupnp]

@kprovost it must be better now

[kprovost]

That fixes retrieval. Deletion of rules is still broken. I've had a quick look and couldn't immediately figure out why. As far as I can tell it's not actually in the pf abstraction code, as that doesn't appear to be getting called.

[kprovost]

No, I was wrong. The bug is that in priv_delete_redirect_rule_desc() we do RULE.action = PF_RDR, but that sets it in struct pfctl_rule (in the LIBPFCTL) case, but we do still use struct pfioc_rule for the DIOCCHANGERULE calls later.

diff --git a/miniupnpd/pf/obsdrdr.c b/miniupnpd/pf/obsdrdr.c
index 6d4d1a2..9d2a392 100644
--- a/miniupnpd/pf/obsdrdr.c
+++ b/miniupnpd/pf/obsdrdr.c
@@ -1137,6 +1141,7 @@ priv_delete_redirect_rule_check_desc(const char * ifname, unsigned short eport,
        strlcpy(pr.anchor, anchor_name, MAXPATHLEN);
 #ifndef PF_NEWSTYLE
        RULE.action = PF_RDR;
+       pr.rule.action = PF_RDR;
 #endif
 #ifdef USE_LIBPFCTL
        if (pfctl_get_rules_info(dev, &ri, PF_RDR, anchor_name) < 0)

fixes that for me.

[miniupnp]

I'm going to merge my branch into master. alea jacta est

[kprovost]

I'm going to merge my branch into master. alea jacta est

I've re-tested master with these changes included. As far as I can tell everything works.