netdev - Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141110165439.GA7788@salvia>
Date:	Mon, 10 Nov 2014 17:54:39 +0100
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	Jesper Dangaard Brouer <brouer@...hat.com>
Cc:	programme110@...il.com, netfilter-devel@...r.kernel.org,
	Florian Westphal <fw@...len.de>, netdev@...r.kernel.org
Subject: Re: [PATCH nf] netfilter: conntrack: fix race in
 __nf_conntrack_confirm against get_next_corpse

On Thu, Nov 06, 2014 at 02:36:48PM +0100, Jesper Dangaard Brouer wrote:
> From: bill bonaparte <programme110@...il.com>
> 
> After removal of the central spinlock nf_conntrack_lock, in
> commit 93bb0ceb75be2 ("netfilter: conntrack: remove central
> spinlock nf_conntrack_lock"), it is possible to race against
> get_next_corpse().
> 
> The race is against the get_next_corpse() cleanup on
> the "unconfirmed" list (a per-cpu list with seperate locking),
> which set the DYING bit.
> 
> Fix this race, in __nf_conntrack_confirm(), by removing the CT
> from unconfirmed list before checking the DYING bit.  In case
> race occured, re-add the CT to the dying list.

This seems correct to me, some side comments.

> Fixes: 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")
> Reported-by: bill bonaparte <programme110@...il.com>
> Signed-off-by: bill bonaparte <programme110@...il.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
> 
>  net/netfilter/nf_conntrack_core.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 5016a69..1072650 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -611,12 +611,15 @@ __nf_conntrack_confirm(struct sk_buff *skb)
>  	 */
>  	NF_CT_ASSERT(!nf_ct_is_confirmed(ct));
>  	pr_debug("Confirming conntrack %p\n", ct);
> -	/* We have to check the DYING flag inside the lock to prevent
> +
> +	/* We have to check the DYING flag after unlink to prevent
>  	   a race against nf_ct_get_next_corpse() possibly called from
>  	   user context, else we insert an already 'dead' hash, blocking
>  	   further use of that particular connection -JM */

While at this, I think it would be good to fix comment style to:

        /* We have ...
         * ...
         */

I can fix this here, no need to resend, just let me know.

> +	nf_ct_del_from_dying_or_unconfirmed_list(ct);
>  
>  	if (unlikely(nf_ct_is_dying(ct))) {
> +		nf_ct_add_to_dying_list(ct);
>  		nf_conntrack_double_unlock(hash, reply_hash);
>  		local_bh_enable();
>  		return NF_ACCEPT;

Not directly related to your patch, but I don't find a good reason why
we're accepting this packet.

If the conntrack from the unconfirmed list is dying, then the object
will be released by when the packet leaves the stack to its
destination. With stateful filtering depending in place, the follow up
packet in the reply direction will likely be considered invalid (if
tcp tracking is on). Fortunately for us, the origin will likely
retransmit the syn again, so the ct will be setup accordingly.

So, why should we allow this to go through?

This return verdict was introduced in: fc35077 ("netfilter:
nf_conntrack: fix a race in __nf_conntrack_confirm against
nf_ct_get_next_corpse()") btw.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html