Skip to content

Commit

Permalink
Merge branch 'tcp-md5-vrf-fix'
Browse files Browse the repository at this point in the history 
Leonard Crestez says:

====================
tcp: md5: Fix overlap between vrf and non-vrf keys

With net.ipv4.tcp_l3mdev_accept=1 it is possible for a listen socket to
accept connection from the same client address in different VRFs. It is
also possible to set different MD5 keys for these clients which differ only
in the tcpm_l3index field.

This appears to work when distinguishing between different VRFs but not
between non-VRF and VRF connections. In particular:

* tcp_md5_do_lookup_exact will match a non-vrf key against a vrf key. This
means that adding a key with l3index != 0 after a key with l3index == 0
will cause the earlier key to be deleted. Both keys can be present if the
non-vrf key is added later.
* _tcp_md5_do_lookup can match a non-vrf key before a vrf key. This casues
failures if the passwords differ.

This can be fixed by making tcp_md5_do_lookup_exact perform an actual exact
comparison on l3index and by making  __tcp_md5_do_lookup perfer vrf-bound
keys above other considerations like prefixlen.

The fact that keys with l3index==0 affect VRF connections is usually not
desirable, VRFs are meant to be completely independent. This behavior needs
to preserved for backwards compatibility. Also, applications can just bind
listen sockets to VRF and never specify TCP_MD5SIG_FLAG_IFINDEX at all.

So far the combination of TCP_MD5SIG_FLAG_IFINDEX with tcpm_ifindex == 0
was an error, accept this to mean "key only applies to default VRF". This
is what applications using VRFs for traffic separation want.

This also contains tests for the second part. It does not contain tests for
overlapping keys, that would require more changes in nettest to add
multiple keys. These scenarios are also covered by my tests for TCP-AO,
especially around this area:
https://github.com/cdleonard/tcp-authopt-test/blob/main/tcp_authopt_test/test_vrf_bind.py

Changes since V2:
* Rename --do-bind-key-ifindex to --force-bind-key-ifindex
* Fix referencing TCP_MD5SIG_FLAG_IFINDEX as TCP_MD5SIG_IFINDEX
Link to v2: https://lore.kernel.org/netdev/cover.1634107317.git.cdleonard@gmail.com/

Changes since V1:
* Accept (TCP_MD5SIG_IFINDEX with tcpm_ifindex == 0)
* Add flags for explicitly including or excluding TCP_MD5SIG_FLAG_IFINDEX
to nettest
* Add few more tests in fcnal-test.sh.
Link to v1: https://lore.kernel.org/netdev/3d8387d499f053dba5cd9184c0f7b8445c4470c6.1633542093.git.cdleonard@gmail.com/
====================

Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
David S. Miller committed Oct 15, 2021
2 parents 46393d6 + 64e4017 commit 4884ddb
Show file tree
Hide file tree
Showing 5 changed files with 130 additions and 23 deletions.
5 changes: 3 additions & 2 deletions include/net/tcp.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1576,6 +1576,7 @@ struct tcp_md5sig_key {
u8 keylen;
u8 family; /* AF_INET or AF_INET6 */
u8 prefixlen;
u8 flags;
union tcp_md5_addr addr;
int l3index; /* set if key added with L3 scope */
u8 key[TCP_MD5SIG_MAXKEYLEN];
Expand Down Expand Up @@ -1621,10 +1622,10 @@ struct tcp_md5sig_pool {
int tcp_v4_md5_hash_skb(char *md5_hash, const struct tcp_md5sig_key *key,
const struct sock *sk, const struct sk_buff *skb);
int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
int family, u8 prefixlen, int l3index,
int family, u8 prefixlen, int l3index, u8 flags,
const u8 *newkey, u8 newkeylen, gfp_t gfp);
int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr,
int family, u8 prefixlen, int l3index);
int family, u8 prefixlen, int l3index, u8 flags);
struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk,
const struct sock *addr_sk);

Expand Down
45 changes: 32 additions & 13 deletions net/ipv4/tcp_ipv4.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1037,6 +1037,20 @@ static void tcp_v4_reqsk_destructor(struct request_sock *req)
DEFINE_STATIC_KEY_FALSE(tcp_md5_needed);
EXPORT_SYMBOL(tcp_md5_needed);

static bool better_md5_match(struct tcp_md5sig_key *old, struct tcp_md5sig_key *new)
{
if (!old)
return true;

/* l3index always overrides non-l3index */
if (old->l3index && new->l3index == 0)
return false;
if (old->l3index == 0 && new->l3index)
return true;

return old->prefixlen < new->prefixlen;
}

/* Find the Key structure for an address. */
struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index,
const union tcp_md5_addr *addr,
Expand All @@ -1059,7 +1073,7 @@ struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index,
lockdep_sock_is_held(sk)) {
if (key->family != family)
continue;
if (key->l3index && key->l3index != l3index)
if (key->flags & TCP_MD5SIG_FLAG_IFINDEX && key->l3index != l3index)
continue;
if (family == AF_INET) {
mask = inet_make_mask(key->prefixlen);
Expand All @@ -1074,8 +1088,7 @@ struct tcp_md5sig_key *__tcp_md5_do_lookup(const struct sock *sk, int l3index,
match = false;
}

if (match && (!best_match ||
key->prefixlen > best_match->prefixlen))
if (match && better_md5_match(best_match, key))
best_match = key;
}
return best_match;
Expand All @@ -1085,7 +1098,7 @@ EXPORT_SYMBOL(__tcp_md5_do_lookup);
static struct tcp_md5sig_key *tcp_md5_do_lookup_exact(const struct sock *sk,
const union tcp_md5_addr *addr,
int family, u8 prefixlen,
int l3index)
int l3index, u8 flags)
{
const struct tcp_sock *tp = tcp_sk(sk);
struct tcp_md5sig_key *key;
Expand All @@ -1105,7 +1118,9 @@ static struct tcp_md5sig_key *tcp_md5_do_lookup_exact(const struct sock *sk,
lockdep_sock_is_held(sk)) {
if (key->family != family)
continue;
if (key->l3index && key->l3index != l3index)
if ((key->flags & TCP_MD5SIG_FLAG_IFINDEX) != (flags & TCP_MD5SIG_FLAG_IFINDEX))
continue;
if (key->l3index != l3index)
continue;
if (!memcmp(&key->addr, addr, size) &&
key->prefixlen == prefixlen)
Expand All @@ -1129,15 +1144,15 @@ EXPORT_SYMBOL(tcp_v4_md5_lookup);

/* This can be called on a newly created socket, from other files */
int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
int family, u8 prefixlen, int l3index,
int family, u8 prefixlen, int l3index, u8 flags,
const u8 *newkey, u8 newkeylen, gfp_t gfp)
{
/* Add Key to the list */
struct tcp_md5sig_key *key;
struct tcp_sock *tp = tcp_sk(sk);
struct tcp_md5sig_info *md5sig;

key = tcp_md5_do_lookup_exact(sk, addr, family, prefixlen, l3index);
key = tcp_md5_do_lookup_exact(sk, addr, family, prefixlen, l3index, flags);
if (key) {
/* Pre-existing entry - just update that one.
* Note that the key might be used concurrently.
Expand Down Expand Up @@ -1182,6 +1197,7 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
key->family = family;
key->prefixlen = prefixlen;
key->l3index = l3index;
key->flags = flags;
memcpy(&key->addr, addr,
(family == AF_INET6) ? sizeof(struct in6_addr) :
sizeof(struct in_addr));
Expand All @@ -1191,11 +1207,11 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
EXPORT_SYMBOL(tcp_md5_do_add);

int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr, int family,
u8 prefixlen, int l3index)
u8 prefixlen, int l3index, u8 flags)
{
struct tcp_md5sig_key *key;

key = tcp_md5_do_lookup_exact(sk, addr, family, prefixlen, l3index);
key = tcp_md5_do_lookup_exact(sk, addr, family, prefixlen, l3index, flags);
if (!key)
return -ENOENT;
hlist_del_rcu(&key->node);
Expand Down Expand Up @@ -1229,6 +1245,7 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, int optname,
const union tcp_md5_addr *addr;
u8 prefixlen = 32;
int l3index = 0;
u8 flags;

if (optlen < sizeof(cmd))
return -EINVAL;
Expand All @@ -1239,14 +1256,16 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, int optname,
if (sin->sin_family != AF_INET)
return -EINVAL;

flags = cmd.tcpm_flags & TCP_MD5SIG_FLAG_IFINDEX;

if (optname == TCP_MD5SIG_EXT &&
cmd.tcpm_flags & TCP_MD5SIG_FLAG_PREFIX) {
prefixlen = cmd.tcpm_prefixlen;
if (prefixlen > 32)
return -EINVAL;
}

if (optname == TCP_MD5SIG_EXT &&
if (optname == TCP_MD5SIG_EXT && cmd.tcpm_ifindex &&
cmd.tcpm_flags & TCP_MD5SIG_FLAG_IFINDEX) {
struct net_device *dev;

Expand All @@ -1267,12 +1286,12 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, int optname,
addr = (union tcp_md5_addr *)&sin->sin_addr.s_addr;

if (!cmd.tcpm_keylen)
return tcp_md5_do_del(sk, addr, AF_INET, prefixlen, l3index);
return tcp_md5_do_del(sk, addr, AF_INET, prefixlen, l3index, flags);

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
return -EINVAL;

return tcp_md5_do_add(sk, addr, AF_INET, prefixlen, l3index,
return tcp_md5_do_add(sk, addr, AF_INET, prefixlen, l3index, flags,
cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
}

Expand Down Expand Up @@ -1596,7 +1615,7 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
* memory, then we end up not copying the key
* across. Shucks.
*/
tcp_md5_do_add(newsk, addr, AF_INET, 32, l3index,
tcp_md5_do_add(newsk, addr, AF_INET, 32, l3index, key->flags,
key->key, key->keylen, GFP_ATOMIC);
sk_nocaps_add(newsk, NETIF_F_GSO_MASK);
}
Expand Down
15 changes: 9 additions & 6 deletions net/ipv6/tcp_ipv6.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, int optname,
struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&cmd.tcpm_addr;
int l3index = 0;
u8 prefixlen;
u8 flags;

if (optlen < sizeof(cmd))
return -EINVAL;
Expand All @@ -609,6 +610,8 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, int optname,
if (sin6->sin6_family != AF_INET6)
return -EINVAL;

flags = cmd.tcpm_flags & TCP_MD5SIG_FLAG_IFINDEX;

if (optname == TCP_MD5SIG_EXT &&
cmd.tcpm_flags & TCP_MD5SIG_FLAG_PREFIX) {
prefixlen = cmd.tcpm_prefixlen;
Expand All @@ -619,7 +622,7 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, int optname,
prefixlen = ipv6_addr_v4mapped(&sin6->sin6_addr) ? 32 : 128;
}

if (optname == TCP_MD5SIG_EXT &&
if (optname == TCP_MD5SIG_EXT && cmd.tcpm_ifindex &&
cmd.tcpm_flags & TCP_MD5SIG_FLAG_IFINDEX) {
struct net_device *dev;

Expand All @@ -640,22 +643,22 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, int optname,
if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
AF_INET, prefixlen,
l3index);
l3index, flags);
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
AF_INET6, prefixlen, l3index);
AF_INET6, prefixlen, l3index, flags);
}

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
return -EINVAL;

if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
AF_INET, prefixlen, l3index,
AF_INET, prefixlen, l3index, flags,
cmd.tcpm_key, cmd.tcpm_keylen,
GFP_KERNEL);

return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
AF_INET6, prefixlen, l3index,
AF_INET6, prefixlen, l3index, flags,
cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
}

Expand Down Expand Up @@ -1404,7 +1407,7 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
* across. Shucks.
*/
tcp_md5_do_add(newsk, (union tcp_md5_addr *)&newsk->sk_v6_daddr,
AF_INET6, 128, l3index, key->key, key->keylen,
AF_INET6, 128, l3index, key->flags, key->key, key->keylen,
sk_gfp_mask(sk, GFP_ATOMIC));
}
#endif
Expand Down
60 changes: 60 additions & 0 deletions tools/testing/selftests/net/fcnal-test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,12 @@ set_sysctl()
run_cmd sysctl -q -w $*
}

# get sysctl values in NS-A
get_sysctl()
{
${NSA_CMD} sysctl -n $*
}

################################################################################
# Setup for tests

Expand Down Expand Up @@ -1003,6 +1009,60 @@ ipv4_tcp_md5()
run_cmd nettest -s -I ${NSA_DEV} -M ${MD5_PW} -m ${NS_NET}
log_test $? 1 "MD5: VRF: Device must be a VRF - prefix"

test_ipv4_md5_vrf__vrf_server__no_bind_ifindex
test_ipv4_md5_vrf__global_server__bind_ifindex0
}

test_ipv4_md5_vrf__vrf_server__no_bind_ifindex()
{
log_start
show_hint "Simulates applications using VRF without TCP_MD5SIG_FLAG_IFINDEX"
run_cmd nettest -s -I ${VRF} -M ${MD5_PW} -m ${NS_NET} --no-bind-key-ifindex &
sleep 1
run_cmd_nsb nettest -r ${NSA_IP} -X ${MD5_PW}
log_test $? 0 "MD5: VRF: VRF-bound server, unbound key accepts connection"

log_start
show_hint "Binding both the socket and the key is not required but it works"
run_cmd nettest -s -I ${VRF} -M ${MD5_PW} -m ${NS_NET} --force-bind-key-ifindex &
sleep 1
run_cmd_nsb nettest -r ${NSA_IP} -X ${MD5_PW}
log_test $? 0 "MD5: VRF: VRF-bound server, bound key accepts connection"
}

test_ipv4_md5_vrf__global_server__bind_ifindex0()
{
# This particular test needs tcp_l3mdev_accept=1 for Global server to accept VRF connections
local old_tcp_l3mdev_accept
old_tcp_l3mdev_accept=$(get_sysctl net.ipv4.tcp_l3mdev_accept)
set_sysctl net.ipv4.tcp_l3mdev_accept=1

log_start
run_cmd nettest -s -M ${MD5_PW} -m ${NS_NET} --force-bind-key-ifindex &
sleep 1
run_cmd_nsb nettest -r ${NSA_IP} -X ${MD5_PW}
log_test $? 2 "MD5: VRF: Global server, Key bound to ifindex=0 rejects VRF connection"

log_start
run_cmd nettest -s -M ${MD5_PW} -m ${NS_NET} --force-bind-key-ifindex &
sleep 1
run_cmd_nsc nettest -r ${NSA_IP} -X ${MD5_PW}
log_test $? 0 "MD5: VRF: Global server, key bound to ifindex=0 accepts non-VRF connection"
log_start

run_cmd nettest -s -M ${MD5_PW} -m ${NS_NET} --no-bind-key-ifindex &
sleep 1
run_cmd_nsb nettest -r ${NSA_IP} -X ${MD5_PW}
log_test $? 0 "MD5: VRF: Global server, key not bound to ifindex accepts VRF connection"

log_start
run_cmd nettest -s -M ${MD5_PW} -m ${NS_NET} --no-bind-key-ifindex &
sleep 1
run_cmd_nsc nettest -r ${NSA_IP} -X ${MD5_PW}
log_test $? 0 "MD5: VRF: Global server, key not bound to ifindex accepts non-VRF connection"

# restore value
set_sysctl net.ipv4.tcp_l3mdev_accept="$old_tcp_l3mdev_accept"
}

ipv4_tcp_novrf()
Expand Down
28 changes: 26 additions & 2 deletions tools/testing/selftests/net/nettest.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <unistd.h>
#include <time.h>
#include <errno.h>
#include <getopt.h>

#include <linux/xfrm.h>
#include <linux/ipsec.h>
Expand Down Expand Up @@ -101,6 +102,8 @@ struct sock_args {
struct sockaddr_in6 v6;
} md5_prefix;
unsigned int prefix_len;
/* 0: default, -1: force off, +1: force on */
int bind_key_ifindex;

/* expected addresses and device index for connection */
const char *expected_dev;
Expand Down Expand Up @@ -271,11 +274,14 @@ static int tcp_md5sig(int sd, void *addr, socklen_t alen, struct sock_args *args
}
memcpy(&md5sig.tcpm_addr, addr, alen);

if (args->ifindex) {
if ((args->ifindex && args->bind_key_ifindex >= 0) || args->bind_key_ifindex >= 1) {
opt = TCP_MD5SIG_EXT;
md5sig.tcpm_flags |= TCP_MD5SIG_FLAG_IFINDEX;

md5sig.tcpm_ifindex = args->ifindex;
log_msg("TCP_MD5SIG_FLAG_IFINDEX set tcpm_ifindex=%d\n", md5sig.tcpm_ifindex);
} else {
log_msg("TCP_MD5SIG_FLAG_IFINDEX off\n", md5sig.tcpm_ifindex);
}

rc = setsockopt(sd, IPPROTO_TCP, opt, &md5sig, sizeof(md5sig));
Expand Down Expand Up @@ -1822,6 +1828,14 @@ static int ipc_parent(int cpid, int fd, struct sock_args *args)
}

#define GETOPT_STR "sr:l:c:p:t:g:P:DRn:M:X:m:d:I:BN:O:SCi6xL:0:1:2:3:Fbq"
#define OPT_FORCE_BIND_KEY_IFINDEX 1001
#define OPT_NO_BIND_KEY_IFINDEX 1002

static struct option long_opts[] = {
{"force-bind-key-ifindex", 0, 0, OPT_FORCE_BIND_KEY_IFINDEX},
{"no-bind-key-ifindex", 0, 0, OPT_NO_BIND_KEY_IFINDEX},
{0, 0, 0, 0}
};

static void print_usage(char *prog)
{
Expand Down Expand Up @@ -1858,6 +1872,10 @@ static void print_usage(char *prog)
" -M password use MD5 sum protection\n"
" -X password MD5 password for client mode\n"
" -m prefix/len prefix and length to use for MD5 key\n"
" --no-bind-key-ifindex: Force TCP_MD5SIG_FLAG_IFINDEX off\n"
" --force-bind-key-ifindex: Force TCP_MD5SIG_FLAG_IFINDEX on\n"
" (default: only if -I is passed)\n"
"\n"
" -g grp multicast group (e.g., 239.1.1.1)\n"
" -i interactive mode (default is echo and terminate)\n"
"\n"
Expand Down Expand Up @@ -1893,7 +1911,7 @@ int main(int argc, char *argv[])
* process input args
*/

while ((rc = getopt(argc, argv, GETOPT_STR)) != -1) {
while ((rc = getopt_long(argc, argv, GETOPT_STR, long_opts, NULL)) != -1) {
switch (rc) {
case 'B':
both_mode = 1;
Expand Down Expand Up @@ -1966,6 +1984,12 @@ int main(int argc, char *argv[])
case 'M':
args.password = optarg;
break;
case OPT_FORCE_BIND_KEY_IFINDEX:
args.bind_key_ifindex = 1;
break;
case OPT_NO_BIND_KEY_IFINDEX:
args.bind_key_ifindex = -1;
break;
case 'X':
args.client_pw = optarg;
break;
Expand Down

0 comments on commit 4884ddb

Please sign in to comment.