Prior to commit c0371da6047a ("put iov_iter into msghdr") in v3.19, there
was no check that the iovec contained enough bytes for a icmp header,
and the read loop would walk across neighboring stack contents. Since
the iov_iter conversion, bad arguments are noticed, but the returned
error is EFAULT. Returning EMSGSIZE is a clearer fix and solves the
problem prior to v3.19.
This was found using trinity with KASAN on v3.18:
BUG: KASAN: stack-out-of-bounds in memcpy_fromiovec+0x60/0x114 at addr ffffffc071077da0
Read of size 8 by task trinity-c2/9623
page:ffffffbe034b9a08 count:0 mapcount:0 mapping: (null) index:0x0
flags: 0x0()
page dumped because: kasan: bad access detected
CPU: 0 PID: 9623 Comm: trinity-c2 Tainted: G BU 3.18.0-dirty #15
Hardware name: Google Tegra210 Smaug Rev 1,3+ (DT)
Call trace:
[<ffffffc000209c98>] dump_backtrace+0x0/0x1ac arch/arm64/kernel/traps.c:90
[<ffffffc000209e54>] show_stack+0x10/0x1c arch/arm64/kernel/traps.c:171
[< inline >] __dump_stack lib/dump_stack.c:15
[<ffffffc000f18dc4>] dump_stack+0x7c/0xd0 lib/dump_stack.c:50
[< inline >] print_address_description mm/kasan/report.c:147
[< inline >] kasan_report_error mm/kasan/report.c:236
[<ffffffc000373dcc>] kasan_report+0x380/0x4b8 mm/kasan/report.c:259
[< inline >] check_memory_region mm/kasan/kasan.c:264
[<ffffffc00037352c>] __asan_load8+0x20/0x70 mm/kasan/kasan.c:507
[<ffffffc0005b9624>] memcpy_fromiovec+0x5c/0x114 lib/iovec.c:15
[< inline >] memcpy_from_msg include/linux/skbuff.h:2667
[<ffffffc000ddeba0>] ping_common_sendmsg+0x50/0x108 net/ipv4/ping.c:674
[<ffffffc000dded30>] ping_v4_sendmsg+0xd8/0x698 net/ipv4/ping.c:714
[<ffffffc000dc91dc>] inet_sendmsg+0xe0/0x12c net/ipv4/af_inet.c:749
[< inline >] __sock_sendmsg_nosec net/socket.c:624
[< inline >] __sock_sendmsg net/socket.c:632
[<ffffffc000cab61c>] sock_sendmsg+0x124/0x164 net/socket.c:643
[< inline >] SYSC_sendto net/socket.c:1797
[<ffffffc000cad270>] SyS_sendto+0x178/0x1d8 net/socket.c:1761
CVE-2016-8399
Reported-by: Qidan He <[email protected]>
Fixes: c319b4d76b9e ("net: ipv4: add IPPROTO_ICMP socket kind")
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
net/ipv4/ping.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 205e2000d395..8257be3f032c 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -654,7 +654,7 @@ int ping_common_sendmsg(int family, struct msghdr *msg, size_t len,
void *user_icmph, size_t icmph_len) {
u8 type, code;
- if (len > 0xFFFF)
+ if (len > 0xFFFF || len < icmph_len)
return -EMSGSIZE;
/*
--
2.7.4
--
Kees Cook
Nexus Security
On Sat, Dec 3, 2016 at 9:58 AM, Kees Cook <[email protected]> wrote:
> - if (len > 0xFFFF)
> + if (len > 0xFFFF || len < icmph_len)
> return -EMSGSIZE;
EMSGSIZE usually means the message is too long. Maybe use EINVAL?
That's what the code will return if the passed-in ICMP header is
invalid (e.g., is not an echo request).
From: Kees Cook <[email protected]>
Date: Fri, 2 Dec 2016 16:58:53 -0800
> diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
> index 205e2000d395..8257be3f032c 100644
> --- a/net/ipv4/ping.c
> +++ b/net/ipv4/ping.c
> @@ -654,7 +654,7 @@ int ping_common_sendmsg(int family, struct msghdr *msg, size_t len,
> void *user_icmph, size_t icmph_len) {
> u8 type, code;
>
> - if (len > 0xFFFF)
> + if (len > 0xFFFF || len < icmph_len)
> return -EMSGSIZE;
As suggested by Lorenzo, please use -EINVAL here.
Thanks.