netdev - Re: [PATCH 3/5] netfilter: add SYNPROXY core/target

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: <20130807205602.GA21463@macbook.localnet>
Date:	Wed, 7 Aug 2013 22:56:03 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:	pablo@...filter.org, netfilter-devel@...r.kernel.org,
	netdev@...r.kernel.org, mph@....com, as@....com
Subject: Re: [PATCH 3/5] netfilter: add SYNPROXY core/target

On Wed, Aug 07, 2013 at 10:26:00PM +0200, Jesper Dangaard Brouer wrote:
> On Wed,  7 Aug 2013 19:42:49 +0200
> Patrick McHardy <kaber@...sh.net> wrote:
> 
> > +	nth = (struct tcphdr *)skb_put(nskb, tcp_hdr_size);
> > +	nth->source	= th->source;
> > +	nth->dest	= th->dest;
> > +	nth->seq	= htonl(ntohl(th->seq) - 1);
> > +	nth->ack_seq	= htonl(ntohl(th->ack_seq) - 1);;
> 
> Strange double ";;"

Thanks, fixed.

> Besides shouldn't nth->ack_seq be zero, in a SYN packet? This is the
> SYN "replayed" towards the server right?
> 
> I also pointed to this in an earlier patch Martin showed me, but he
> reported that changing this resulted in bad behavior.  So, I would
> request Martin to re-test this part.

Right, it should be zero, but it doesn't matter since the ACK flag isn't
set. This is used to propagate the sequence number to the hook function
to initialize the sequence adjustment data. While in the target function,
we don't have any connection tracking state to store this in. We could
set it to zero after that, but it shouldn't matter.

> > +static unsigned int
> > +synproxy_tg4(struct sk_buff *skb, const struct xt_action_param *par)
> > +{
> > +	const struct xt_synproxy_info *info = par->targinfo;
> > +	struct synproxy_net *snet = synproxy_pernet(dev_net(par->in));
> > +	struct synproxy_options opts = {};
> > +	struct tcphdr *th, _th;
> > +
> > +	if (nf_ip_checksum(skb, par->hooknum, par->thoff, IPPROTO_TCP))
> > +		return NF_DROP;
> > +
> > +	th = skb_header_pointer(skb, par->thoff, sizeof(_th), &_th);
> > +	if (th == NULL)
> > +		return NF_DROP;
> > +
> > +	synproxy_parse_options(skb, par->thoff, th, &opts);
> > +
> > +	if (th->syn) {
> > +		/* Initial SYN from client */
> > +		this_cpu_inc(snet->stats->syn_received);
> > +
> > +		if (th->ece && th->cwr)
> > +			opts.options |= XT_SYNPROXY_OPT_ECN;
> > +
> > +		opts.options &= info->options;
> > +		if (opts.options & XT_SYNPROXY_OPT_TIMESTAMP)
> > +			synproxy_init_timestamp_cookie(info, &opts);
> > +		else
> > +			opts.options &= ~(XT_SYNPROXY_OPT_WSCALE |
> > +					  XT_SYNPROXY_OPT_SACK_PERM |
> > +					  XT_SYNPROXY_OPT_ECN);
> > +
> > +		synproxy_send_client_synack(skb, th, &opts);
> > +	} else if (th->ack && !(th->fin || th->rst)) {
> 
> This could also match SYN+ACK... we are only interested in the ACK
> (from the 3WHS) here, right?

Right, we shouldn't see a SYN/ACK here, but I'll add an explicit check.

> > +			switch (opcode) {
> > +			case TCPOPT_MSS:
> > +				if (opsize == TCPOLEN_MSS) {
> > +					 opts->mss = get_unaligned_be16(ptr);
> 
> Strange indention, extra space before "opts->mss".
> 
> > +					 opts->options |= XT_SYNPROXY_OPT_MSS;
> 
> Strange indention, extra space before "opts->options".

Thanks, fixed.

> > +static int synproxy_cpu_seq_show(struct seq_file *seq, void *v)
> > +{
> > +	struct synproxy_stats *stats = v;
> > +
> > +	if (v == SEQ_START_TOKEN) {
> > +		seq_printf(seq, "syn_received\tcookie_invalid\tcookie_valid\n");
> > +		return 0;
> > +	}
> > +
> > +	seq_printf(seq, "%08u\t%08x\t%08x\n",
> 
> Shouldn't all numbers be printed in hex? (%08u -> %08x)

Right, fixed.

> Besides when using net->proc_net_stat, then the first entry is usually
> "entries" which is not percpu, this will likely confusing the tool:
>   lnstat -f synproxy -c 42

I'll look into that.

Thanks Jesper.
--
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