Re: [PATCH nf-next v2] netfilter: nf_tables: Add SYNPROXY support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Pablo, comments below.

On 6/20/19 4:10 PM, Pablo Neira Ayuso wrote:
> On Wed, Jun 19, 2019 at 08:06:54PM +0200, Fernando Fernandez Mancera wrote:
> [...]
>> diff --git a/net/netfilter/nft_synproxy.c b/net/netfilter/nft_synproxy.c
>> new file mode 100644
>> index 000000000000..3ef7f1dc50be
>> --- /dev/null
>> +++ b/net/netfilter/nft_synproxy.c
>> @@ -0,0 +1,327 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
> 
> Remove empty line above.
> 
>> +#include <linux/types.h>
>> +
> 
> Same here.
> 
>> +#include <net/ip.h>
>> +#include <net/tcp.h>
>> +#include <net/netlink.h>
>> +
> 
> Same here.
> 
>> +#include <net/netfilter/nf_tables.h>
>> +#include <net/netfilter/nf_conntrack.h>
>> +#include <net/netfilter/nf_conntrack_ecache.h>
> 
> I don't think we need this ecache.h header.
> 
>> +#include <net/netfilter/nf_conntrack_extend.h>
> 
> You can remove _extend.h, already included by _synproxy.h
> 
>> +#include <net/netfilter/nf_conntrack_seqadj.h>
> 
> Do we need _seqadj.h?
> 

_seqadj it is requiered by nf_conntrack_synproxy.h. I suggest to move
this include statement there.

>> +#include <net/netfilter/nf_conntrack_synproxy.h>
>> +#include <net/netfilter/nf_synproxy.h>
>> +
> 
> remove empty line.
> 
>> +#include <linux/netfilter/nf_tables.h>
>> +#include <linux/netfilter/nf_SYNPROXY.h>
>> +
>> +struct nft_synproxy {
[...]
>> +
>> +	regs->verdict.code = NFT_CONTINUE;
>> +}
>> +
>> +#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
>> +static void nft_synproxy_eval_v6(const struct nft_expr *expr,
>> +				 struct nft_regs *regs,
>> +				 const struct nft_pktinfo *pkt)
>> +{
>> +	struct nft_synproxy *priv = nft_expr_priv(expr);
>> +	struct nf_synproxy_info info = priv->info;
>> +	struct synproxy_options opts = {};
>> +	struct net *net = nft_net(pkt);
>> +	struct synproxy_net *snet = synproxy_pernet(net);
>> +	struct sk_buff *skb = pkt->skb;
>> +	int thoff = pkt->xt.thoff;
>> +	const struct tcphdr *tcp;
>> +	struct tcphdr _tcph;
>> +
>> +	if (nf_ip_checksum(skb, nft_hook(pkt), thoff, IPPROTO_TCP)) {
>> +		regs->verdict.code = NF_DROP;
>> +		return;
>> +	}
>> +
>> +	tcp = skb_header_pointer(skb, ip_hdrlen(skb),
> 
> ip_hdrlen() won't fly for IPv6.
> 

I've decided to use pkt->xt.thoff instead of ip_hdrlen(skb) in order to
do it compatible with IPv4 and IPv6. Do you agree with that?

>> +				 sizeof(struct tcphdr), &_tcph);
>> +	if (!tcp) {
>> +		regs->verdict.code = NF_DROP;
>> +		return;
>> +	}
>> +
>> +	if (!synproxy_parse_options(skb, thoff, tcp, &opts)) {
>> +		regs->verdict.code = NF_DROP;
>> +		return;
>> +	}
>> +
>> +	if (tcp->syn) {
> 
> From here...
> 
>> +		/* Initial SYN from client */
>> +		this_cpu_inc(snet->stats->syn_received);
>> +
>> +		if (tcp->ece && tcp->cwr)
>> +			opts.options |= NF_SYNPROXY_OPT_ECN;
>> +
>> +		opts.options &= priv->info.options;
>> +		if (opts.options & NF_SYNPROXY_OPT_TIMESTAMP)
>> +			synproxy_init_timestamp_cookie(&info, &opts);
>> +		else
>> +			opts.options &= ~(NF_SYNPROXY_OPT_WSCALE |
>> +					  NF_SYNPROXY_OPT_SACK_PERM |
>> +					  NF_SYNPROXY_OPT_ECN);
> 
> ... to here. Could you wrap this code into a function? eg.
> 
>         nft_synproxy_tcp_options(...);
> 
> that can be shared by v4 and v6.
> 
>> +
>> +		synproxy_send_client_synack_ipv6(net, skb, tcp, &opts);
>> +		consume_skb(skb);
>> +		regs->verdict.code = NF_STOLEN;
>> +		return;
>> +	} else if (tcp->ack) {
>> +		/* ACK from client */
>> +		if (synproxy_recv_client_ack_ipv6(net, skb, tcp, &opts,
>> +						  ntohl(tcp->seq))) {
>> +			consume_skb(skb);
>> +			regs->verdict.code = NF_STOLEN;
>> +		} else {
>> +			regs->verdict.code = NF_DROP;
>> +		}
>> +		return;
>> +	}
>> +
>> +	regs->verdict.code = NFT_CONTINUE;
>> +}
>> +#endif /* CONFIG_NF_TABLES_IPV6*/
>> +
>> +static void nft_synproxy_eval(const struct nft_expr *expr,
>> +			      struct nft_regs *regs,
>> +			      const struct nft_pktinfo *pkt)
>> +{
>> +	struct sk_buff *skb = pkt->skb;
>> +	const struct tcphdr *tcp;
>> +	struct tcphdr _tcph;
>> +
>> +	tcp = skb_header_pointer(skb, ip_hdrlen(skb),
>> +				 sizeof(struct tcphdr), &_tcph);
>> +	if (!tcp) {
>> +		regs->verdict.code = NFT_BREAK;
>> +		return;
>> +	}
> 
> Hm. You should check for pkt->tprot to check if this is really TCP
> before trying to fetch the tcp header.
>>> +	switch (skb->protocol) {
>> +	case htons(ETH_P_IP):
>> +		nft_synproxy_eval_v4(expr, regs, pkt);
>> +		return;
>> +#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
>> +	case htons(ETH_P_IPV6):
>> +		nft_synproxy_eval_v6(expr, regs, pkt);
>> +		return;
>> +#endif
>> +	}
>> +	regs->verdict.code = NFT_BREAK;
>> +}
>> +
>> +static int nft_synproxy_init(const struct nft_ctx *ctx,
>> +			     const struct nft_expr *expr,
>> +			     const struct nlattr * const tb[])
>> +{
>> +	struct synproxy_net *snet = synproxy_pernet(ctx->net);
>> +	struct nft_synproxy *priv = nft_expr_priv(expr);
>> +	u32 flags;
>> +	int err;
>> +
>> +	err = nf_ct_netns_get(ctx->net, ctx->family);
>> +	if (err)
>> +		goto nf_ct_failure;
> 
> You just:
> 
>                 return err;
> 
> here, if nf_ct_netns_get() fails.
> 
>> +
>> +	switch (ctx->family) {
>> +	case NFPROTO_IPV4:
>> +		err = nf_synproxy_ipv4_init(snet, ctx->net);
>> +		if (err)
>> +			goto nf_ct_failure;
>> +		snet->hook_ref4++;
> 
> Why this manual bump of the reference counter? Doesn't
> nf_synproxy_ipv4_init() deal with this?
>>> +		break;
>> +#if IS_ENABLED(IPV6)
>> +	case NFPROTO_IPV6:
>> +		err = nf_synproxy_ipv6_init(snet, ctx->net);
>> +		if (err)
>> +			goto nf_ct_failure;
>> +		snet->hook_ref6++;
>> +		break;
>> +	case NFPROTO_INET:
>> +	case NFPROTO_BRIDGE:
>> +		err = nf_synproxy_ipv4_init(snet, ctx->net);
>> +		if (err)
>> +			goto nf_ct_failure;
>> +		err = nf_synproxy_ipv6_init(snet, ctx->net);
>> +		if (err)
>> +			goto nf_ct_failure;
>> +		snet->hook_ref4++;
>> +		snet->hook_ref6++;
>> +		break;
>> +#endif
>> +	}
>> +
>> +	if (tb[NFTA_SYNPROXY_MSS])
>> +		priv->info.mss = ntohs(nla_get_be16(tb[NFTA_SYNPROXY_MSS]));
>> +	if (tb[NFTA_SYNPROXY_WSCALE])
>> +		priv->info.wscale = nla_get_u8(tb[NFTA_SYNPROXY_WSCALE]);
>> +	if (tb[NFTA_SYNPROXY_FLAGS]) {
>> +		flags = ntohl(nla_get_be32(tb[NFTA_SYNPROXY_FLAGS]));
>> +		if (flags != 0 && (flags & NF_SYNPROXY_OPT_MASK) == 0)
>> +			return -EINVAL;
> 
> This -EINVAL ignores error nf_ct_failure error path. I suggest you
> move this code up to the top of nft_synproxy_init(), before calling
> nf_ct_netns_get().
> 
>> +		priv->info.options = flags;
>> +	}
>> +	return 0;
>> +
>> +nf_ct_failure:
>> +	nf_ct_netns_put(ctx->net, ctx->family);
>> +	return err;
>> +}
>> +
>> +static void nft_synproxy_destroy(const struct nft_ctx *ctx,
>> +				 const struct nft_expr *expr)
>> +{
>> +	struct synproxy_net *snet = synproxy_pernet(ctx->net);
>> +
>> +	switch (ctx->family) {
>> +	case NFPROTO_IPV4:
>> +		nf_synproxy_ipv4_fini(snet, ctx->net);
>> +		break;
>> +#if IS_ENABLED(IPV6)
> 
> This should be CONFIG_IPV6, right?
> 

Yes, but I think we should check CONFIG_NF_TABLES_IPV6 instead. What do
you think?

>> +	case NFPROTO_IPV6:
>> +		nf_synproxy_ipv6_fini(snet, ctx->net);
>> +		break;
> 
> #endif
> 
> for IPV6 here.
> 
>> +	case NFPROTO_INET:
> 
> Missing NFPROTO_BRIDGE here.
> 
>> +		nf_synproxy_ipv4_fini(snet, ctx->net);
>> +		nf_synproxy_ipv6_fini(snet, ctx->net);
>> +		break;
>> +#endif
>> +	}
>> +	nf_ct_netns_put(ctx->net, ctx->family);
>> +}
>> +
>> +static int nft_synproxy_dump(struct sk_buff *skb, const struct nft_expr *expr)
>> +{
>> +	const struct nft_synproxy *priv = nft_expr_priv(expr);
>> +
>> +	if (nla_put_be16(skb, NFTA_SYNPROXY_MSS, ntohs(priv->info.mss)) ||
>> +	    nla_put_u8(skb, NFTA_SYNPROXY_WSCALE, priv->info.wscale) ||
>> +	    nla_put_be32(skb, NFTA_SYNPROXY_FLAGS, ntohl(priv->info.options)))
>> +		goto nla_put_failure;
>> +
>> +	return 0;
>> +
>> +nla_put_failure:
>> +	return -1;
>> +}
>> +
>> +static int nft_synproxy_validate(const struct nft_ctx *ctx,
>> +				 const struct nft_expr *expr,
>> +				 const struct nft_data **data)
>> +{
>> +	return nft_chain_validate_hooks(ctx->chain, (1 << NF_INET_LOCAL_IN) |
>> +						    (1 << NF_INET_FORWARD));
>> +}
>> +
>> +static struct nft_expr_type nft_synproxy_type;
>> +static const struct nft_expr_ops nft_synproxy_ops = {
>> +	.eval		= nft_synproxy_eval,
>> +	.size		= NFT_EXPR_SIZE(sizeof(struct nft_synproxy)),
>> +	.init		= nft_synproxy_init,
>> +	.destroy	= nft_synproxy_destroy,
>> +	.dump		= nft_synproxy_dump,
>> +	.type		= &nft_synproxy_type,
>> +	.validate	= nft_synproxy_validate,
>> +};
>> +
>> +static struct nft_expr_type nft_synproxy_type __read_mostly = {
>> +	.ops		= &nft_synproxy_ops,
>> +	.name		= "synproxy",
>> +	.owner		= THIS_MODULE,
>> +	.policy		= nft_synproxy_policy,
>> +	.maxattr	= NFTA_OSF_MAX,
>> +};
>> +
>> +static int __init nft_synproxy_module_init(void)
>> +{
>> +	return nft_register_expr(&nft_synproxy_type);
>> +}
>> +
>> +static void __exit nft_synproxy_module_exit(void)
>> +{
>> +	return nft_unregister_expr(&nft_synproxy_type);
>> +}
>> +
>> +module_init(nft_synproxy_module_init);
>> +module_exit(nft_synproxy_module_exit);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Fernando Fernandez <ffmancera@xxxxxxxxxx>");
>> +MODULE_ALIAS_NFT_EXPR("synproxy");
>> -- 
>> 2.20.1
>>

Thanks Pablo!



[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux