[1/1] netfilter: ipset: Check IPSET_ATTR_ETHER netlink attribute length

Julia Lawall pointed out that IPSET_ATTR_ETHER netlink attribute length
was not checked explicitly, just for the maximum possible size. Malicious
netlink clients could send shorter attribute and thus resulting a kernel
read after the buffer.

The patch adds the explicit length checkings.

Reported-by: Julia Lawall <julia.lawall@lip6.fr>
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/netfilter/ipset/ip_set_bitmap_ipmac.c | 2 ++
 net/netfilter/ipset/ip_set_hash_mac.c     | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

Hi Jozsef,

On 03/08/2016 08:44 PM, Jozsef Kadlecsik wrote:
> Julia Lawall pointed out that IPSET_ATTR_ETHER netlink attribute length
> was not checked explicitly, just for the maximum possible size. Malicious
> netlink clients could send shorter attribute and thus resulting a kernel
> read after the buffer.
>
> The patch adds the explicit length checkings.
>
> Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> ---
>   net/netfilter/ipset/ip_set_bitmap_ipmac.c | 2 ++
>   net/netfilter/ipset/ip_set_hash_mac.c     | 3 ++-
>   2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> index 29dde20..9a065f6 100644
> --- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> +++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> @@ -267,6 +267,8 @@ bitmap_ipmac_uadt(struct ip_set *set, struct nlattr *tb[],
>
>   	e.id = ip_to_id(map, ip);
>   	if (tb[IPSET_ATTR_ETHER]) {
> +		if (nla_len(tb[IPSET_ATTR_ETHER]) != ETH_ALEN)
> +			return -IPSET_ERR_PROTOCOL;

Any reason to not just remove the 'NLA_BINARY' from the policy?

include/net/netlink.h +191:

  * Meaning of `len' field:
[...]
  *    NLA_BINARY           Maximum length of attribute payload
[...]
  *    All other            Minimum length of attribute payload

validate_nla() will just do the right thing and check for minlen.

Thanks,
Daniel

>   		memcpy(e.ether, nla_data(tb[IPSET_ATTR_ETHER]), ETH_ALEN);
>   		e.add_mac = 1;
>   	}
> diff --git a/net/netfilter/ipset/ip_set_hash_mac.c b/net/netfilter/ipset/ip_set_hash_mac.c
> index f1e7d2c..8f004ed 100644
> --- a/net/netfilter/ipset/ip_set_hash_mac.c
> +++ b/net/netfilter/ipset/ip_set_hash_mac.c
> @@ -110,7 +110,8 @@ hash_mac4_uadt(struct ip_set *set, struct nlattr *tb[],
>   	if (tb[IPSET_ATTR_LINENO])
>   		*lineno = nla_get_u32(tb[IPSET_ATTR_LINENO]);
>
> -	if (unlikely(!tb[IPSET_ATTR_ETHER]))
> +	if (unlikely(!tb[IPSET_ATTR_ETHER] ||
> +		     nla_len(tb[IPSET_ATTR_ETHER]) != ETH_ALEN))
>   		return -IPSET_ERR_PROTOCOL;
>
>   	ret = ip_set_get_extensions(set, tb, &ext);
>

--
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

Hi Daniel,

On Tue, 8 Mar 2016, Daniel Borkmann wrote:

> On 03/08/2016 08:44 PM, Jozsef Kadlecsik wrote:
> > Julia Lawall pointed out that IPSET_ATTR_ETHER netlink attribute length
> > was not checked explicitly, just for the maximum possible size. Malicious
> > netlink clients could send shorter attribute and thus resulting a kernel
> > read after the buffer.
> > 
> > The patch adds the explicit length checkings.
> > 
> > Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> > Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
> > ---
> >   net/netfilter/ipset/ip_set_bitmap_ipmac.c | 2 ++
> >   net/netfilter/ipset/ip_set_hash_mac.c     | 3 ++-
> >   2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > index 29dde20..9a065f6 100644
> > --- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > +++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> > @@ -267,6 +267,8 @@ bitmap_ipmac_uadt(struct ip_set *set, struct nlattr
> > *tb[],
> > 
> >   	e.id = ip_to_id(map, ip);
> >   	if (tb[IPSET_ATTR_ETHER]) {
> > +		if (nla_len(tb[IPSET_ATTR_ETHER]) != ETH_ALEN)
> > +			return -IPSET_ERR_PROTOCOL;
> 
> Any reason to not just remove the 'NLA_BINARY' from the policy?
> 
> include/net/netlink.h +191:
> 
>  * Meaning of `len' field:
> [...]
>  *    NLA_BINARY           Maximum length of attribute payload
> [...]
>  *    All other            Minimum length of attribute payload
> 
> validate_nla() will just do the right thing and check for minlen.

The exact payload length must be checked anyway, because we copy the 
attribute into a buffer[ETH_ALEN]. So that'd mean a bigger patch :-)

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary
--
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