[PATCH 0/1] ipset patch for nf

[PATCH 0/1] ipset patch for nf

[Jozsef Kadlecsik]

Hi Pablo,

Please apply the next patch against the nf tree:

- Julia Lawall pointed out that IPSET_ATTR_ETHER netlink attribute
  length was not checked explicitly. The patch adds the missing
  checkings.

The patch should be applied to the older stable kernel branches too.

Best regards,
Jozsef

The following changes since commit 45040978c8994d1401baf5cc5ac71c1495d4e120:

  netfilter: ipset: Fix set:list type crash when flush/dump set in parallel (2016-02-24 20:32:21 +0100)

are available in the git repository at:

  git://blackhole.kfki.hu/nf master

for you to fetch changes up to d8aacd87180141ff6b812b53de77a4336e87c91a:

  netfilter: ipset: Check IPSET_ATTR_ETHER netlink attribute length (2016-03-08 20:36:17 +0100)

----------------------------------------------------------------
Jozsef Kadlecsik (1):
      netfilter: ipset: Check IPSET_ATTR_ETHER netlink attribute length

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

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

[Jozsef Kadlecsik]

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;
 		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);
-- 
1.8.5.1

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

[Daniel Borkmann]

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

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

[Jozsef Kadlecsik]

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

Re: [PATCH 0/1] ipset patch for nf

[Pablo Neira Ayuso]

On Tue, Mar 08, 2016 at 08:44:19PM +0100, Jozsef Kadlecsik wrote:
> Hi Pablo,
> 
> Please apply the next patch against the nf tree:
> 
> - Julia Lawall pointed out that IPSET_ATTR_ETHER netlink attribute
>   length was not checked explicitly. The patch adds the missing
>   checkings.
> 
> The patch should be applied to the older stable kernel branches too.
> 
> Best regards,
> Jozsef
> 
> The following changes since commit 45040978c8994d1401baf5cc5ac71c1495d4e120:
> 
>   netfilter: ipset: Fix set:list type crash when flush/dump set in parallel (2016-02-24 20:32:21 +0100)
> 
> are available in the git repository at:
> 
>   git://blackhole.kfki.hu/nf master

This patch also came with the previous pull, so this will show up in
the nf-next tree too.

Thanks.