[libnftnl PATCH 1/2] Avoid out of bound reads in tests.

[libnftnl PATCH 1/2] Avoid out of bound reads in tests.

[Maya Rashish]

Our string isn't NUL-terminated. To avoid reading past
the last character, use strndup.

Signed-off-by: Maya Rashish <mrashish@redhat.com>
---
  tests/nft-expr_match-test.c  | 2 +-
  tests/nft-expr_target-test.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/nft-expr_match-test.c b/tests/nft-expr_match-test.c
index 39a49d8..f6b7bc0 100644
--- a/tests/nft-expr_match-test.c
+++ b/tests/nft-expr_match-test.c
@@ -71,7 +71,7 @@ int main(int argc, char *argv[])

  	nftnl_expr_set_str(ex, NFTNL_EXPR_MT_NAME, "Tests");
  	nftnl_expr_set_u32(ex, NFTNL_EXPR_MT_REV, 0x12345678);
-	nftnl_expr_set(ex, NFTNL_EXPR_MT_INFO, strdup(data), sizeof(data));
+	nftnl_expr_set(ex, NFTNL_EXPR_MT_INFO, strndup(data, sizeof(data)), sizeof(data));
  	nftnl_rule_add_expr(a, ex);

  	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, AF_INET, 0, 1234);
diff --git a/tests/nft-expr_target-test.c b/tests/nft-expr_target-test.c
index ba56b27..a135b9c 100644
--- a/tests/nft-expr_target-test.c
+++ b/tests/nft-expr_target-test.c
@@ -71,7 +71,7 @@ int main(int argc, char *argv[])

  	nftnl_expr_set(ex, NFTNL_EXPR_TG_NAME, "test", strlen("test"));
  	nftnl_expr_set_u32(ex, NFTNL_EXPR_TG_REV, 0x56781234);
-	nftnl_expr_set(ex, NFTNL_EXPR_TG_INFO, strdup(data), sizeof(data));
+	nftnl_expr_set(ex, NFTNL_EXPR_TG_INFO, strndup(data, sizeof(data)), sizeof(data));
  	nftnl_rule_add_expr(a, ex);

  	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, AF_INET, 0, 1234);
-- 
2.29.2

Re: [libnftnl PATCH 1/2] Avoid out of bound reads in tests.

[Pablo Neira Ayuso]

Hi Maya,

On Wed, Feb 17, 2021 at 10:45:45PM +0200, Maya Rashish wrote:
> Our string isn't NUL-terminated. To avoid reading past
> the last character, use strndup.

Is this a theoretical problem or some static analisys tool is
reporting out-of-bound memread?

> Signed-off-by: Maya Rashish <mrashish@redhat.com>
> ---
>  tests/nft-expr_match-test.c  | 2 +-
>  tests/nft-expr_target-test.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/nft-expr_match-test.c b/tests/nft-expr_match-test.c
> index 39a49d8..f6b7bc0 100644
> --- a/tests/nft-expr_match-test.c
> +++ b/tests/nft-expr_match-test.c
> @@ -71,7 +71,7 @@ int main(int argc, char *argv[])
> 
>  	nftnl_expr_set_str(ex, NFTNL_EXPR_MT_NAME, "Tests");
>  	nftnl_expr_set_u32(ex, NFTNL_EXPR_MT_REV, 0x12345678);
> -	nftnl_expr_set(ex, NFTNL_EXPR_MT_INFO, strdup(data), sizeof(data));
> +	nftnl_expr_set(ex, NFTNL_EXPR_MT_INFO, strndup(data, sizeof(data)), sizeof(data));
>  	nftnl_rule_add_expr(a, ex);
> 
>  	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, AF_INET, 0, 1234);
> diff --git a/tests/nft-expr_target-test.c b/tests/nft-expr_target-test.c
> index ba56b27..a135b9c 100644
> --- a/tests/nft-expr_target-test.c
> +++ b/tests/nft-expr_target-test.c
> @@ -71,7 +71,7 @@ int main(int argc, char *argv[])
> 
>  	nftnl_expr_set(ex, NFTNL_EXPR_TG_NAME, "test", strlen("test"));
>  	nftnl_expr_set_u32(ex, NFTNL_EXPR_TG_REV, 0x56781234);
> -	nftnl_expr_set(ex, NFTNL_EXPR_TG_INFO, strdup(data), sizeof(data));
> +	nftnl_expr_set(ex, NFTNL_EXPR_TG_INFO, strndup(data, sizeof(data)), sizeof(data));
>  	nftnl_rule_add_expr(a, ex);
> 
>  	nlh = nftnl_rule_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE, AF_INET, 0, 1234);
> -- 
> 2.29.2
> 

Re: [libnftnl PATCH 1/2] Avoid out of bound reads in tests.

[Maya Rashish]

Hi Pablo,

On 18/02/21 1:00 am, Pablo Neira Ayuso wrote:
> Hi Maya,
> 
> On Wed, Feb 17, 2021 at 10:45:45PM +0200, Maya Rashish wrote:
>> Our string isn't NUL-terminated. To avoid reading past
>> the last character, use strndup.
> 
> Is this a theoretical problem or some static analisys tool is
> reporting out-of-bound memread?

As background, I had a difficult to diagnose stack corruption
with a patched older version. I was hoping it'd just show up
by running the tests with address sanitizer (I edited the
Makefiles to add CFLAGS=-fsanitize=address and LDFLAGS=-lasan
after configure) but it didn't.

Address sanitizer usually reports actual problems, it runs the
actual code with some elaborate memory map tricks that lets it
detect violations.

But might as well make the tests all run without complaints
from address sanitizer while I am doing this.