diff mbox

tcp: md5: fix md5 RST when both sides have listener

Message ID 62162DF05402B341B3DB59932A1FA992B5B5B929BD@EUSAACMS0702.eamcs.ericsson.se
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Shawn Lu Jan. 31, 2012, 8:39 a.m. UTC 
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(-)

--
1.7.0.4

-----Original Message-----
From: Eric Dumazet [mailto:eric.dumazet@gmail.com] 
Sent: Monday, January 30, 2012 6:16 PM
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 31 janvier 2012 03:07, Shawn Lu <shawn.lu@ericsson.com> a écrit :
> 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 |   31 ++++++++++++++++++++++++++++++-
>  net/ipv6/tcp_ipv6.c |   34 ++++++++++++++++++++++++++++++++--
>  2 files changed, 62 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 
> 337ba4c..b2358b4 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -601,6 +601,9 @@ 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;
>  #endif
>        struct net *net;
>
> @@ -631,7 +634,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.
> +                */
> +               sk = 
> + __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 (!sk)
> +                       return;

Now you have a socket in sk, you also are responsible to release the refcount taken in __inet_lookup_listener()

> +               key = tcp_v4_md5_do_lookup(sk, ip_hdr(skb)->saddr);
> +               if (!key)
> +                       return;

leak refcount

> +               genhash = tcp_v4_md5_hash_skb(newhash, key,
> +                                             NULL, NULL, skb);
> +               if (genhash || memcmp(hash_location, newhash, 16) != 
> + 0)
> +                       return;

same leak

> +
> +       } 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) | diff --git 
> a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index 3edd05a..1da99fd 
> 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1074,6 +1074,12 @@ 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;
> +#endif
>
>        if (th->rst)
>                return;
> @@ -1082,8 +1088,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.
> +                */
> +               sk = inet6_lookup_listener(dev_net(skb_dst(skb)->dev),
> +                                          &tcp_hashinfo, 
> + &ipv6h->daddr,
> +                                          ntohs(th->source), 
> + inet6_iif(skb));
> +               if (!sk)
> +                       return;
> +
> +               key = tcp_v6_md5_do_lookup(sk, &ipv6h->saddr);
> +               if (!key)
> +                       return;

same leak

> +
> +               genhash = tcp_v6_md5_hash_skb(newhash, key,
> +                                             NULL, NULL, skb);
> +               if (genhash || memcmp(hash_location, newhash, 16) != 
> + 0)
> +                       return;

same leak

> +       } else {
> +               key = sk ? tcp_v6_md5_do_lookup(sk, &ipv6h->saddr) : 
> + NULL;
> +       }
>  #endif
>
>        if (th->ack)
> --
> 1.7.0.4
>
> --
> 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
--
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

Comments

Eric Dumazet Jan. 31, 2012, 9:05 a.m. UTC | #1 
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
Eric Dumazet Jan. 31, 2012, 1:33 p.m. UTC | #2 
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
Shawn Lu Jan. 31, 2012, 6:15 p.m. UTC | #3 
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 mbox

Patch

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,