From: Eric Dumazet <hidden> Date: 2018-01-28 17:54:09
On Sun, 2018-01-28 at 07:41 -0800, Eric Dumazet wrote:
From: Eric Dumazet <edumazet@google.com>
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.
On Sun, 2018-01-28 at 07:41 -0800, Eric Dumazet wrote:
quoted
From: Eric Dumazet <edumazet@google.com>
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.
@@ -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;
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.
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2018-02-02 11:49:42
On Sun, Jan 28, 2018 at 09:54:05AM -0800, Eric Dumazet wrote:
On Sun, 2018-01-28 at 07:41 -0800, Eric Dumazet wrote:
quoted
From: Eric Dumazet <edumazet@google.com>
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.
@@ -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;
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!
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2018-02-02 11:55:22
On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
[...]
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);
}
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!
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2018-02-02 11:56:40
On Fri, Feb 02, 2018 at 12:55:22PM +0100, Pablo Neira Ayuso wrote:
On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote:
[...]
quoted
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);
}
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.
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.
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.