[PATCH net] packet: validate address length

[PATCH net] packet: validate address length

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Packet sockets with SOCK_DGRAM may pass an address for use in
dev_hard_header. Ensure that it is of sufficient length.

Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/packet/af_packet.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 6655793765b2d..5dda263b4a0a1 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2627,6 +2627,8 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		proto	= saddr->sll_protocol;
 		addr	= saddr->sll_addr;
 		dev = dev_get_by_index(sock_net(&po->sk), saddr->sll_ifindex);
+		if (addr && dev && saddr->sll_halen < dev->addr_len)
+			goto out;
 	}
 
 	err = -ENXIO;
@@ -2825,6 +2827,8 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		proto	= saddr->sll_protocol;
 		addr	= saddr->sll_addr;
 		dev = dev_get_by_index(sock_net(sk), saddr->sll_ifindex);
+		if (addr && dev && saddr->sll_halen < dev->addr_len)
+			goto out;
 	}
 
 	err = -ENXIO;
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH net] packet: validate address length

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Fri, 21 Dec 2018 12:06:59 -0500

> From: Willem de Bruijn <willemb@google.com>
> 
> Packet sockets with SOCK_DGRAM may pass an address for use in
> dev_hard_header. Ensure that it is of sufficient length.
> 
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Applied and queued up for -stable, thanks.

Re: [PATCH net] packet: validate address length

[Ido Schimmel]

On Fri, Dec 21, 2018 at 12:06:59PM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> Packet sockets with SOCK_DGRAM may pass an address for use in
> dev_hard_header. Ensure that it is of sufficient length.
> 
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Willem,

We use mausezahn [1] in some of the tests under
tools/testing/selftests/net/forwarding/ and I started observing failures
today. Bisected it down to this patch. It seems that mausezahn passes
'sll_halen=0' [2]. Can you please take a look and adjust the check?

Thanks

[1] https://github.com/netsniff-ng/netsniff-ng

[2]
With patch:
# strace -e network mausezahn dummy0 -c 1 -p 64 -a de:ad:be:ef:13:37 -t ip -q
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 3
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 3
socket(AF_PACKET, SOCK_RAW, 768)        = 3
setsockopt(3, SOL_SOCKET, SO_BROADCAST, [1], 4) = 0
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 4
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 4
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 3
socket(AF_PACKET, SOCK_RAW, 768)        = 3
setsockopt(3, SOL_SOCKET, SO_BROADCAST, [1], 4) = 0
sendto(3, "\377\377\377\377\377\377\336\255\276\357\0237\10\0E\0\0T\0\0\0\0\377\0\273\252\377\377\377\377\377\377"..., 98, 0, {sa_family=AF_PACKET, sll_protocol=htons(ETH_P_ALL), sll_ifindex=if_nametoindex("dummy0"), sll_hatype=ARPHRD_NETROM, sll_pkttype=PACKET_HOST, sll_halen=0}, 20) = -1 EINVAL (Invalid argument)
+++ exited with 0 +++

Without patch:
# strace -e network mausezahn dummy0 -c 1 -p 64 -a de:ad:be:ef:13:37 -t ip -q
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 3
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 3
socket(AF_PACKET, SOCK_RAW, 768)        = 3
setsockopt(3, SOL_SOCKET, SO_BROADCAST, [1], 4) = 0
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 4
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 4
socket(AF_INET, SOCK_DGRAM, IPPROTO_IP) = 3
socket(AF_PACKET, SOCK_RAW, 768)        = 3
setsockopt(3, SOL_SOCKET, SO_BROADCAST, [1], 4) = 0
sendto(3, "\377\377\377\377\377\377\336\255\276\357\0237\10\0E\0\0T\0\0\0\0\377\0\273\252\377\377\377\377\377\377"..., 98, 0, {sa_family=AF_PACKET, sll_protocol=htons(ETH_P_ALL), sll_ifindex=if_nametoindex("dummy0"), sll_hatype=ARPHRD_NETROM, sll_pkttype=PACKET_HOST, sll_halen=0}, 20) = 98
+++ exited with 0 +++

Re: [PATCH net] packet: validate address length

[Willem de Bruijn]

On Sat, Dec 22, 2018 at 10:39 AM Ido Schimmel <idosch@idosch.org> wrote:
>
> On Fri, Dec 21, 2018 at 12:06:59PM -0500, Willem de Bruijn wrote:
> > From: Willem de Bruijn <willemb@google.com>
> >
> > Packet sockets with SOCK_DGRAM may pass an address for use in
> > dev_hard_header. Ensure that it is of sufficient length.
> >
> > Reported-by: syzbot <syzkaller@googlegroups.com>
> > Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> Willem,
>
> We use mausezahn [1] in some of the tests under
> tools/testing/selftests/net/forwarding/ and I started observing failures
> today. Bisected it down to this patch. It seems that mausezahn passes
> 'sll_halen=0' [2]. Can you please take a look and adjust the check?


Thanks for the report, Ido. I should have checked for obvious case
myself. Indeed the fix as I sent it is incorrect: saddr->sll_addr is not
a pointer, but an array, so addr is never zero. This refinement should
fix it and allow the tests to succeed.

@@ -2825,7 +2825,7 @@ static int packet_snd(struct socket *sock,
struct msghdr *msg, size_t len)
                if (msg-&gt;msg_namelen &lt; (saddr-&gt;sll_halen +
offsetof(struct sockaddr_ll, sll_addr)))
                        goto out;
                proto   = saddr-&gt;sll_protocol;
-               addr    = saddr-&gt;sll_addr;
+               addr    = saddr-&gt;sll_halen ? saddr-&gt;sll_addr : 0;
                dev = dev_get_by_index(sock_net(sk), saddr-&gt;sll_ifindex);
                if (addr &amp;&amp; dev &amp;&amp; saddr-&gt;sll_halen
&lt; dev-&gt;addr_len)
                        goto out;

(and same in tpacket_snd)