[PATCH 1/3] main: fix ASAN -fsanizize=address error

[PATCH 1/3] main: fix ASAN -fsanizize=address error

[Michael Braun]

Signed-of-by: Michael Braun <michael-dev@fami-braun.de>

nft list table bridge t
=================================================================
==28552==ERROR: AddressSanitizer: global-buffer-overflow on address 0x5579c662e816 at pc 0x7fc2803246aa bp 0x7fff495c86f0 sp 0x7fff495c7ea0
WRITE of size 2 at 0x5579c662e816 thread T0
    #0 0x7fc2803246a9 in vsprintf (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x546a9)
    #1 0x7fc2803249f6 in __interceptor_sprintf (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x549f6)
    #2 0x5579c661e7d2 in get_optstring nftables/src/main.c:128
    #3 0x5579c66202af in main nftables/src/main.c:315
    #4 0x7fc27ea7b09a in __libc_start_main ../csu/libc-start.c:308
    #5 0x5579c661e439 in _start (nftables/src/.libs/nft+0x9439)

0x5579c662e816 is located 0 bytes to the right of global variable 'optstring' defined in 'main.c:121:14' (0x5579c662e800) of size 22
0x5579c662e816 is located 42 bytes to the left of global variable 'options' defined in 'main.c:137:23' (0x5579c662e840) of size 672
SUMMARY: AddressSanitizer: global-buffer-overflow (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x546a9) in vsprintf
Shadow bytes around the buggy address:
  0x0aafb8cbdcb0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0aafb8cbdcc0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0aafb8cbdcd0: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x0aafb8cbdce0: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x0aafb8cbdcf0: 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9
=>0x0aafb8cbdd00: 00 00[06]f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0aafb8cbdd10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0aafb8cbdd20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0aafb8cbdd30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0aafb8cbdd40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0aafb8cbdd50: 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9
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
==28552==ABORTING
---
 src/main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/main.c b/src/main.c
index 3dc6b62c..d213c601 100644
--- a/src/main.c
+++ b/src/main.c
@@ -124,10 +124,10 @@ static const char *get_optstring(void)
 		size_t i, j;
 
 		optstring[0] = '+';
-		for (i = 0, j = 1; i < NR_NFT_OPTIONS; i++)
-			j += sprintf(optstring + j, "%c%s",
-				     nft_options[i].val,
-				     nft_options[i].arg ? ":" : "");
+		for (i = 0, j = 1; i < NR_NFT_OPTIONS && j < sizeof(optstring); i++)
+			j += snprintf(optstring + j, sizeof(optstring) - j, "%c%s",
+				      nft_options[i].val,
+				      nft_options[i].arg ? ":" : "");
 	}
 	return optstring;
 }
-- 
2.20.1

[PATCH 2/3] utils: fix UBSAN warning in fls

[Michael Braun]

../include/utils.h:120:5: runtime error: left shift of 1103101952 by 1 places cannot be represented in type 'int'

Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
---
 include/utils.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/utils.h b/include/utils.h
index 647e8bbe..f45f2513 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -94,7 +94,7 @@
  * This is defined the same way as ffs.
  * Note fls(0) = 0, fls(1) = 1, fls(0x80000000) = 32.
  */
-static inline int fls(int x)
+static inline int fls(uint32_t x)
 {
 	int r = 32;
 
-- 
2.20.1

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

[Michael Braun]

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
 	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);
 	concat_expr->len = dtype->size;
 
 	return concat_expr;
-- 
2.20.1

Re: [PATCH 1/3] main: fix ASAN -fsanizize=address error

[Pablo Neira Ayuso]

Applied, thanks.

Re: [PATCH 2/3] utils: fix UBSAN warning in fls

[Pablo Neira Ayuso]

On Fri, May 01, 2020 at 05:48:17PM +0200, Michael Braun wrote:
> ../include/utils.h:120:5: runtime error: left shift of 1103101952 by 1 places cannot be represented in type 'int'

Also applied, thanks.

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

[Pablo Neira Ayuso]

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.

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

[michael-dev]

Am 01.05.2020 21:27, schrieb Pablo Neira Ayuso:
> On Fri, May 01, 2020 at 05:48:18PM +0200, Michael Braun wrote:
>> +	if (dtype == expr->dtype)
>> +		return; // do not free dtype before incrementing refcnt again
> 
> The problem you describe (use-after-free) happens in this case, right?

The problem is more likely due to concat_expr_parse_udata not calling 
datatype_get,
because otherwise datatype_get would be in the backtrace of ASAN.

> 
>         datatype_set(expr, expr->dtype);
> 
> Or am I missing anything?

But while debugging the above output, I added assert(dtype != 
expr->dtype) here
and that crashed. So I'm sure something like this is happening.
And the whole thing was nasty to debug, so I added this one just be sure 
it does not happen again.

As ASAN should hit on datatype_get incrementing refcnt if datatype_free 
had actually freed it,
assert was probaby not seeing an DTYPE_F_ALLOC instance.
But I dig not deeper here, as I felt this return is safe to add.

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

This is what caused the ASAN output above to go away.

Regards,
M. Braun

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

[Pablo Neira Ayuso]

On Fri, May 01, 2020 at 09:59:35PM +0200, michael-dev wrote:
> Am 01.05.2020 21:27, schrieb Pablo Neira Ayuso:
> > On Fri, May 01, 2020 at 05:48:18PM +0200, Michael Braun wrote:
> > > +	if (dtype == expr->dtype)
> > > +		return; // do not free dtype before incrementing refcnt again
> > 
> > The problem you describe (use-after-free) happens in this case, right?
> 
> The problem is more likely due to concat_expr_parse_udata not calling
> datatype_get,
> because otherwise datatype_get would be in the backtrace of ASAN.
> 
> > 
> >         datatype_set(expr, expr->dtype);
> > 
> > Or am I missing anything?
> 
> But while debugging the above output, I added assert(dtype != expr->dtype)
> here
> and that crashed. So I'm sure something like this is happening.

Right.

# nft add rule ip x y ct state new,established,related,untracked
# nft list ruleset
nft: datatype.c:1086: datatype_set: Assertion `expr->dtype != dtype' failed.
Aborted

> And the whole thing was nasty to debug, so I added this one just be sure it
> does not happen again.
> 
> As ASAN should hit on datatype_get incrementing refcnt if datatype_free had
> actually freed it,
> assert was probaby not seeing an DTYPE_F_ALLOC instance.
> But I dig not deeper here, as I felt this return is safe to add.

I'm going to apply this. I think it's safe to turn this into noop.