[nf] netfilter: ipt_CLUSTERIP: Allow configuring --local-node 0 again

From: Tobias Brunner <tobias@strongswan.org>

This is useful as it prevents that the nodes handle any packets until
they are assigned their responsibilities (e.g. by a HA daemon).

Fixes: 1a38956cce5e ("netfilter: ipt_CLUSTERIP: fix out-of-bounds accesses in clusterip_tg_check()").
Signed-off-by: Tobias Brunner <tobias@strongswan.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/ipv4/netfilter/ipt_CLUSTERIP.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Hi Tobias,

On Tue, Mar 27, 2018 at 12:11:01PM +0200, Pablo Neira Ayuso wrote:
> diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
> index f302c8e0b2e5..d7bfaf7184ce 100644
> --- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
> +++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
> @@ -537,7 +537,8 @@ static int clusterip_tg_check(const struct xt_tgchk_param *par)
>  		return -EINVAL;
>  	}
>  	for (i = 0; i < cipinfo->num_local_nodes; i++) {
> -		if (cipinfo->local_nodes[i] - 1 >=
> +		if (cipinfo->local_nodes[i] != 0 &&
> +		    cipinfo->local_nodes[i] - 1 >=

The bitmap assumes that the value is always > 0. If you look at:

static void
clusterip_config_init_nodelist(struct clusterip_config *c,
                               const struct ipt_clusterip_tgt_info *i)
{
        int n;

        for (n = 0; n < i->num_local_nodes; n++)
                set_bit(i->local_nodes[n] - 1, &c->local_nodes);
}

If we allow i->local_nodes[n] == 0, then this sets bit -1 (underflow).

Could you post your configuration? Hence we can have a look at what was
working before Dmitry's fix.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Pablo,

> The bitmap assumes that the value is always > 0. If you look at:
> 
> static void
> clusterip_config_init_nodelist(struct clusterip_config *c,
>                                const struct ipt_clusterip_tgt_info *i)
> {
>         int n;
> 
>         for (n = 0; n < i->num_local_nodes; n++)
>                 set_bit(i->local_nodes[n] - 1, &c->local_nodes);
> }
> 
> If we allow i->local_nodes[n] == 0, then this sets bit -1 (underflow).

You're right.  However, by accident, it did what we wanted.  It
basically sets the MSB of c->local_nodes, but since num_total_nodes is
limited to 16 this meant that it looks to ClusterIP like no nodes are local.

> Could you post your configuration? Hence we can have a look at what was
> working before Dmitry's fix.

We used something like this:

iptables -A INPUT -i eth0 -d 192.168.0.5 -j CLUSTERIP --new --hashmode
sourceip --clustermac 01:00:c0:a8:00:05 --total-nodes 2 --local-node 0

What we actually wanted was to configure this without --local-node at
all.  The local nodes should have been assigned later.  Since that's
rejected by iptables we used --local-node 0, which was accepted before
Dmitry's fix and did what we wanted.

So my patch is definitely not correct and I guess adding the possibility
to omit --local-node isn't worth the effort for a deprecated module.  I
think I'll just add my patch to our HA patchset, which modifies
ipt_CLUSTERIP anyway, until we have a replacement for that.

Thanks,
Tobias
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Mar 29, 2018 at 12:47:07PM +0200, Tobias Brunner wrote:
> Hi Pablo,
> 
> > The bitmap assumes that the value is always > 0. If you look at:
> > 
> > static void
> > clusterip_config_init_nodelist(struct clusterip_config *c,
> >                                const struct ipt_clusterip_tgt_info *i)
> > {
> >         int n;
> > 
> >         for (n = 0; n < i->num_local_nodes; n++)
> >                 set_bit(i->local_nodes[n] - 1, &c->local_nodes);
> > }
> > 
> > If we allow i->local_nodes[n] == 0, then this sets bit -1 (underflow).
> 
> You're right.  However, by accident, it did what we wanted.  It
> basically sets the MSB of c->local_nodes, but since num_total_nodes is
> limited to 16 this meant that it looks to ClusterIP like no nodes are local.

OK, so your idea was to add an all zero bitmap.

> > Could you post your configuration? Hence we can have a look at what was
> > working before Dmitry's fix.
> 
> We used something like this:
> 
> iptables -A INPUT -i eth0 -d 192.168.0.5 -j CLUSTERIP --new --hashmode
> sourceip --clustermac 01:00:c0:a8:00:05 --total-nodes 2 --local-node 0
> 
> What we actually wanted was to configure this without --local-node at
> all.  The local nodes should have been assigned later.  Since that's
> rejected by iptables we used --local-node 0, which was accepted before
> Dmitry's fix and did what we wanted.
> 
> So my patch is definitely not correct and I guess adding the possibility
> to omit --local-node isn't worth the effort for a deprecated module.  I
> think I'll just add my patch to our HA patchset, which modifies
> ipt_CLUSTERIP anyway, until we have a replacement for that.

Makes sense to me, thanks Tobias!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html