Skip to contentSkip to conversation

netfilter: ip6t_eui64: validate MAC header before using it

PATCH In review Ren Wei 3 participants 7 messages Mar 31 – Apr 4 1 message not in the archive

Subsystems (3)
Networking Network drivers netfilter
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ren Wei

author
#1 lore raw

PATCH 1/1 netfilter: ip6t_eui64: validate MAC header before using it

From: Zhengchuan Liang <[email protected]>

eui64_mt6() derives a modified EUI-64 from the Ethernet source address and compares it with the low 64 bits of the IPv6 source address.

The match unconditionally reaches skb_mac_header() and eth_hdr(skb) after a guard that only rejects an invalid MAC header when par->fragoff != 0. As a result, non-fragment packets can still reach eth_hdr(skb) even when the skb has no MAC header set, or when the MAC header does not cover a full Ethernet header.

Fix this by first checking that the MAC header is set and spans a full Ethernet header before accessing it, then using that validated header directly for the EUI-64 comparison. Preserve the existing hotdrop behavior for non-first fragments with an invalid MAC header.

Fixes 1da177e4c3f41 ("Linux-2.6.12-rc2")
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Zhengchuan Liang <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 net/ipv6/netfilter/ip6t_eui64.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
net/ipv6/netfilter/ip6t_eui64.c +16 -7
index d704f7ed300c2..dbf64948d72c0 100644
@@ -19,21 +19,30 @@ MODULE_AUTHOR("Andras Kis-Szabo <[email protected]>");
 static bool
 eui64_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 {
+	const unsigned char *mac;
+	const struct ethhdr *eth;
 	unsigned char eui64[8];
 
-	if (!(skb_mac_header(skb) >= skb->head &&
-	      skb_mac_header(skb) + ETH_HLEN <= skb->data) &&
-	    par->fragoff != 0) {
-		par->hotdrop = true;
+	if (!skb_mac_header_was_set(skb)) {
+		if (par->fragoff != 0)
+			par->hotdrop = true;
 		return false;
 	}
 
+	mac = skb_mac_header(skb);
+	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
+		if (par->fragoff != 0)
+			par->hotdrop = true;
+		return false;
+	}
+	eth = (const struct ethhdr *)mac;
+
 	memset(eui64, 0, sizeof(eui64));
 
-	if (eth_hdr(skb)->h_proto == htons(ETH_P_IPV6)) {
+	if (eth->h_proto == htons(ETH_P_IPV6)) {
 		if (ipv6_hdr(skb)->version == 0x6) {
-			memcpy(eui64, eth_hdr(skb)->h_source, 3);
-			memcpy(eui64 + 5, eth_hdr(skb)->h_source + 3, 3);
+			memcpy(eui64, eth->h_source, 3);
+			memcpy(eui64 + 5, eth->h_source + 3, 3);
 			eui64[3] = 0xff;
 			eui64[4] = 0xfe;
 			eui64[0] ^= 0x02;
Signature · 1 line
2.43.0
maintainer
#2 lore raw
Quoted 12 lines · Ren Wei

From: Zhengchuan Liang <[email protected]>

eui64_mt6() derives a modified EUI-64 from the Ethernet source address and compares it with the low 64 bits of the IPv6 source address.

The match unconditionally reaches skb_mac_header() and eth_hdr(skb) after a guard that only rejects an invalid MAC header when par->fragoff != 0. As a result, non-fragment packets can still reach eth_hdr(skb) even when the skb has no MAC header set, or when the MAC header does not cover a full Ethernet header.

Quoted 4 lines

Fix this by first checking that the MAC header is set and spans a full Ethernet header before accessing it, then using that validated header directly for the EUI-64 comparison. Preserve the existing hotdrop behavior for non-first fragments with an invalid MAC header.

I find this rather confusing. E.g. why is net/netfilter/xt_mac.c safe? I get the feeling that this patch is not sufficient?

net/ipv6/netfilter/ip6t_eui64.c MODULE_AUTHOR("Andras Kis-Szabo <[email protected]>"); · line 26

Quoted 1 line
net/ipv6/netfilter/ip6t_eui64.c +1 -0
+	if (!skb_mac_header_was_set(skb)) {

Makes sense to me.

net/ipv6/netfilter/ip6t_eui64.c MODULE_AUTHOR("Andras Kis-Szabo <[email protected]>"); · line 36

Quoted 5 lines
net/ipv6/netfilter/ip6t_eui64.c +5 -0
+	mac = skb_mac_header(skb);
+	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
+		if (par->fragoff != 0)
+			par->hotdrop = true;
+		return false;

Why do we still need this stunt? Why not:

mac_len = skb_mac_header_len(skb);

if (mac_len < ETH_HLEN) {
	par->hotdrop = true;
	return false;
}

?

I also feel there should be a check for ethernet, i.e.

        if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)

... like in xt_mac.c.

maintainer
#3 lore raw
Quoted 39 lines · Florian Westphal
Quoted 12 lines · Ren Wei

From: Zhengchuan Liang <[email protected]>

eui64_mt6() derives a modified EUI-64 from the Ethernet source address and compares it with the low 64 bits of the IPv6 source address.

The match unconditionally reaches skb_mac_header() and eth_hdr(skb) after a guard that only rejects an invalid MAC header when par->fragoff != 0. As a result, non-fragment packets can still reach eth_hdr(skb) even when the skb has no MAC header set, or when the MAC header does not cover a full Ethernet header.

Quoted 4 lines

Fix this by first checking that the MAC header is set and spans a full Ethernet header before accessing it, then using that validated header directly for the EUI-64 comparison. Preserve the existing hotdrop behavior for non-first fragments with an invalid MAC header.

I find this rather confusing. E.g. why is net/netfilter/xt_mac.c safe? I get the feeling that this patch is not sufficient?

net/ipv6/netfilter/ip6t_eui64.c MODULE_AUTHOR("Andras Kis-Szabo <[email protected]>"); · line 26

Quoted 1 line
net/ipv6/netfilter/ip6t_eui64.c +1 -0
+	if (!skb_mac_header_was_set(skb)) {

Makes sense to me.

net/ipv6/netfilter/ip6t_eui64.c MODULE_AUTHOR("Andras Kis-Szabo <[email protected]>"); · line 36

Quoted 5 lines
net/ipv6/netfilter/ip6t_eui64.c +5 -0
+	mac = skb_mac_header(skb);
+	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
+		if (par->fragoff != 0)
+			par->hotdrop = true;
+		return false;

Why do we still need this stunt? Why not:

mac_len = skb_mac_header_len(skb);

if (mac_len < ETH_HLEN) {
	par->hotdrop = true;
	return false;
}
Quoted 6 lines

?

I also feel there should be a check for ethernet, i.e.

        if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)

... like in xt_mac.c.

There are other suspicious spots, e.g. in nf_log_syslog.c and in ipset.

Will you make a patch to cover all of these?

maintainer
#4 lore raw

[ Trimming Cc list ]

Quoted 42 lines · Florian Westphal
Quoted 41 lines · Florian Westphal
Quoted 12 lines · Ren Wei

From: Zhengchuan Liang <[email protected]>

eui64_mt6() derives a modified EUI-64 from the Ethernet source address and compares it with the low 64 bits of the IPv6 source address.

The match unconditionally reaches skb_mac_header() and eth_hdr(skb) after a guard that only rejects an invalid MAC header when par->fragoff != 0. As a result, non-fragment packets can still reach eth_hdr(skb) even when the skb has no MAC header set, or when the MAC header does not cover a full Ethernet header.

Quoted 4 lines

Fix this by first checking that the MAC header is set and spans a full Ethernet header before accessing it, then using that validated header directly for the EUI-64 comparison. Preserve the existing hotdrop behavior for non-first fragments with an invalid MAC header.

I find this rather confusing. E.g. why is net/netfilter/xt_mac.c safe? I get the feeling that this patch is not sufficient?

net/ipv6/netfilter/ip6t_eui64.c MODULE_AUTHOR("Andras Kis-Szabo <[email protected]>"); · line 26

Quoted 1 line
net/ipv6/netfilter/ip6t_eui64.c +1 -0
+	if (!skb_mac_header_was_set(skb)) {

Makes sense to me.

net/ipv6/netfilter/ip6t_eui64.c MODULE_AUTHOR("Andras Kis-Szabo <[email protected]>"); · line 36

Quoted 5 lines
net/ipv6/netfilter/ip6t_eui64.c +5 -0
+	mac = skb_mac_header(skb);
+	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
+		if (par->fragoff != 0)
+			par->hotdrop = true;
+		return false;

Why do we still need this stunt? Why not:

mac_len = skb_mac_header_len(skb);

if (mac_len < ETH_HLEN) {
	par->hotdrop = true;
	return false;
}

?

Quoted 8 lines
Quoted 4 lines

I also feel there should be a check for ethernet, i.e.

        if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)

... like in xt_mac.c.

There are other suspicious spots, e.g. in nf_log_syslog.c and in ipset.

Will you make a patch to cover all of these?

Ping. Will you followup or do you expect us to take over?

#5 lore raw
Quoted 42 lines · Florian Westphal

[ Trimming Cc list ]

Quoted 39 lines · Florian Westphal
Quoted 38 lines · Florian Westphal
Quoted 12 lines · Ren Wei

From: Zhengchuan Liang <[email protected]>

eui64_mt6() derives a modified EUI-64 from the Ethernet source address and compares it with the low 64 bits of the IPv6 source address.

The match unconditionally reaches skb_mac_header() and eth_hdr(skb) after a guard that only rejects an invalid MAC header when par->fragoff != 0. As a result, non-fragment packets can still reach eth_hdr(skb) even when the skb has no MAC header set, or when the MAC header does not cover a full Ethernet header.

Quoted 4 lines

Fix this by first checking that the MAC header is set and spans a full Ethernet header before accessing it, then using that validated header directly for the EUI-64 comparison. Preserve the existing hotdrop behavior for non-first fragments with an invalid MAC header.

I find this rather confusing. E.g. why is net/netfilter/xt_mac.c safe? I get the feeling that this patch is not sufficient?

net/ipv6/netfilter/ip6t_eui64.c MODULE_AUTHOR("Andras Kis-Szabo <[email protected]>"); · line 26

Quoted 1 line
net/ipv6/netfilter/ip6t_eui64.c +1 -0
+	if (!skb_mac_header_was_set(skb)) {

Makes sense to me.

net/ipv6/netfilter/ip6t_eui64.c MODULE_AUTHOR("Andras Kis-Szabo <[email protected]>"); · line 36

Quoted 5 lines
net/ipv6/netfilter/ip6t_eui64.c +5 -0
+	mac = skb_mac_header(skb);
+	if (mac < skb->head || mac + ETH_HLEN > skb->data) {
+		if (par->fragoff != 0)
+			par->hotdrop = true;
+		return false;

Why do we still need this stunt? Why not:

mac_len = skb_mac_header_len(skb);

if (mac_len < ETH_HLEN) {
	par->hotdrop = true;
	return false;
}

?

Quoted 8 lines
Quoted 7 lines
Quoted 4 lines

I also feel there should be a check for ethernet, i.e.

         if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)

... like in xt_mac.c.

There are other suspicious spots, e.g. in nf_log_syslog.c and in ipset.

Will you make a patch to cover all of these?

Ping. Will you followup or do you expect us to take over?

Sorry for our late reply. We will try to send version 2 in 2 days.

author
#6 lore raw

PATCH v2 1/2 netfilter: ip6t_eui64: reject invalid MAC header for all packets

In reply to a message not in the archive

From: Zhengchuan Liang <[email protected]>

eui64_mt6() derives a modified EUI-64 from the Ethernet source address and compares it with the low 64 bits of the IPv6 source address.

The existing guard only rejects an invalid MAC header when par->fragoff != 0. For packets with par->fragoff == 0, eui64_mt6() can still reach eth_hdr(skb) even when the MAC header is not valid.

Fix this by removing the par->fragoff != 0 condition so that packets with an invalid MAC header are rejected before accessing eth_hdr(skb).

Fixes 1da177e4c3f41 ("Linux-2.6.12-rc2")
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Zhengchuan Liang <[email protected]>
Signed-off-by Ren Wei <[email protected]>

---
Changes in v2:
This patch specifically fixes the memory-safety issue in eui64_mt6():
the old guard only rejected invalid MAC headers for non-first fragments, while our PoC triggers the KASAN report with a non-fragment packet (par->fragoff == 0). This patch is sufficient enough to fix the memory-safety issue, and we do not find other eth_hdr(skb) users with similar use-after-free or out-of-bound issues.

 net/ipv6/netfilter/ip6t_eui64.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
net/ipv6/netfilter/ip6t_eui64.c +1 -2
index d704f7ed300c2..da69a27e8332c 100644
@@ -22,8 +22,7 @@ eui64_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 	unsigned char eui64[8];
 
 	if (!(skb_mac_header(skb) >= skb->head &&
-	      skb_mac_header(skb) + ETH_HLEN <= skb->data) &&
-	    par->fragoff != 0) {
+	      skb_mac_header(skb) + ETH_HLEN <= skb->data)) {
 		par->hotdrop = true;
 		return false;
 	}
Signature · 1 line
2.43.0
author
#7 lore raw

PATCH v2 2/2 netfilter: require Ethernet MAC header before using eth_hdr()

In reply to a message not in the archive

From: Zhengchuan Liang <[email protected]>

ip6t_eui64, xt_mac, the bitmap:ip,mac, hash:ip,mac, and hash:mac ipset types, and nf_log_syslog access eth_hdr(skb) after either assuming that the skb is associated with an Ethernet device or checking only that the ETH_HLEN bytes at skb_mac_header(skb) lie between skb->head and skb->data.

Make these paths first verify that the skb is associated with an Ethernet device, that the MAC header was set, and that it spans at least a full Ethernet header before accessing eth_hdr(skb).

Suggested-by Florian Westphal <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Zhengchuan Liang <[email protected]>
Signed-off-by Ren Wei <[email protected]>

---
Changes in v2:
This patch addresses suspicious eth_hdr(skb) users by requiring an Ethernet device together with a valid MAC header before accessing it.

 net/ipv6/netfilter/ip6t_eui64.c           | 7 +++++--
 net/netfilter/ipset/ip_set_bitmap_ipmac.c | 5 +++--
 net/netfilter/ipset/ip_set_hash_ipmac.c   | 9 +++++----
 net/netfilter/ipset/ip_set_hash_mac.c     | 5 +++--
 net/netfilter/nf_log_syslog.c             | 7 ++++++-
 net/netfilter/xt_mac.c                    | 4 +---
 6 files changed, 23 insertions(+), 14 deletions(-)
net/ipv6/netfilter/ip6t_eui64.c +5 -2
index da69a27e8332c..bbb684f9964c0 100644
@@ -7,6 +7,7 @@
 #include <linux/module.h>
 #include <linux/skbuff.h>
 #include <linux/ipv6.h>
+#include <linux/if_arp.h>
 #include <linux/if_ether.h>
 
 #include <linux/netfilter/x_tables.h>

@@ -21,8 +22,10 @@ eui64_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 {
 	unsigned char eui64[8];
 
-	if (!(skb_mac_header(skb) >= skb->head &&
-	      skb_mac_header(skb) + ETH_HLEN <= skb->data)) {
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER)
+		return false;
+
+	if (!skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN) {
 		par->hotdrop = true;
 		return false;
 	}
net/netfilter/ipset/ip_set_bitmap_ipmac.c +3 -2
index 2c625e0f49ec0..752f59ef87442 100644
@@ -11,6 +11,7 @@
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
 #include <linux/errno.h>
+#include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <linux/netlink.h>
 #include <linux/jiffies.h>

@@ -220,8 +221,8 @@ bitmap_ipmac_kadt(struct ip_set *set, const struct sk_buff *skb,
 		return -IPSET_ERR_BITMAP_RANGE;
 
 	/* Backward compatibility: we don't check the second flag */
-	if (skb_mac_header(skb) < skb->head ||
-	    (skb_mac_header(skb) + ETH_HLEN) > skb->data)
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER ||
+	    !skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return -EINVAL;
 
 	e.id = ip_to_id(map, ip);
net/netfilter/ipset/ip_set_hash_ipmac.c +5 -4
index 467c59a83c0ab..b9a2681e24888 100644
@@ -11,6 +11,7 @@
 #include <linux/skbuff.h>
 #include <linux/errno.h>
 #include <linux/random.h>
+#include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <net/ip.h>
 #include <net/ipv6.h>

@@ -89,8 +90,8 @@ hash_ipmac4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_ipmac4_elem e = { .ip = 0, { .foo[0] = 0, .foo[1] = 0 } };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	if (skb_mac_header(skb) < skb->head ||
-	    (skb_mac_header(skb) + ETH_HLEN) > skb->data)
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER ||
+	    !skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return -EINVAL;
 
 	if (opt->flags & IPSET_DIM_TWO_SRC)

@@ -205,8 +206,8 @@ hash_ipmac6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	};
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	if (skb_mac_header(skb) < skb->head ||
-	    (skb_mac_header(skb) + ETH_HLEN) > skb->data)
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER ||
+	    !skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return -EINVAL;
 
 	if (opt->flags & IPSET_DIM_TWO_SRC)
net/netfilter/ipset/ip_set_hash_mac.c +3 -2
index 718814730acf6..41a122591fe24 100644
@@ -8,6 +8,7 @@
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
 #include <linux/errno.h>
+#include <linux/if_arp.h>
 #include <linux/if_ether.h>
 #include <net/netlink.h>
 

@@ -77,8 +78,8 @@ hash_mac4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	struct hash_mac4_elem e = { { .foo[0] = 0, .foo[1] = 0 } };
 	struct ip_set_ext ext = IP_SET_INIT_KEXT(skb, opt, set);
 
-	if (skb_mac_header(skb) < skb->head ||
-	    (skb_mac_header(skb) + ETH_HLEN) > skb->data)
+	if (!skb->dev || skb->dev->type != ARPHRD_ETHER ||
+	    !skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return -EINVAL;
 
 	if (opt->flags & IPSET_DIM_ONE_SRC)
net/netfilter/nf_log_syslog.c +4 -1
index 41503847d9d7f..f62049d429653 100644
@@ -78,7 +78,9 @@ dump_arp_packet(struct nf_log_buf *m,
 	else
 		logflags = NF_LOG_DEFAULT_MASK;
 
-	if (logflags & NF_LOG_MACDECODE) {
+	if ((logflags & NF_LOG_MACDECODE) &&
+	    skb->dev && skb->dev->type == ARPHRD_ETHER &&
+	    skb_mac_header_was_set(skb) &&
+	    skb_mac_header_len(skb) >= ETH_HLEN) {
 		nf_log_buf_add(m, "MACSRC=%pM MACDST=%pM ",
 			       eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest);

nf_log_dump_vlan(m, skb);
@@ -789,6 +791,9 @@ static void dump_mac_header(struct nf_log_buf *m,

switch (dev->type) {
case ARPHRD_ETHER:
+ if (!skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
+ return;
+
nf_log_buf_add(m, "MACSRC=%pM MACDST=%pM ",
eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest);
nf_log_dump_vlan(m, skb);

net/netfilter/xt_mac.c +1 -3
index 81649da57ba5d..4798cd2ca26ed 100644
@@ -29,9 +29,7 @@ static bool mac_mt(const struct sk_buff *skb, struct xt_action_param *par)
 
 	if (skb->dev == NULL || skb->dev->type != ARPHRD_ETHER)
 		return false;
-	if (skb_mac_header(skb) < skb->head)
-		return false;
-	if (skb_mac_header(skb) + ETH_HLEN > skb->data)
+	if (!skb_mac_header_was_set(skb) || skb_mac_header_len(skb) < ETH_HLEN)
 		return false;
 	ret  = ether_addr_equal(eth_hdr(skb)->h_source, info->srcaddr);
 	ret ^= info->invert;
Signature · 1 line
2.43.0