| Message ID | 8710122d42aa1f3e081812f2abf406973f834982.1524818458.git.pabeni@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | netfilter: ebtables: handle string from userspace with care | expand |
Paolo Abeni <pabeni@redhat.com> wrote: > strlcpy() can't be safely used on a user-space provided string, > as it can try to read beyond the buffer's end, if the latter is > not NULL terminated. Yes. > Leveraging the above, syzbot has been able to trigger the following > splat: > > BUG: KASAN: stack-out-of-bounds in strlcpy include/linux/string.h:300 > [inline] > BUG: KASAN: stack-out-of-bounds in compat_mtw_from_user > net/bridge/netfilter/ebtables.c:1957 [inline] > BUG: KASAN: stack-out-of-bounds in ebt_size_mwt > net/bridge/netfilter/ebtables.c:2059 [inline] > BUG: KASAN: stack-out-of-bounds in size_entry_mwt > net/bridge/netfilter/ebtables.c:2155 [inline] > BUG: KASAN: stack-out-of-bounds in compat_copy_entries+0x96c/0x14a0 > net/bridge/netfilter/ebtables.c:2194 > Write of size 33 at addr ffff8801b0abf888 by task syz-executor0/4504 Which is weird, I don't understand this report. The code IS wrong, but it should cause out-of-bounds read (strlen on src), but not out-of-bounds write. Yes, I sent a recent patch (dceb48d86b4871984b8ce9ad5057fb2c01aa33de in nf.git) that would now allow to get rid of the strlcpy and use the source directly. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 27, 2018 at 11:26 AM, Florian Westphal <fw@strlen.de> wrote: > Paolo Abeni <pabeni@redhat.com> wrote: >> strlcpy() can't be safely used on a user-space provided string, >> as it can try to read beyond the buffer's end, if the latter is >> not NULL terminated. > > Yes. > >> Leveraging the above, syzbot has been able to trigger the following >> splat: >> >> BUG: KASAN: stack-out-of-bounds in strlcpy include/linux/string.h:300 >> [inline] >> BUG: KASAN: stack-out-of-bounds in compat_mtw_from_user >> net/bridge/netfilter/ebtables.c:1957 [inline] >> BUG: KASAN: stack-out-of-bounds in ebt_size_mwt >> net/bridge/netfilter/ebtables.c:2059 [inline] >> BUG: KASAN: stack-out-of-bounds in size_entry_mwt >> net/bridge/netfilter/ebtables.c:2155 [inline] >> BUG: KASAN: stack-out-of-bounds in compat_copy_entries+0x96c/0x14a0 >> net/bridge/netfilter/ebtables.c:2194 >> Write of size 33 at addr ffff8801b0abf888 by task syz-executor0/4504 > > Which is weird, I don't understand this report. > The code IS wrong, but it should cause out-of-bounds read (strlen on > src), but not out-of-bounds write. Please see this for explanation: https://groups.google.com/d/msg/syzkaller-bugs/-Jyti8zBWjU/6n-fkmXeBAAJ The stack overwrite actually happens here. > Yes, I sent a recent patch (dceb48d86b4871984b8ce9ad5057fb2c01aa33de in > nf.git) that would now allow to get rid of the strlcpy and use the > source directly. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 27, 2018 at 10:45:31AM +0200, Paolo Abeni wrote: > strlcpy() can't be safely used on a user-space provided string, > as it can try to read beyond the buffer's end, if the latter is > not NULL terminated. Applied, thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c index 28a4c3490359..6ba639f6c51d 100644 --- a/net/bridge/netfilter/ebtables.c +++ b/net/bridge/netfilter/ebtables.c @@ -1954,7 +1954,8 @@ static int compat_mtw_from_user(struct compat_ebt_entry_mwt *mwt, int off, pad = 0; unsigned int size_kern, match_size = mwt->match_size; - strlcpy(name, mwt->u.name, sizeof(name)); + if (strscpy(name, mwt->u.name, sizeof(name)) < 0) + return -EINVAL; if (state->buf_kern_start) dst = state->buf_kern_start + state->buf_kern_offset;
strlcpy() can't be safely used on a user-space provided string, as it can try to read beyond the buffer's end, if the latter is not NULL terminated. Leveraging the above, syzbot has been able to trigger the following splat: BUG: KASAN: stack-out-of-bounds in strlcpy include/linux/string.h:300 [inline] BUG: KASAN: stack-out-of-bounds in compat_mtw_from_user net/bridge/netfilter/ebtables.c:1957 [inline] BUG: KASAN: stack-out-of-bounds in ebt_size_mwt net/bridge/netfilter/ebtables.c:2059 [inline] BUG: KASAN: stack-out-of-bounds in size_entry_mwt net/bridge/netfilter/ebtables.c:2155 [inline] BUG: KASAN: stack-out-of-bounds in compat_copy_entries+0x96c/0x14a0 net/bridge/netfilter/ebtables.c:2194 Write of size 33 at addr ffff8801b0abf888 by task syz-executor0/4504 CPU: 0 PID: 4504 Comm: syz-executor0 Not tainted 4.17.0-rc2+ #40 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x1b9/0x294 lib/dump_stack.c:113 print_address_description+0x6c/0x20b mm/kasan/report.c:256 kasan_report_error mm/kasan/report.c:354 [inline] kasan_report.cold.7+0x242/0x2fe mm/kasan/report.c:412 check_memory_region_inline mm/kasan/kasan.c:260 [inline] check_memory_region+0x13e/0x1b0 mm/kasan/kasan.c:267 memcpy+0x37/0x50 mm/kasan/kasan.c:303 strlcpy include/linux/string.h:300 [inline] compat_mtw_from_user net/bridge/netfilter/ebtables.c:1957 [inline] ebt_size_mwt net/bridge/netfilter/ebtables.c:2059 [inline] size_entry_mwt net/bridge/netfilter/ebtables.c:2155 [inline] compat_copy_entries+0x96c/0x14a0 net/bridge/netfilter/ebtables.c:2194 compat_do_replace+0x483/0x900 net/bridge/netfilter/ebtables.c:2285 compat_do_ebt_set_ctl+0x2ac/0x324 net/bridge/netfilter/ebtables.c:2367 compat_nf_sockopt net/netfilter/nf_sockopt.c:144 [inline] compat_nf_setsockopt+0x9b/0x140 net/netfilter/nf_sockopt.c:156 compat_ip_setsockopt+0xff/0x140 net/ipv4/ip_sockglue.c:1279 inet_csk_compat_setsockopt+0x97/0x120 net/ipv4/inet_connection_sock.c:1041 compat_tcp_setsockopt+0x49/0x80 net/ipv4/tcp.c:2901 compat_sock_common_setsockopt+0xb4/0x150 net/core/sock.c:3050 __compat_sys_setsockopt+0x1ab/0x7c0 net/compat.c:403 __do_compat_sys_setsockopt net/compat.c:416 [inline] __se_compat_sys_setsockopt net/compat.c:413 [inline] __ia32_compat_sys_setsockopt+0xbd/0x150 net/compat.c:413 do_syscall_32_irqs_on arch/x86/entry/common.c:323 [inline] do_fast_syscall_32+0x345/0xf9b arch/x86/entry/common.c:394 entry_SYSENTER_compat+0x70/0x7f arch/x86/entry/entry_64_compat.S:139 RIP: 0023:0xf7fb3cb9 RSP: 002b:00000000fff0c26c EFLAGS: 00000282 ORIG_RAX: 000000000000016e RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000000000 RDX: 0000000000000080 RSI: 0000000020000300 RDI: 00000000000005f4 RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 The buggy address belongs to the page: page:ffffea0006c2afc0 count:0 mapcount:0 mapping:0000000000000000 index:0x0 flags: 0x2fffc0000000000() raw: 02fffc0000000000 0000000000000000 0000000000000000 00000000ffffffff raw: 0000000000000000 ffffea0006c20101 0000000000000000 0000000000000000 page dumped because: kasan: bad access detected Fix the issue replacing the unsafe function with strscpy() and taking care of possible errors. Fixes: 81e675c227ec ("netfilter: ebtables: add CONFIG_COMPAT support") Reported-and-tested-by: syzbot+4e42a04e0bc33cb6c087@syzkaller.appspotmail.com Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- Notes: others strlcpy() usage in ebtables.c look safe, as the source string cames from the kernel. --- net/bridge/netfilter/ebtables.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)