conntrackd: Fix "Address Accept" filter case
diff mbox series

Message ID AM0PR02MB5492D0F9BEB5814637C7D5C3AA1E0@AM0PR02MB5492.eurprd02.prod.outlook.com
State Under Review
Delegated to: Pablo Neira
Headers show
Series
  • conntrackd: Fix "Address Accept" filter case
Related show

Commit Message

Robin Geuze May 28, 2019, 7:03 a.m. UTC
This fixes a bug in the Address Accept filter case where if you only specify either addresses or masks it would never match.

Signed-off-by: Robin Geuze <robing@transip.nl>
---
  src/filter.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Pablo Neira Ayuso May 30, 2019, 2:43 p.m. UTC | #1
On Tue, May 28, 2019 at 07:03:59AM +0000, Robin Geuze wrote:
> This fixes a bug in the Address Accept filter case where if you only
> specify either addresses or masks it would never match.

Thanks Robin.

Would you post an example configuration that is broken? I would like
to place it in the commit message.

> Signed-off-by: Robin Geuze <robing@transip.nl>
> ---
>   src/filter.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/filter.c b/src/filter.c
> index 00a5e96..07b2e1d 100644
> --- a/src/filter.c
> +++ b/src/filter.c
> @@ -335,16 +335,22 @@ ct_filter_check(struct ct_filter *f, const struct nf_conntrack *ct)
>  		switch(nfct_get_attr_u8(ct, ATTR_L3PROTO)) {
>  		case AF_INET:
>  			ret = vector_iterate(f->v, ct, __ct_filter_test_mask4);
> -			if (ret ^ f->logic[CT_FILTER_ADDRESS])
> +			if (ret && f->logic[CT_FILTER_ADDRESS]) {
> +				break;
> +			} else if (ret && !f->logic[CT_FILTER_ADDRESS]) {
>  				return 0;
> +			}
>  			ret = __ct_filter_test_ipv4(f, ct);
>  			if (ret ^ f->logic[CT_FILTER_ADDRESS])
>  				return 0;
>  			break;
>  		case AF_INET6:
>  			ret = vector_iterate(f->v6, ct, __ct_filter_test_mask6);
> -			if (ret ^ f->logic[CT_FILTER_ADDRESS])
> +			if (ret && f->logic[CT_FILTER_ADDRESS]) {
> +				break;
> +			} else if (ret && !f->logic[CT_FILTER_ADDRESS]) {
>  				return 0;
> +			}
>  			ret = __ct_filter_test_ipv6(f, ct);
>  			if (ret ^ f->logic[CT_FILTER_ADDRESS])
>  				return 0;
> -- 
> 2.20.1
Robin Geuze May 31, 2019, 12:55 p.m. UTC | #2
Hey Pablo,

Broken cases (will never match):

Filter From Usespace {
    Address Accept {
        IPv4_address 127.0.0.1
    }
}

Filter From Usespace {
    Address Accept {
        IPv4_address 0.0.0.0/0
    }
}

Only way to "make it work" with the old code (only matches 127.0.0.1):
Filter From Usespace {
    Address Accept {
        IPv4_address 127.0.0.1
        IPv4_address 0.0.0.0/0
    }
}

Note: This only fixes the Userspace filtering. The Kernelspace filtering seems to have the same issue, but I haven't checked the code to see whether that is really the case.

From: Pablo Neira Ayuso <pablo@netfilter.org>
Sent: Thursday, May 30, 2019 4:43 PM
To: Robin Geuze
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [PATCH] conntrackd: Fix "Address Accept" filter case
 
On Tue, May 28, 2019 at 07:03:59AM +0000, Robin Geuze wrote:
> This fixes a bug in the Address Accept filter case where if you only
> specify either addresses or masks it would never match.

Thanks Robin.

Would you post an example configuration that is broken? I would like
to place it in the commit message.

> Signed-off-by: Robin Geuze <robing@transip.nl>
> ---
>   src/filter.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/filter.c b/src/filter.c
> index 00a5e96..07b2e1d 100644
> --- a/src/filter.c
> +++ b/src/filter.c
> @@ -335,16 +335,22 @@ ct_filter_check(struct ct_filter *f, const struct nf_conntrack *ct)
>                switch(nfct_get_attr_u8(ct, ATTR_L3PROTO)) {
>                case AF_INET:
>                        ret = vector_iterate(f->v, ct, __ct_filter_test_mask4);
> -                     if (ret ^ f->logic[CT_FILTER_ADDRESS])
> +                     if (ret && f->logic[CT_FILTER_ADDRESS]) {
> +                             break;
> +                     } else if (ret && !f->logic[CT_FILTER_ADDRESS]) {
>                                return 0;
> +                     }
>                        ret = __ct_filter_test_ipv4(f, ct);
>                        if (ret ^ f->logic[CT_FILTER_ADDRESS])
>                                return 0;
>                        break;
>                case AF_INET6:
>                        ret = vector_iterate(f->v6, ct, __ct_filter_test_mask6);
> -                     if (ret ^ f->logic[CT_FILTER_ADDRESS])
> +                     if (ret && f->logic[CT_FILTER_ADDRESS]) {
> +                             break;
> +                     } else if (ret && !f->logic[CT_FILTER_ADDRESS]) {
>                                return 0;
> +                     }
>                        ret = __ct_filter_test_ipv6(f, ct);
>                        if (ret ^ f->logic[CT_FILTER_ADDRESS])
>                                return 0;
> --
> 2.20.1

Patch
diff mbox series

diff --git a/src/filter.c b/src/filter.c
index 00a5e96..07b2e1d 100644
--- a/src/filter.c
+++ b/src/filter.c
@@ -335,16 +335,22 @@  ct_filter_check(struct ct_filter *f, const struct nf_conntrack *ct)
 		switch(nfct_get_attr_u8(ct, ATTR_L3PROTO)) {
 		case AF_INET:
 			ret = vector_iterate(f->v, ct, __ct_filter_test_mask4);
-			if (ret ^ f->logic[CT_FILTER_ADDRESS])
+			if (ret && f->logic[CT_FILTER_ADDRESS]) {
+				break;
+			} else if (ret && !f->logic[CT_FILTER_ADDRESS]) {
 				return 0;
+			}
 			ret = __ct_filter_test_ipv4(f, ct);
 			if (ret ^ f->logic[CT_FILTER_ADDRESS])
 				return 0;
 			break;
 		case AF_INET6:
 			ret = vector_iterate(f->v6, ct, __ct_filter_test_mask6);
-			if (ret ^ f->logic[CT_FILTER_ADDRESS])
+			if (ret && f->logic[CT_FILTER_ADDRESS]) {
+				break;
+			} else if (ret && !f->logic[CT_FILTER_ADDRESS]) {
 				return 0;
+			}
 			ret = __ct_filter_test_ipv6(f, ct);
 			if (ret ^ f->logic[CT_FILTER_ADDRESS])
 				return 0;