[RFC nf-next PATCH] netfilter: nf_conntrack_proto_tcp: propagate IP_CT_TCP_FLAG_BE_LIBERAL

[RFC nf-next PATCH] netfilter: nf_conntrack_proto_tcp: propagate IP_CT_TCP_FLAG_BE_LIBERAL

[Arturo Borrero Gonzalez]

According to Mathew Heard, the IP_CT_TCP_FLAG_BE_LIBERAL
is not being propagated properly while using userspace conntrackd to
replicate connections states in a firewall cluster.

This change modifies the behaviour of the engine to always be liberal in
the reply direction if we were liberal in the original direction as well.

More info in the Netfilter bugzilla:
 https://bugzilla.netfilter.org/show_bug.cgi?id=1087

Suggested-by: Mathew Heard <mat999@gmail.com>
Signed-off-by: Arturo Borrero Gonzalez <arturo@debian.org>
---
RFC: I don't fully understand this patch. Specifically, I don't understand
why this can't be done from userspace, in conntrackd, when creating/updating
synced conntracks. We could just set the new/updated conntrack with the flags
we want, don't we?

Also, I don't fully understand the consecuences of doing this flags change
in the middle of tcp_packet().

So, please, review the patch and give us comments.

 net/netfilter/nf_conntrack_proto_tcp.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 69f6877..ed16acf 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -835,6 +835,13 @@ static int tcp_packet(struct nf_conn *ct,
 	new_state = tcp_conntracks[dir][index][old_state];
 	tuple = &ct->tuplehash[dir].tuple;
 
+	/* if we are liberal in one direction, so be it in the other */
+	if (ct->proto.tcp.seen[IP_CT_DIR_ORIGINAL].flags &
+	    IP_CT_TCP_FLAG_BE_LIBERAL) {
+		ct->proto.tcp.seen[IP_CT_DIR_REPLY].flags |=
+			IP_CT_TCP_FLAG_BE_LIBERAL;
+	}
+
 	switch (new_state) {
 	case TCP_CONNTRACK_SYN_SENT:
 		if (old_state < TCP_CONNTRACK_TIME_WAIT)

Re: [RFC nf-next PATCH] netfilter: nf_conntrack_proto_tcp: propagate IP_CT_TCP_FLAG_BE_LIBERAL

[Pablo Neira Ayuso]

On Thu, Oct 20, 2016 at 11:00:49AM +0200, Arturo Borrero Gonzalez wrote:
> According to Mathew Heard, the IP_CT_TCP_FLAG_BE_LIBERAL
> is not being propagated properly while using userspace conntrackd to
> replicate connections states in a firewall cluster.
> 
> This change modifies the behaviour of the engine to always be liberal in
> the reply direction if we were liberal in the original direction as well.
> 
> More info in the Netfilter bugzilla:
>  https://bugzilla.netfilter.org/show_bug.cgi?id=1087
> 
> Suggested-by: Mathew Heard <mat999@gmail.com>
> Signed-off-by: Arturo Borrero Gonzalez <arturo@debian.org>
> ---
> RFC: I don't fully understand this patch. Specifically, I don't understand
> why this can't be done from userspace, in conntrackd, when creating/updating
> synced conntracks. We could just set the new/updated conntrack with the flags
> we want, don't we?
> 
> Also, I don't fully understand the consecuences of doing this flags change
> in the middle of tcp_packet().
> 
> So, please, review the patch and give us comments.

There is a 'TCPWindowTracking' option that you can set on from the
configuration file.

Is that probably what Mathew needs?

Re: [RFC nf-next PATCH] netfilter: nf_conntrack_proto_tcp: propagate IP_CT_TCP_FLAG_BE_LIBERAL

[Arturo Borrero Gonzalez]

On 20 October 2016 at 20:14, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> There is a 'TCPWindowTracking' option that you can set on from the
> configuration file.
>
> Is that probably what Mathew needs?

@Mathew, could you please share what are your problems with this
conntrackd code?


/* disable TCP window tracking for recovered connections if required */
if (nfct_attr_is_set(ct, ATTR_TCP_STATE)) {
    uint8_t flags = IP_CT_TCP_FLAG_SACK_PERM;

    if (!CONFIG(sync).tcp_window_tracking)
        flags |= IP_CT_TCP_FLAG_BE_LIBERAL;
    else
        flags |= IP_CT_TCP_FLAG_WINDOW_SCALE;
[...]

Re: [RFC nf-next PATCH] netfilter: nf_conntrack_proto_tcp: propagate IP_CT_TCP_FLAG_BE_LIBERAL

[Arturo Borrero Gonzalez]

(please keep the netfilter-devel list in CC)

On 21 October 2016 at 09:18, Mathew Heard <mat999@gmail.com> wrote:
> That's been covered already.
>
> The problem with it is that only the ORIG side of the connection ends
> up set. REPLY does not.
>
> I don't know the fundamental reason why this occurs, only the effect.
>

In that same function, in conntrackd:
 http://git.netfilter.org/conntrack-tools/tree/src/netlink.c#n256

we set the same flags in both original and reply directions:

nfct_set_attr_u8(ct, ATTR_TCP_FLAGS_ORIG, flags);
nfct_set_attr_u8(ct, ATTR_TCP_MASK_ORIG, flags);
nfct_set_attr_u8(ct, ATTR_TCP_FLAGS_REPL, flags);
nfct_set_attr_u8(ct, ATTR_TCP_MASK_REPL, flags);

Re: [RFC nf-next PATCH] netfilter: nf_conntrack_proto_tcp: propagate IP_CT_TCP_FLAG_BE_LIBERAL

[Mathew Heard]

However under testing, in practice is not. As covered in the bug.

Fields: CTA_IP_V4_DST, CTA_PROTOINFO_TCP_FLAGS_ORIGINAL &
CTA_PROTOINFO_TCP_FLAGS_REPLY
Result: "**.**.56.135: 10 3"

It's only being set on one side. I believe this is because the reply
side flags are being set/initialised after the fact (i.e where they
are initialised in that function for incoming connections would do it
too).


On Fri, Oct 21, 2016 at 6:22 PM, Arturo Borrero Gonzalez
<arturo@debian.org> wrote:
> (please keep the netfilter-devel list in CC)
>
> On 21 October 2016 at 09:18, Mathew Heard <mat999@gmail.com> wrote:
>> That's been covered already.
>>
>> The problem with it is that only the ORIG side of the connection ends
>> up set. REPLY does not.
>>
>> I don't know the fundamental reason why this occurs, only the effect.
>>
>
> In that same function, in conntrackd:
>  http://git.netfilter.org/conntrack-tools/tree/src/netlink.c#n256
>
> we set the same flags in both original and reply directions:
>
> nfct_set_attr_u8(ct, ATTR_TCP_FLAGS_ORIG, flags);
> nfct_set_attr_u8(ct, ATTR_TCP_MASK_ORIG, flags);
> nfct_set_attr_u8(ct, ATTR_TCP_FLAGS_REPL, flags);
> nfct_set_attr_u8(ct, ATTR_TCP_MASK_REPL, flags);

Re: [RFC nf-next PATCH] netfilter: nf_conntrack_proto_tcp: propagate IP_CT_TCP_FLAG_BE_LIBERAL

[Pablo Neira Ayuso]

On Fri, Oct 21, 2016 at 06:26:28PM +1100, Mathew Heard wrote:
> However under testing, in practice is not. As covered in the bug.
> 
> Fields: CTA_IP_V4_DST, CTA_PROTOINFO_TCP_FLAGS_ORIGINAL &
> CTA_PROTOINFO_TCP_FLAGS_REPLY
> Result: "**.**.56.135: 10 3"

>From where are you printing this? userspace or kernel?

> It's only being set on one side. I believe this is because the reply
> side flags are being set/initialised after the fact (i.e where they
> are initialised in that function for incoming connections would do it
> too).

Please develop this a bit more.

Is there anything we should know on your infrastructure? eg. kernel
and library version, what architecture you using?

Asking this because I found an old report on problems on ARM that the
submitter never confirmed to be fixed.

Thanks.

Re: [RFC nf-next PATCH] netfilter: nf_conntrack_proto_tcp: propagate IP_CT_TCP_FLAG_BE_LIBERAL

[Mathew Heard]

On Fri, Oct 21, 2016 at 8:56 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Oct 21, 2016 at 06:26:28PM +1100, Mathew Heard wrote:
>> However under testing, in practice is not. As covered in the bug.
>>
>> Fields: CTA_IP_V4_DST, CTA_PROTOINFO_TCP_FLAGS_ORIGINAL &
>> CTA_PROTOINFO_TCP_FLAGS_REPLY
>> Result: "**.**.56.135: 10 3"
>
> From where are you printing this? userspace or kernel?
>

CTA_* comes from libnetfilter_conntrack, which is userspace.

I have however also printk'ed flags in the kernel during testing and
seen the same (further confirmed by the crude fix working).

>> It's only being set on one side. I believe this is because the reply
>> side flags are being set/initialised after the fact (i.e where they
>> are initialised in that function for incoming connections would do it
>> too).
>
> Please develop this a bit more.
>
> Is there anything we should know on your infrastructure? eg. kernel
> and library version, what architecture you using?
>
> Asking this because I found an old report on problems on ARM that the
> submitter never confirmed to be fixed.
>
> Thanks.

AMD64 in both native (staging) and virtual (dev) environments.

Originally we found this issue with incoming connections, however due
to it being simpler to test I moved to testing outgoing.

I hope this ASCII diagram survives the mail system. Test System:

[NAT Router A]  -----
         |                    \_____
    Conntrackd          ______ Target Box
         |                   /
[NAT Router B] ------

Target box connected via GRE (internal network range 10.x.x.x). Router
A and B both with standard DNAT & SNAT rules to provide connectivity &
port forwards. To test, I just change the route of an outgoing
connection from Target Box on Target Box mid connection (i.e via using
"ip rule")

With TCP window tracking disabled using sysctl's, or with the crude
patch this all works as expected.
Without the patch, due to the tcp flags of the reply side not
containing the correct flags, it does not.

Inbound testing was tested similarly, but by moving BGP announcements
between routers.

I have also replicated the same results in our staging environment but
thats substantially more complex.

Regards,
Mathew