netfilter buglog - May 2023 - [Bug 1685] New: Calling the nftnl_set_free function may trigger the "double free" problem.

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

            Bug ID: 1685
           Summary: Calling the nftnl_set_free function may trigger the
                    "double free" problem.
           Product: libnftnl
           Version: unspecified
          Hardware: All
                OS: All
            Status: NEW
          Severity: critical
          Priority: P5
         Component: libnftnl
          Assignee: pablo at netfilter.org
          Reporter: vchanger123456 at 163.com

If users call the nftnl_set_unset function to release data and then call the
nftnl_set_free, this problem occurs.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230531/e90f2e7d/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

--- Comment #1 from Chen Zhen <vchanger123456 at 163.com> ---
Reproduction code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <netinet/in.h>
#include <linux/netfilter/nf_tables.h>

#include <libnftnl/set.h>

int main(int argc, char *argv[])
{
    struct nftnl_set *a = NULL;
        struct nftnl_expr *ex;
    char buf[4096];
    struct nlmsghdr *nlh;

    a = nftnl_set_alloc();
        ex = nftnl_expr_alloc("meta");
    if (a == NULL || ex == NULL)
        print_err("OOM");

    nftnl_expr_set_u32(ex, NFTNL_EXPR_META_KEY, 0x1234568);
    nftnl_expr_set_u32(ex, NFTNL_EXPR_META_DREG, 0x78123456);

    nftnl_set_set_str(a, NFTNL_SET_TABLE, "test-table");
    nftnl_set_set_str(a, NFTNL_SET_NAME, "test-name");
    nftnl_set_set_u32(a, NFTNL_SET_FLAGS, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_KEY_TYPE, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_KEY_LEN, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_DATA_TYPE, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_DATA_LEN, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_FAMILY, 0x12345678);
    nftnl_set_set_str(a, NFTNL_SET_USERDATA, "testing user data");
    nftnl_set_set(a, NFTNL_SET_EXPR, ex);

    nftnl_set_unset(a, NFTNL_SET_EXPR);
    nftnl_set_free(a);

    printf("ok");
    return 0;
}


Code from:
https://git.netfilter.org/libnftnl/tree/tests/nft-set-test.c
https://git.netfilter.org/libnftnl/tree/tests/nft-expr_meta-test.c

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230531/9d7da748/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

--- Comment #2 from Chen Zhen <vchanger123456 at 163.com> ---
Sorry, there is a problem with the above code. Updated:

Reproduction code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <netinet/in.h>
#include <linux/netfilter/nf_tables.h>

#include <libnftnl/set.h>
#include <libnftnl/expr.h>

int main(int argc, char *argv[])
{
    struct nftnl_set *a = NULL;
        struct nftnl_expr *ex;

    a = nftnl_set_alloc();
        ex = nftnl_expr_alloc("meta");
    if (a == NULL || ex == NULL)
        printf("OOM\n");

    nftnl_expr_set_u32(ex, NFTNL_EXPR_META_KEY, 0x1234568);
    nftnl_expr_set_u32(ex, NFTNL_EXPR_META_DREG, 0x78123456);

    nftnl_set_set_str(a, NFTNL_SET_TABLE, "test-table");
    nftnl_set_set_str(a, NFTNL_SET_NAME, "test-name");
    nftnl_set_set_u32(a, NFTNL_SET_FLAGS, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_KEY_TYPE, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_KEY_LEN, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_DATA_TYPE, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_DATA_LEN, 0x12345678);
    nftnl_set_set_u32(a, NFTNL_SET_FAMILY, 0x12345678);
    nftnl_set_set_str(a, NFTNL_SET_USERDATA, "testing user data");
    nftnl_set_set(a, NFTNL_SET_EXPR, ex);

    nftnl_set_unset(a, NFTNL_SET_EXPR);
    nftnl_set_free(a);

    printf("ok\n");
    return 0;
}

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230531/60a2fef4/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

Phil Sutter <phil at nwl.cc> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |phil at nwl.cc

--- Comment #3 from Phil Sutter <phil at nwl.cc> ---
Fix submitted upstream:
https://lore.kernel.org/netfilter-devel/20230531123256.4882-1-phil at nwl.cc/

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230531/603aad08/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

--- Comment #4 from Chen Zhen <vchanger123456 at 163.com> ---
Thank you very much for your reply and the provided patch. This patch is valid
and I have verified it.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230531/bd229404/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

--- Comment #5 from Chen Zhen <vchanger123456 at 163.com> ---
Is there a problem with this patch? I have verified it by the reproduction code
above.

>From 325df1f49bb273177a9f47f60ea9baa4f3f3197d Mon Sep 17 00:00:00 2001
From: sxt1001 <sxt1001 at qq.com>
Date: Wed, 31 May 2023 21:01:47 +0800
Subject: [PATCH] Fix double free

---
 src/set.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/set.c b/src/set.c
index c46f827..738cc24 100644
--- a/src/set.c
+++ b/src/set.c
@@ -54,8 +54,11 @@ void nftnl_set_free(const struct nftnl_set *s)
        if (s->flags & (1 << NFTNL_SET_USERDATA))
                xfree(s->user.data);

-       list_for_each_entry_safe(expr, next, &s->expr_list, head)
-               nftnl_expr_free(expr);
+       if (s->flags & (1 << NFTNL_SET_EXPR))
+       {
+               list_for_each_entry_safe(expr, next, &s->expr_list, head)
+                       nftnl_expr_free(expr);
+       }

        list_for_each_entry_safe(elem, tmp, &s->element_list, head) {
                list_del(&elem->head);
--
2.33.0

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230531/95d58288/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

--- Comment #6 from Phil Sutter <phil at nwl.cc> ---
(In reply to Chen Zhen from comment #5)
> Is there a problem with this patch? I have verified it by the reproduction
> code above.
> 
> 
> From 325df1f49bb273177a9f47f60ea9baa4f3f3197d Mon Sep 17 00:00:00 2001
> From: sxt1001 <sxt1001 at qq.com>
> Date: Wed, 31 May 2023 21:01:47 +0800
> Subject: [PATCH] Fix double free
> 
> ---
>  src/set.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/set.c b/src/set.c
> index c46f827..738cc24 100644
> --- a/src/set.c
> +++ b/src/set.c
> @@ -54,8 +54,11 @@ void nftnl_set_free(const struct nftnl_set *s)
>         if (s->flags & (1 << NFTNL_SET_USERDATA))
>                 xfree(s->user.data);
> 
> -       list_for_each_entry_safe(expr, next, &s->expr_list, head)
> -               nftnl_expr_free(expr);
> +       if (s->flags & (1 << NFTNL_SET_EXPR))
> +       {
> +               list_for_each_entry_safe(expr, next, &s->expr_list,
head)
> +                       nftnl_expr_free(expr);
> +       }
There are more places where elements are freed but not removed from list. It is
safer to not leave the list in such state but instead make sure things stay
consistent. See the code dealing with element_list which does it.

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230531/88c49a28/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

--- Comment #7 from Pablo Neira Ayuso <pablo at netfilter.org>
---
> diff --git a/src/set.c b/src/set.c
> index c46f827..738cc24 100644
> --- a/src/set.c
> +++ b/src/set.c
> @@ -54,8 +54,11 @@ void nftnl_set_free(const struct nftnl_set *s)
>         if (s->flags & (1 << NFTNL_SET_USERDATA))
>                 xfree(s->user.data);
> 
> -       list_for_each_entry_safe(expr, next, &s->expr_list, head)
> -               nftnl_expr_free(expr);
> +       if (s->flags & (1 << NFTNL_SET_EXPR))
> +       {
> +               list_for_each_entry_safe(expr, next, &s->expr_list,
head)
> +                       nftnl_expr_free(expr);
> +       }
Maybe this instead?

          list_for_each_entry_safe(expr, next, &s->expr_list, head) {
                  list_del(&expr->list);
                  nftnl_expr_free(expr);
          }

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230601/59c940b0/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

Pablo Neira Ayuso <pablo at netfilter.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Pablo Neira Ayuso <pablo at netfilter.org> ---
Phil already fix it here:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230531123256.4882-1-phil
at nwl.cc/

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230601/0eddda81/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

--- Comment #9 from Chen Zhen <vchanger123456 at 163.com> ---
(In reply to Pablo Neira Ayuso from comment #8)
> Phil already fix it here:
> 
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230531123256.
> 4882-1-phil at nwl.cc/
Yes, this patch can fix the problem, but do I think we should add the if
(s->flags & (1 << NFTNL_SET_EXPR)) judgment to the nftnl_set_free
function?

-    list_for_each_entry_safe(expr, next, &s->expr_list, head)
+       if (s->flags & (1 << NFTNL_SET_EXPR)){
+        list_for_each_entry_safe(expr, next, &s->expr_list, head) {
+        list_del(&expr->head);
         nftnl_expr_free(expr);
+        }
+       }

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230602/82a77781/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

--- Comment #10 from Pablo Neira Ayuso <pablo at netfilter.org> ---
(In reply to Chen Zhen from comment #9)
> (In reply to Pablo Neira Ayuso from comment #8)
> > Phil already fix it here:
> > 
> >
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20230531123256.
> > 4882-1-phil at nwl.cc/
> 
> Yes, this patch can fix the problem, but do I think we should add the if
> (s->flags & (1 << NFTNL_SET_EXPR)) judgment to the
nftnl_set_free function?
There is NFTNL_SET_EXPRESSIONS that uses this list too.

List would be empty (but correctly initialized) if NFTNL_SET_EXPR or
NFTNL_SET_EXPRESSIONS are unset.

I believe this is sufficient.
> -	list_for_each_entry_safe(expr, next, &s->expr_list, head)
> +       if (s->flags & (1 << NFTNL_SET_EXPR)){
> +	    list_for_each_entry_safe(expr, next, &s->expr_list, head) {
> +		list_del(&expr->head);
>  		nftnl_expr_free(expr);
> +	    }
> +       }
-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230602/dd1ecf6a/attachment.html>

https://bugzilla.netfilter.org/show_bug.cgi?id=1685

--- Comment #11 from Pablo Neira Ayuso <pablo at netfilter.org> ---
Upstream commit:

http://git.netfilter.org/libnftnl/commit/?id=2d83a7d4f58fbf6eaa9aeace49c78d91a86a3b28

-- 
You are receiving this mail because:
You are watching all bug changes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.netfilter.org/pipermail/netfilter-buglog/attachments/20230602/5bf7baaa/attachment.html>