netfilter: xt_hashlimit: do not allow empty names

PATCH In review Eric Dumazet 4 participants 9 messages Jan 28, 2018 – Feb 2, 2018

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

Eric Dumazet

author

2018-01-28 15:41 UTC #0 lore raw

PATCH 1/1 netfilter: xt_hashlimit: do not allow empty names

From: Eric Dumazet <[email protected]>

Syzbot reported a WARN() in proc_create_data() [1]

Issue here is that xt_hashlimit does not check that user space provided an empty table name.

[1]
name len 0

WARNING: CPU: 0 PID: 3680 at fs/proc/generic.c:354 __proc_create+0x696/0x880 fs/proc/generic.c:354
Kernel panic - not syncing: panic_on_warn set ...

CPU: 0 PID: 3680 Comm: syzkaller464755 Not tainted 4.15.0-rc9+ #283
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:17 [inline]
 dump_stack+0x194/0x257 lib/dump_stack.c:53
 panic+0x1e4/0x41c kernel/panic.c:183
 __warn+0x1dc/0x200 kernel/panic.c:547
 report_bug+0x211/0x2d0 lib/bug.c:184
 fixup_bug.part.11+0x37/0x80 arch/x86/kernel/traps.c:178
 fixup_bug arch/x86/kernel/traps.c:247 [inline]
 do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:296
 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:315
 invalid_op+0x22/0x40 arch/x86/entry/entry_64.S:1096
RIP: 0010:__proc_create+0x696/0x880 fs/proc/generic.c:354
RSP: 0018:ffff8801d9607410 EFLAGS: 00010286
RAX: dffffc0000000008 RBX: 1ffff1003b2c0e87 RCX: ffffffff8159ebae
RDX: 0000000000000000 RSI: 1ffff1003b284970 RDI: 0000000000000293
RBP: ffff8801d9607580 R08: 1ffff1003b2c0e15 R09: 0000000000000000
R10: ffff8801d96072c8 R11: 0000000000000000 R12: ffff8801d981ef28
R13: ffff8801d9607558 R14: 0000000000000000 R15: ffff8801d9607518
 proc_create_data+0x76/0x180 fs/proc/generic.c:488
 htable_create net/netfilter/xt_hashlimit.c:333 [inline]
 hashlimit_mt_check_common.isra.9+0xaee/0x1420 net/netfilter/xt_hashlimit.c:900
 hashlimit_mt_check_v1+0x48d/0x640 net/netfilter/xt_hashlimit.c:926
 xt_check_match+0x231/0x7d0 net/netfilter/x_tables.c:465
 check_match net/ipv4/netfilter/ip_tables.c:479 [inline]
 find_check_match net/ipv4/netfilter/ip_tables.c:495 [inline]
 find_check_entry.isra.8+0x3fc/0xcb0 net/ipv4/netfilter/ip_tables.c:544
 translate_table+0xed1/0x1610 net/ipv4/netfilter/ip_tables.c:730
 do_replace net/ipv4/netfilter/ip_tables.c:1148 [inline]
 do_ipt_set_ctl+0x370/0x5f0 net/ipv4/netfilter/ip_tables.c:1682
 nf_sockopt net/netfilter/nf_sockopt.c:106 [inline]
 nf_setsockopt+0x67/0xc0 net/netfilter/nf_sockopt.c:115
 ip_setsockopt+0xa1/0xb0 net/ipv4/ip_sockglue.c:1256
 tcp_setsockopt+0x82/0xd0 net/ipv4/tcp.c:2875
 sock_common_setsockopt+0x95/0xd0 net/core/sock.c:2968
 SYSC_setsockopt net/socket.c:1831 [inline]
 SyS_setsockopt+0x189/0x360 net/socket.c:1810
 entry_SYSCALL_64_fastpath+0x29/0xa0

Signed-off-by Eric Dumazet <[email protected]>

Reported-by syzbot <[email protected]>

---
net/netfilter/xt_hashlimit.c | 2 ++

 1 file changed, 2 insertions(+)

net/netfilter/xt_hashlimit.c +2 -0

index 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75 100644
@@ -894,6 +894,8 @@ static int hashlimit_mt_check_common(const struct xt_mtchk_param *par,
 			return -ERANGE;
 	}
 
+	if (!name[0])
+		return -EINVAL;
 	mutex_lock(&hashlimit_mutex);
 	*hinfo = htable_find_get(net, name, par->family);
 	if (*hinfo == NULL) {

Eric Dumazet

author

2018-01-28 17:54 UTC #1 lore raw

Quoted 18 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>
Syzbot reported a WARN() in proc_create_data() [1]
Issue here is that xt_hashlimit does not check that user space provided an empty table name.
Signed-off-by Eric Dumazet <[email protected]>
Reported-by syzbot <[email protected]>
---
net/netfilter/xt_hashlimit.c | 2 ++
 1 file changed, 2 insertions(+)
net/netfilter/xt_hashlimit.c +0 -0
index 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75 100644

net/netfilter/xt_hashlimit.c static int hashlimit_mt_check_common(const struct xt_mtchk_param *par, · line 901

Quoted 8 lines

net/netfilter/xt_hashlimit.c +2 -0

@@ -894,6 +894,8 @@ static int hashlimit_mt_check_common(const struct xt_mtchk_param *par,
 			return -ERANGE;
 	}
 
+	if (!name[0])
+		return -EINVAL;
 	mutex_lock(&hashlimit_mutex);
 	*hinfo = htable_find_get(net, name, par->family);
 	if (*hinfo == NULL) {

I wonder if we should also check if name includes a '/' ?

if (!name[0] || strchr(name, '/'))

    return -EINVAL;

Florian Westphal

maintainer

2018-01-28 21:54 UTC #2 lore raw

Quoted 30 lines · Eric Dumazet

Quoted 18 lines · Eric Dumazet
From: Eric Dumazet <[email protected]>
Syzbot reported a WARN() in proc_create_data() [1]
Issue here is that xt_hashlimit does not check that user space provided an empty table name.
Signed-off-by Eric Dumazet <[email protected]>
Reported-by syzbot <[email protected]>
---
net/netfilter/xt_hashlimit.c | 2 ++
 1 file changed, 2 insertions(+)
net/netfilter/xt_hashlimit.c +0 -0
index 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75 100644
net/netfilter/xt_hashlimit.c static int hashlimit_mt_check_common(const struct xt_mtchk_param *par, · line 901
Quoted 8 lines
net/netfilter/xt_hashlimit.c +2 -0
@@ -894,6 +894,8 @@ static int hashlimit_mt_check_common(const struct xt_mtchk_param *par,
 			return -ERANGE;
 	}
 
+	if (!name[0])
+		return -EINVAL;
 	mutex_lock(&hashlimit_mutex);
 	*hinfo = htable_find_get(net, name, par->family);
 	if (*hinfo == NULL) {
I wonder if we should also check if name includes a '/' ?

Quoted 2 lines

if (!name[0] || strchr(name, '/'))
    return -EINVAL;

I think there should be a helper for this to reject

- 0 length
- /
- .
- ..

in hashlimit case name is IFNMASIZ so we could even use dev_valid_name() here.

Pablo Neira Ayuso

maintainer

2018-02-02 11:49 UTC #3 lore raw

In reply to Eric Dumazet

Quoted 30 lines · Eric Dumazet

Quoted 18 lines · Eric Dumazet
From: Eric Dumazet <[email protected]>
Syzbot reported a WARN() in proc_create_data() [1]
Issue here is that xt_hashlimit does not check that user space provided an empty table name.
Signed-off-by Eric Dumazet <[email protected]>
Reported-by syzbot <[email protected]>
---
net/netfilter/xt_hashlimit.c | 2 ++
 1 file changed, 2 insertions(+)
net/netfilter/xt_hashlimit.c +0 -0
index 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75 100644
net/netfilter/xt_hashlimit.c static int hashlimit_mt_check_common(const struct xt_mtchk_param *par, · line 901
Quoted 8 lines
net/netfilter/xt_hashlimit.c +2 -0
@@ -894,6 +894,8 @@ static int hashlimit_mt_check_common(const struct xt_mtchk_param *par,
 			return -ERANGE;
 	}
 
+	if (!name[0])
+		return -EINVAL;
 	mutex_lock(&hashlimit_mutex);
 	*hinfo = htable_find_get(net, name, par->family);
 	if (*hinfo == NULL) {
I wonder if we should also check if name includes a '/' ?

Quoted 2 lines

if (!name[0] || strchr(name, '/'))
    return -EINVAL;

We can probably add our own variant of dev_valid_name?

bool nf_valid_name(const char *name, size_t len)

{
        if (*name == '\0')
                return false;
        if (strlen(name) >= len)
                return false;
        if (!strcmp(name, ".") || !strcmp(name, ".."))
                return false;

        while (*name) {
                if (*name == '/' || *name == ':' || isspace(*name))
                        return false;
                name++;
        }
        return true;
}

Or place this in the core, something like:

bool net_valid_name(const char *name, size_t len)

{
        ...
}

then use it from dev_valid_name()

bool dev_valid_name(const char *name)

{
        return net_valid_name(name, IFNAMSIZ);
}

@Eric, I can give it a shot here to this, just let me know. Thanks!

Pablo Neira Ayuso

maintainer

2018-02-02 11:55 UTC #4 lore raw

On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
[...]

Quoted 8 lines

Or place this in the core, something like:
bool net_valid_name(const char *name, size_t len)
{
        ...
}

Quoted 6 lines

then use it from dev_valid_name()
bool dev_valid_name(const char *name)
{
        return net_valid_name(name, IFNAMSIZ);
}

Just to clarify, I think I prefer this approach, probably other subsystem can benefit from this generic approach too.

BTW, I wonder if we should use strnlen() instead of strlen() in dev_valid_name(), I guess this not useful for device name since they are guaranteed to be nul-terminated, but netfilter this would be good given userspace can send us a non nul-terminated string. But in general, I think it doesn't harm to use strnlen() in dev_valid_name().

Am I missing anything in all these tricky string handling? Thanks!

Pablo Neira Ayuso

maintainer

2018-02-02 11:56 UTC #5 lore raw

Quoted 17 lines · Pablo Neira Ayuso

On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
[...]
Quoted 8 lines
Or place this in the core, something like:
bool net_valid_name(const char *name, size_t len)
{
        ...
}
Quoted 6 lines
then use it from dev_valid_name()
bool dev_valid_name(const char *name)
{
        return net_valid_name(name, IFNAMSIZ);
}

Quoted 8 lines

Just to clarify, I think I prefer this approach, probably other subsystem can benefit from this generic approach too.
BTW, I wonder if we should use strnlen() instead of strlen() in dev_valid_name(), I guess this not useful for device name since they are guaranteed to be nul-terminated, but netfilter this would be good given userspace can send us a non nul-terminated string. But in general, I think it doesn't harm to use strnlen() in dev_valid_name().

Argh sorry, I'm talking about the hypothetical net_valid_name() function, not dev_valid_name() itself.

Jan Engelhardt

2018-02-02 12:12 UTC #6 lore raw

In reply to Pablo Neira Ayuso

Quoted 8 lines · Pablo Neira Ayuso

On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
[...]
Quoted 4 lines
bool net_valid_name(const char *name, size_t len)
{
        ...
}
Am I missing anything in all these tricky string handling? Thanks!

One will have to watch out for calls like

net_valid_name(NLA_DATA(some_ifla_name), IFNAMSIZ)

because quite some nlattrs (IFLA_NAME, IFLA_IFNAME to boot) are NLA_STRINGs (rather than NLA_NUL_STRINGs) and they are not \0-terminated.

Pablo Neira Ayuso

maintainer

2018-02-02 12:17 UTC #7 lore raw

Quoted 9 lines · Jan Engelhardt

Quoted 7 lines · Pablo Neira Ayuso
On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
[...]
Quoted 4 lines
bool net_valid_name(const char *name, size_t len)
{
        ...
}

Quoted 8 lines

Quoted 1 line
Am I missing anything in all these tricky string handling? Thanks!
One will have to watch out for calls like
net_valid_name(NLA_DATA(some_ifla_name), IFNAMSIZ)
because quite some nlattrs (IFLA_NAME, IFLA_IFNAME to boot) are NLA_STRINGs (rather than NLA_NUL_STRINGs) and they are not \0-terminated.

As long as we use nla_str*() helpers, we're all good.

Eric Dumazet

author

2018-02-02 16:27 UTC #8 lore raw

In reply to Pablo Neira Ayuso

Quoted 1 line · Pablo Neira Ayuso

@Eric, I can give it a shot here to this, just let me know. Thanks!

Please go ahead Pablo !

Thanks.