Hi,
On Fri, 21 Apr 2017 00:27:04 +0200
Alexandr Nedvedicky <[email protected]> wrote:
> I finally got to your patch. I'm fine with your to extend pfctl kill
> operation.
>
> I've found just few nits in your change.
>
> 1) your manpage diff should include one more change below:
> --------8<---------------8<---------------8<------------------8<--------
> diff -r c435e977886a -r 40aa0af6704c src/sbin/pfctl/pfctl.8
> --- a/src/sbin/pfctl/pfctl.8 Thu Apr 20 19:53:47 2017 +0200
> +++ b/src/sbin/pfctl/pfctl.8 Thu Apr 20 22:55:43 2017 +0200
> @@ -229,7 +229,7 @@
> entries from the first host/network to the second.
> .It Xo
> .Fl k
> -.Ar host | network | label | id
> +.Ar host | network | label | key | id
> .Xc
> Kill all of the state entries matching the specified
> .Ar host ,
> --------8<---------------8<---------------8<------------------8<--------
Yes, thanks.
> 2) pfctl.c, line exceeds 80 characters
> --------8<---------------8<---------------8<------------------8<--------
> diff -r c435e977886a -r 40aa0af6704c src/sbin/pfctl/pfctl.c
> --- a/src/sbin/pfctl/pfctl.c Thu Apr 20 19:53:47 2017 +0200
> +++ b/src/sbin/pfctl/pfctl.c Thu Apr 20 22:55:43 2017 +0200
> @@ -716,8 +716,8 @@
> i++;
> }
> if (i != 4)
> - errx(1, "pfctl_key_kill_states: key must be \"protocol
> host1:port1 "
> - "direction host2:port2\" format");
> + errx(1, "pfctl_key_kill_states: key must be "
> + "\"protocol host1:port1 direction host2:port2\" format");
>
> if ((p = getprotobyname(tokens[0])) == NULL)
> errx(1, "invalid protocol: %s", tokens[0]);
> --------8<---------------8<---------------8<------------------8<--------
Also yes, I didn't notice this.
> 3) pf_ioctl.c, there are two nits:
> - I think we don't need to try to match label. at least the
> pfctl_key_kill_states() does not set the label in key
Right. The condition is to delete with "key", matching the label is
not needed.
> - I think your if () branch should always break from switch ()
> statement, regardless states got killed or not. Not doing so
> will make kill operation to walk through entire state table.
> I think it is not desired.
Indeed. I overlooked this.
> --------8<---------------8<---------------8<------------------8<--------
> diff -r c435e977886a src/sys/net/pf_ioctl.c
> --- a/src/sys/net/pf_ioctl.c Thu Apr 20 19:53:47 2017 +0200
> +++ b/src/sys/net/pf_ioctl.c Thu Apr 20 23:55:09 2017 +0200
> @@ -1504,19 +1504,14 @@
> (!psk->psk_ifname[0] ||
> (si->s->kif != pfi_all &&
> !strcmp(psk->psk_ifname,
> - si->s->kif->pfik_name))) &&
> - (!psk->psk_label[0] ||
> - (si->s->rule.ptr->label[0] &&
> - !strcmp(psk->psk_label,
> - si->s->rule.ptr->label)))) {
> + si->s->kif->pfik_name)))) {
> pf_remove_state(si->s);
> killed++;
> }
> }
> - if (killed) {
> + if (killed)
> psk->psk_killed = killed;
> - break;
> - }
> + break;
> }
>
> for (s = RB_MIN(pf_state_tree_id, &tree_id); s;
> --------8<---------------8<---------------8<------------------8<--------
>
> There is one more thing I was thinking of. The code, which you are adding to
> pfioctl() essentially duplicates pf_find_state(). With little effort we can
> use
> pf_find_state() to find states to remove. I just gave it a try, the patch is
> further below. However I think we should leave it for another day. It would
> be too big step for now.
It seems good direction.
> Apart items 1 - 3, I'm OK with your change.
Thank you for your ok and useful feedbacks.
I'll use the updated diff attached below.
Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.308
diff -u -p -r1.308 pf_ioctl.c
--- sys/net/pf_ioctl.c 17 Mar 2017 17:19:16 -0000 1.308
+++ sys/net/pf_ioctl.c 21 Apr 2017 04:38:59 -0000
@@ -1444,11 +1444,14 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
case DIOCKILLSTATES: {
struct pf_state *s, *nexts;
- struct pf_state_key *sk;
+ struct pf_state_item *si, *sit;
+ struct pf_state_key *sk, key;
struct pf_addr *srcaddr, *dstaddr;
u_int16_t srcport, dstport;
struct pfioc_state_kill *psk = (struct pfioc_state_kill *)addr;
- u_int killed = 0;
+ u_int i, killed = 0;
+ const int dirs[] = { PF_IN, PF_OUT };
+ int sidx, didx;
if (psk->psk_pfcmp.id) {
if (psk->psk_pfcmp.creatorid == 0)
@@ -1457,6 +1460,57 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
pf_remove_state(s);
psk->psk_killed = 1;
}
+ break;
+ }
+
+ if (psk->psk_af && psk->psk_proto &&
+ psk->psk_src.port_op == PF_OP_EQ &&
+ psk->psk_dst.port_op == PF_OP_EQ) {
+
+ key.af = psk->psk_af;
+ key.proto = psk->psk_proto;
+ key.rdomain = psk->psk_rdomain;
+
+ for (i = 0; i < nitems(dirs); i++) {
+ if (dirs[i] == PF_IN) {
+ sidx = 0;
+ didx = 1;
+ } else {
+ sidx = 1;
+ didx = 0;
+ }
+ PF_ACPY(&key.addr[sidx],
+ &psk->psk_src.addr.v.a.addr, key.af);
+ PF_ACPY(&key.addr[didx],
+ &psk->psk_dst.addr.v.a.addr, key.af);
+ key.port[sidx] = psk->psk_src.port[0];
+ key.port[didx] = psk->psk_dst.port[0];
+
+ sk = RB_FIND(pf_state_tree, &pf_statetbl, &key);
+ if (sk == NULL)
+ continue;
+
+ TAILQ_FOREACH_SAFE(si, &sk->states, entry, sit)
+ if (((si->s->key[PF_SK_WIRE]->af ==
+ si->s->key[PF_SK_STACK]->af &&
+ sk == (dirs[i] == PF_IN ?
+ si->s->key[PF_SK_WIRE] :
+ si->s->key[PF_SK_STACK])) ||
+ (si->s->key[PF_SK_WIRE]->af !=
+ si->s->key[PF_SK_STACK]->af &&
+ dirs[i] == PF_IN &&
+ (sk == si->s->key[PF_SK_STACK] ||
+ sk == si->s->key[PF_SK_WIRE]))) &&
+ (!psk->psk_ifname[0] ||
+ (si->s->kif != pfi_all &&
+ !strcmp(psk->psk_ifname,
+ si->s->kif->pfik_name)))) {
+ pf_remove_state(si->s);
+ killed++;
+ }
+ }
+ if (killed)
+ psk->psk_killed = killed;
break;
}
Index: sbin/pfctl/pfctl.8
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl.8,v
retrieving revision 1.167
diff -u -p -r1.167 pfctl.8
--- sbin/pfctl/pfctl.8 26 Jan 2017 18:45:12 -0000 1.167
+++ sbin/pfctl/pfctl.8 21 Apr 2017 04:39:59 -0000
@@ -229,12 +229,13 @@ option may be specified, which will kill
entries from the first host/network to the second.
.It Xo
.Fl k
-.Ar host | network | label | id
+.Ar host | network | label | key | id
.Xc
Kill all of the state entries matching the specified
.Ar host ,
.Ar network ,
.Ar label ,
+.Ar key ,
or
.Ar id .
.Pp
@@ -266,7 +267,7 @@ To kill all states with the target
.Pp
.Dl # pfctl -k 0.0.0.0/0 -k host2
.Pp
-It is also possible to kill states by rule label or state ID.
+It is also possible to kill states by rule label, state key or state ID.
In this mode the first
.Fl k
argument is used to specify the type
@@ -276,6 +277,17 @@ from rules carrying the label
.Dq foobar :
.Pp
.Dl # pfctl -k label -k foobar
+.Pp
+To kill one specific state by its key
+(protocol, host1, port1, direction, host2 and port2 in the same format
+of pfctl -s state),
+use the
+.Ar key
+modifier and as a second argument the state key.
+To kill a state whose protocol is TCP and originating from
+10.0.0.101:32123 to 10.0.0.1:80 use:
+.Pp
+.Dl # pfctl -k key -k 'tcp 10.0.0.1:80 <- 10.0.0.101:32123'
.Pp
To kill one specific state by its unique state ID
(as shown by pfctl -s state -vv),
Index: sbin/pfctl/pfctl.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.339
diff -u -p -r1.339 pfctl.c
--- sbin/pfctl/pfctl.c 27 Mar 2017 17:38:09 -0000 1.339
+++ sbin/pfctl/pfctl.c 21 Apr 2017 04:40:00 -0000
@@ -72,6 +72,8 @@ int pfctl_kill_src_nodes(int, const cha
int pfctl_net_kill_states(int, const char *, int, int);
int pfctl_label_kill_states(int, const char *, int, int);
int pfctl_id_kill_states(int, int);
+int pfctl_key_kill_states(int, const char *, int, int);
+int pfctl_parse_host(char *, struct pf_rule_addr *);
void pfctl_init_options(struct pfctl *);
int pfctl_load_options(struct pfctl *);
int pfctl_load_limit(struct pfctl *, unsigned int, unsigned int);
@@ -683,6 +685,122 @@ pfctl_id_kill_states(int dev, int opts)
return (0);
}
+int
+pfctl_key_kill_states(int dev, const char *iface, int opts, int rdomain)
+{
+ struct pfioc_state_kill psk;
+ char *s, *token, *tokens[4];
+ struct protoent *p;
+ u_int i, sidx, didx;
+
+ if (state_killers != 2 || (strlen(state_kill[1]) == 0)) {
+ warnx("no key specified");
+ usage();
+ }
+ memset(&psk, 0, sizeof(psk));
+
+ if (iface != NULL && strlcpy(psk.psk_ifname, iface,
+ sizeof(psk.psk_ifname)) >= sizeof(psk.psk_ifname))
+ errx(1, "invalid interface: %s", iface);
+
+ psk.psk_rdomain = rdomain;
+
+ s = strdup(state_kill[1]);
+ if (!s)
+ errx(1, "pfctl_key_kill_states: strdup");
+ i = 0;
+ while ((token = strsep(&s, " \t")) != NULL)
+ if (*token != '\0') {
+ if (i < 4)
+ tokens[i] = token;
+ i++;
+ }
+ if (i != 4)
+ errx(1, "pfctl_key_kill_states: key must be "
+ "\"protocol host1:port1 direction host2:port2\" format");
+
+ if ((p = getprotobyname(tokens[0])) == NULL)
+ errx(1, "invalid protocol: %s", tokens[0]);
+ psk.psk_proto = p->p_proto;
+
+ if (strcmp(tokens[2], "->") == 0) {
+ sidx = 1;
+ didx = 3;
+ } else if (strcmp(tokens[2], "<-") == 0) {
+ sidx = 3;
+ didx = 1;
+ } else
+ errx(1, "invalid direction: %s", tokens[2]);
+
+ if (pfctl_parse_host(tokens[sidx], &psk.psk_src) == -1)
+ errx(1, "invalid host: %s", tokens[sidx]);
+ if (pfctl_parse_host(tokens[didx], &psk.psk_dst) == -1)
+ errx(1, "invalid host: %s", tokens[didx]);
+
+ if (ioctl(dev, DIOCKILLSTATES, &psk))
+ err(1, "DIOCKILLSTATES");
+
+ if ((opts & PF_OPT_QUIET) == 0)
+ fprintf(stderr, "killed %d states\n", psk.psk_killed);
+
+ return (0);
+}
+
+int
+pfctl_parse_host(char *str, struct pf_rule_addr *addr)
+{
+ char *s = NULL, *sbs, *sbe;
+ struct addrinfo hints, *ai;
+ struct sockaddr_in *sin4;
+ struct sockaddr_in6 *sin6;
+
+ s = strdup(str);
+ if (!s)
+ errx(1, "pfctl_parse_host: strdup");
+
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_socktype = SOCK_DGRAM; /* dummy */
+ hints.ai_flags = AI_NUMERICHOST;
+
+ if ((sbs = strchr(s, '[')) != NULL || (sbe = strrchr(s, ']')) != NULL) {
+ hints.ai_family = AF_INET6;
+ *(sbs++) = *sbe = '\0';
+ } else if ((sbs = strchr(s, ':')) != NULL) {
+ hints.ai_family = AF_INET;
+ *(sbs++) = '\0';
+ } else
+ goto error;
+
+ if (getaddrinfo(s, sbs, &hints, &ai) != 0)
+ goto error;
+
+ switch (ai->ai_family) {
+ case AF_INET:
+ sin4 = (struct sockaddr_in *)ai->ai_addr;
+ addr->addr.v.a.addr.v4 = sin4->sin_addr;
+ addr->port[0] = sin4->sin_port;
+ break;
+
+ case AF_INET6:
+ sin6 = (struct sockaddr_in6 *)ai->ai_addr;
+ addr->addr.v.a.addr.v6 = sin6->sin6_addr;
+ addr->port[0] = sin6->sin6_port;
+ break;
+ }
+ freeaddrinfo(ai);
+ free(s);
+
+ memset(&addr->addr.v.a.mask, 0xff, sizeof(struct pf_addr));
+ addr->port_op = PF_OP_EQ;
+ addr->addr.type = PF_ADDR_ADDRMASK;
+
+ return (0);
+
+ error:
+ free(s);
+ return (-1);
+}
+
void
pfctl_print_rule_counters(struct pf_rule *rule, int opts)
{
@@ -2427,6 +2545,8 @@ main(int argc, char *argv[])
pfctl_label_kill_states(dev, ifaceopt, opts, rdomain);
else if (!strcmp(state_kill[0], "id"))
pfctl_id_kill_states(dev, opts);
+ else if (!strcmp(state_kill[0], "key"))
+ pfctl_key_kill_states(dev, ifaceopt, opts, rdomain);
else
pfctl_net_kill_states(dev, ifaceopt, opts, rdomain);
}
