Re: git: 324fd7ec4043 - main - libpfctl: introduce a handle-enabled variant of pfctl_add_rule()

On 4 Jan 2024, at 22:11, Kristof Provost <kp@FreeBSD.org> wrote:
> 
> The branch main has been updated by kp:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=324fd7ec40439e6b3916429a69956d7acf74eb19
> 
> commit 324fd7ec40439e6b3916429a69956d7acf74eb19
> Author:     Kristof Provost <kp@FreeBSD.org>
> AuthorDate: 2024-01-04 12:45:56 +0000
> Commit:     Kristof Provost <kp@FreeBSD.org>
> CommitDate: 2024-01-04 22:10:44 +0000
> 
>    libpfctl: introduce a handle-enabled variant of pfctl_add_rule()
> 
>    Introduce pfctl_add_rule_h(), which takes a pfctl_handle rather than a
>    file descriptor (which it didn't use). This means that library users can
>    open the handle while they're running as root, but later drop privileges
>    and still add rules to pf.

Given libpfctl is an INTERALLIB, why do we need to care about this
compatibility (and live with this cruft) instead of just changing
pfctl_add_rule to the new thing?

Jess

>    Sponsored by:   Rubicon Communications, LLC ("Netgate")
> ---
> contrib/pf/ftp-proxy/filter.c  | 10 +++++++---
> contrib/pf/tftp-proxy/filter.c | 12 +++++++++---
> lib/libpfctl/libpfctl.c        | 29 +++++++++++++++++++++++------
> lib/libpfctl/libpfctl.h        |  3 +++
> 4 files changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/contrib/pf/ftp-proxy/filter.c b/contrib/pf/ftp-proxy/filter.c
> index 4277e079f3be..612e35c4ac6e 100644
> --- a/contrib/pf/ftp-proxy/filter.c
> +++ b/contrib/pf/ftp-proxy/filter.c
> @@ -58,6 +58,7 @@ static uint32_t pfpool_ticket;
> static struct pfioc_trans pft;
> static struct pfioc_trans_e pfte[TRANS_SIZE];
> static int dev, rule_log;
> +static struct pfctl_handle *pfh = NULL;
> static const char *qname, *tagname;
> 
> int
> @@ -73,7 +74,7 @@ add_filter(u_int32_t id, u_int8_t dir, struct sockaddr *src,
> return (-1);
> 
> pfrule.direction = dir;
> - if (pfctl_add_rule(dev, &pfrule, pfanchor, pfanchor_call,
> + if (pfctl_add_rule_h(pfh, &pfrule, pfanchor, pfanchor_call,
>    pfticket, pfpool_ticket))
> return (-1);
> 
> @@ -108,7 +109,7 @@ add_nat(u_int32_t id, struct sockaddr *src, struct sockaddr *dst,
> 
> pfrule.rpool.proxy_port[0] = nat_range_low;
> pfrule.rpool.proxy_port[1] = nat_range_high;
> - if (pfctl_add_rule(dev, &pfrule, pfanchor, pfanchor_call,
> + if (pfctl_add_rule_h(pfh, &pfrule, pfanchor, pfanchor_call,
>    pfticket, pfpool_ticket))
> return (-1);
> 
> @@ -141,7 +142,7 @@ add_rdr(u_int32_t id, struct sockaddr *src, struct sockaddr *dst,
> return (-1);
> 
> pfrule.rpool.proxy_port[0] = rdr_port;
> - if (pfctl_add_rule(dev, &pfrule, pfanchor, pfanchor_call,
> + if (pfctl_add_rule_h(pfh, &pfrule, pfanchor, pfanchor_call,
>    pfticket, pfpool_ticket))
> return (-1);
> 
> @@ -182,6 +183,9 @@ init_filter(const char *opt_qname, const char *opt_tagname, int opt_verbose)
> dev = open("/dev/pf", O_RDWR); 
> if (dev == -1)
> err(1, "open /dev/pf");
> + pfh = pfctl_open(PF_DEVICE);
> + if (pfh == NULL)
> + err(1, "pfctl_open");
> status = pfctl_get_status(dev);
> if (status == NULL)
> err(1, "DIOCGETSTATUS");
> diff --git a/contrib/pf/tftp-proxy/filter.c b/contrib/pf/tftp-proxy/filter.c
> index 966628464d28..f372ddd0aeae 100644
> --- a/contrib/pf/tftp-proxy/filter.c
> +++ b/contrib/pf/tftp-proxy/filter.c
> @@ -62,6 +62,7 @@ static char pfanchor_call[PF_ANCHOR_NAME_SIZE];
> static struct pfioc_trans pft;
> static struct pfioc_trans_e pfte[TRANS_SIZE];
> static int dev, rule_log;
> +static struct pfctl_handle *pfh = NULL;
> static char *qname;
> 
> int
> @@ -77,7 +78,7 @@ add_filter(u_int32_t id, u_int8_t dir, struct sockaddr *src,
> return (-1);
> 
> pfrule.direction = dir;
> - if (pfctl_add_rule(dev, &pfrule, pfanchor, pfanchor_call,
> + if (pfctl_add_rule_h(pfh, &pfrule, pfanchor, pfanchor_call,
>    pfticket, pfpool_ticket))
> return (-1);
> 
> @@ -112,7 +113,7 @@ add_nat(u_int32_t id, struct sockaddr *src, struct sockaddr *dst,
> 
> pfrule.rpool.proxy_port[0] = nat_range_low;
> pfrule.rpool.proxy_port[1] = nat_range_high;
> - if (pfctl_add_rule(dev, &pfrule, pfanchor, pfanchor_call,
> + if (pfctl_add_rule_h(pfh, &pfrule, pfanchor, pfanchor_call,
>    pfticket, pfpool_ticket))
> return (-1);
> 
> @@ -145,7 +146,7 @@ add_rdr(u_int32_t id, struct sockaddr *src, struct sockaddr *dst,
> return (-1);
> 
> pfrule.rpool.proxy_port[0] = rdr_port;
> - if (pfctl_add_rule(dev, &pfrule, pfanchor, pfanchor_call,
> + if (pfctl_add_rule_h(pfh, &pfrule, pfanchor, pfanchor_call,
>    pfticket, pfpool_ticket))
> return (-1);
> 
> @@ -187,6 +188,11 @@ init_filter(char *opt_qname, int opt_verbose)
> syslog(LOG_ERR, "can't open /dev/pf");
> exit(1);
> }
> + pfh = pfctl_open(PF_DEVICE);
> + if (pfh == NULL) {
> + syslog(LOG_ERR, "can't pfctl_open()");
> + exit(1);
> + }
> status = pfctl_get_status(dev);
> if (status == NULL) {
> syslog(LOG_ERR, "DIOCGETSTATUS");
> diff --git a/lib/libpfctl/libpfctl.c b/lib/libpfctl/libpfctl.c
> index 94949a5a7337..2db3f0ede99f 100644
> --- a/lib/libpfctl/libpfctl.c
> +++ b/lib/libpfctl/libpfctl.c
> @@ -1116,20 +1116,37 @@ snl_add_msg_attr_pf_rule(struct snl_writer *nw, uint32_t type, const struct pfct
> int
> pfctl_add_rule(int dev __unused, const struct pfctl_rule *r, const char *anchor,
>     const char *anchor_call, uint32_t ticket, uint32_t pool_ticket)
> +{
> + struct pfctl_handle *h;
> + int ret;
> +
> + h = pfctl_open(PF_DEVICE);
> + if (h == NULL)
> + return (ENODEV);
> +
> + ret = pfctl_add_rule_h(h, r, anchor, anchor_call, ticket, pool_ticket);
> +
> + pfctl_close(h);
> +
> + return (ret);
> +}
> +
> +int
> +pfctl_add_rule_h(struct pfctl_handle *h, const struct pfctl_rule *r,
> +    const char *anchor, const char *anchor_call, uint32_t ticket,
> +    uint32_t pool_ticket)
> {
> struct snl_writer nw;
> - struct snl_state ss = {};
> struct snl_errmsg_data e = {};
> struct nlmsghdr *hdr;
> uint32_t seq_id;
> int family_id;
> 
> - snl_init(&ss, NETLINK_GENERIC);
> - family_id = snl_get_genl_family(&ss, PFNL_FAMILY_NAME);
> + family_id = snl_get_genl_family(&h->ss, PFNL_FAMILY_NAME);
> if (family_id == 0)
> return (ENOTSUP);
> 
> - snl_init_writer(&ss, &nw);
> + snl_init_writer(&h->ss, &nw);
> hdr = snl_create_genl_msg_request(&nw, family_id, PFNL_CMD_ADDRULE);
> hdr->nlmsg_flags |= NLM_F_DUMP;
> snl_add_msg_attr_u32(&nw, PF_ART_TICKET, ticket);
> @@ -1144,10 +1161,10 @@ pfctl_add_rule(int dev __unused, const struct pfctl_rule *r, const char *anchor,
> 
> seq_id = hdr->nlmsg_seq;
> 
> - if (! snl_send_message(&ss, hdr))
> + if (! snl_send_message(&h->ss, hdr))
> return (ENXIO);
> 
> - while ((hdr = snl_read_reply_multi(&ss, seq_id, &e)) != NULL) {
> + while ((hdr = snl_read_reply_multi(&h->ss, seq_id, &e)) != NULL) {
> }
> 
> return (e.error);
> diff --git a/lib/libpfctl/libpfctl.h b/lib/libpfctl/libpfctl.h
> index f128e5340891..cd72d04d6715 100644
> --- a/lib/libpfctl/libpfctl.h
> +++ b/lib/libpfctl/libpfctl.h
> @@ -421,6 +421,9 @@ int pfctl_get_clear_rule(int dev, uint32_t nr, uint32_t ticket,
> int pfctl_add_rule(int dev, const struct pfctl_rule *r,
>    const char *anchor, const char *anchor_call, uint32_t ticket,
>    uint32_t pool_ticket);
> +int pfctl_add_rule_h(struct pfctl_handle *h, const struct pfctl_rule *r,
> +    const char *anchor, const char *anchor_call, uint32_t ticket,
> +    uint32_t pool_ticket);
> int pfctl_set_keepcounters(int dev, bool keep);
> int pfctl_get_creatorids(struct pfctl_handle *h, uint32_t *creators, size_t *len);
>