From: Roy Marples Date: Fri, 15 May 2020 21:29:30 +0000 (+0100) Subject: ARP: Remove ability to filter specific addresses X-Git-Tag: v9.1.0~58 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f55742151ec03f517471ab9a9724e5c1359fbc30;p=thirdparty%2Fdhcpcd.git 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. --- diff --git a/src/arp.c b/src/arp.c index f13fa139..dfb94403 100644 --- a/src/arp.c +++ b/src/arp.c @@ -601,38 +601,6 @@ arp_announceaddr(struct dhcpcd_ctx *ctx, const struct in_addr *ia) 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 @@ arp_new(struct interface *ifp, const struct in_addr *addr) 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 @@ arp_free(struct arp_state *astate) 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) diff --git a/src/arp.h b/src/arp.h index db3019a6..182041c8 100644 --- a/src/arp.h +++ b/src/arp.h @@ -95,7 +95,6 @@ struct iarp_state { #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 *); diff --git a/src/bpf.c b/src/bpf.c index 1517355f..96be4e53 100644 --- a/src/bpf.c +++ b/src/bpf.c @@ -359,95 +359,6 @@ bpf_close(struct interface *ifp, int fd) 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 @@ static const struct bpf_insn bpf_arp_filter [] = { #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 @@ bpf_arp_rw(struct interface *ifp, int fd, bool read) 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); + /* Past the filer so return the packet. */ + BPF_SET_STMT(bp, BPF_RET + BPF_K, arp_len); bp++; - BPF_SET_STMT(bp, BPF_RET + BPF_K, 0); - 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 const struct bpf_insn bpf_bootp_write[] = { 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 @@ bpf_bootp_rw(struct interface *ifp, int fd, bool read) 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++; diff --git a/src/dhcp.c b/src/dhcp.c index dba0f1ed..0a0c0a84 100644 --- a/src/dhcp.c +++ b/src/dhcp.c @@ -2092,11 +2092,6 @@ dhcp_arp_not_found(struct arp_state *astate) * 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; } diff --git a/src/privsep-bpf.c b/src/privsep-bpf.c index 06247c0f..ed8fa5b8 100644 --- a/src/privsep-bpf.c +++ b/src/privsep-bpf.c @@ -90,6 +90,7 @@ ps_bpf_recvbpf(void *arg) } #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 @@ ps_bpf_arp_addr(uint16_t cmd, struct ps_process *psp, struct msghdr *msg) 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 @@ ps_bpf_recvmsgcb(void *arg, struct ps_msghdr *psm, struct msghdr *msg) #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 @@ ps_bpf_cmd(struct dhcpcd_ctx *ctx, struct ps_msghdr *psm, struct msghdr *msg) 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: