Message ID | 62162DF05402B341B3DB59932A1FA992B5B5B929BD@EUSAACMS0702.eamcs.ericsson.se |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Le mardi 31 janvier 2012 à 03:39 -0500, Shawn Lu a écrit : > Resubmit after fixing the sk refcount leak problem pointed out by Eric. > > > TCP RST mechanism is broken in TCP md5(RFC2385).When > connection is gone, md5 key is lost, sending RST without > md5 hash is deem to ignored by peer. This can be a problem > since RST help protocal like bgp fast recove from peer crash. > > In most case, users of tcp md5, such as bgp and ldp, have > listeners on both sides. md5 keys for peers are saved in > listening socket. When passive side connection is gone, > we can still get md5 key from listening socket. When active > side of connection is gone, we can try to find listening socket > through source port, and then md5 key. > we are not loosing sercuriy here: packet is valified checked with > md5 hash. No RST is generated if md5 hash doesn't match or no md5 > key can be found. > > Signed-off-by: Shawn Lu <shawn.lu@ericsson.com> > --- > net/ipv4/tcp_ipv4.c | 38 +++++++++++++++++++++++++++++++++++++- > net/ipv6/tcp_ipv6.c | 41 +++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 76 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 337ba4c..6ed1c4a 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -601,6 +601,10 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) > struct ip_reply_arg arg; > #ifdef CONFIG_TCP_MD5SIG > struct tcp_md5sig_key *key; > + __u8 *hash_location = NULL; > + unsigned char newhash[16]; > + int genhash; > + struct sock *sk1 = NULL; > #endif > struct net *net; > > @@ -631,7 +635,33 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) > arg.iov[0].iov_len = sizeof(rep.th); > > #ifdef CONFIG_TCP_MD5SIG > - key = sk ? tcp_v4_md5_do_lookup(sk, ip_hdr(skb)->saddr) : NULL; > + hash_location = tcp_parse_md5sig_option(th); > + if (!sk && 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 = __inet_lookup_listener(dev_net(skb_dst(skb)->dev), > + &tcp_hashinfo, ip_hdr(skb)->daddr, > + ntohs(th->source), inet_iif(skb)); > + /* don't send rst if it can't find key */ > + if (!sk1) > + return; > + key = tcp_v4_md5_do_lookup(sk1, ip_hdr(skb)->saddr); Hmm... The second problem is that its not safe to call tcp_v4_md5_do_lookup() on an unlocked socket. And locking a listener is way too expensive, since a listener socket is already a contention point. An attacker could send forged tcp md5 packets to slow down a server. A proper patch needs RCU conversion first. > + if (!key) > + goto release_sk1; > + genhash = tcp_v4_md5_hash_skb(newhash, key, > + NULL, NULL, skb); > + if (genhash || memcmp(hash_location, newhash, 16) != 0) > + goto release_sk1; > + > + } else { > + key = sk ? tcp_v4_md5_do_lookup(sk, ip_hdr(skb)->saddr) : NULL; > + } > + > if (key) { > rep.opt[0] = htonl((TCPOPT_NOP << 24) | > (TCPOPT_NOP << 16) | > @@ -659,6 +689,12 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) > > TCP_INC_STATS_BH(net, TCP_MIB_OUTSEGS); > TCP_INC_STATS_BH(net, TCP_MIB_OUTRSTS); > + > +#ifdef CONFIG_TCP_MD5SIG > +release_sk1: > + if (sk1) > + sock_put(sk1); > +#endif > } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le mardi 31 janvier 2012 à 10:05 +0100, Eric Dumazet a écrit : > Hmm... The second problem is that its not safe to call > tcp_v4_md5_do_lookup() on an unlocked socket. > > And locking a listener is way too expensive, since a listener socket is > already a contention point. > > An attacker could send forged tcp md5 packets to slow down a server. > > A proper patch needs RCU conversion first. I am working on this RCU conversion and big md5 cleanup, using a single list of keys. You can then add your fix on top of this work. union tcp_md5_addr { struct in_addr a4; #if IS_ENABLED(CONFIG_IPV6) struct in6_addr a6; #endif }; /* - key database */ struct tcp_md5sig_key { struct hlist_node node; u8 keylen; u8 family; /* AF_INET or AF_INET6 */ union tcp_md5_addr addr; u8 key[TCP_MD5SIG_MAXKEYLEN]; struct rcu_head rcu; }; /* - sock block */ struct tcp_md5sig_info { struct hlist_head head; }; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks! Eric. Good catch. Will using RCU to pretect the md5 key. -----Original Message----- From: Eric Dumazet [mailto:eric.dumazet@gmail.com] Sent: Tuesday, January 31, 2012 5:34 AM To: Shawn Lu Cc: davem@davemloft.net; netdev@vger.kernel.org; xiaoclu@gmail.com Subject: RE: [PATCH] tcp: md5: fix md5 RST when both sides have listener Le mardi 31 janvier 2012 à 10:05 +0100, Eric Dumazet a écrit : > Hmm... The second problem is that its not safe to call > tcp_v4_md5_do_lookup() on an unlocked socket. > > And locking a listener is way too expensive, since a listener socket > is already a contention point. > > An attacker could send forged tcp md5 packets to slow down a server. > > A proper patch needs RCU conversion first. I am working on this RCU conversion and big md5 cleanup, using a single list of keys. You can then add your fix on top of this work. [shawnlu] Good idea. I will resumit using rcu. union tcp_md5_addr { struct in_addr a4; #if IS_ENABLED(CONFIG_IPV6) struct in6_addr a6; #endif }; /* - key database */ struct tcp_md5sig_key { struct hlist_node node; u8 keylen; u8 family; /* AF_INET or AF_INET6 */ union tcp_md5_addr addr; u8 key[TCP_MD5SIG_MAXKEYLEN]; struct rcu_head rcu; }; /* - sock block */ struct tcp_md5sig_info { struct hlist_head head; }; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 337ba4c..6ed1c4a 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -601,6 +601,10 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) struct ip_reply_arg arg; #ifdef CONFIG_TCP_MD5SIG struct tcp_md5sig_key *key; + __u8 *hash_location = NULL; + unsigned char newhash[16]; + int genhash; + struct sock *sk1 = NULL; #endif struct net *net; @@ -631,7 +635,33 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) arg.iov[0].iov_len = sizeof(rep.th); #ifdef CONFIG_TCP_MD5SIG - key = sk ? tcp_v4_md5_do_lookup(sk, ip_hdr(skb)->saddr) : NULL; + hash_location = tcp_parse_md5sig_option(th); + if (!sk && 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 = __inet_lookup_listener(dev_net(skb_dst(skb)->dev), + &tcp_hashinfo, ip_hdr(skb)->daddr, + ntohs(th->source), inet_iif(skb)); + /* don't send rst if it can't find key */ + if (!sk1) + return; + key = tcp_v4_md5_do_lookup(sk1, ip_hdr(skb)->saddr); + if (!key) + goto release_sk1; + genhash = tcp_v4_md5_hash_skb(newhash, key, + NULL, NULL, skb); + if (genhash || memcmp(hash_location, newhash, 16) != 0) + goto release_sk1; + + } else { + key = sk ? tcp_v4_md5_do_lookup(sk, ip_hdr(skb)->saddr) : NULL; + } + if (key) { rep.opt[0] = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) | @@ -659,6 +689,12 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) TCP_INC_STATS_BH(net, TCP_MIB_OUTSEGS); TCP_INC_STATS_BH(net, TCP_MIB_OUTRSTS); + +#ifdef CONFIG_TCP_MD5SIG +release_sk1: + if (sk1) + sock_put(sk1); +#endif } /* The code following below sending ACKs in SYN-RECV and TIME-WAIT states diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index 3edd05a..32507e8 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -1074,6 +1074,13 @@ static void tcp_v6_send_reset(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 + __u8 *hash_location = NULL; + struct ipv6hdr *ipv6h = ipv6_hdr(skb); + unsigned char newhash[16]; + int genhash; + struct sock *sk1 = NULL; +#endif if (th->rst) return; @@ -1082,8 +1089,32 @@ static void tcp_v6_send_reset(struct sock *sk, struct sk_buff *skb) return; #ifdef CONFIG_TCP_MD5SIG - if (sk) - key = tcp_v6_md5_do_lookup(sk, &ipv6_hdr(skb)->saddr); + hash_location = tcp_parse_md5sig_option(th); + if (!sk && 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, &ipv6h->daddr, + ntohs(th->source), inet6_iif(skb)); + if (!sk1) + return; + + key = tcp_v6_md5_do_lookup(sk1, &ipv6h->saddr); + if (!key) + goto release_sk1; + + genhash = tcp_v6_md5_hash_skb(newhash, key, + NULL, NULL, skb); + if (genhash || memcmp(hash_location, newhash, 16) != 0) + goto release_sk1; + } else { + key = sk ? tcp_v6_md5_do_lookup(sk, &ipv6h->saddr) : NULL; + } #endif if (th->ack) @@ -1093,6 +1124,12 @@ static void tcp_v6_send_reset(struct sock *sk, struct sk_buff *skb) (th->doff << 2); tcp_v6_send_response(skb, seq, ack_seq, 0, 0, key, 1, 0); + +#ifdef CONFIG_TCP_MD5SIG +release_sk1: + if (sk1) + sock_put(sk1); +#endif } static void tcp_v6_send_ack(struct sk_buff *skb, u32 seq, u32 ack, u32 win, u32 ts,