Mercurial > hg > dhcpcd

changeset 5229:bb468c1a3b46 draft

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
ARP: Remove ability to filter specific addresses This is only really needed for long lasting ARP, which is only used for IPv4 address defence. Modern NetBSD does not need this and it fails to work with OpenBSD Pledge. FreeBSD Capsicum is more secure without this as the BPF fd can then be locked for other changes [1]. That just leaves Linux and Solaris. If anyone feels dhcpcd is processing to much ARP then please implement RFC 5227 in the kernel like NetBSD. [1] Locking the BPF fd is questionable because the inet proxy using sendmsg can send any packet to any destination.
author Roy Marples <roy@marples.name>
date Fri, 15 May 2020 22:29:30 +0100
parents 82c7e8204e9b
children 92569921a974
files src/arp.c src/arp.h src/bpf.c src/dhcp.c src/privsep-bpf.c
diffstat 5 files changed, 26 insertions(+), 260 deletions(-) [+]
line wrap: on
line diff
--- a/src/arp.c	Fri May 15 20:23:55 2020 +0100
+++ b/src/arp.c	Fri May 15 22:29:30 2020 +0100
@@ -601,38 +601,6 @@
 	arp_ifannounceaddr(iff, ia);
 }
 
-void
-arp_change(struct arp_state *astate, const struct in_addr *addr)
-{
-	struct interface *ifp = astate->iface;
-	struct iarp_state *state = ARP_STATE(ifp);
-
-#ifdef PRIVSEP
-	if (!IN_IS_ADDR_UNSPECIFIED(&astate->addr) &&
-	    IN_PRIVSEP_SE(ifp->ctx))
-	{
-		if (ps_bpf_deladdr(ifp, &astate->addr) == -1)
-			logerr(__func__);
-	}
-#endif
-
-	if (addr != NULL)
-		astate->addr = *addr;
-	else
-		astate->addr.s_addr = INADDR_ANY;
-
-#ifdef PRIVSEP
-	if (addr != NULL && IN_PRIVSEP_SE(ifp->ctx)) {
-		if (ps_bpf_addaddr(ifp, addr) == -1)
-			logerr(__func__);
-	} else
-#endif
-	if (state->bpf_fd != -1) {
-		if (bpf_arp(ifp, state->bpf_fd) == -1)
-			logerr(__func__); /* try and continue */
-	}
-}
-
 struct arp_state *
 arp_new(struct interface *ifp, const struct in_addr *addr)
 {
@@ -669,7 +637,10 @@
 	astate->iface = ifp;
 	state = ARP_STATE(ifp);
 	TAILQ_INSERT_TAIL(&state->arp_states, astate, next);
-	arp_change(astate, addr);
+	if (state->bpf_fd != -1) {
+		if (bpf_arp(ifp, state->bpf_fd) == -1)
+			logerr(__func__); /* try and continue */
+	}
 	return astate;
 }
 
@@ -691,7 +662,6 @@
 
 	ifp = astate->iface;
 	eloop_timeout_delete(ifp->ctx->eloop, NULL, astate);
-	arp_change(astate, NULL);
 	state =	ARP_STATE(ifp);
 	TAILQ_REMOVE(&state->arp_states, astate, next);
 	if (astate->free_cb)
--- a/src/arp.h	Fri May 15 20:23:55 2020 +0100
+++ b/src/arp.h	Fri May 15 22:29:30 2020 +0100
@@ -95,7 +95,6 @@
 #ifdef ARP
 void arp_packet(struct interface *, uint8_t *, size_t);
 struct arp_state *arp_new(struct interface *, const struct in_addr *);
-void arp_change(struct arp_state *, const struct in_addr *);
 void arp_probe(struct arp_state *);
 void arp_announce(struct arp_state *);
 void arp_announceaddr(struct dhcpcd_ctx *, const struct in_addr *);
--- a/src/bpf.c	Fri May 15 20:23:55 2020 +0100
+++ b/src/bpf.c	Fri May 15 22:29:30 2020 +0100
@@ -359,95 +359,6 @@
 	return close(fd);
 }
 
-/* Normally this is needed by bootp.
- * Once that uses this again, the ARP guard here can be removed. */
-#ifdef ARP
-#define BPF_CMP_HWADDR_LEN	((((HWADDR_LEN / 4) + 2) * 2) + 1)
-static unsigned int
-bpf_cmp_hwaddr(struct bpf_insn *bpf, size_t bpf_len, size_t off,
-    bool equal, uint8_t *hwaddr, size_t hwaddr_len)
-{
-	struct bpf_insn *bp;
-	size_t maclen, nlft, njmps;
-	uint32_t mac32;
-	uint16_t mac16;
-	uint8_t jt, jf;
-
-	/* Calc the number of jumps */
-	if ((hwaddr_len / 4) >= 128) {
-		errno = EINVAL;
-		return 0;
-	}
-	njmps = (hwaddr_len / 4) * 2; /* 2 instructions per check */
-	/* We jump after the 1st check. */
-	if (njmps)
-		njmps -= 2;
-	nlft = hwaddr_len % 4;
-	if (nlft) {
-		njmps += (nlft / 2) * 2;
-		nlft = nlft % 2;
-		if (nlft)
-			njmps += 2;
-
-	}
-
-	/* Skip to positive finish. */
-	njmps++;
-	if (equal) {
-		jt = (uint8_t)njmps;
-		jf = 0;
-	} else {
-		jt = 0;
-		jf = (uint8_t)njmps;
-	}
-
-	bp = bpf;
-	for (; hwaddr_len > 0;
-	     hwaddr += maclen, hwaddr_len -= maclen, off += maclen)
-	{
-		if (bpf_len < 3) {
-			errno = ENOBUFS;
-			return 0;
-		}
-		bpf_len -= 3;
-
-		if (hwaddr_len >= 4) {
-			maclen = sizeof(mac32);
-			memcpy(&mac32, hwaddr, maclen);
-			BPF_SET_STMT(bp, BPF_LD + BPF_W + BPF_IND, off);
-			bp++;
-			BPF_SET_JUMP(bp, BPF_JMP + BPF_JEQ + BPF_K,
-			             htonl(mac32), jt, jf);
-		} else if (hwaddr_len >= 2) {
-			maclen = sizeof(mac16);
-			memcpy(&mac16, hwaddr, maclen);
-			BPF_SET_STMT(bp, BPF_LD + BPF_H + BPF_IND, off);
-			bp++;
-			BPF_SET_JUMP(bp, BPF_JMP + BPF_JEQ + BPF_K,
-			             htons(mac16), jt, jf);
-		} else {
-			maclen = sizeof(*hwaddr);
-			BPF_SET_STMT(bp, BPF_LD + BPF_B + BPF_IND, off);
-			bp++;
-			BPF_SET_JUMP(bp, BPF_JMP + BPF_JEQ + BPF_K,
-			             *hwaddr, jt, jf);
-		}
-		if (jt)
-			jt = (uint8_t)(jt - 2);
-		if (jf)
-			jf = (uint8_t)(jf - 2);
-		bp++;
-	}
-
-	/* Last step is always return failure.
-	 * Next step is a positive finish. */
-	BPF_SET_STMT(bp, BPF_RET + BPF_K, 0);
-	bp++;
-
-	return (unsigned int)(bp - bpf);
-}
-#endif
-
 #ifdef ARP
 static const struct bpf_insn bpf_arp_ether [] = {
 	/* Check this is an ARP packet. */
@@ -493,18 +404,14 @@
 #define BPF_ARP_ADDRS_LEN	1 + (ARP_ADDRS_MAX * 2) + 3 + \
 				(ARP_ADDRS_MAX * 2) + 1
 
-#define BPF_ARP_LEN		BPF_ARP_ETHER_LEN + BPF_ARP_FILTER_LEN + \
-				BPF_CMP_HWADDR_LEN + BPF_ARP_ADDRS_LEN
+#define BPF_ARP_LEN		BPF_ARP_ETHER_LEN + BPF_ARP_FILTER_LEN
 
-static int
-bpf_arp_rw(struct interface *ifp, int fd, bool read)
+int
+bpf_arp(struct interface *ifp, int fd)
 {
-	struct bpf_insn bpf[BPF_ARP_LEN];
+	struct bpf_insn bpf[BPF_ARP_LEN + 1];
 	struct bpf_insn *bp;
-	struct iarp_state *state;
 	uint16_t arp_len;
-	struct arp_state *astate;
-	size_t naddrs;
 
 	if (fd == -1)
 		return 0;
@@ -526,92 +433,21 @@
 	memcpy(bp, bpf_arp_filter, sizeof(bpf_arp_filter));
 	bp += BPF_ARP_FILTER_LEN;
 
-#ifdef BIOCSETWF
-	if (!read) {
-		/* All passed, return the packet. */
-		BPF_SET_STMT(bp, BPF_RET + BPF_K, BPF_WHOLEPACKET);
-		bp++;
-
-		return bpf_wattach(fd, bpf, (unsigned int)(bp - bpf));
-	}
-#else
-	UNUSED(read);
-#endif
-
-	/* Ensure it's not from us. */
-	bp += bpf_cmp_hwaddr(bp, BPF_CMP_HWADDR_LEN, sizeof(struct arphdr),
-	                     false, ifp->hwaddr, ifp->hwlen);
-
-	state = ARP_STATE(ifp);
-	/* privsep may not have an initial state yet. */
-	if (state == NULL || TAILQ_FIRST(&state->arp_states) == NULL)
-		goto noaddrs;
-
-	/* Match sender protocol address */
-	BPF_SET_STMT(bp, BPF_LD + BPF_W + BPF_IND,
-	             sizeof(struct arphdr) + ifp->hwlen);
-	bp++;
-	naddrs = 0;
-	TAILQ_FOREACH(astate, &state->arp_states, next) {
-		if (IN_IS_ADDR_UNSPECIFIED(&astate->addr))
-			continue;
-		if (++naddrs > ARP_ADDRS_MAX) {
-			errno = ENOBUFS;
-			logerr(__func__);
-			break;
-		}
-		BPF_SET_JUMP(bp, BPF_JMP + BPF_JEQ + BPF_K,
-		             htonl(astate->addr.s_addr), 0, 1);
-		bp++;
-		BPF_SET_STMT(bp, BPF_RET + BPF_K, arp_len);
-		bp++;
-	}
-
-	/* If we didn't match sender, then we're only interested in
-	 * ARP probes to us, so check the null host sender. */
-	BPF_SET_JUMP(bp, BPF_JMP + BPF_JEQ + BPF_K, INADDR_ANY, 1, 0);
-	bp++;
-	BPF_SET_STMT(bp, BPF_RET + BPF_K, 0);
+	/* Past the filer so return the packet. */
+	BPF_SET_STMT(bp, BPF_RET + BPF_K, arp_len);
 	bp++;
 
-	/* Match target protocol address */
-	BPF_SET_STMT(bp, BPF_LD + BPF_W + BPF_IND,
-	             (sizeof(struct arphdr)
-		     + (size_t)(ifp->hwlen * 2) + sizeof(in_addr_t)));
-	bp++;
-	naddrs = 0;
-	TAILQ_FOREACH(astate, &state->arp_states, next) {
-		if (++naddrs > ARP_ADDRS_MAX) {
-			/* Already logged error above. */
-			break;
-		}
-		BPF_SET_JUMP(bp, BPF_JMP + BPF_JEQ + BPF_K,
-		             htonl(astate->addr.s_addr), 0, 1);
-		bp++;
-		BPF_SET_STMT(bp, BPF_RET + BPF_K, arp_len);
-		bp++;
-	}
-
-noaddrs:
-	/* Return nothing, no protocol address match. */
-	BPF_SET_STMT(bp, BPF_RET + BPF_K, 0);
-	bp++;
-
-	return bpf_attach(fd, bpf, (unsigned int)(bp - bpf));
-}
-
-int
-bpf_arp(struct interface *ifp, int fd)
-{
+	if (bpf_attach(fd, bpf, (unsigned int)(bp - bpf)) == -1)
+		return -1;
 
 #ifdef BIOCSETWF
-	if (bpf_arp_rw(ifp, fd, false) == -1)
+	if (bpf_wattach(fd, bpf, (unsigned int)(bp - bpf)) == -1 ||
+	    ioctl(fd, BIOCLOCK) == -1)
 		return -1;
 #endif
 
-	return bpf_arp_rw(ifp, fd, true);
+	return 0;
 }
-
 #endif
 
 #ifdef ARPHRD_NONE
@@ -689,10 +525,7 @@
 static int
 bpf_bootp_rw(struct interface *ifp, int fd, bool read)
 {
-#if 0
-	const struct dhcp_state *state = D_CSTATE(ifp);
-#endif
-	struct bpf_insn bpf[BPF_BOOTP_LEN];
+	struct bpf_insn bpf[BPF_BOOTP_LEN + 1];
 	struct bpf_insn *bp;
 
 	if (fd == -1)
@@ -738,42 +571,6 @@
 	memcpy(bp, bpf_bootp_read, sizeof(bpf_bootp_read));
 	bp += BPF_BOOTP_READ_LEN;
 
-	/* These checks won't work when same IP exists on other interfaces. */
-#if 0
-	if (ifp->hwlen <= sizeof(((struct bootp *)0)->chaddr))
-		bp += bpf_cmp_hwaddr(bp, BPF_BOOTP_CHADDR_LEN,
-		                     offsetof(struct bootp, chaddr),
-				     true, ifp->hwaddr, ifp->hwlen);
-
-	/* Make sure the BOOTP packet is for us. */
-	if (state->state == DHS_BOUND) {
-		/* If bound, we only expect FORCERENEW messages
-		 * and they need to be unicast to us.
-		 * Move back to the IP header in M0 and check dst. */
-		BPF_SET_STMT(bp, BPF_LDX + BPF_W + BPF_MEM, 0);
-		bp++;
-		BPF_SET_STMT(bp, BPF_LD + BPF_W + BPF_IND,
-		             offsetof(struct ip, ip_dst));
-		bp++;
-		BPF_SET_JUMP(bp, BPF_JMP + BPF_JEQ + BPF_K,
-		             htonl(state->lease.addr.s_addr), 1, 0);
-		bp++;
-		BPF_SET_STMT(bp, BPF_RET + BPF_K, 0);
-		bp++;
-	} else {
-		/* As we're not bound, we need to check xid to ensure
-		 * it's a reply to our transaction. */
-		BPF_SET_STMT(bp, BPF_LD + BPF_W + BPF_IND,
-		             offsetof(struct bootp, xid));
-		bp++;
-		BPF_SET_JUMP(bp, BPF_JMP + BPF_JEQ + BPF_K,
-		             state->xid, 1, 0);
-		bp++;
-		BPF_SET_STMT(bp, BPF_RET + BPF_K, 0);
-		bp++;
-	}
-#endif
-
 	/* All passed, return the packet. */
 	BPF_SET_STMT(bp, BPF_RET + BPF_K, BPF_WHOLEPACKET);
 	bp++;
--- a/src/dhcp.c	Fri May 15 20:23:55 2020 +0100
+++ b/src/dhcp.c	Fri May 15 22:29:30 2020 +0100
@@ -2092,11 +2092,6 @@
 		 * address or hwaddr, so move to the next
 		 * arping profile */
 		if (++state->arping_index < ifo->arping_len) {
-			struct in_addr addr = {
-				.s_addr = ifo->arping[state->arping_index]
-			};
-
-			arp_change(astate, &addr);
 			arp_probe(astate);
 			return;
 		}
--- a/src/privsep-bpf.c	Fri May 15 20:23:55 2020 +0100
+++ b/src/privsep-bpf.c	Fri May 15 22:29:30 2020 +0100
@@ -90,6 +90,7 @@
 }
 
 #ifdef ARP
+#if !defined(HAVE_CAPSICUM) && !defined(HAVE_PLEDGE)
 static ssize_t
 ps_bpf_arp_addr(uint16_t cmd, struct ps_process *psp, struct msghdr *msg)
 {
@@ -125,6 +126,7 @@
 	return bpf_arp(ifp, psp->psp_work_fd);
 }
 #endif
+#endif
 
 static ssize_t
 ps_bpf_recvmsgcb(void *arg, struct ps_msghdr *psm, struct msghdr *msg)
@@ -134,7 +136,11 @@
 
 #ifdef ARP
 	if (psm->ps_cmd & (PS_START | PS_DELETE))
+#if !defined(HAVE_CAPSICUM) && !defined(HAVE_PLEDGE)
 		return ps_bpf_arp_addr(psm->ps_cmd, psp, msg);
+#else
+		return 0;
+#endif
 #endif
 
 	return bpf_send(&psp->psp_ifp, psp->psp_work_fd, psp->psp_proto,
@@ -276,13 +282,12 @@
 		return -1;
 	case 0:
 #ifdef HAVE_CAPSICUM
-	if (cap_enter() == -1 && errno != ENOSYS)
-		logerr("%s: cap_enter", __func__);
+		if (cap_enter() == -1 && errno != ENOSYS)
+			logerr("%s: cap_enter", __func__);
 #endif
 #ifdef HAVE_PLEDGE
-	/* Cant change BPF fitler for ARP yet. */
-	if (cmd != PS_BPF_ARP && pledge("stdio", NULL) == -1)
-		logerr("%s: pledge", __func__);
+		if (pledge("stdio", NULL) == -1)
+			logerr("%s: pledge", __func__);
 #endif
 		break;
 	default: