[nf-next,v3] netfilter: nft_ct: add ct expectations support

This patch allows to add, list and delete expectations via nft objref
infrastructure and assigning these expectations via nft rule.

This allows manual port triggering when no helper is defined to manage a
specific protocol. For example, if I have an online game which protocol
is based on initial connection to TCP port 9753 of the server, and where
the server opens a connection to port 9876, I can set rules as follow:

table ip filter {
    ct expectation mygame {
        protocol udp;
        dport 9876;
    }

    chain input {
        type filter hook input priority 0; policy drop;
        tcp dport 9753 ct expectation set "mygame";
    }

    chain output {
        type filter hook output priority 0; policy drop;
        udp dport 9876 ct status expected accept;
    }
}

Signed-off-by: Stéphane Veyret <sveyret@gmail.com>
---
 include/uapi/linux/netfilter/nf_tables.h |  13 ++-
 net/netfilter/nft_ct.c                   | 121 ++++++++++++++++++++++-
 2 files changed, 131 insertions(+), 3 deletions(-)

On Fri, May 17, 2019 at 06:40:29PM +0200, Stéphane Veyret wrote:
> This patch allows to add, list and delete expectations via nft objref
> infrastructure and assigning these expectations via nft rule.
> 
> This allows manual port triggering when no helper is defined to manage a
> specific protocol. For example, if I have an online game which protocol
> is based on initial connection to TCP port 9753 of the server, and where
> the server opens a connection to port 9876, I can set rules as follow:
> 
> table ip filter {
>     ct expectation mygame {
>         protocol udp;
>         dport 9876;

I think we should set a maximum number of expectations to be created,
as a mandatory field, eg.

          size 10;

>     }
> 
>     chain input {
>         type filter hook input priority 0; policy drop;
>         tcp dport 9753 ct expectation set "mygame";
>     }
> 
>     chain output {
>         type filter hook output priority 0; policy drop;
>         udp dport 9876 ct status expected accept;
>     }
> }
[...]
> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index f043936763f3..d01cb175ab30 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c
> @@ -24,6 +24,7 @@
>  #include <net/netfilter/nf_conntrack_labels.h>
>  #include <net/netfilter/nf_conntrack_timeout.h>
>  #include <net/netfilter/nf_conntrack_l4proto.h>
> +#include <net/netfilter/nf_conntrack_expect.h>
>  
>  struct nft_ct {
>  	enum nft_ct_keys	key:8;
> @@ -790,6 +791,114 @@ static struct nft_expr_type nft_notrack_type __read_mostly = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +struct nft_ct_expect_obj {
> +	int			l3num;
> +	u8			l4proto;
> +	__be16		dport;
> +	u32			timeout;
> +};
> +
> +static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
> +				   const struct nlattr * const tb[],
> +				   struct nft_object *obj)
> +{
> +	struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> +	int ret;
> +
> +	if (!tb[NFTA_CT_EXPECT_L4PROTO] ||
> +		!tb[NFTA_CT_EXPECT_DPORT] ||
> +		!tb[NFTA_CT_EXPECT_TIMEOUT])

Coding style: Align parameter to parens:

	if (!tb[NFTA_CT_EXPECT_L4PROTO] ||
            !tb[NFTA_CT_EXPECT_DPORT] ||
            !tb[NFTA_CT_EXPECT_TIMEOUT])
		return -EINVAL;

> +	priv->l3num = ctx->family;

priv->l3num is only set and never used, remove it. You'll also have to
remove NFTA_CT_EXPECT_L3PROTO.

> +	if (tb[NFTA_CT_EXPECT_L3PROTO])
> +		priv->l3num = ntohs(nla_get_be16(tb[NFTA_CT_EXPECT_L3PROTO]));
> +
> +	priv->l4proto = nla_get_u8(tb[NFTA_CT_EXPECT_L4PROTO]);
> +	priv->dport = nla_get_be16(tb[NFTA_CT_EXPECT_DPORT]);
> +	priv->timeout = nla_get_u32(tb[NFTA_CT_EXPECT_TIMEOUT]);
> +
> +	ret = nf_ct_netns_get(ctx->net, ctx->family);

Just:

        return nf_ct_netns_get(ctx->net, ctx->family);

should be fine.

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static void nft_ct_expect_obj_destroy(const struct nft_ctx *ctx,
> +				       struct nft_object *obj)
> +{
> +	nf_ct_netns_put(ctx->net, ctx->family);
> +}
> +
> +static int nft_ct_expect_obj_dump(struct sk_buff *skb,
> +				   struct nft_object *obj, bool reset)
> +{
> +	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> +
> +	if (nla_put_be16(skb, NFTA_CT_EXPECT_L3PROTO, htons(priv->l3num)) ||
> +		nla_put_u8(skb, NFTA_CT_EXPECT_L4PROTO, priv->l4proto) ||
> +		nla_put_be16(skb, NFTA_CT_EXPECT_DPORT, priv->dport) ||
> +		nla_put_u32(skb, NFTA_CT_EXPECT_TIMEOUT, priv->timeout))
> +	return -1;

Coding style: Align parameter to parens:

	if (nla_put_be16(skb, NFTA_CT_EXPECT_L3PROTO, htons(priv->l3num)) ||
            nla_put_u8(skb, NFTA_CT_EXPECT_L4PROTO, priv->l4proto) ||
            nla_put_be16(skb, NFTA_CT_EXPECT_DPORT, priv->dport) ||
            nla_put_u32(skb, NFTA_CT_EXPECT_TIMEOUT, priv->timeout))
                return -1;

> +	return 0;
> +}
> +
> +static void nft_ct_expect_obj_eval(struct nft_object *obj,
> +				    struct nft_regs *regs,
> +				    const struct nft_pktinfo *pkt)
> +{
> +	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> +	enum ip_conntrack_info ctinfo;
> +	struct nf_conn *ct = nf_ct_get(pkt->skb, &ctinfo);
> +	int dir = CTINFO2DIR(ctinfo);
> +	struct nf_conntrack_expect *exp;

Please, revert xmas tree for variable definitions, ie.

	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
	struct nf_conntrack_expect *exp;
	enum ip_conntrack_info ctinfo;
	int dir = CTINFO2DIR(ctinfo);
	struct nf_conn *ct;

Then, you have to check if ct is unset or is untrackedie.

        ct = nf_ct_get(pkt->skb, &ctinfo);
        if (!ct || ctinfo == IP_CT_UNTRACKED)
                goto err;

        ...
err:
        regs->verdict.code = NFT_BREAK;

> +	nf_ct_helper_ext_add(ct, GFP_ATOMIC);

I think you don't need nf_ct_helper_ext_add(...);

> +	exp = nf_ct_expect_alloc(ct);
> +	if (exp == NULL) {
> +		regs->verdict.code = NF_DROP;
> +		return;
> +	}
> +
> +	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, priv->l3num,
> +		&ct->tuplehash[!dir].tuple.src.u3,
> +		&ct->tuplehash[!dir].tuple.dst.u3,
> +		priv->l4proto, NULL, &priv->dport);

Coding style: Align parameter to parens:

	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, priv->l3num,
                          &ct->tuplehash[!dir].tuple.src.u3,
                          &ct->tuplehash[!dir].tuple.dst.u3,
                          priv->l4proto, NULL, &priv->dport);

> +	exp->timeout.expires = jiffies + priv->timeout * HZ;
> +
> +	if (nf_ct_expect_related(exp) != 0) {
> +		regs->verdict.code = NF_DROP;
> +	}

No need for curly braces here, with single statement, the following is
fine:

	if (nf_ct_expect_related(exp) != 0)
                regs->verdict.code = NF_DROP;

Thanks for submitting your patch.

Thank you Pablo for your feedback. See my comments below.

Le mer. 22 mai 2019 à 10:46, Pablo Neira Ayuso <pablo@netfilter.org> a écrit :
> I think we should set a maximum number of expectations to be created,
> as a mandatory field, eg.
>
>           size 10;

I feel it would be complicated to set, as it would require to keep
track of all expectations set using this definition, and moreover,
check if those expectations are still alive, or deleted because
already used or timed out.

> > +     priv->l3num = ctx->family;
>
> priv->l3num is only set and never used, remove it. You'll also have to

priv->l3num is used for setting expectation, in function
nft_ct_expect_obj_eval (see the call to nf_ct_expect_init).

> > +     nf_ct_helper_ext_add(ct, GFP_ATOMIC);
>
> I think you don't need nf_ct_helper_ext_add(...);

Actually, I had to add this instruction. While testing the feature, i
saw that, even if no helper is really set on the connection,
expectation functions require NF_CT_EXT_HELPER to be set on master
connection. Without it, there would be some null pointer exception,
which fortunately is checked at expectation creation by
__nf_ct_expect_check.

Regards,

Stéphane.

On Wed, May 22, 2019 at 01:39:57PM +0200, Stéphane Veyret wrote:
[...]
> Le mer. 22 mai 2019 à 10:46, Pablo Neira Ayuso <pablo@netfilter.org> a écrit :
> > I think we should set a maximum number of expectations to be created,
> > as a mandatory field, eg.
> >
> >           size 10;
>
> I feel it would be complicated to set, as it would require to keep
> track of all expectations set using this definition, and moreover,
> check if those expectations are still alive, or deleted because
> already used or timed out.

You can use the 'expecting[0]' counter in the ct helper extension to
limit the number of expectations per conntrack entry:

struct nf_conn_help {
[...]
        /* Current number of expected connections */
        u8 expecting[NF_CT_MAX_EXPECT_CLASSES];
};

You have to check if the ct helper area exists in first place.

> > > +     priv->l3num = ctx->family;
> >
> > priv->l3num is only set and never used, remove it. You'll also have to
>
> priv->l3num is used for setting expectation, in function
> nft_ct_expect_obj_eval (see the call to nf_ct_expect_init).

OK, thanks for explaining.

Still this new expectation extension won't work with NFPROTO_INET
tables though, since the expectation infrastructure does not know what
to do with NFPROTO_INET.

If NFPROTO_INET is specified, you could just fetch the l3proto from
the ct object, from the packet path by when you call
nf_ct_expect_init().

> > > +     nf_ct_helper_ext_add(ct, GFP_ATOMIC);
> >
> > I think you don't need nf_ct_helper_ext_add(...);
>
> Actually, I had to add this instruction. While testing the feature, i
> saw that, even if no helper is really set on the connection,
> expectation functions require NF_CT_EXT_HELPER to be set on master
> connection. Without it, there would be some null pointer exception,
> which fortunately is checked at expectation creation by
> __nf_ct_expect_check.

Thanks again for explaining.

You still have to check if the conntrack already has a helper
extensions, otherwise I'm afraid this resets it for this conntrack.