Re: [PATCH 3/3] datatype: fix double-free resulting in use-after-free in datatype_free

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

On Fri, May 01, 2020 at 05:48:18PM +0200, Michael Braun wrote:
> nft list table bridge t
> table bridge t {
>         set s4 {
>                 typeof ip saddr . ip daddr
>                 elements = { 1.0.0.1 . 2.0.0.2 }
>         }
> }
> =================================================================
> ==24334==ERROR: AddressSanitizer: heap-use-after-free on address 0x6080000000a8 at pc 0x7fe0e67df0ad bp 0x7ffff83e88c0 sp 0x7ffff83e88b8
> READ of size 4 at 0x6080000000a8 thread T0
>     #0 0x7fe0e67df0ac in datatype_free nftables/src/datatype.c:1110
>     #1 0x7fe0e67e2092 in expr_free nftables/src/expression.c:89
>     #2 0x7fe0e67a855e in set_free nftables/src/rule.c:359
>     #3 0x7fe0e67b2f3e in table_free nftables/src/rule.c:1263
>     #4 0x7fe0e67a70ce in __cache_flush nftables/src/rule.c:299
>     #5 0x7fe0e67a71c7 in cache_release nftables/src/rule.c:305
>     #6 0x7fe0e68dbfa9 in nft_ctx_free nftables/src/libnftables.c:292
>     #7 0x55f00fbe0051 in main nftables/src/main.c:469
>     #8 0x7fe0e553309a in __libc_start_main ../csu/libc-start.c:308
>     #9 0x55f00fbdd429 in _start (nftables/src/.libs/nft+0x9429)
> 
> 0x6080000000a8 is located 8 bytes inside of 96-byte region [0x6080000000a0,0x608000000100)
> freed by thread T0 here:
>     #0 0x7fe0e6e70fb0 in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xe8fb0)
>     #1 0x7fe0e68b8122 in xfree nftables/src/utils.c:29
>     #2 0x7fe0e67df2e5 in datatype_free nftables/src/datatype.c:1117
>     #3 0x7fe0e67e2092 in expr_free nftables/src/expression.c:89
>     #4 0x7fe0e67a83fe in set_free nftables/src/rule.c:356
>     #5 0x7fe0e67b2f3e in table_free nftables/src/rule.c:1263
>     #6 0x7fe0e67a70ce in __cache_flush nftables/src/rule.c:299
>     #7 0x7fe0e67a71c7 in cache_release nftables/src/rule.c:305
>     #8 0x7fe0e68dbfa9 in nft_ctx_free nftables/src/libnftables.c:292
>     #9 0x55f00fbe0051 in main nftables/src/main.c:469
>     #10 0x7fe0e553309a in __libc_start_main ../csu/libc-start.c:308
> 
> previously allocated by thread T0 here:
>     #0 0x7fe0e6e71330 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xe9330)
>     #1 0x7fe0e68b813d in xmalloc nftables/src/utils.c:36
>     #2 0x7fe0e68b8296 in xzalloc nftables/src/utils.c:65
>     #3 0x7fe0e67de7d5 in dtype_alloc nftables/src/datatype.c:1065
>     #4 0x7fe0e67df862 in concat_type_alloc nftables/src/datatype.c:1146
>     #5 0x7fe0e67ea852 in concat_expr_parse_udata nftables/src/expression.c:954
>     #6 0x7fe0e685dc94 in set_make_key nftables/src/netlink.c:718
>     #7 0x7fe0e685e177 in netlink_delinearize_set nftables/src/netlink.c:770
>     #8 0x7fe0e685f667 in list_set_cb nftables/src/netlink.c:895
>     #9 0x7fe0e4f95a03 in nftnl_set_list_foreach src/set.c:904
> 
> SUMMARY: AddressSanitizer: heap-use-after-free nftables/src/datatype.c:1110 in datatype_free
> Shadow bytes around the buggy address:
>   0x0c107fff7fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x0c107fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x0c107fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x0c107fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x0c107fff8000: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
> =>0x0c107fff8010: fa fa fa fa fd[fd]fd fd fd fd fd fd fd fd fd fd
>   0x0c107fff8020: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
>   0x0c107fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c107fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c107fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c107fff8060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
> Shadow byte legend (one shadow byte represents 8 application bytes):
>   Addressable:           00
>   Partially addressable: 01 02 03 04 05 06 07
>   Heap left redzone:       fa
>   Freed heap region:       fd
>   Stack left redzone:      f1
>   Stack mid redzone:       f2
>   Stack right redzone:     f3
>   Stack after return:      f5
>   Stack use after scope:   f8
>   Global redzone:          f9
>   Global init order:       f6
>   Poisoned by user:        f7
>   Container overflow:      fc
>   Array cookie:            ac
>   Intra object redzone:    bb
>   ASan internal:           fe
>   Left alloca redzone:     ca
>   Right alloca redzone:    cb
> ==24334==ABORTING
> ---
>  src/datatype.c   | 2 ++
>  src/expression.c | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/datatype.c b/src/datatype.c
> index 095598d9..0110846f 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -1083,6 +1083,8 @@ struct datatype *datatype_get(const struct datatype *ptr)
>  
>  void datatype_set(struct expr *expr, const struct datatype *dtype)
>  {
> +	if (dtype == expr->dtype)
> +		return; // do not free dtype before incrementing refcnt again

This makes sense indeed. If the same dtype is set, then turning this
to noop is fine.

The problem you describe (use-after-free) happens in this case, right?

        datatype_set(expr, expr->dtype);

Or am I missing anything?

>  	datatype_free(expr->dtype);
>  	expr->dtype = datatype_get(dtype);
>  }
> diff --git a/src/expression.c b/src/expression.c
> index 6605beb3..a6bde70f 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -955,7 +955,7 @@ static struct expr *concat_expr_parse_udata(const struct nftnl_udata *attr)
>  	if (!dtype)
>  		goto err_free;
>  
> -	concat_expr->dtype = dtype;
> +	concat_expr->dtype = datatype_get(dtype);

This is also good indeed.

Thanks.