Re: [libnftnl PATCH 2/2 v2] Avoid out of bounds read from data

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Maya,

On Thu, Feb 18, 2021 at 01:06:38PM +0200, Maya Rashish wrote:
> When data is smaller than the destination, &ctr->pkts.
> 
> This might introduce some issues since we're now not
> filling the rest of the memory, but filling out with
> uninitialized garbage is probably as bad as leaving it
> as garbage.

Probably you could update src/expr/ to use nftnl_assert_validate() to
sanity check the input data length?

Please, have a look at nftnl_assert_attr_exists() and
nftnl_assert_validate().

> Signed-off-by: Maya Rashish <mrashish@redhat.com>
> ---
>  include/utils.h    | 2 ++
>  src/expr/counter.c | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/utils.h b/include/utils.h
> index 8af5a8e..6b22e46 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -67,6 +67,8 @@ void __nftnl_assert_attr_exists(uint16_t attr, uint16_t attr_max,
> 
>  #define array_size(arr)		(sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
> 
> +#define	MIN(a,b)		((a) > (b) ? (b) : (a))
> +
>  const char *nftnl_family2str(uint32_t family);
>  int nftnl_str2family(const char *family);
> 
> diff --git a/src/expr/counter.c b/src/expr/counter.c
> index 89a602e..fb036dd 100644
> --- a/src/expr/counter.c
> +++ b/src/expr/counter.c
> @@ -35,10 +35,10 @@ nftnl_expr_counter_set(struct nftnl_expr *e, uint16_t type,
> 
>  	switch(type) {
>  	case NFTNL_EXPR_CTR_BYTES:
> -		memcpy(&ctr->bytes, data, sizeof(ctr->bytes));
> +		memcpy(&ctr->bytes, data, MIN(data_len, sizeof(ctr->bytes)));
>  		break;
>  	case NFTNL_EXPR_CTR_PACKETS:
> -		memcpy(&ctr->pkts, data, sizeof(ctr->pkts));
> +		memcpy(&ctr->pkts, data, MIN(data_len, sizeof(ctr->pkts)));
>  		break;
>  	default:
>  		return -1;
> -- 
> 2.29.2
>