dhcpcd-discuss

Hi Kenny

On 26/04/2019 21:06, Kenny Napier wrote:
>     I have seen a __memcpy_chk() detected buffer over flow crash occur 
> twice using version 7.1.0
>
> The crash occurs in arp.c in the arp_packet() function during probing of 
> a new address.
>
> The line is the last memcpy in that routine where it is attempting to 
> copy the target ip address from the arp packet into a
>
> internal structure. I noticed this routine use to have some bounds 
> checking at the top that has been commented out for a long
>
> time.   The comment says the BPF filters does this work now.
>
>
> I tried to verify the bpf filter is really doing this work and noticed a 
> few things that look odd.
>
>
> bpf.c : bfp_arp_filter []
>
>
>     /* Make sure this is an ARP REQUEST. */
>          BPF_STMT(BPF_LD + BPF_H + BPF_IND, offsetof(struct arphdr, ar_op)),
>          BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ARPOP_REQUEST, 2, 0),
>          /* or ARP REPLY. */
>          BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ARPOP_REPLY, 1, 1),
>          BPF_STMT(BPF_RET + BPF_K, 0),
>    /* Make sure the protocol length matches. */
>          BPF_STMT(BPF_LD + BPF_B + BPF_IND, offsetof(struct arphdr, 
> ar_pln)),
>          BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, sizeof(in_addr_t), 1, 0),
>          BPF_STMT(BPF_RET + BPF_K, 0),
>
> It looks like it intended to only let ARP_REQUEST and ARP_REPLY's 
> through the filter but the  1,1 after the REPLY would
> let anything through this check.
>
> Also in the installer routine bfp_arp(...) the structure at the top used 
> to hold the built up the filter does not take into account the
> bfp_arp_ether array that is copied in first.
>
> struct bpf_insn bpf[3+ bpf_arp_filter_len + bpf_arp_hw + bpf_arp_extra];
>
> It looks like we would always overflow this stack buffer. I don't know 
> if the filter would get installed as intended if that is happening.

You're right!
I've attached a patch which should fix both issues.

> The crash is very rare.  I have no idea how to make it happen on demand.
> Looking for thoughts on the crash and my bpf code observations.

You would need several active ARP states I think - ensure the DHCP 
server delays the reply so that IPv4LL ARP probes happen at the same 
DHCP ARP probes happen.

I've tried to repliate this using valgind, but no memory errors are 
reported :/
Let me know how the patch works for you!

Roy

diff --git a/src/bpf.c b/src/bpf.c
index c2c14dc8..e85cd4f3 100644
--- a/src/bpf.c
+++ b/src/bpf.c
@@ -301,6 +301,7 @@ bpf_close(struct interface *ifp, int 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)
@@ -414,7 +415,7 @@ static const struct bpf_insn bpf_arp_ether [] = {
 	         sizeof(((struct ether_arp *)0)->arp_sha), 1, 0),
 	BPF_STMT(BPF_RET + BPF_K, 0),
 };
-#define bpf_arp_ether_len	__arraycount(bpf_arp_ether)
+#define BPF_ARP_ETHER_LEN	__arraycount(bpf_arp_ether)
 
 static const struct bpf_insn bpf_arp_filter [] = {
 	/* Make sure this is for IP. */
@@ -425,21 +426,25 @@ static const struct bpf_insn bpf_arp_filter [] = {
 	BPF_STMT(BPF_LD + BPF_H + BPF_IND, offsetof(struct arphdr, ar_op)),
 	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ARPOP_REQUEST, 2, 0),
 	/* or ARP REPLY. */
-	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ARPOP_REPLY, 1, 1),
+	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ARPOP_REPLY, 1, 0),
 	BPF_STMT(BPF_RET + BPF_K, 0),
 	/* Make sure the protocol length matches. */
 	BPF_STMT(BPF_LD + BPF_B + BPF_IND, offsetof(struct arphdr, ar_pln)),
 	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, sizeof(in_addr_t), 1, 0),
 	BPF_STMT(BPF_RET + BPF_K, 0),
 };
-#define bpf_arp_filter_len	__arraycount(bpf_arp_filter)
-#define bpf_arp_extra		((((ARP_ADDRS_MAX + 1) * 2) * 2) + 2)
-#define bpf_arp_hw		((((HWADDR_LEN / 4) + 2) * 2) + 1)
+#define BPF_ARP_FILTER_LEN	__arraycount(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
 
 int
 bpf_arp(struct interface *ifp, int fd)
 {
-	struct bpf_insn bpf[3+ bpf_arp_filter_len + bpf_arp_hw + bpf_arp_extra];
+	struct bpf_insn bpf[BPF_ARP_LEN];
 	struct bpf_insn *bp;
 	struct iarp_state *state;
 	uint16_t arp_len;
@@ -452,7 +457,7 @@ bpf_arp(struct interface *ifp, int fd)
 	switch(ifp->family) {
 	case ARPHRD_ETHER:
 		memcpy(bp, bpf_arp_ether, sizeof(bpf_arp_ether));
-		bp += bpf_arp_ether_len;
+		bp += BPF_ARP_ETHER_LEN;
 		arp_len = sizeof(struct ether_header)+sizeof(struct ether_arp);
 		break;
 	default:
@@ -462,10 +467,10 @@ bpf_arp(struct interface *ifp, int fd)
 
 	/* Copy in the main filter. */
 	memcpy(bp, bpf_arp_filter, sizeof(bpf_arp_filter));
-	bp += bpf_arp_filter_len;
+	bp += BPF_ARP_FILTER_LEN;
 
 	/* Ensure it's not from us. */
-	bp += bpf_cmp_hwaddr(bp, bpf_arp_hw, sizeof(struct arphdr),
+	bp += bpf_cmp_hwaddr(bp, BPF_CMP_HWADDR_LEN, sizeof(struct arphdr),
 	                     false, ifp->hwaddr, ifp->hwlen);
 
 	state = ARP_STATE(ifp);