Re: [PATCH nf] netfilter: nf_tables: Ensure u8 attributes are loaded from u32 within the bounds

[Laura Garcia <nevola@gmail.com>]

On Thu, Sep 22, 2016 at 04:58:36PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Sep 14, 2016 at 03:00:02PM +0200, Laura Garcia Liebana wrote:
> > Check storage of u32 netlink attributes in smaller resources. This
> > validation is usually required when the u32 netlink attributes are being
> > stored in a private structure size of u8 in the kernel.
> 
> Applied with changes, no need to resend. If I break anything, just
> follow up on top.
> 
> I have rewritten this description and the documentation on the code a bit.
> 
> More changes:
> 
> > 4da449a ("netfilter: nft_exthdr: Add size check on u8 nft_exthdr
> > attributes")
> 
> Always use 12 bytes commit-ids. 4da449a is too short, given the number
> of changes we're getting in the kernel tree, this may become ambiguous
> at some point so it won't be unique.
> 
> You can achieve this via: git log --oneline --abbrev=12
> 
> More comments below.
> 
> > Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > Signed-off-by: Laura Garcia Liebana <nevola@gmail.com>
> > ---
> >  include/net/netfilter/nf_tables.h |  2 ++
> >  net/netfilter/nf_tables_api.c     | 26 ++++++++++++++++++++++++++
> >  net/netfilter/nft_bitwise.c       | 10 +++++++++-
> >  net/netfilter/nft_byteorder.c     | 17 +++++++++++++++--
> >  net/netfilter/nft_cmp.c           |  6 +++++-
> >  net/netfilter/nft_exthdr.c        | 19 ++++++++++++-------
> >  net/netfilter/nft_immediate.c     |  4 ++++
> >  7 files changed, 73 insertions(+), 11 deletions(-)
> > 
> > diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> > index f2f1339..608130f 100644
> > --- a/include/net/netfilter/nf_tables.h
> > +++ b/include/net/netfilter/nf_tables.h
> > @@ -127,6 +127,8 @@ static inline enum nft_registers nft_type_to_reg(enum nft_data_types type)
> >  	return type == NFT_DATA_VERDICT ? NFT_REG_VERDICT : NFT_REG_1 * NFT_REG_SIZE / NFT_REG32_SIZE;
> >  }
> >  
> > +unsigned int nft_parse_u32_check(const struct nlattr *attr, int maxlen,
> > +				 u32 *dest);
> 
> I have renamed maxlen to max, so this fits in one line.
> 
> >  unsigned int nft_parse_register(const struct nlattr *attr);
> >  int nft_dump_register(struct sk_buff *skb, unsigned int attr, unsigned int reg);
> >  
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 7e1c876..d7b000f 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -4343,6 +4343,32 @@ static int nf_tables_check_loops(const struct nft_ctx *ctx,
> >  }
> >  
> >  /**
> > + *	nft_parse_u32_check - parse a u32 value to store the value into a
> > + *			      smaller resource.
> > + *
> > + *	@attr: netlink attribute
> > + *	@maxlen: maximum value to be stored in dest
> > + *	@dest: pointer to the resource
> > + *
> > + *	Parse and store a given u32 value into a resource. Returns an error
> > + *	ERANGE if the value will overload the maxlen, otherwise a 0 will be
> > + *	returned and the value is stored into dest.
> 
> I've rewritten this a bit.
> 
> > + */
> > +unsigned int nft_parse_u32_check(const struct nlattr *attr, int maxlen,
> 
> I have rename maxlen to max. Probably maxval would have been better,
> anyway.
> 
> > +				 u32 *dest)
> > +{
> > +	int reg;
> 
> Variable names are important, they provide context when reading the
> code. Look, from the 'reg' name it seems we're fetching a register.
> However, we are obtaining a value, so I renamed this to val. Try to
> select the right name next time.
> 
> > +
> > +	reg = ntohl(nla_get_be32(attr));
> > +	if (reg > maxlen)
> > +		return -ERANGE;
> > +
> > +	*dest = reg;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(nft_parse_u32_check);
> > +
> > +/**
> >   *	nft_parse_register - parse a register value from a netlink attribute
> >   *
> >   *	@attr: netlink attribute
> > diff --git a/net/netfilter/nft_bitwise.c b/net/netfilter/nft_bitwise.c
> > index d71cc18..e7f23a1 100644
> > --- a/net/netfilter/nft_bitwise.c
> > +++ b/net/netfilter/nft_bitwise.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/netlink.h>
> >  #include <linux/netfilter.h>
> >  #include <linux/netfilter/nf_tables.h>
> > +#include <linux/static_key.h>
> >  #include <net/netfilter/nf_tables_core.h>
> >  #include <net/netfilter/nf_tables.h>
> >  
> > @@ -52,6 +53,7 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
> >  {
> >  	struct nft_bitwise *priv = nft_expr_priv(expr);
> >  	struct nft_data_desc d1, d2;
> > +	u32 len;
> >  	int err;
> >  
> >  	if (tb[NFTA_BITWISE_SREG] == NULL ||
> > @@ -61,7 +63,13 @@ static int nft_bitwise_init(const struct nft_ctx *ctx,
> >  	    tb[NFTA_BITWISE_XOR] == NULL)
> >  		return -EINVAL;
> >  
> > -	priv->len  = ntohl(nla_get_be32(tb[NFTA_BITWISE_LEN]));
> > +	err  = nft_parse_u32_check(tb[NFTA_BITWISE_LEN], U8_MAX,
> > +				   &len);
>                                    ^^^^
> 
> This line break was unnecessary. We can have lines 80-chars long, and
> for some reason you broken the line before the limit. I have repaired
> this.
> 
> > +	if (err < 0)
> > +		return err;
> > +
> > +	priv->len = len;
> > +
> >  	priv->sreg = nft_parse_register(tb[NFTA_BITWISE_SREG]);
> >  	err = nft_validate_register_load(priv->sreg, priv->len);
> >  	if (err < 0)
> > diff --git a/net/netfilter/nft_byteorder.c b/net/netfilter/nft_byteorder.c
> > index b78c28b..8cac219 100644
> > --- a/net/netfilter/nft_byteorder.c
> > +++ b/net/netfilter/nft_byteorder.c
> > @@ -99,6 +99,7 @@ static int nft_byteorder_init(const struct nft_ctx *ctx,
> >  			      const struct nlattr * const tb[])
> >  {
> >  	struct nft_byteorder *priv = nft_expr_priv(expr);
> > +	u32 size, len;
> >  	int err;
> >  
> >  	if (tb[NFTA_BYTEORDER_SREG] == NULL ||
> > @@ -117,7 +118,13 @@ static int nft_byteorder_init(const struct nft_ctx *ctx,
> >  		return -EINVAL;
> >  	}
> >  
> > -	priv->size = ntohl(nla_get_be32(tb[NFTA_BYTEORDER_SIZE]));
> > +	err = nft_parse_u32_check(tb[NFTA_BYTEORDER_SIZE], U8_MAX,
> > +				  &size);
> 
> Same here, and everywhere else.
> 
> > +	if (err < 0)
> > +		return err;
> > +
> > +	priv->size = size;
> > +
> >  	switch (priv->size) {
> >  	case 2:
> >  	case 4:
> > @@ -128,7 +135,13 @@ static int nft_byteorder_init(const struct nft_ctx *ctx,
> >  	}
> >  
> >  	priv->sreg = nft_parse_register(tb[NFTA_BYTEORDER_SREG]);
> > -	priv->len  = ntohl(nla_get_be32(tb[NFTA_BYTEORDER_LEN]));
> > +	err = nft_parse_u32_check(tb[NFTA_BYTEORDER_LEN], U8_MAX,
> > +				  &len);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	priv->len = len;
> > +
> >  	err = nft_validate_register_load(priv->sreg, priv->len);
> >  	if (err < 0)
> >  		return err;
> > diff --git a/net/netfilter/nft_cmp.c b/net/netfilter/nft_cmp.c
> > index e25b35d..3d31432 100644
> > --- a/net/netfilter/nft_cmp.c
> > +++ b/net/netfilter/nft_cmp.c
> > @@ -84,8 +84,12 @@ static int nft_cmp_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
> >  	if (err < 0)
> >  		return err;
> >  
> > -	priv->op  = ntohl(nla_get_be32(tb[NFTA_CMP_OP]));
> > +	if (desc.len > U8_MAX)
> > +		return -ERANGE;
> >  	priv->len = desc.len;
> > +
> > +	priv->op  = ntohl(nla_get_be32(tb[NFTA_CMP_OP]));
> 
> Look, this line has moved for no reason. This just adds noise to the
> patch, so modify only the code that you strictly need, to get it
> smaller.

The reason of this movement was to return the error as soon as possible.