From: Roy Marples Date: Fri, 11 Oct 2019 10:24:38 +0000 (+0100) Subject: BPF: Move validation logic from BPF to consumers X-Git-Tag: v8.1.0~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8baf5c7ce7bb047d4d766fdfd95f5eee1baeaebe;p=thirdparty%2Fdhcpcd.git BPF: Move validation logic from BPF to consumers Even though we program the BPF filter should we trust it? On Linux at least there is a window between opening the socket, binding the interface and setting the filter where we receive data. This data is NOT checked OR flushed and IS returned when reading. We have no way of flushing it other than reading these packets! But we don't know if they passed the filter or not ..... so we need to validate each and every packet that comes through ourselves as well. Even if Linux does fix this sorry state, who is to say other kernels don't have bugs causing a similar effect? As such, let's strive to keep the filters just for pattern matching to avoid waking dhcpcd up. --- diff --git a/src/arp.c b/src/arp.c index 594a5854..ba3755e6 100644 --- a/src/arp.c +++ b/src/arp.c @@ -129,7 +129,6 @@ arp_report_conflicted(const struct arp_state *astate, inet_ntoa(astate->addr)); } - static void arp_found(struct arp_state *astate, const struct arp_msg *amsg) { @@ -179,6 +178,35 @@ arp_found(struct arp_state *astate, const struct arp_msg *amsg) astate->defend_failed_cb(astate); } +static bool +arp_validate(const struct interface *ifp, struct arphdr *arp) +{ + + /* Families must match */ + if (arp->ar_hrd != htons(ifp->family)) + return false; + + /* Protocol must be IP. */ + if (arp->ar_pro != htons(ETHERTYPE_IP)) + return false; + + /* lladdr length matches */ + if (arp->ar_hln != ifp->hwlen) + return false; + + /* Protocol length must match in_addr_t */ + if (arp->ar_pln != sizeof(in_addr_t)) + return false; + + /* Only these types are recognised */ + if (arp->ar_op != htons(ARPOP_REPLY) && + arp->ar_op != htons(ARPOP_REQUEST)) + return false; + + return true; +} + + static void arp_packet(struct interface *ifp, uint8_t *data, size_t len) { @@ -194,25 +222,12 @@ arp_packet(struct interface *ifp, uint8_t *data, size_t len) return; memcpy(&ar, data, sizeof(ar)); - /* These checks are enforced in the BPF filter. */ -#if 0 - /* Families must match */ - if (ar.ar_hrd != htons(ifp->family)) - return; - /* Protocol must be IP. */ - if (ar.ar_pro != htons(ETHERTYPE_IP)) - continue; - /* lladdr length matches */ - if (ar.ar_hln != ifp->hwlen) - continue; - /* Protocol length must match in_addr_t */ - if (ar.ar_pln != sizeof(arm.sip.s_addr)) - return; - /* Only these types are recognised */ - if (ar.ar_op != htons(ARPOP_REPLY) && - ar.ar_op != htons(ARPOP_REQUEST)) - continue; + if (!arp_validate(ifp, &ar)) { +#ifdef BPF_DEBUG + logerrx("%s: ARP BPF validation failure", ifp->name); #endif + return; + } /* Get pointers to the hardware addresses */ hw_s = data + sizeof(ar); diff --git a/src/bpf.c b/src/bpf.c index 51094b4b..6c31bc2c 100644 --- a/src/bpf.c +++ b/src/bpf.c @@ -410,13 +410,7 @@ bpf_cmp_hwaddr(struct bpf_insn *bpf, size_t bpf_len, size_t off, #endif #ifdef ARP - static const struct bpf_insn bpf_arp_ether [] = { - /* Ensure packet is at least correct size. */ - BPF_STMT(BPF_LD + BPF_W + BPF_LEN, 0), - BPF_JUMP(BPF_JMP + BPF_JGE + BPF_K, sizeof(struct ether_arp), 1, 0), - BPF_STMT(BPF_RET + BPF_K, 0), - /* Check this is an ARP packet. */ BPF_STMT(BPF_LD + BPF_H + BPF_ABS, offsetof(struct ether_header, ether_type)), @@ -552,17 +546,8 @@ bpf_arp(struct interface *ifp, int fd) } #endif -#define BPF_M_FHLEN 0 -#define BPF_M_IPHLEN 1 -#define BPF_M_IPLEN 2 -#define BPF_M_UDP 3 -#define BPF_M_UDPLEN 4 - #ifdef ARPHRD_NONE static const struct bpf_insn bpf_bootp_none[] = { - /* Set the frame header length to zero. */ - BPF_STMT(BPF_LD + BPF_IMM, 0), - BPF_STMT(BPF_ST, BPF_M_FHLEN), }; #define BPF_BOOTP_NONE_LEN __arraycount(bpf_bootp_none) #endif @@ -574,10 +559,8 @@ static const struct bpf_insn bpf_bootp_ether[] = { BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_IP, 1, 0), BPF_STMT(BPF_RET + BPF_K, 0), - /* Load frame header length into X. */ - BPF_STMT(BPF_LDX + BPF_W + BPF_IMM, sizeof(struct ether_header)), - /* Copy frame header length to memory */ - BPF_STMT(BPF_STX, BPF_M_FHLEN), + /* Advance to the IP header. */ + BPF_STMT(BPF_LDX + BPF_K, sizeof(struct ether_header)), }; #define BPF_BOOTP_ETHER_LEN __arraycount(bpf_bootp_ether) @@ -591,15 +574,6 @@ static const struct bpf_insn bpf_bootp_filter[] = { BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, 0x40, 1, 0), BPF_STMT(BPF_RET + BPF_K, 0), - /* Ensure IP header length is big enough and - * store the IP header length in memory. */ - BPF_STMT(BPF_LD + BPF_B + BPF_IND, 0), - BPF_STMT(BPF_ALU + BPF_AND + BPF_K, 0x0f), - BPF_STMT(BPF_ALU + BPF_MUL + BPF_K, 4), - BPF_JUMP(BPF_JMP + BPF_JGE + BPF_K, sizeof(struct ip), 1, 0), - BPF_STMT(BPF_RET + BPF_K, 0), - BPF_STMT(BPF_ST, BPF_M_IPHLEN), - /* Make sure it's a UDP packet. */ BPF_STMT(BPF_LD + BPF_B + BPF_IND, offsetof(struct ip, ip_p)), BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_UDP, 1, 0), @@ -610,52 +584,17 @@ static const struct bpf_insn bpf_bootp_filter[] = { BPF_JUMP(BPF_JMP + BPF_JSET + BPF_K, 0x1fff, 0, 1), BPF_STMT(BPF_RET + BPF_K, 0), - /* Ensure IP length is big enough to hold the UDP + BOOTP payload and - * store IP length in memory. */ - BPF_STMT(BPF_LD + BPF_H + BPF_IND, offsetof(struct ip, ip_len)), - BPF_JUMP(BPF_JMP + BPF_JGE + BPF_K, BOOTP_MIN_SIZE, 1, 0), - BPF_STMT(BPF_RET + BPF_K, 0), - BPF_STMT(BPF_ST, BPF_M_IPLEN), - /* Advance to the UDP header. */ - BPF_STMT(BPF_LD + BPF_MEM, BPF_M_IPHLEN), + BPF_STMT(BPF_LD + BPF_B + BPF_IND, 0), + BPF_STMT(BPF_ALU + BPF_AND + BPF_K, 0x0f), + BPF_STMT(BPF_ALU + BPF_MUL + BPF_K, 4), BPF_STMT(BPF_ALU + BPF_ADD + BPF_X, 0), BPF_STMT(BPF_MISC + BPF_TAX, 0), - /* Store UDP location */ - BPF_STMT(BPF_STX, BPF_M_UDP), - /* Make sure it's from and to the right port. */ BPF_STMT(BPF_LD + BPF_W + BPF_IND, 0), BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, (BOOTPS << 16) + BOOTPC, 1, 0), BPF_STMT(BPF_RET + BPF_K, 0), - - /* Store UDP length. */ - BPF_STMT(BPF_LD + BPF_H + BPF_IND, offsetof(struct udphdr, uh_ulen)), - BPF_STMT(BPF_ST, BPF_M_UDPLEN), - - /* Ensure that UDP length + IP header length == IP length */ - /* Copy IP header length to X. */ - BPF_STMT(BPF_LDX + BPF_MEM, BPF_M_IPHLEN), - /* Add UDP length (A) to IP header length (X). */ - BPF_STMT(BPF_ALU + BPF_ADD + BPF_X, 0), - /* Store result in X. */ - BPF_STMT(BPF_MISC + BPF_TAX, 0), - /* Copy IP length to A. */ - BPF_STMT(BPF_LD + BPF_MEM, BPF_M_IPLEN), - /* Ensure X == A. */ - BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_X, 0, 1, 0), - BPF_STMT(BPF_RET + BPF_K, 0), - - /* Advance to the BOOTP packet. */ - BPF_STMT(BPF_LD + BPF_MEM, BPF_M_UDP), - BPF_STMT(BPF_ALU + BPF_ADD + BPF_K, sizeof(struct udphdr)), - BPF_STMT(BPF_MISC + BPF_TAX, 0), - - /* Make sure it's BOOTREPLY. */ - BPF_STMT(BPF_LD + BPF_B + BPF_IND, offsetof(struct bootp, op)), - BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, BOOTREPLY, 1, 0), - BPF_STMT(BPF_RET + BPF_K, 0), }; #define BPF_BOOTP_FILTER_LEN __arraycount(bpf_bootp_filter) @@ -735,14 +674,8 @@ bpf_bootp(struct interface *ifp, int fd) } #endif - /* All passed, return the packet - frame length + ip length */ - BPF_SET_STMT(bp, BPF_LD + BPF_MEM, BPF_M_FHLEN); - bp++; - BPF_SET_STMT(bp, BPF_LDX + BPF_MEM, BPF_M_IPLEN); - bp++; - BPF_SET_STMT(bp, BPF_ALU + BPF_ADD + BPF_X, 0); - bp++; - BPF_SET_STMT(bp, BPF_RET + BPF_A, 0); + /* All passed, return the packet. */ + BPF_SET_STMT(bp, BPF_RET + BPF_K, BPF_WHOLEPACKET); bp++; return bpf_attach(fd, bpf, (unsigned int)(bp - bpf)); diff --git a/src/bpf.h b/src/bpf.h index e886f9c3..46bc6c37 100644 --- a/src/bpf.h +++ b/src/bpf.h @@ -34,6 +34,25 @@ #define BPF_PARTIALCSUM (1U << 2) #define BPF_BCAST (1U << 3) +/* + * Even though we program the BPF filter should we trust it? + * On Linux at least there is a window between opening the socket, + * binding the interface and setting the filter where we receive data. + * This data is NOT checked OR flushed and IS returned when reading. + * We have no way of flushing it other than reading these packets! + * But we don't know if they passed the filter or not ..... so we need + * to validate each and every packet that comes through ourselves as well. + * Even if Linux does fix this sorry state, who is to say other kernels + * don't have bugs causing a similar effect? + * + * As such, let's strive to keep the filters just for pattern matching + * to avoid waking dhcpcd up. + * + * If you want to be notified of any packet failing the BPF filter, + * define BPF_DEBUG below. + */ +//#define BPF_DEBUG + #include "dhcpcd.h" extern const char *bpf_name; diff --git a/src/dhcp.c b/src/dhcp.c index 76ead6dc..ab0ae90b 100644 --- a/src/dhcp.c +++ b/src/dhcp.c @@ -3270,9 +3270,40 @@ get_udp_data(void *packet, size_t *len) return p; } -static int -valid_udp_packet(void *packet, size_t plen, struct in_addr *from, - unsigned int flags) +static bool +is_packet_udp_bootp(void *packet, size_t plen) +{ + struct ip *ip = packet; + size_t ip_hlen; + struct udphdr *udp; + + if (sizeof(*ip) > plen) + return false; + + if (ip->ip_v != IPVERSION || ip->ip_p != IPPROTO_UDP) + return false; + + /* Sanity. */ + if (ntohs(ip->ip_len) != plen) + return false; + + ip_hlen = (size_t)ip->ip_hl * 4; + /* Check we have a UDP header and BOOTP. */ + if (ip_hlen + sizeof(*udp) + offsetof(struct bootp, vend) > plen) + return false; + + /* Check it's to and from the right ports. */ + udp = (struct udphdr *)(void *)((char *)ip + ip_hlen); + if (udp->uh_dport != htons(BOOTPC) || udp->uh_sport != htons(BOOTPS)) + return false; + + return true; +} + +/* Lengths have already been checked. */ +static bool +checksums_valid(void *packet, + struct in_addr *from, unsigned int flags) { struct ip *ip = packet; struct ip pseudo_ip = { @@ -3281,69 +3312,34 @@ valid_udp_packet(void *packet, size_t plen, struct in_addr *from, .ip_dst = ip->ip_dst }; size_t ip_hlen; - uint16_t ip_len, udp_len, uh_sum; + uint16_t udp_len, uh_sum; struct udphdr *udp; uint32_t csum; - if (plen < sizeof(*ip)) { - if (from != NULL) - from->s_addr = INADDR_ANY; - errno = ERANGE; - return -1; - } - if (from != NULL) from->s_addr = ip->ip_src.s_addr; - /* Check we have the IP header */ ip_hlen = (size_t)ip->ip_hl * 4; - if (ip_hlen > plen) { - errno = ENOBUFS; - return -1; - } - - if (in_cksum(ip, ip_hlen, NULL) != 0) { - errno = EINVAL; - return -1; - } + if (in_cksum(ip, ip_hlen, NULL) != 0) + return false; - /* Check we have a payload */ - ip_len = ntohs(ip->ip_len); - if (ip_len <= ip_hlen + sizeof(*udp)) { - errno = ERANGE; - return -1; - } - /* Check IP doesn't go beyond the payload */ - if (ip_len > plen) { - errno = ENOBUFS; - return -1; - } + if (flags & BPF_PARTIALCSUM) + return 0; - /* Check UDP doesn't go beyond the payload */ udp = (struct udphdr *)(void *)((char *)ip + ip_hlen); - udp_len = ntohs(udp->uh_ulen); - if (udp_len > plen - ip_hlen) { - errno = ENOBUFS; - return -1; - } - - if (udp->uh_sum == 0 || flags & BPF_PARTIALCSUM) + if (udp->uh_sum == 0) return 0; /* UDP checksum is based on a pseudo IP header alongside * the UDP header and payload. */ + udp_len = ntohs(udp->uh_ulen); uh_sum = udp->uh_sum; udp->uh_sum = 0; pseudo_ip.ip_len = udp->uh_ulen; csum = 0; in_cksum(&pseudo_ip, sizeof(pseudo_ip), &csum); csum = in_cksum(udp, udp_len, &csum); - if (csum != uh_sum) { - errno = EINVAL; - return -1; - } - - return 0; + return csum == uh_sum; } static void @@ -3352,13 +3348,12 @@ dhcp_handlebootp(struct interface *ifp, struct bootp *bootp, size_t len, { size_t v; - /* udp_len must be correct because the values are checked in - * valid_udp_packet(). */ if (len < offsetof(struct bootp, vend)) { logerrx("%s: truncated packet (%zu) from %s", ifp->name, len, inet_ntoa(*from)); return; } + /* To make our IS_DHCP macro easy, ensure the vendor * area has at least 4 octets. */ v = len - offsetof(struct bootp, vend); @@ -3378,14 +3373,17 @@ dhcp_handlebpf(struct interface *ifp, uint8_t *data, size_t len) size_t udp_len; const struct dhcp_state *state = D_CSTATE(ifp); - if (valid_udp_packet(data, len, &from, state->bpf_flags) == -1) { - const char *errstr; + /* Validate filter. */ + if (!is_packet_udp_bootp(data, len)) { +#ifdef BPF_DEBUG + logerrx("%s: DHCP BPF validation failure", ifp->name); +#endif + return; + } - if (errno == EINVAL) - errstr = "checksum failure"; - else - errstr = "invalid UDP packet"; - logerrx("%s: %s from %s", ifp->name, errstr, inet_ntoa(from)); + if (!checksums_valid(data, &from, state->bpf_flags)) { + logerrx("%s: checksum failure from %s", + ifp->name, inet_ntoa(from)); return; } diff --git a/src/if-linux.c b/src/if-linux.c index 18044bbf..fd472785 100644 --- a/src/if-linux.c +++ b/src/if-linux.c @@ -1383,45 +1383,55 @@ if_initrt(struct dhcpcd_ctx *ctx, rb_tree_t *kroutes, int af) #ifdef INET /* Linux is a special snowflake when it comes to BPF. */ const char *bpf_name = "Packet Socket"; -#define BPF_BUFFER_LEN 1500 +/* Linux is a special snowflake for opening BPF. */ int bpf_open(struct interface *ifp, int (*filter)(struct interface *, int)) { struct ipv4_state *state; -/* Linux is a special snowflake for opening BPF. */ int s; union sockunion { struct sockaddr sa; struct sockaddr_ll sll; struct sockaddr_storage ss; - } su; + } su = { + .sll.sll_family = PF_PACKET, + .sll.sll_protocol = htons(ETH_P_ALL), + .sll.sll_ifindex = (int)ifp->index, + }; #ifdef PACKET_AUXDATA int n; #endif -#define SF SOCK_CLOEXEC | SOCK_NONBLOCK - if ((s = xsocket(PF_PACKET, SOCK_RAW | SF, htons(ETH_P_ALL))) == -1) - return -1; -#undef SF - /* Allocate a suitably large buffer for a single packet. */ state = ipv4_getstate(ifp); if (state == NULL) - goto eexit; + return -1; if (state->buffer_size < ETH_DATA_LEN) { void *nb; if ((nb = realloc(state->buffer, ETH_DATA_LEN)) == NULL) - goto eexit; + return -1; state->buffer = nb; state->buffer_size = ETH_DATA_LEN; state->buffer_len = state->buffer_pos = 0; } + +#define SF SOCK_CLOEXEC | SOCK_NONBLOCK + if ((s = xsocket(PF_PACKET, SOCK_RAW | SF, htons(ETH_P_ALL))) == -1) + return -1; +#undef SF + + /* We cannot validate the correct interface, + * so we MUST set this first. */ + if (bind(s, &su.sa, sizeof(su.sll)) == -1) + goto eexit; + if (filter(ifp, s) != 0) goto eexit; + /* In the ideal world, this would be set before the bind and filter. */ #ifdef PACKET_AUXDATA n = 1; if (setsockopt(s, SOL_PACKET, PACKET_AUXDATA, &n, sizeof(n)) != 0) { @@ -1430,19 +1440,18 @@ bpf_open(struct interface *ifp, int (*filter)(struct interface *, int)) } #endif - memset(&su, 0, sizeof(su)); - su.sll.sll_family = PF_PACKET; - su.sll.sll_protocol = htons(ETH_P_ALL); - su.sll.sll_ifindex = (int)ifp->index; - if (bind(s, &su.sa, sizeof(su.sll)) == -1) - goto eexit; + /* + * At this point we could have received packets for the wrong + * interface or which don't pass the filter. + * Linux should flush upon setting the filter like every other OS. + * There is no way of flushing them from userland. + * As such, consumers need to inspect each packet to ensure it's valid. + * Or to put it another way, don't trust the Linux BPF filter. + */ + return s; eexit: - if (state != NULL) { - free(state->buffer); - state->buffer = NULL; - } close(s); return -1; }