Bug 255859

Summary: ipfilter/netinent: ip_nat memory leak and use-after-free
Product: Base System Reporter: lylgood
Component: kernAssignee: Cy Schubert <cy>
Status: Closed FIXED    
Severity: Affects Many People CC: cy, markj, net
Priority: --- Keywords: needs-qa
Version: CURRENTFlags: koobs: mfc-stable13?
koobs: mfc-stable12?
koobs: mfc-stable11?
Hardware: Any   
OS: Any   
Attachments:
Description Flags
correct in_tqehead index number none

Description lylgood 2021-05-14 08:26:38 UTC
Created attachment 224922 [details]
correct in_tqehead index number

Bug File: contrib/ipfilter/netinet/ip_nat.c

In function ipf_nat_rule_deref, if (n->in_tqehead[0] != NULL) is true,
n->in_tqehead[1] will be freed in ipf_freetimeoutqueue() via KFREE().
But the freed pointer n->in_tqehead[1] is still used in later
ipf_deletetimeoutqueue(n->in_tqehead[1]), which is a use after free bug.

According the around code pattern, i think this bug is caused by mistyping.
My patch correct the index number of n->in_tqehead, if (n->in_tqehead[0] != NULL) is true.
Comment 1 lylgood 2021-05-14 08:29:20 UTC
Comment on attachment 224922 [details]
correct in_tqehead index number

>diff --git a/contrib/ipfilter/netinet/ip_nat.c b/contrib/ipfilter/netinet/ip_nat.c.orig
>index 0475a4386079..41e51880b3dd 100644
>--- a/contrib/ipfilter/netinet/ip_nat.c
>+++ b/contrib/ipfilter/netinet/ip_nat.c.orig
>@@ -6245,7 +6245,7 @@ ipf_nat_rule_deref(softc, inp)
> 
> 	if (n->in_tqehead[0] != NULL) {
> 		if (ipf_deletetimeoutqueue(n->in_tqehead[0]) == 0) {
>+			ipf_freetimeoutqueue(softc, n->in_tqehead[0]);
>-			ipf_freetimeoutqueue(softc, n->in_tqehead[1]);
> 		}
> 	}
>
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2021-05-25 15:36:07 UTC
Cy, could you take a look at this?
Comment 3 Cy Schubert freebsd_committer freebsd_triage 2021-05-25 18:02:03 UTC
There is no such file in contrib/ipfilter. Look in sys/contrib/ipfilter/netinet/ip_nat.c, line 6248.

slippy$ pwd
/home/cy/freebsd/git/src/sys/contrib/ipfilter/netinet
slippy$ cd ../../..
slippy$ patch -C -p1 < /tmp/attachment.cgi
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|--- b/contrib/ipfilter/netinet/ip_nat.c.orig	
|+++ b/contrib/ipfilter/netinet/ip_nat.c
--------------------------
Patching file contrib/ipfilter/netinet/ip_nat.c using Plan A...
Reversed (or previously applied) patch detected!  Assume -R? [y] ^C%                                                                                                          slippy$ 

The patch is already applied.
Comment 4 Cy Schubert freebsd_committer freebsd_triage 2021-05-25 18:02:52 UTC
(In reply to lylgood from comment #1)
So, you're saying the reverse should the correct patch.

I'll take a look at this and compare notes with NetBSD.
Comment 5 Cy Schubert freebsd_committer freebsd_triage 2021-05-25 18:37:57 UTC
This is indeed a memory leak, though not a use after free. The bug also exists in NetBSD.
Comment 6 commit-hook freebsd_committer freebsd_triage 2021-05-25 18:59:51 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=323a4e2c4e285e6f8eee8db3fe2cb7490a734da0

commit 323a4e2c4e285e6f8eee8db3fe2cb7490a734da0
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2021-05-25 18:54:49 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2021-05-25 18:58:14 +0000

    ipfilter: Fix ip_nat memory leak and use-after-free

    Unfortunately the wrong elemet is freed, also resulting in use-after-free.

    PR:             255859
    Submitted by:   lylgood@foxmail.com
    Reported by:    lylgood@foxmail.com
    MFC after:      3 days

 sys/contrib/ipfilter/netinet/ip_nat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 7 commit-hook freebsd_committer freebsd_triage 2021-06-03 00:56:47 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=2fb377976493cd961dfe1908d1c565742e79bb4a

commit 2fb377976493cd961dfe1908d1c565742e79bb4a
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2021-05-25 18:54:49 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2021-06-03 00:54:30 +0000

    ipfilter: Fix ip_nat memory leak and use-after-free

    Unfortunately the wrong elemet is freed, also resulting in use-after-free.

    PR:             255859
    Submitted by:   lylgood@foxmail.com
    Reported by:    lylgood@foxmail.com

    (cherry picked from commit 323a4e2c4e285e6f8eee8db3fe2cb7490a734da0)

 sys/contrib/ipfilter/netinet/ip_nat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Comment 8 commit-hook freebsd_committer freebsd_triage 2021-06-03 00:58:49 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=c8773c8018e74a34a5d9e7ec6d66f4311148f975

commit c8773c8018e74a34a5d9e7ec6d66f4311148f975
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2021-05-25 18:54:49 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2021-06-03 00:57:45 +0000

    ipfilter: Fix ip_nat memory leak and use-after-free

    Unfortunately the wrong elemet is freed, also resulting in use-after-free.

    PR:             255859
    Submitted by:   lylgood@foxmail.com
    Reported by:    lylgood@foxmail.com

    (cherry picked from commit 323a4e2c4e285e6f8eee8db3fe2cb7490a734da0)

 sys/contrib/ipfilter/netinet/ip_nat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)