Re: [MPTCP] [PATCH 13/18] tcp_md5: Detect key inside tcp_v6_send_response instead of passing it as an argument

[Christoph Paasch <cpaasch at apple.com>]

[-- Attachment #1: Type: text/plain, Size: 7330 bytes --]

On 05/10/17 - 13:24:14, Mat Martineau wrote:
> 
> On Tue, 3 Oct 2017, Christoph Paasch wrote:
> 
> > Signed-off-by: Christoph Paasch <cpaasch(a)apple.com>
> > ---
> > net/ipv6/tcp_ipv6.c | 106 ++++++++++++++++++++++++++--------------------------
> > 1 file changed, 53 insertions(+), 53 deletions(-)
> > 
> > diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> > index 17f7e0cd8755..f1afa3236c4a 100644
> > --- a/net/ipv6/tcp_ipv6.c
> > +++ b/net/ipv6/tcp_ipv6.c
> > @@ -775,8 +775,7 @@ static const struct tcp_request_sock_ops tcp_request_sock_ipv6_ops = {
> > 
> > static void tcp_v6_send_response(const struct sock *sk, struct sk_buff *skb, u32 seq,
> > 				 u32 ack, u32 win, u32 tsval, u32 tsecr,
> > -				 int oif, struct tcp_md5sig_key *key, int rst,
> > -				 u8 tclass, __be32 label)
> > +				 int oif, int rst, u8 tclass, __be32 label)
> > {
> > 	const struct tcphdr *th = tcp_hdr(skb);
> > 	struct tcphdr *t1;
> > @@ -789,6 +788,12 @@ static void tcp_v6_send_response(const struct sock *sk, struct sk_buff *skb, u32
> > 	struct dst_entry *dst;
> > 	__be32 *topt;
> > 
> > +#ifdef CONFIG_TCP_MD5SIG
> > +	struct tcp_md5sig_key *key = NULL;
> > +	const __u8 *hash_location = NULL;
> > +	struct ipv6hdr *ipv6h = ipv6_hdr(skb);
> > +#endif
> > +
> > 	buff = alloc_skb(MAX_HEADER + sizeof(struct ipv6hdr) + tot_len,
> > 			 GFP_ATOMIC);
> > 	if (!buff)
> > @@ -822,6 +827,48 @@ static void tcp_v6_send_response(const struct sock *sk, struct sk_buff *skb, u32
> > 	}
> > 
> > #ifdef CONFIG_TCP_MD5SIG
> > +	rcu_read_lock();
> > +	hash_location = tcp_parse_md5sig_option(th);
> > +	if (sk && sk_fullsock(sk)) {
> > +		key = tcp_v6_md5_do_lookup(sk, &ipv6h->saddr);
> > +	} else if (sk && sk->sk_state == TCP_TIME_WAIT) {
> > +		struct tcp_timewait_sock *tcptw = tcp_twsk(sk);
> > +
> > +		key = tcp_twsk_md5_key(tcptw);
> > +	} else if (sk && sk->sk_state == TCP_NEW_SYN_RECV) {
> > +		key = tcp_v6_md5_do_lookup(sk, &ipv6h->daddr);
> > +	} else if (hash_location) {
> > +		unsigned char newhash[16];
> > +		struct sock *sk1 = NULL;
> > +		int genhash;
> > +
> > +		/* active side is lost. Try to find listening socket through
> > +		 * source port, and then find md5 key through listening socket.
> > +		 * we are not loose security here:
> > +		 * Incoming packet is checked with md5 hash with finding key,
> > +		 * no RST generated if md5 hash doesn't match.
> > +		 */
> > +		sk1 = inet6_lookup_listener(dev_net(skb_dst(skb)->dev),
> > +					    &tcp_hashinfo, NULL, 0,
> > +					    &ipv6h->saddr,
> > +					    th->source, &ipv6h->daddr,
> > +					    ntohs(th->source), tcp_v6_iif(skb),
> > +					    tcp_v6_sdif(skb));
> > +		if (!sk1)
> > +			goto go_on;
> > +
> > +		key = tcp_v6_md5_do_lookup(sk1, &ipv6h->saddr);
> > +		if (!key)
> > +			goto go_on;
> > +
> > +		genhash = tcp_v6_md5_hash_skb(newhash, key, NULL, skb);
> > +		if (genhash || memcmp(hash_location, newhash, 16) != 0)
> > +			goto go_on;
> 
> I see where this came from in tcp_v6_send_reset(), but the pasted code
> doesn't preserve the same behavior

Which part are you worried about here? Which behavior would change?

> (and the goto does nothing, so the
> conditional does nothing). 

True, this goto here is not necessary.


Christoph

> I haven't looked ahead yet to see if it gets
> fixed up in a later patch, but it seems like the intent is for these patches
> to be bisectable.
> 
> 
> Mat
> 
> 
> > +	}
> > +
> > +go_on:
> > +	rcu_read_unlock();
> > +
> > 	if (key) {
> > 		*topt++ = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) |
> > 				(TCPOPT_MD5SIG << 8) | TCPOLEN_MD5SIG);
> > @@ -882,14 +929,6 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
> > {
> > 	const struct tcphdr *th = tcp_hdr(skb);
> > 	u32 seq = 0, ack_seq = 0;
> > -	struct tcp_md5sig_key *key = NULL;
> > -#ifdef CONFIG_TCP_MD5SIG
> > -	const __u8 *hash_location = NULL;
> > -	struct ipv6hdr *ipv6h = ipv6_hdr(skb);
> > -	unsigned char newhash[16];
> > -	int genhash;
> > -	struct sock *sk1 = NULL;
> > -#endif
> > 	int oif;
> > 
> > 	if (th->rst)
> > @@ -901,38 +940,6 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
> > 	if (!sk && !ipv6_unicast_destination(skb))
> > 		return;
> > 
> > -#ifdef CONFIG_TCP_MD5SIG
> > -	rcu_read_lock();
> > -	hash_location = tcp_parse_md5sig_option(th);
> > -	if (sk && sk_fullsock(sk)) {
> > -		key = tcp_v6_md5_do_lookup(sk, &ipv6h->saddr);
> > -	} else if (hash_location) {
> > -		/*
> > -		 * active side is lost. Try to find listening socket through
> > -		 * source port, and then find md5 key through listening socket.
> > -		 * we are not loose security here:
> > -		 * Incoming packet is checked with md5 hash with finding key,
> > -		 * no RST generated if md5 hash doesn't match.
> > -		 */
> > -		sk1 = inet6_lookup_listener(dev_net(skb_dst(skb)->dev),
> > -					   &tcp_hashinfo, NULL, 0,
> > -					   &ipv6h->saddr,
> > -					   th->source, &ipv6h->daddr,
> > -					   ntohs(th->source), tcp_v6_iif(skb),
> > -					   tcp_v6_sdif(skb));
> > -		if (!sk1)
> > -			goto out;
> > -
> > -		key = tcp_v6_md5_do_lookup(sk1, &ipv6h->saddr);
> > -		if (!key)
> > -			goto out;
> > -
> > -		genhash = tcp_v6_md5_hash_skb(newhash, key, NULL, skb);
> > -		if (genhash || memcmp(hash_location, newhash, 16) != 0)
> > -			goto out;
> > -	}
> > -#endif
> > -
> > 	if (th->ack)
> > 		seq = ntohl(th->ack_seq);
> > 	else
> > @@ -940,20 +947,14 @@ static void tcp_v6_send_reset(const struct sock *sk, struct sk_buff *skb)
> > 			  (th->doff << 2);
> > 
> > 	oif = sk ? sk->sk_bound_dev_if : 0;
> > -	tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, key, 1, 0, 0);
> > -
> > -#ifdef CONFIG_TCP_MD5SIG
> > -out:
> > -	rcu_read_unlock();
> > -#endif
> > +	tcp_v6_send_response(sk, skb, seq, ack_seq, 0, 0, 0, oif, 1, 0, 0);
> > }
> > 
> > static void tcp_v6_send_ack(const struct sock *sk, struct sk_buff *skb, u32 seq,
> > 			    u32 ack, u32 win, u32 tsval, u32 tsecr, int oif,
> > -			    struct tcp_md5sig_key *key, u8 tclass,
> > -			    __be32 label)
> > +			    u8 tclass, __be32 label)
> > {
> > -	tcp_v6_send_response(sk, skb, seq, ack, win, tsval, tsecr, oif, key, 0,
> > +	tcp_v6_send_response(sk, skb, seq, ack, win, tsval, tsecr, oif, 0,
> > 			     tclass, label);
> > }
> > 
> > @@ -965,7 +966,7 @@ static void tcp_v6_timewait_ack(struct sock *sk, struct sk_buff *skb)
> > 	tcp_v6_send_ack(sk, skb, tcptw->tw_snd_nxt, tcptw->tw_rcv_nxt,
> > 			tcptw->tw_rcv_wnd >> tw->tw_rcv_wscale,
> > 			tcp_time_stamp_raw() + tcptw->tw_ts_offset,
> > -			tcptw->tw_ts_recent, tw->tw_bound_dev_if, tcp_twsk_md5_key(tcptw),
> > +			tcptw->tw_ts_recent, tw->tw_bound_dev_if,
> > 			tw->tw_tclass, cpu_to_be32(tw->tw_flowlabel));
> > 
> > 	inet_twsk_put(tw);
> > @@ -988,7 +989,6 @@ static void tcp_v6_reqsk_send_ack(const struct sock *sk, struct sk_buff *skb,
> > 			req->rsk_rcv_wnd >> inet_rsk(req)->rcv_wscale,
> > 			tcp_time_stamp_raw() + tcp_rsk(req)->ts_off,
> > 			req->ts_recent, sk->sk_bound_dev_if,
> > -			tcp_v6_md5_do_lookup(sk, &ipv6_hdr(skb)->daddr),
> > 			0, 0);
> > }
> > 
> > -- 
> > 2.14.1
> > 
> > 
> 
> --
> Mat Martineau
> Intel OTC