| Message ID | 1517154099.3715.77.camel@gmail.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] netfilter: xt_hashlimit: do not allow empty names | expand |
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. > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: syzbot <syzkaller@googlegroups.com> > --- > net/netfilter/xt_hashlimit.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c > index 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75 100644 > --- a/net/netfilter/xt_hashlimit.c > +++ b/net/netfilter/xt_hashlimit.c > @@ -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;
Eric Dumazet <eric.dumazet@gmail.com> wrote: > 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. > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Reported-by: syzbot <syzkaller@googlegroups.com> > > --- > > net/netfilter/xt_hashlimit.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c > > index 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75 100644 > > --- a/net/netfilter/xt_hashlimit.c > > +++ b/net/netfilter/xt_hashlimit.c > > @@ -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.
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: > > 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. > > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Reported-by: syzbot <syzkaller@googlegroups.com> > > --- > > net/netfilter/xt_hashlimit.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c > > index 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75 100644 > > --- a/net/netfilter/xt_hashlimit.c > > +++ b/net/netfilter/xt_hashlimit.c > > @@ -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!
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!
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: > [...] > > > > 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.
On Friday 2018-02-02 12:55, Pablo Neira Ayuso wrote: >On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote: >[...] >> 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.
On Fri, Feb 02, 2018 at 01:12:08PM +0100, Jan Engelhardt wrote: > On Friday 2018-02-02 12:55, Pablo Neira Ayuso wrote: > > >On Fri, Feb 02, 2018 at 12:49:38PM +0100, Pablo Neira Ayuso wrote: > >[...] > >> 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. As long as we use nla_str*() helpers, we're all good.
On Fri, 2018-02-02 at 12:49 +0100, Pablo Neira Ayuso wrote:
> @Eric, I can give it a shot here to this, just let me know. Thanks!
Please go ahead Pablo !
Thanks.
diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c index 5da8746f7b88ff4c9446f256e542e823a6a561b0..eae732e013df92a364b500645360d4606c283a75 100644 --- a/net/netfilter/xt_hashlimit.c +++ b/net/netfilter/xt_hashlimit.c @@ -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) {