Re: PROBLEM: Injected conntrack lost helper

Re: PROBLEM: Injected conntrack lost helper

[Florian Westphal]

Pham Thanh Tuyen <phamtyn@gmail.com> wrote:

[ moving to netfilter-devel ]

> > > My name is Pham Thanh Tuyen. I found a bug related to the ctnetlink
> > > and conntrack subsystems. Details are as follows:
> > > 
> > > 1. Summary: Injected conntrack lost helper
> > > 
> > > 2. Description: When a conntrack whose helper is injected from
> > > userspace, the ctnetlink creates helper for it but NAT then loses
> > > the helper in case the user defined helper explicitly with CT
> > > target.

Hmm.  If you insert a conntrack entry from userspace, it will already
be confirmed, so nat rules will have no effect, and template CT rules
are irrelevant wrt. to the helper, as extension can only be created if
the conntrack is not in the hash yet.

Can you describe the steps to reproduce this bug?

Re: PROBLEM: Injected conntrack lost helper

[Pham Thanh Tuyen]

1. Make sure to disable automatic helper assignment using the command 
'sudo sysctl net.netfilter.nf_conntrack_helper=0'
2. Use NAT
3. Set up a fault tolerant firewall system and then make the active 
firewall machine to fail. The backup firewall machine will recover the 
connection but lose the helper. (Simpler way is to use conntrack -I to 
inject the conntrack).


On 1/31/22 18:20, Florian Westphal wrote:
> Pham Thanh Tuyen <phamtyn@gmail.com> wrote:
>
> [ moving to netfilter-devel ]
>
>>>> My name is Pham Thanh Tuyen. I found a bug related to the ctnetlink
>>>> and conntrack subsystems. Details are as follows:
>>>>
>>>> 1. Summary: Injected conntrack lost helper
>>>>
>>>> 2. Description: When a conntrack whose helper is injected from
>>>> userspace, the ctnetlink creates helper for it but NAT then loses
>>>> the helper in case the user defined helper explicitly with CT
>>>> target.
> Hmm.  If you insert a conntrack entry from userspace, it will already
> be confirmed, so nat rules will have no effect, and template CT rules
> are irrelevant wrt. to the helper, as extension can only be created if
> the conntrack is not in the hash yet.
>
> Can you describe the steps to reproduce this bug?

Re: PROBLEM: Injected conntrack lost helper

[Pham Thanh Tuyen]

1. Make sure to disable automatic helper assignment using the command 
'sudo sysctl -w net.netfilter.nf_conntrack_helper=0'
2. Use NAT
3. Set up a fault tolerant firewall system and then make the active 
firewall machine to fail. The backup firewall machine will recover the 
connection but lose the helper. (Simpler way is to use conntrack -I to 
inject the conntrack).

On 1/31/22 20:47, Pham Thanh Tuyen wrote:
> 1. Make sure to disable automatic helper assignment using the command 
> 'sudo sysctl net.netfilter.nf_conntrack_helper=0'
> 2. Use NAT
> 3. Set up a fault tolerant firewall system and then make the active 
> firewall machine to fail. The backup firewall machine will recover the 
> connection but lose the helper. (Simpler way is to use conntrack -I to 
> inject the conntrack).
>
>
> On 1/31/22 18:20, Florian Westphal wrote:
>> Pham Thanh Tuyen <phamtyn@gmail.com> wrote:
>>
>> [ moving to netfilter-devel ]
>>
>>>>> My name is Pham Thanh Tuyen. I found a bug related to the ctnetlink
>>>>> and conntrack subsystems. Details are as follows:
>>>>>
>>>>> 1. Summary: Injected conntrack lost helper
>>>>>
>>>>> 2. Description: When a conntrack whose helper is injected from
>>>>> userspace, the ctnetlink creates helper for it but NAT then loses
>>>>> the helper in case the user defined helper explicitly with CT
>>>>> target.
>> Hmm.  If you insert a conntrack entry from userspace, it will already
>> be confirmed, so nat rules will have no effect, and template CT rules
>> are irrelevant wrt. to the helper, as extension can only be created if
>> the conntrack is not in the hash yet.
>>
>> Can you describe the steps to reproduce this bug?

Re: PROBLEM: Injected conntrack lost helper

[Pham Thanh Tuyen]

When the conntrack is created, the extension is created before the 
conntrack is assigned confirmed and inserted into the hash table. But 
the function ctnetlink_setup_nat() causes loss of helper in the 
mentioned situation. I mention the template because it's seamless in the 
__nf_ct_try_assign_helper() function. Please double check.

On 1/31/22 18:20, Florian Westphal wrote:
> Pham Thanh Tuyen <phamtyn@gmail.com> wrote:
>
> [ moving to netfilter-devel ]
>
>>>> My name is Pham Thanh Tuyen. I found a bug related to the ctnetlink
>>>> and conntrack subsystems. Details are as follows:
>>>>
>>>> 1. Summary: Injected conntrack lost helper
>>>>
>>>> 2. Description: When a conntrack whose helper is injected from
>>>> userspace, the ctnetlink creates helper for it but NAT then loses
>>>> the helper in case the user defined helper explicitly with CT
>>>> target.
> Hmm.  If you insert a conntrack entry from userspace, it will already
> be confirmed, so nat rules will have no effect, and template CT rules
> are irrelevant wrt. to the helper, as extension can only be created if
> the conntrack is not in the hash yet.
>
> Can you describe the steps to reproduce this bug?

Re: PROBLEM: Injected conntrack lost helper

[Pablo Neira Ayuso]

On Tue, Feb 01, 2022 at 10:08:55AM +0700, Pham Thanh Tuyen wrote:
> When the conntrack is created, the extension is created before the conntrack
> is assigned confirmed and inserted into the hash table. But the function
> ctnetlink_setup_nat() causes loss of helper in the mentioned situation. I
> mention the template because it's seamless in the
> __nf_ct_try_assign_helper() function. Please double check.

Conntrack entries that are created via ctnetlink as IPS_CONFIRMED always
set on.

The helper code is only exercised from the packet path for conntrack
entries that are newly created.

Re: PROBLEM: Injected conntrack lost helper

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Feb 01, 2022 at 10:08:55AM +0700, Pham Thanh Tuyen wrote:
> > When the conntrack is created, the extension is created before the conntrack
> > is assigned confirmed and inserted into the hash table. But the function
> > ctnetlink_setup_nat() causes loss of helper in the mentioned situation. I
> > mention the template because it's seamless in the
> > __nf_ct_try_assign_helper() function. Please double check.
> 
> Conntrack entries that are created via ctnetlink as IPS_CONFIRMED always
> set on.
>
> The helper code is only exercised from the packet path for conntrack
> entries that are newly created.

I suspect this is the most simple fix, might make sense to also
update the comment of IPS_HELPER to say that it means 'explicitly
attached via ctnetlink or ruleset'.

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2313,6 +2313,9 @@ ctnetlink_create_conntrack(struct net *net,
 
 			/* not in hash table yet so not strictly necessary */
 			RCU_INIT_POINTER(help->helper, helper);
+
+			/* explicitly attached from userspace */
+			ct->status |= IPS_HELPER;
 		}
 	} else {
 		/* try an implicit helper assignation */

Re: PROBLEM: Injected conntrack lost helper

[Pham Thanh Tuyen]

Previously I also thought ct->status |= IPS_HELPER; is ok, but after 
internal pointer assigning with RCU_INIT_POINTER() need external pointer 
assigning with rcu_assign_pointer() in __nf_ct_try_assign_helper() function.

On 2/1/22 19:04, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> On Tue, Feb 01, 2022 at 10:08:55AM +0700, Pham Thanh Tuyen wrote:
>>> When the conntrack is created, the extension is created before the conntrack
>>> is assigned confirmed and inserted into the hash table. But the function
>>> ctnetlink_setup_nat() causes loss of helper in the mentioned situation. I
>>> mention the template because it's seamless in the
>>> __nf_ct_try_assign_helper() function. Please double check.
>> Conntrack entries that are created via ctnetlink as IPS_CONFIRMED always
>> set on.
>>
>> The helper code is only exercised from the packet path for conntrack
>> entries that are newly created.
> I suspect this is the most simple fix, might make sense to also
> update the comment of IPS_HELPER to say that it means 'explicitly
> attached via ctnetlink or ruleset'.
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -2313,6 +2313,9 @@ ctnetlink_create_conntrack(struct net *net,
>   
>   			/* not in hash table yet so not strictly necessary */
>   			RCU_INIT_POINTER(help->helper, helper);
> +
> +			/* explicitly attached from userspace */
> +			ct->status |= IPS_HELPER;
>   		}
>   	} else {
>   		/* try an implicit helper assignation */

Re: PROBLEM: Injected conntrack lost helper

[Florian Westphal]

Pham Thanh Tuyen <phamtyn@gmail.com> wrote:
> Previously I also thought ct->status |= IPS_HELPER; is ok, but after
> internal pointer assigning with RCU_INIT_POINTER() need external pointer
> assigning with rcu_assign_pointer() in __nf_ct_try_assign_helper() function.

I'm not following.

__nf_ct_try_assign_helper() doesn't do anything once that flag is set,
so how could the helper get lost later?

Re: PROBLEM: Injected conntrack lost helper

[Pham Thanh Tuyen]

That's okay, so fix it like you ct->status |= IPS_HELPER;

On 2/1/22 21:14, Florian Westphal wrote:
> Pham Thanh Tuyen <phamtyn@gmail.com> wrote:
>> Previously I also thought ct->status |= IPS_HELPER; is ok, but after
>> internal pointer assigning with RCU_INIT_POINTER() need external pointer
>> assigning with rcu_assign_pointer() in __nf_ct_try_assign_helper() function.
> I'm not following.
>
> __nf_ct_try_assign_helper() doesn't do anything once that flag is set,
> so how could the helper get lost later?

Re: PROBLEM: Injected conntrack lost helper

[Pablo Neira Ayuso]

On Tue, Feb 01, 2022 at 01:04:54PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Feb 01, 2022 at 10:08:55AM +0700, Pham Thanh Tuyen wrote:
> > > When the conntrack is created, the extension is created before the conntrack
> > > is assigned confirmed and inserted into the hash table. But the function
> > > ctnetlink_setup_nat() causes loss of helper in the mentioned situation. I
> > > mention the template because it's seamless in the
> > > __nf_ct_try_assign_helper() function. Please double check.
> > 
> > Conntrack entries that are created via ctnetlink as IPS_CONFIRMED always
> > set on.
> >
> > The helper code is only exercised from the packet path for conntrack
> > entries that are newly created.
> 
> I suspect this is the most simple fix, might make sense to also
> update the comment of IPS_HELPER to say that it means 'explicitly
> attached via ctnetlink or ruleset'.
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -2313,6 +2313,9 @@ ctnetlink_create_conntrack(struct net *net,
>  
>  			/* not in hash table yet so not strictly necessary */
>  			RCU_INIT_POINTER(help->helper, helper);
> +
> +			/* explicitly attached from userspace */
> +			ct->status |= IPS_HELPER;
>  		}
>  	} else {
>  		/* try an implicit helper assignation */

This also LGTM.

I'd suggest you update the .h file to describe that ctnetlink also
sets on this bit.

Thanks