[PATCH v4] netfilter: conntrack: clamp timeouts to INT_MAX

[PATCH v4] netfilter: conntrack: clamp timeouts to INT_MAX

[Jay Elliott]

When the conntracking code multiplies a timeout by HZ, it can overflow
from positive to negative; this causes it to instantly expire.  To
protect against this the multiplication is done in 64-bit so we can
prevent it from exceeding INT_MAX.

Signed-off-by: Jay Elliott <jelliott@arista.com>
---
 net/netfilter/nf_conntrack_netlink.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index de4053d..41db7e3 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1560,9 +1560,11 @@ static int ctnetlink_change_helper(struct nf_conn *ct,
 static int ctnetlink_change_timeout(struct nf_conn *ct,
 				    const struct nlattr * const cda[])
 {
-	u_int32_t timeout = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
+	u64 timeout = (u64)ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ;
 
-	ct->timeout = nfct_time_stamp + timeout * HZ;
+	if (timeout > INT_MAX)
+		timeout = INT_MAX;
+	ct->timeout = nfct_time_stamp + (u32)timeout;
 
 	if (test_bit(IPS_DYING_BIT, &ct->status))
 		return -ETIME;
@@ -1762,6 +1764,7 @@ static int change_seq_adj(struct nf_ct_seqadj *seq,
 	int err = -EINVAL;
 	struct nf_conntrack_helper *helper;
 	struct nf_conn_tstamp *tstamp;
+	u64 timeout;
 
 	ct = nf_conntrack_alloc(net, zone, otuple, rtuple, GFP_ATOMIC);
 	if (IS_ERR(ct))
@@ -1770,7 +1773,10 @@ static int change_seq_adj(struct nf_ct_seqadj *seq,
 	if (!cda[CTA_TIMEOUT])
 		goto err1;
 
-	ct->timeout = nfct_time_stamp + ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ;
+	timeout = (u64)ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ;
+	if (timeout > INT_MAX)
+		timeout = INT_MAX;
+	ct->timeout = (u32)timeout + nfct_time_stamp;
 
 	rcu_read_lock();
  	if (cda[CTA_HELP]) {
-- 
1.8.1.4

Re: [PATCH v4] netfilter: conntrack: clamp timeouts to INT_MAX

[Pablo Neira Ayuso]

On Wed, Nov 15, 2017 at 03:01:13PM -0800, Jay Elliott wrote:
> When the conntracking code multiplies a timeout by HZ, it can overflow
> from positive to negative; this causes it to instantly expire.  To
> protect against this the multiplication is done in 64-bit so we can
> prevent it from exceeding INT_MAX.

This is something you must be observing via conntrack utility or
conntrackd?

> Signed-off-by: Jay Elliott <jelliott@arista.com>
> ---
>  net/netfilter/nf_conntrack_netlink.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index de4053d..41db7e3 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1560,9 +1560,11 @@ static int ctnetlink_change_helper(struct nf_conn *ct,
>  static int ctnetlink_change_timeout(struct nf_conn *ct,
>  				    const struct nlattr * const cda[])
>  {
> -	u_int32_t timeout = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
> +	u64 timeout = (u64)ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ;
>  
> -	ct->timeout = nfct_time_stamp + timeout * HZ;
> +	if (timeout > INT_MAX)
> +		timeout = INT_MAX;
> +	ct->timeout = nfct_time_stamp + (u32)timeout;
>  
>  	if (test_bit(IPS_DYING_BIT, &ct->status))
>  		return -ETIME;
> @@ -1762,6 +1764,7 @@ static int change_seq_adj(struct nf_ct_seqadj *seq,
>  	int err = -EINVAL;
>  	struct nf_conntrack_helper *helper;
>  	struct nf_conn_tstamp *tstamp;
> +	u64 timeout;
>  
>  	ct = nf_conntrack_alloc(net, zone, otuple, rtuple, GFP_ATOMIC);
>  	if (IS_ERR(ct))
> @@ -1770,7 +1773,10 @@ static int change_seq_adj(struct nf_ct_seqadj *seq,
>  	if (!cda[CTA_TIMEOUT])
>  		goto err1;
>  
> -	ct->timeout = nfct_time_stamp + ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ;
> +	timeout = (u64)ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ;
> +	if (timeout > INT_MAX)
> +		timeout = INT_MAX;
> +	ct->timeout = (u32)timeout + nfct_time_stamp;
>  
>  	rcu_read_lock();
>   	if (cda[CTA_HELP]) {
> -- 
> 1.8.1.4
> 

Re: [PATCH v4] netfilter: conntrack: clamp timeouts to INT_MAX

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Nov 15, 2017 at 03:01:13PM -0800, Jay Elliott wrote:
> > When the conntracking code multiplies a timeout by HZ, it can overflow
> > from positive to negative; this causes it to instantly expire.  To
> > protect against this the multiplication is done in 64-bit so we can
> > prevent it from exceeding INT_MAX.
> 
> This is something you must be observing via conntrack utility or
> conntrackd?

nf_ct_is_expired() will consider a ct as expired if the delta
between current time and the (future) timeout is > 2**31.

This triggers when ctnetlink configures a timeout larger than 2**31.

The patch looks correct to me.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH v4] netfilter: conntrack: clamp timeouts to INT_MAX

[Pablo Neira Ayuso]

On Tue, Nov 28, 2017 at 01:12:44AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Nov 15, 2017 at 03:01:13PM -0800, Jay Elliott wrote:
> > > When the conntracking code multiplies a timeout by HZ, it can overflow
> > > from positive to negative; this causes it to instantly expire.  To
> > > protect against this the multiplication is done in 64-bit so we can
> > > prevent it from exceeding INT_MAX.
> > 
> > This is something you must be observing via conntrack utility or
> > conntrackd?
> 
> nf_ct_is_expired() will consider a ct as expired if the delta
> between current time and the (future) timeout is > 2**31.
> 
> This triggers when ctnetlink configures a timeout larger than 2**31.
> 
> The patch looks correct to me.
> 
> Acked-by: Florian Westphal <fw@strlen.de>

I'll pass this to net instead, this is likely breaking conntrackd.

Thanks.