| Message ID | AM0PR02MB5492D0F9BEB5814637C7D5C3AA1E0@AM0PR02MB5492.eurprd02.prod.outlook.com |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | conntrackd: Fix "Address Accept" filter case | expand |
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
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
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;
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(-)