* [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
@ 2021-08-09 21:35 ` Leonard Crestez
2021-08-10 20:41 ` Dmitry Safonov
2021-08-09 21:35 ` [RFCv2 2/9] docs: Add user documentation for tcp_authopt Leonard Crestez
` (7 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Leonard Crestez @ 2021-08-09 21:35 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
This commit add support to add and remove keys but does not use them
further.
Similar to tcp md5 a single point to a struct tcp_authopt_info* struct
is added to struct tcp_sock in order to avoid increasing memory usage
for everybody. The data structures related to tcp_authopt are
initialized on setsockopt and only removed on socket close.
Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
include/linux/tcp.h | 6 ++
include/net/tcp.h | 1 +
include/net/tcp_authopt.h | 55 ++++++++++++
include/uapi/linux/tcp.h | 72 ++++++++++++++++
net/ipv4/Kconfig | 14 ++++
net/ipv4/Makefile | 1 +
net/ipv4/tcp.c | 27 ++++++
net/ipv4/tcp_authopt.c | 172 ++++++++++++++++++++++++++++++++++++++
net/ipv4/tcp_ipv4.c | 2 +
9 files changed, 350 insertions(+)
create mode 100644 include/net/tcp_authopt.h
create mode 100644 net/ipv4/tcp_authopt.c
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 48d8a363319e..cfddfc720b00 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -140,10 +140,12 @@ struct tcp_request_sock {
static inline struct tcp_request_sock *tcp_rsk(const struct request_sock *req)
{
return (struct tcp_request_sock *)req;
}
+struct tcp_authopt_info;
+
struct tcp_sock {
/* inet_connection_sock has to be the first member of tcp_sock */
struct inet_connection_sock inet_conn;
u16 tcp_header_len; /* Bytes of tcp header to send */
u16 gso_segs; /* Max number of segs per GSO packet */
@@ -403,10 +405,14 @@ struct tcp_sock {
/* TCP MD5 Signature Option information */
struct tcp_md5sig_info __rcu *md5sig_info;
#endif
+#ifdef CONFIG_TCP_AUTHOPT
+ struct tcp_authopt_info __rcu *authopt_info;
+#endif
+
/* TCP fastopen related information */
struct tcp_fastopen_request *fastopen_req;
/* fastopen_rsk points to request_sock that resulted in this big
* socket. Used to retransmit SYNACKs etc.
*/
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3166dc15d7d6..bb76554e8fe5 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -182,10 +182,11 @@ void tcp_time_wait(struct sock *sk, int state, int timeo);
#define TCPOPT_WINDOW 3 /* Window scaling */
#define TCPOPT_SACK_PERM 4 /* SACK Permitted */
#define TCPOPT_SACK 5 /* SACK Block */
#define TCPOPT_TIMESTAMP 8 /* Better RTT estimations/PAWS */
#define TCPOPT_MD5SIG 19 /* MD5 Signature (RFC2385) */
+#define TCPOPT_AUTHOPT 29 /* Auth Option (RFC5925) */
#define TCPOPT_MPTCP 30 /* Multipath TCP (RFC6824) */
#define TCPOPT_FASTOPEN 34 /* Fast open (RFC7413) */
#define TCPOPT_EXP 254 /* Experimental */
/* Magic number to be after the option value for sharing TCP
* experimental options. See draft-ietf-tcpm-experimental-options-00.txt
diff --git a/include/net/tcp_authopt.h b/include/net/tcp_authopt.h
new file mode 100644
index 000000000000..458d108bb7a8
--- /dev/null
+++ b/include/net/tcp_authopt.h
@@ -0,0 +1,55 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _LINUX_TCP_AUTHOPT_H
+#define _LINUX_TCP_AUTHOPT_H
+
+#include <uapi/linux/tcp.h>
+
+/* Representation of a Master Key Tuple as per RFC5925 */
+struct tcp_authopt_key_info {
+ struct hlist_node node;
+ /* Local identifier */
+ u32 local_id;
+ u32 flags;
+ /* Wire identifiers */
+ u8 send_id, recv_id;
+ u8 alg_id;
+ u8 keylen;
+ u8 key[TCP_AUTHOPT_MAXKEYLEN];
+ struct rcu_head rcu;
+ struct sockaddr_storage addr;
+};
+
+/* Per-socket information regarding tcp_authopt */
+struct tcp_authopt_info {
+ /* List of tcp_authopt_key_info */
+ struct hlist_head head;
+ u32 flags;
+ u32 src_isn;
+ u32 dst_isn;
+ struct rcu_head rcu;
+};
+
+#ifdef CONFIG_TCP_AUTHOPT
+void tcp_authopt_clear(struct sock *sk);
+int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen);
+int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *key);
+int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen);
+#else
+static inline int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen)
+{
+ return -ENOPROTOOPT;
+}
+static inline int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *key)
+{
+ return -ENOPROTOOPT;
+}
+static inline void tcp_authopt_clear(struct sock *sk)
+{
+}
+static inline int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
+{
+ return -ENOPROTOOPT;
+}
+#endif
+
+#endif /* _LINUX_TCP_AUTHOPT_H */
diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index 8fc09e8638b3..bc47664156eb 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -126,10 +126,12 @@ enum {
#define TCP_INQ 36 /* Notify bytes available to read as a cmsg on read */
#define TCP_CM_INQ TCP_INQ
#define TCP_TX_DELAY 37 /* delay outgoing packets by XX usec */
+#define TCP_AUTHOPT 38 /* TCP Authentication Option (RFC2385) */
+#define TCP_AUTHOPT_KEY 39 /* TCP Authentication Option update key (RFC2385) */
#define TCP_REPAIR_ON 1
#define TCP_REPAIR_OFF 0
#define TCP_REPAIR_OFF_NO_WP -1 /* Turn off without window probes */
@@ -340,10 +342,80 @@ struct tcp_diag_md5sig {
__u16 tcpm_keylen;
__be32 tcpm_addr[4];
__u8 tcpm_key[TCP_MD5SIG_MAXKEYLEN];
};
+/**
+ * enum tcp_authopt_flag - flags for `tcp_authopt.flags`
+ */
+enum tcp_authopt_flag {
+ /**
+ * @TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED:
+ * Configure behavior of segments with TCP-AO coming from hosts for which no
+ * key is configured. The default recommended by RFC is to silently accept
+ * such connections.
+ */
+ TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED = (1 << 2),
+};
+
+/**
+ * struct tcp_authopt - Per-socket options related to TCP Authentication Option
+ */
+struct tcp_authopt {
+ /** @flags: Combination of &enum tcp_authopt_flag */
+ __u32 flags;
+};
+
+/**
+ * enum tcp_authopt_key_flag - flags for `tcp_authopt.flags`
+ *
+ * @TCP_AUTHOPT_KEY_DEL: Delete the key by local_id and ignore all other fields.
+ * @TCP_AUTHOPT_KEY_EXCLUDE_OPTS: Exclude TCP options from signature.
+ * @TCP_AUTHOPT_KEY_ADDR_BIND: Key only valid for `tcp_authopt.addr`
+ */
+enum tcp_authopt_key_flag {
+ TCP_AUTHOPT_KEY_DEL = (1 << 0),
+ TCP_AUTHOPT_KEY_EXCLUDE_OPTS = (1 << 1),
+ TCP_AUTHOPT_KEY_ADDR_BIND = (1 << 2),
+};
+
+/* for TCP_AUTHOPT_KEY socket option */
+#define TCP_AUTHOPT_MAXKEYLEN 80
+
+enum tcp_authopt_alg {
+ TCP_AUTHOPT_ALG_HMAC_SHA_1_96 = 1,
+ TCP_AUTHOPT_ALG_AES_128_CMAC_96 = 2,
+};
+
+/**
+ * struct tcp_authopt_key - TCP Authentication KEY
+ *
+ * Each key is identified by a non-zero local_id which is managed by the application.
+ */
+struct tcp_authopt_key {
+ /** @flags: Combination of &enum tcp_authopt_key_flag */
+ __u32 flags;
+ /** @local_id: Local identifier */
+ __u32 local_id;
+ /** @send_id: keyid value for send */
+ __u8 send_id;
+ /** @recv_id: keyid value for receive */
+ __u8 recv_id;
+ /** @alg: One of &enum tcp_authopt_alg */
+ __u8 alg;
+ /** @keylen: Length of the key buffer */
+ __u8 keylen;
+ /** @key: Secret key */
+ __u8 key[TCP_AUTHOPT_MAXKEYLEN];
+ /**
+ * @addr: Key is only valid for this address
+ *
+ * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
+ */
+ struct __kernel_sockaddr_storage addr;
+};
+
/* setsockopt(fd, IPPROTO_TCP, TCP_ZEROCOPY_RECEIVE, ...) */
#define TCP_RECEIVE_ZEROCOPY_FLAG_TLB_CLEAN_HINT 0x1
struct tcp_zerocopy_receive {
__u64 address; /* in: address of mapping */
diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
index 87983e70f03f..6459f4ea6f1d 100644
--- a/net/ipv4/Kconfig
+++ b/net/ipv4/Kconfig
@@ -740,5 +740,19 @@ config TCP_MD5SIG
RFC2385 specifies a method of giving MD5 protection to TCP sessions.
Its main (only?) use is to protect BGP sessions between core routers
on the Internet.
If unsure, say N.
+
+config TCP_AUTHOPT
+ bool "TCP: Authentication Option support (RFC5925)"
+ select CRYPTO
+ select CRYPTO_SHA1
+ select CRYPTO_HMAC
+ select CRYPTO_AES
+ select CRYPTO_CMAC
+ help
+ RFC5925 specifies a new method of giving protection to TCP sessions.
+ Its intended use is to protect BGP sessions between core routers
+ on the Internet. It obsoletes TCP MD5 (RFC2385) but is incompatible.
+
+ If unsure, say N.
diff --git a/net/ipv4/Makefile b/net/ipv4/Makefile
index bbdd9c44f14e..d336f32ce177 100644
--- a/net/ipv4/Makefile
+++ b/net/ipv4/Makefile
@@ -59,10 +59,11 @@ obj-$(CONFIG_TCP_CONG_NV) += tcp_nv.o
obj-$(CONFIG_TCP_CONG_VENO) += tcp_veno.o
obj-$(CONFIG_TCP_CONG_SCALABLE) += tcp_scalable.o
obj-$(CONFIG_TCP_CONG_LP) += tcp_lp.o
obj-$(CONFIG_TCP_CONG_YEAH) += tcp_yeah.o
obj-$(CONFIG_TCP_CONG_ILLINOIS) += tcp_illinois.o
+obj-$(CONFIG_TCP_AUTHOPT) += tcp_authopt.o
obj-$(CONFIG_NET_SOCK_MSG) += tcp_bpf.o
obj-$(CONFIG_BPF_SYSCALL) += udp_bpf.o
obj-$(CONFIG_NETLABEL) += cipso_ipv4.o
obj-$(CONFIG_XFRM) += xfrm4_policy.o xfrm4_state.o xfrm4_input.o \
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index f931def6302e..fd90e80afa2c 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -271,10 +271,11 @@
#include <net/icmp.h>
#include <net/inet_common.h>
#include <net/tcp.h>
#include <net/mptcp.h>
+#include <net/tcp_authopt.h>
#include <net/xfrm.h>
#include <net/ip.h>
#include <net/sock.h>
#include <linux/uaccess.h>
@@ -3573,10 +3574,16 @@ static int do_tcp_setsockopt(struct sock *sk, int level, int optname,
case TCP_MD5SIG:
case TCP_MD5SIG_EXT:
err = tp->af_specific->md5_parse(sk, optname, optval, optlen);
break;
#endif
+ case TCP_AUTHOPT:
+ err = tcp_set_authopt(sk, optval, optlen);
+ break;
+ case TCP_AUTHOPT_KEY:
+ err = tcp_set_authopt_key(sk, optval, optlen);
+ break;
case TCP_USER_TIMEOUT:
/* Cap the max time in ms TCP will retry or probe the window
* before giving up and aborting (ETIMEDOUT) a connection.
*/
if (val < 0)
@@ -4219,10 +4226,30 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
if (!err && copy_to_user(optval, &zc, len))
err = -EFAULT;
return err;
}
#endif
+#ifdef CONFIG_TCP_AUTHOPT
+ case TCP_AUTHOPT: {
+ struct tcp_authopt info;
+
+ if (get_user(len, optlen))
+ return -EFAULT;
+
+ lock_sock(sk);
+ tcp_get_authopt_val(sk, &info);
+ release_sock(sk);
+
+ len = min_t(unsigned int, len, sizeof(info));
+ if (put_user(len, optlen))
+ return -EFAULT;
+ if (copy_to_user(optval, &info, len))
+ return -EFAULT;
+ return 0;
+ }
+#endif
+
default:
return -ENOPROTOOPT;
}
if (put_user(len, optlen))
diff --git a/net/ipv4/tcp_authopt.c b/net/ipv4/tcp_authopt.c
new file mode 100644
index 000000000000..5fa7bce8891b
--- /dev/null
+++ b/net/ipv4/tcp_authopt.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/kernel.h>
+#include <net/tcp.h>
+#include <net/tcp_authopt.h>
+#include <crypto/hash.h>
+#include <trace/events/tcp.h>
+
+struct tcp_authopt_key_info *__tcp_authopt_key_info_lookup(const struct sock *sk,
+ struct tcp_authopt_info *info,
+ int key_id)
+{
+ struct tcp_authopt_key_info *key;
+
+ hlist_for_each_entry_rcu(key, &info->head, node, lockdep_sock_is_held(sk))
+ if (key->local_id == key_id)
+ return key;
+
+ return NULL;
+}
+
+static struct tcp_authopt_info *__tcp_authopt_info_get_or_create(struct sock *sk)
+{
+ struct tcp_sock *tp = tcp_sk(sk);
+ struct tcp_authopt_info *info;
+
+ info = rcu_dereference_check(tp->authopt_info, lockdep_sock_is_held(sk));
+ if (info)
+ return info;
+
+ info = kmalloc(sizeof(*info), GFP_KERNEL | __GFP_ZERO);
+ if (!info)
+ return ERR_PTR(-ENOMEM);
+
+ sk_nocaps_add(sk, NETIF_F_GSO_MASK);
+ INIT_HLIST_HEAD(&info->head);
+ rcu_assign_pointer(tp->authopt_info, info);
+
+ return info;
+}
+
+int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen)
+{
+ struct tcp_authopt opt;
+ struct tcp_authopt_info *info;
+
+ WARN_ON(!lockdep_sock_is_held(sk));
+
+ /* If userspace optlen is too short fill the rest with zeros */
+ if (optlen > sizeof(opt))
+ return -EINVAL;
+ memset(&opt, 0, sizeof(opt));
+ if (copy_from_sockptr(&opt, optval, optlen))
+ return -EFAULT;
+
+ info = __tcp_authopt_info_get_or_create(sk);
+ if (IS_ERR(info))
+ return PTR_ERR(info);
+
+ info->flags = opt.flags & TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED);
+
+ return 0;
+}
+
+int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt)
+{
+ struct tcp_sock *tp = tcp_sk(sk);
+ struct tcp_authopt_info *info;
+
+ WARN_ON(!lockdep_sock_is_held(sk));
+ memset(opt, 0, sizeof(*opt));
+ info = rcu_dereference_check(tp->authopt_info, lockdep_sock_is_held(sk));
+ if (!info)
+ return -EINVAL;
+ opt->flags = info->flags & TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED;
+
+ return 0;
+}
+
+static void tcp_authopt_key_del(struct sock *sk,
+ struct tcp_authopt_info *info,
+ struct tcp_authopt_key_info *key)
+{
+ hlist_del_rcu(&key->node);
+ atomic_sub(sizeof(*key), &sk->sk_omem_alloc);
+ kfree_rcu(key, rcu);
+}
+
+/* free info and keys but don't touch tp->authopt_info */
+void __tcp_authopt_info_free(struct sock *sk, struct tcp_authopt_info *info)
+{
+ struct hlist_node *n;
+ struct tcp_authopt_key_info *key;
+
+ hlist_for_each_entry_safe(key, n, &info->head, node)
+ tcp_authopt_key_del(sk, info, key);
+ kfree_rcu(info, rcu);
+}
+
+/* free everything and clear tcp_sock.authopt_info to NULL */
+void tcp_authopt_clear(struct sock *sk)
+{
+ struct tcp_authopt_info *info;
+
+ info = rcu_dereference_protected(tcp_sk(sk)->authopt_info, lockdep_sock_is_held(sk));
+ if (info) {
+ __tcp_authopt_info_free(sk, info);
+ tcp_sk(sk)->authopt_info = NULL;
+ }
+}
+
+int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
+{
+ struct tcp_authopt_key opt;
+ struct tcp_authopt_info *info;
+ struct tcp_authopt_key_info *key_info;
+
+ /* If userspace optlen is too short fill the rest with zeros */
+ if (optlen > sizeof(opt))
+ return -EINVAL;
+ memset(&opt, 0, sizeof(opt));
+ if (copy_from_sockptr(&opt, optval, optlen))
+ return -EFAULT;
+
+ if (opt.keylen > TCP_AUTHOPT_MAXKEYLEN)
+ return -EINVAL;
+
+ if (opt.local_id == 0)
+ return -EINVAL;
+
+ /* Delete is a special case: we ignore all fields other than local_id */
+ if (opt.flags & TCP_AUTHOPT_KEY_DEL) {
+ info = rcu_dereference_check(tcp_sk(sk)->authopt_info, lockdep_sock_is_held(sk));
+ if (!info)
+ return -ENOENT;
+ key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
+ if (!key_info)
+ return -ENOENT;
+ tcp_authopt_key_del(sk, info, key_info);
+ return 0;
+ }
+
+ /* Initialize tcp_authopt_info if not already set */
+ info = __tcp_authopt_info_get_or_create(sk);
+ if (IS_ERR(info))
+ return PTR_ERR(info);
+
+ /* check key family */
+ if (opt.flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
+ if (sk->sk_family != opt.addr.ss_family)
+ return -EINVAL;
+ }
+
+ /* If an old value exists for same local_id it is deleted */
+ key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
+ if (key_info)
+ tcp_authopt_key_del(sk, info, key_info);
+ key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL | __GFP_ZERO);
+ if (!key_info)
+ return -ENOMEM;
+ key_info->local_id = opt.local_id;
+ key_info->flags = opt.flags & (TCP_AUTHOPT_KEY_EXCLUDE_OPTS | TCP_AUTHOPT_KEY_ADDR_BIND);
+ key_info->send_id = opt.send_id;
+ key_info->recv_id = opt.recv_id;
+ key_info->alg_id = opt.alg;
+ key_info->keylen = opt.keylen;
+ memcpy(key_info->key, opt.key, opt.keylen);
+ memcpy(&key_info->addr, &opt.addr, sizeof(key_info->addr));
+ hlist_add_head_rcu(&key_info->node, &info->head);
+
+ return 0;
+}
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 2e62e0d6373a..1348615c7576 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -60,10 +60,11 @@
#include <net/net_namespace.h>
#include <net/icmp.h>
#include <net/inet_hashtables.h>
#include <net/tcp.h>
+#include <net/tcp_authopt.h>
#include <net/transp_v6.h>
#include <net/ipv6.h>
#include <net/inet_common.h>
#include <net/timewait_sock.h>
#include <net/xfrm.h>
@@ -2256,10 +2257,11 @@ void tcp_v4_destroy_sock(struct sock *sk)
tcp_clear_md5_list(sk);
kfree_rcu(rcu_dereference_protected(tp->md5sig_info, 1), rcu);
tp->md5sig_info = NULL;
}
#endif
+ tcp_authopt_clear(sk);
/* Clean up a referenced TCP bind bucket. */
if (inet_csk(sk)->icsk_bind_hash)
inet_put_port(sk);
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-09 21:35 ` [RFCv2 1/9] tcp: authopt: Initial support and key management Leonard Crestez
@ 2021-08-10 20:41 ` Dmitry Safonov
2021-08-11 8:29 ` Leonard Crestez
0 siblings, 1 reply; 24+ messages in thread
From: Dmitry Safonov @ 2021-08-10 20:41 UTC (permalink / raw)
To: Leonard Crestez
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
Hi Leonard,
On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <cdleonard@gmail.com> wrote:
[..]
> +/* Representation of a Master Key Tuple as per RFC5925 */
> +struct tcp_authopt_key_info {
> + struct hlist_node node;
> + /* Local identifier */
> + u32 local_id;
There is no local_id in RFC5925, what's that?
An MKT is identified by (send_id, recv_id), together with
(src_addr/src_port, dst_addr/dst_port).
Why introducing something new to already complicated RFC?
> + u32 flags;
> + /* Wire identifiers */
> + u8 send_id, recv_id;
> + u8 alg_id;
> + u8 keylen;
> + u8 key[TCP_AUTHOPT_MAXKEYLEN];
> + struct rcu_head rcu;
This is unaligned and will add padding.
I wonder if it's also worth saving some bytes by something like
struct tcp_ao_key *key;
With
struct tcp_ao_key {
u8 keylen;
u8 key[0];
};
Hmm?
> + struct sockaddr_storage addr;
> +};
> +
> +/* Per-socket information regarding tcp_authopt */
> +struct tcp_authopt_info {
> + /* List of tcp_authopt_key_info */
> + struct hlist_head head;
> + u32 flags;
> + u32 src_isn;
> + u32 dst_isn;
> + struct rcu_head rcu;
Ditto, adds padding on 64-bit.
[..]
> +++ b/include/uapi/linux/tcp.h
> @@ -126,10 +126,12 @@ enum {
> #define TCP_INQ 36 /* Notify bytes available to read as a cmsg on read */
>
> #define TCP_CM_INQ TCP_INQ
>
> #define TCP_TX_DELAY 37 /* delay outgoing packets by XX usec */
> +#define TCP_AUTHOPT 38 /* TCP Authentication Option (RFC2385) */
> +#define TCP_AUTHOPT_KEY 39 /* TCP Authentication Option update key (RFC2385) */
RFC2385 is md5 one.
Also, should there be TCP_AUTHOPT_ADD_KEY, TCP_AUTHOPT_DELTE_KEY
instead of using flags inside setsocketopt()? (no hard feelings)
[..]
> +/**
> + * enum tcp_authopt_flag - flags for `tcp_authopt.flags`
> + */
> +enum tcp_authopt_flag {
> + /**
> + * @TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED:
> + * Configure behavior of segments with TCP-AO coming from hosts for which no
> + * key is configured. The default recommended by RFC is to silently accept
> + * such connections.
> + */
> + TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED = (1 << 2),
> +};
> +
> +/**
> + * struct tcp_authopt - Per-socket options related to TCP Authentication Option
> + */
> +struct tcp_authopt {
> + /** @flags: Combination of &enum tcp_authopt_flag */
> + __u32 flags;
> +};
I'm not sure what's the use of enum here, probably:
#define TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED BIT(2)
[..]
> +struct tcp_authopt_key {
> + /** @flags: Combination of &enum tcp_authopt_key_flag */
> + __u32 flags;
> + /** @local_id: Local identifier */
> + __u32 local_id;
> + /** @send_id: keyid value for send */
> + __u8 send_id;
> + /** @recv_id: keyid value for receive */
> + __u8 recv_id;
> + /** @alg: One of &enum tcp_authopt_alg */
> + __u8 alg;
> + /** @keylen: Length of the key buffer */
> + __u8 keylen;
> + /** @key: Secret key */
> + __u8 key[TCP_AUTHOPT_MAXKEYLEN];
> + /**
> + * @addr: Key is only valid for this address
> + *
> + * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
> + */
> + struct __kernel_sockaddr_storage addr;
> +};
It'll be an ABI if this is accepted. As it is - it can't support RFC5925 fully.
Extending syscall ABI is painful. I think, even the initial ABI *must* support
all possible features of the RFC.
In other words, there must be src_addr, src_port, dst_addr and dst_port.
I can see from docs you've written you don't want to support a mix of different
addr/port MKTs, so you can return -EINVAL or -ENOTSUPP for any value
but zero.
This will create an ABI that can be later extended to fully support RFC.
The same is about key: I don't think you need to define/use tcp_authopt_alg.
Just use algo name - that way TCP-AO will automatically be able to use
any algo supported by crypto engine.
See how xfrm does it, e.g.:
struct xfrm_algo_auth {
char alg_name[64];
unsigned int alg_key_len; /* in bits */
unsigned int alg_trunc_len; /* in bits */
char alg_key[0];
};
So you can let a user chose maclen instead of hard-coding it.
Much more extendable than what you propose.
[..]
> +#ifdef CONFIG_TCP_AUTHOPT
> + case TCP_AUTHOPT: {
> + struct tcp_authopt info;
> +
> + if (get_user(len, optlen))
> + return -EFAULT;
> +
> + lock_sock(sk);
> + tcp_get_authopt_val(sk, &info);
> + release_sock(sk);
> +
> + len = min_t(unsigned int, len, sizeof(info));
> + if (put_user(len, optlen))
> + return -EFAULT;
> + if (copy_to_user(optval, &info, len))
> + return -EFAULT;
> + return 0;
> + }
I'm pretty sure it's not a good choice to write partly tcp_authopt.
And a user has no way to check what's the correct len on this kernel.
Instead of len = min_t(unsigned int, len, sizeof(info)), it should be
if (len != sizeof(info))
return -EINVAL;
[..]
> +int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen)
> +{
> + struct tcp_authopt opt;
> + struct tcp_authopt_info *info;
> +
> + WARN_ON(!lockdep_sock_is_held(sk));
sock_owned_by_me(sk)
> +
> + /* If userspace optlen is too short fill the rest with zeros */
> + if (optlen > sizeof(opt))
> + return -EINVAL;
> + memset(&opt, 0, sizeof(opt));
it's 4 bytes, why not just (optlen != sizeof(opt))?
[..]
> +int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt)
> +{
> + struct tcp_sock *tp = tcp_sk(sk);
> + struct tcp_authopt_info *info;
> +
> + WARN_ON(!lockdep_sock_is_held(sk));
sock_owned_by_me(sk)
[..]
> +int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
> +{
> + struct tcp_authopt_key opt;
> + struct tcp_authopt_info *info;
> + struct tcp_authopt_key_info *key_info;
> +
> + /* If userspace optlen is too short fill the rest with zeros */
> + if (optlen > sizeof(opt))
> + return -EINVAL;
> + memset(&opt, 0, sizeof(opt));
> + if (copy_from_sockptr(&opt, optval, optlen))
> + return -EFAULT;
Again, not a good practice to zero-extend struct. Enforce proper size
with -EINVAL.
[..]
> + /* Initialize tcp_authopt_info if not already set */
> + info = __tcp_authopt_info_get_or_create(sk);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
> +
> + /* check key family */
> + if (opt.flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
> + if (sk->sk_family != opt.addr.ss_family)
> + return -EINVAL;
> + }
Probably, can be in the reverse order, so that
__tcp_authopt_info_get_or_create()
won't allocate memory.
> + /* If an old value exists for same local_id it is deleted */
> + key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
> + if (key_info)
> + tcp_authopt_key_del(sk, info, key_info);
> + key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL | __GFP_ZERO);
> + if (!key_info)
> + return -ENOMEM;
1. You don't need sock_kmalloc() together with tcp_authopt_key_del().
It just frees the memory and allocates it back straight away - no
sense in doing that.
2. I think RFC says you must not allow a user to change an existing key:
> MKT parameters are not changed. Instead, new MKTs can be installed, and a connection
> can change which MKT it uses.
IIUC, it means that one can't just change an existing MKT, but one can
remove and later
add MKT with the same (send_id, recv_id, src_addr/port, dst_addr/port).
So, a reasonable thing to do:
if (key_info)
return -EEXIST.
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-10 20:41 ` Dmitry Safonov
@ 2021-08-11 8:29 ` Leonard Crestez
2021-08-11 13:42 ` David Ahern
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-11 8:29 UTC (permalink / raw)
To: Dmitry Safonov
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 8/10/21 11:41 PM, Dmitry Safonov wrote:
> Hi Leonard,
>
> On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <cdleonard@gmail.com> wrote:
> [..]
>> +/* Representation of a Master Key Tuple as per RFC5925 */
>> +struct tcp_authopt_key_info {
>> + struct hlist_node node;
>> + /* Local identifier */
>> + u32 local_id;
>
> There is no local_id in RFC5925, what's that?
> An MKT is identified by (send_id, recv_id), together with
> (src_addr/src_port, dst_addr/dst_port).
> Why introducing something new to already complicated RFC?
It was there to simplify user interface and initial implementation.
But it seems that BGP listeners already needs to support multiple
keychains for different peers so identifying the key by (send_id,
recv_id, binding) is easier for userspace to work with. Otherwise they
need to create their own local_id mapping internally.
>> + u32 flags;
>> + /* Wire identifiers */
>> + u8 send_id, recv_id;
>> + u8 alg_id;
>> + u8 keylen;
>> + u8 key[TCP_AUTHOPT_MAXKEYLEN];
>> + struct rcu_head rcu;
>
> This is unaligned and will add padding.
Not clear padding matters. Moving rcu_head higher would avoid it, is
that what you're suggesting.
> I wonder if it's also worth saving some bytes by something like
> struct tcp_ao_key *key;
>
> With
> struct tcp_ao_key {
> u8 keylen;
> u8 key[0];
> };
>
> Hmm?
This increases memory management complexity for very minor gains. Very
few tcp_authopt_key will ever be created.
>> + struct sockaddr_storage addr;
>> +};
>> +
>> +/* Per-socket information regarding tcp_authopt */
>> +struct tcp_authopt_info {
>> + /* List of tcp_authopt_key_info */
>> + struct hlist_head head;
>> + u32 flags;
>> + u32 src_isn;
>> + u32 dst_isn;
>> + struct rcu_head rcu;
>
> Ditto, adds padding on 64-bit.
>
> [..]
>> +++ b/include/uapi/linux/tcp.h
>> @@ -126,10 +126,12 @@ enum {
>> #define TCP_INQ 36 /* Notify bytes available to read as a cmsg on read */
>>
>> #define TCP_CM_INQ TCP_INQ
>>
>> #define TCP_TX_DELAY 37 /* delay outgoing packets by XX usec */
>> +#define TCP_AUTHOPT 38 /* TCP Authentication Option (RFC2385) */
>> +#define TCP_AUTHOPT_KEY 39 /* TCP Authentication Option update key (RFC2385) */
>
> RFC2385 is md5 one.
> Also, should there be TCP_AUTHOPT_ADD_KEY, TCP_AUTHOPT_DELTE_KEY
> instead of using flags inside setsocketopt()? (no hard feelings)
Fixed RFC reference.
TCP_AUTHOPT_DELETE_KEY could be clearer, I just wanted to avoid bloating
the sockopt space. But there's not any scarcity.
For reference tcp_md5 handles key deletion based on keylen == 0. This
seems wrong to me, an empty key is in fact valid though not realistic.
If local_id is removed in favor of "full match on id and binding" then
the delete sockopt would still need most of a full struct
tcp_authopt_key anyway.
> [..]
>> +/**
>> + * enum tcp_authopt_flag - flags for `tcp_authopt.flags`
>> + */
>> +enum tcp_authopt_flag {
>> + /**
>> + * @TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED:
>> + * Configure behavior of segments with TCP-AO coming from hosts for which no
>> + * key is configured. The default recommended by RFC is to silently accept
>> + * such connections.
>> + */
>> + TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED = (1 << 2),
>> +};
>> +
>> +/**
>> + * struct tcp_authopt - Per-socket options related to TCP Authentication Option
>> + */
>> +struct tcp_authopt {
>> + /** @flags: Combination of &enum tcp_authopt_flag */
>> + __u32 flags;
>> +};
>
> I'm not sure what's the use of enum here, probably: > #define TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED BIT(2)
This is an enum because it looks nice in kernel-doc. I couldn't find a
way to attach docs to a macro and include it somewhere else. BTW, the
enum gains more members later.
As for BIT() it doesn't see to be allowed in uapi and there were recent
changes removing such usage.
> [..]
>> +struct tcp_authopt_key {
>> + /** @flags: Combination of &enum tcp_authopt_key_flag */
>> + __u32 flags;
>> + /** @local_id: Local identifier */
>> + __u32 local_id;
>> + /** @send_id: keyid value for send */
>> + __u8 send_id;
>> + /** @recv_id: keyid value for receive */
>> + __u8 recv_id;
>> + /** @alg: One of &enum tcp_authopt_alg */
>> + __u8 alg;
>> + /** @keylen: Length of the key buffer */
>> + __u8 keylen;
>> + /** @key: Secret key */
>> + __u8 key[TCP_AUTHOPT_MAXKEYLEN];
>> + /**
>> + * @addr: Key is only valid for this address
>> + *
>> + * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
>> + */
>> + struct __kernel_sockaddr_storage addr;
>> +};
>
> It'll be an ABI if this is accepted. As it is - it can't support RFC5925 fully.
> Extending syscall ABI is painful. I think, even the initial ABI *must* support
> all possible features of the RFC.
> In other words, there must be src_addr, src_port, dst_addr and dst_port.
> I can see from docs you've written you don't want to support a mix of different
> addr/port MKTs, so you can return -EINVAL or -ENOTSUPP for any value
> but zero.
> This will create an ABI that can be later extended to fully support RFC.
RFC states that MKT connection identifiers can be specified using ranges
and wildcards and the details are up to the implementation. Keys are
*NOT* just bound to a classical TCP 4-tuple.
* src_addr and src_port is implicit in socket binding. Maybe in theory
src_addr they could apply for a server socket bound to 0.0.0.0:PORT but
userspace can just open more sockets.
* dst_port is not supported by MD5 and I can't think of any useful
usecase. This is either well known (179 for BGP) or auto-allocated.
* tcp_md5 was recently enhanced allow a "prefixlen" for addr and
"l3mdev" ifindex binding.
This last point shows that the binding features people require can't be
easily predicted in advance so it's better to allow the rules to be
extended.
> The same is about key: I don't think you need to define/use tcp_authopt_alg.
> Just use algo name - that way TCP-AO will automatically be able to use
> any algo supported by crypto engine.
> See how xfrm does it, e.g.:
> struct xfrm_algo_auth {
> char alg_name[64];
> unsigned int alg_key_len; /* in bits */
> unsigned int alg_trunc_len; /* in bits */
> char alg_key[0];
> };
>
> So you can let a user chose maclen instead of hard-coding it.
> Much more extendable than what you propose.
This complicates ABI and implementation with features that are not
needed. I'd much rather only expose an enum of real-world tcp-ao algorithms.
> [..]
>> +#ifdef CONFIG_TCP_AUTHOPT
>> + case TCP_AUTHOPT: {
>> + struct tcp_authopt info;
>> +
>> + if (get_user(len, optlen))
>> + return -EFAULT;
>> +
>> + lock_sock(sk);
>> + tcp_get_authopt_val(sk, &info);
>> + release_sock(sk);
>> +
>> + len = min_t(unsigned int, len, sizeof(info));
>> + if (put_user(len, optlen))
>> + return -EFAULT;
>> + if (copy_to_user(optval, &info, len))
>> + return -EFAULT;
>> + return 0;
>> + }
>
> I'm pretty sure it's not a good choice to write partly tcp_authopt.
> And a user has no way to check what's the correct len on this kernel.
> Instead of len = min_t(unsigned int, len, sizeof(info)), it should be
> if (len != sizeof(info))
> return -EINVAL;
Purpose is to allow sockopts to grow as md5 has grown.
> [..]
>> +int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen)
>> +{
>> + struct tcp_authopt opt;
>> + struct tcp_authopt_info *info;
>> +
>> + WARN_ON(!lockdep_sock_is_held(sk));
>
> sock_owned_by_me(sk)
Yes, this seems better. I think there are several socket locking
mistakes in later commits. The patch adding support for detailed keyid
control probably needs bh_lock_sock to avoid races between key selection
and caching on send and key manipulation by userspace.
>> + /* If userspace optlen is too short fill the rest with zeros */
>> + if (optlen > sizeof(opt))
>> + return -EINVAL;
>> + memset(&opt, 0, sizeof(opt));
>
> it's 4 bytes, why not just (optlen != sizeof(opt))?
It's extended in a later commit to add detailed keyid control.
> [..]
>> +int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt)
>> +{
>> + struct tcp_sock *tp = tcp_sk(sk);
>> + struct tcp_authopt_info *info;
>> +
>> + WARN_ON(!lockdep_sock_is_held(sk));
>
> sock_owned_by_me(sk)
>
> [..]
>> +int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
>> +{
>> + struct tcp_authopt_key opt;
>> + struct tcp_authopt_info *info;
>> + struct tcp_authopt_key_info *key_info;
>> +
>> + /* If userspace optlen is too short fill the rest with zeros */
>> + if (optlen > sizeof(opt))
>> + return -EINVAL;
>> + memset(&opt, 0, sizeof(opt));
>> + if (copy_from_sockptr(&opt, optval, optlen))
>> + return -EFAULT;
>
> Again, not a good practice to zero-extend struct. Enforce proper size
> with -EINVAL.
>
> [..]
>> + /* Initialize tcp_authopt_info if not already set */
>> + info = __tcp_authopt_info_get_or_create(sk);
>> + if (IS_ERR(info))
>> + return PTR_ERR(info);
>> +
>> + /* check key family */
>> + if (opt.flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
>> + if (sk->sk_family != opt.addr.ss_family)
>> + return -EINVAL;
>> + }
>
> Probably, can be in the reverse order, so that
> __tcp_authopt_info_get_or_create()
> won't allocate memory.
OK. It only saves a tiny bit of memory on API misuse.
>> + /* If an old value exists for same local_id it is deleted */
>> + key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
>> + if (key_info)
>> + tcp_authopt_key_del(sk, info, key_info);
>> + key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL | __GFP_ZERO);
>> + if (!key_info)
>> + return -ENOMEM;
>
> 1. You don't need sock_kmalloc() together with tcp_authopt_key_del().
> It just frees the memory and allocates it back straight away - no
> sense in doing that.
The list is scanned in multiple places in later commits using nothing
but an rcu_read_lock, this means that keys can't be updated in-place.
> 2. I think RFC says you must not allow a user to change an existing key:
>> MKT parameters are not changed. Instead, new MKTs can be installed, and a connection
>> can change which MKT it uses.
>
> IIUC, it means that one can't just change an existing MKT, but one can
> remove and later
> add MKT with the same (send_id, recv_id, src_addr/port, dst_addr/port).
>
> So, a reasonable thing to do:
> if (key_info)
> return -EEXIST.
You're right, making the user delete keys explicitly is better.
--
Regards,
Leonard
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 8:29 ` Leonard Crestez
@ 2021-08-11 13:42 ` David Ahern
2021-08-11 19:11 ` Leonard Crestez
2021-08-11 14:31 ` Dmitry Safonov
2021-08-12 19:46 ` Leonard Crestez
2 siblings, 1 reply; 24+ messages in thread
From: David Ahern @ 2021-08-11 13:42 UTC (permalink / raw)
To: Leonard Crestez, Dmitry Safonov
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 8/11/21 2:29 AM, Leonard Crestez wrote:
> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
>> Hi Leonard,
>>
>> On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <cdleonard@gmail.com>
>> wrote:
>> [..]
>>> +/* Representation of a Master Key Tuple as per RFC5925 */
>>> +struct tcp_authopt_key_info {
>>> + struct hlist_node node;
>>> + /* Local identifier */
>>> + u32 local_id;
>>
>> There is no local_id in RFC5925, what's that?
>> An MKT is identified by (send_id, recv_id), together with
>> (src_addr/src_port, dst_addr/dst_port).
>> Why introducing something new to already complicated RFC?
>
> It was there to simplify user interface and initial implementation.
>
> But it seems that BGP listeners already needs to support multiple
> keychains for different peers so identifying the key by (send_id,
> recv_id, binding) is easier for userspace to work with. Otherwise they
> need to create their own local_id mapping internally.
>
any proposed simplification needs to be well explained and how it
relates to the RFC spec.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 13:42 ` David Ahern
@ 2021-08-11 19:11 ` Leonard Crestez
2021-08-11 20:26 ` Dmitry Safonov
2021-08-11 20:26 ` David Ahern
0 siblings, 2 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-11 19:11 UTC (permalink / raw)
To: David Ahern, Leonard Crestez, Dmitry Safonov
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 11.08.2021 16:42, David Ahern wrote:
> On 8/11/21 2:29 AM, Leonard Crestez wrote:
>> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
>>> Hi Leonard,
>>>
>>> On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <cdleonard@gmail.com>
>>> wrote:
>>> [..]
>>>> +/* Representation of a Master Key Tuple as per RFC5925 */
>>>> +struct tcp_authopt_key_info {
>>>> + struct hlist_node node;
>>>> + /* Local identifier */
>>>> + u32 local_id;
>>>
>>> There is no local_id in RFC5925, what's that?
>>> An MKT is identified by (send_id, recv_id), together with
>>> (src_addr/src_port, dst_addr/dst_port).
>>> Why introducing something new to already complicated RFC?
>>
>> It was there to simplify user interface and initial implementation.
>>
>> But it seems that BGP listeners already needs to support multiple
>> keychains for different peers so identifying the key by (send_id,
>> recv_id, binding) is easier for userspace to work with. Otherwise they
>> need to create their own local_id mapping internally.
>>
>
> any proposed simplification needs to be well explained and how it
> relates to the RFC spec.
The local_id only exists between userspace and kernel so it's not really
covered by the RFC.
There are objections to this and it seems to be unhelpful for userspace
zo I will replace it with match by binding.
BTW: another somewhat dubious simplification is that I offloaded the RFC
requirement to never add overlapping keys to userspace. So if userspace
adds keys with same recvid that match the same TCP 4-tuple then
connections will just start failing.
It's arguably fine to allow userspace misconfiguration to cause failures.
--
Regards,
Leonard
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 19:11 ` Leonard Crestez
@ 2021-08-11 20:26 ` Dmitry Safonov
2021-08-11 20:26 ` David Ahern
1 sibling, 0 replies; 24+ messages in thread
From: Dmitry Safonov @ 2021-08-11 20:26 UTC (permalink / raw)
To: Leonard Crestez, David Ahern
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 8/11/21 8:11 PM, Leonard Crestez wrote:
> On 11.08.2021 16:42, David Ahern wrote:
[..]
>>
>> any proposed simplification needs to be well explained and how it
>> relates to the RFC spec.
>
> The local_id only exists between userspace and kernel so it's not really
> covered by the RFC.
>
> There are objections to this and it seems to be unhelpful for userspace
> zo I will replace it with match by binding.
>
> BTW: another somewhat dubious simplification is that I offloaded the RFC
> requirement to never add overlapping keys to userspace. So if userspace
> adds keys with same recvid that match the same TCP 4-tuple then
> connections will just start failing.
>
> It's arguably fine to allow userspace misconfiguration to cause failures.
I think it's fine. But worth documenting. Also, keep in mind that
someone in userspace with his funny ideas might start relying on such
behavior in future.
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 19:11 ` Leonard Crestez
2021-08-11 20:26 ` Dmitry Safonov
@ 2021-08-11 20:26 ` David Ahern
1 sibling, 0 replies; 24+ messages in thread
From: David Ahern @ 2021-08-11 20:26 UTC (permalink / raw)
To: Leonard Crestez, Dmitry Safonov
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 8/11/21 1:11 PM, Leonard Crestez wrote:
> On 11.08.2021 16:42, David Ahern wrote:
>> On 8/11/21 2:29 AM, Leonard Crestez wrote:
>>> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
>>>> Hi Leonard,
>>>>
>>>> On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <cdleonard@gmail.com>
>>>> wrote:
>>>> [..]
>>>>> +/* Representation of a Master Key Tuple as per RFC5925 */
>>>>> +struct tcp_authopt_key_info {
>>>>> + struct hlist_node node;
>>>>> + /* Local identifier */
>>>>> + u32 local_id;
>>>>
>>>> There is no local_id in RFC5925, what's that?
>>>> An MKT is identified by (send_id, recv_id), together with
>>>> (src_addr/src_port, dst_addr/dst_port).
>>>> Why introducing something new to already complicated RFC?
>>>
>>> It was there to simplify user interface and initial implementation.
>>>
>>> But it seems that BGP listeners already needs to support multiple
>>> keychains for different peers so identifying the key by (send_id,
>>> recv_id, binding) is easier for userspace to work with. Otherwise they
>>> need to create their own local_id mapping internally.
>>>
>>
>> any proposed simplification needs to be well explained and how it
>> relates to the RFC spec.
>
> The local_id only exists between userspace and kernel so it's not really
> covered by the RFC.
My point is that you need to document the uapi (however it ends up) and
how it is used to achieve the intent of the RFC.
>
> There are objections to this and it seems to be unhelpful for userspace
> zo I will replace it with match by binding.
>
> BTW: another somewhat dubious simplification is that I offloaded the RFC
> requirement to never add overlapping keys to userspace. So if userspace
> adds keys with same recvid that match the same TCP 4-tuple then
> connections will just start failing.
>
> It's arguably fine to allow userspace misconfiguration to cause failures.
>
> --
> Regards,
> Leonard
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 8:29 ` Leonard Crestez
2021-08-11 13:42 ` David Ahern
@ 2021-08-11 14:31 ` Dmitry Safonov
2021-08-11 17:15 ` David Ahern
2021-08-11 19:08 ` Leonard Crestez
2021-08-12 19:46 ` Leonard Crestez
2 siblings, 2 replies; 24+ messages in thread
From: Dmitry Safonov @ 2021-08-11 14:31 UTC (permalink / raw)
To: Leonard Crestez
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 8/11/21 9:29 AM, Leonard Crestez wrote:
> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
[..]
>>> + u32 flags;
>>> + /* Wire identifiers */
>>> + u8 send_id, recv_id;
>>> + u8 alg_id;
>>> + u8 keylen;
>>> + u8 key[TCP_AUTHOPT_MAXKEYLEN];
>>> + struct rcu_head rcu;
>>
>> This is unaligned and will add padding.
>
> Not clear padding matters. Moving rcu_head higher would avoid it, is
> that what you're suggesting.
Yes.
>> I wonder if it's also worth saving some bytes by something like
>> struct tcp_ao_key *key;
>>
>> With
>> struct tcp_ao_key {
>> u8 keylen;
>> u8 key[0];
>> };
>>
>> Hmm?
>
> This increases memory management complexity for very minor gains. Very
> few tcp_authopt_key will ever be created.
The change doesn't seem to be big, like:
--- a/net/ipv4/tcp_authopt.c
+++ b/net/ipv4/tcp_authopt.c
@@ -422,8 +422,16 @@ int tcp_set_authopt_key(struct sock *sk, sockptr_t
optval, unsig>
key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
if (key_info)
tcp_authopt_key_del(sk, info, key_info);
+
+ key = sock_kmalloc(sk, sizeof(*key) + opt.keylen, GFP_KERNEL |
__GFP_ZERO);
+ if (!key) {
+ tcp_authopt_alg_release(alg);
+ return -ENOMEM;
+ }
+
key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL |
__GFP_ZERO);
if (!key_info) {
+ sock_kfree_s(sk, key, sizeof(*key) + opt.keylen);
tcp_authopt_alg_release(alg);
return -ENOMEM;
}
I don't know, probably it'll be enough for every user to limit their
keys by length of 80, but if one day it won't be enough - this ABI will
be painful to extend.
[..]
>>> +#define TCP_AUTHOPT 38 /* TCP Authentication
>>> Option (RFC2385) */
>>> +#define TCP_AUTHOPT_KEY 39 /* TCP Authentication
>>> Option update key (RFC2385) */
>>
>> RFC2385 is md5 one.
>> Also, should there be TCP_AUTHOPT_ADD_KEY, TCP_AUTHOPT_DELTE_KEY
>> instead of using flags inside setsocketopt()? (no hard feelings)
>
> Fixed RFC reference.
>
> TCP_AUTHOPT_DELETE_KEY could be clearer, I just wanted to avoid bloating
> the sockopt space. But there's not any scarcity.
>
> For reference tcp_md5 handles key deletion based on keylen == 0. This
> seems wrong to me, an empty key is in fact valid though not realistic.
>
> If local_id is removed in favor of "full match on id and binding" then
> the delete sockopt would still need most of a full struct
> tcp_authopt_key anyway.
Sounds like a plan.
[..]>> I'm not sure what's the use of enum here, probably: > #define
>> TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED BIT(2)
>
> This is an enum because it looks nice in kernel-doc. I couldn't find a
> way to attach docs to a macro and include it somewhere else.
Yeah, ok, seems like good justification.
> BTW, the enum gains more members later.
>
> As for BIT() it doesn't see to be allowed in uapi and there were recent
> changes removing such usage.
Ok, I just saw it's still used in include/uapi, but not aware of the
removal.
>
>> [..]
>>> +struct tcp_authopt_key {
>>> + /** @flags: Combination of &enum tcp_authopt_key_flag */
>>> + __u32 flags;
>>> + /** @local_id: Local identifier */
>>> + __u32 local_id;
>>> + /** @send_id: keyid value for send */
>>> + __u8 send_id;
>>> + /** @recv_id: keyid value for receive */
>>> + __u8 recv_id;
>>> + /** @alg: One of &enum tcp_authopt_alg */
>>> + __u8 alg;
>>> + /** @keylen: Length of the key buffer */
>>> + __u8 keylen;
>>> + /** @key: Secret key */
>>> + __u8 key[TCP_AUTHOPT_MAXKEYLEN];
>>> + /**
>>> + * @addr: Key is only valid for this address
>>> + *
>>> + * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
>>> + */
>>> + struct __kernel_sockaddr_storage addr;
>>> +};
>>
>> It'll be an ABI if this is accepted. As it is - it can't support
>> RFC5925 fully.
>> Extending syscall ABI is painful. I think, even the initial ABI *must*
>> support
>> all possible features of the RFC.
>> In other words, there must be src_addr, src_port, dst_addr and dst_port.
>> I can see from docs you've written you don't want to support a mix of
>> different
>> addr/port MKTs, so you can return -EINVAL or -ENOTSUPP for any value
>> but zero.
>> This will create an ABI that can be later extended to fully support RFC.
>
> RFC states that MKT connection identifiers can be specified using ranges
> and wildcards and the details are up to the implementation. Keys are
> *NOT* just bound to a classical TCP 4-tuple.
>
> * src_addr and src_port is implicit in socket binding. Maybe in theory
> src_addr they could apply for a server socket bound to 0.0.0.0:PORT but
> userspace can just open more sockets.
> * dst_port is not supported by MD5 and I can't think of any useful
> usecase. This is either well known (179 for BGP) or auto-allocated.
> * tcp_md5 was recently enhanced allow a "prefixlen" for addr and
> "l3mdev" ifindex binding.
>
> This last point shows that the binding features people require can't be
> easily predicted in advance so it's better to allow the rules to be
> extended.
Yeah, I see both changes you mention went on easy way as they reused
existing paddings in the ABI structures.
Ok, if you don't want to reserve src_addr/src_port/dst_addr/dst_port,
than how about reserving some space for those instead?
>> The same is about key: I don't think you need to define/use
>> tcp_authopt_alg.
>> Just use algo name - that way TCP-AO will automatically be able to use
>> any algo supported by crypto engine.
>> See how xfrm does it, e.g.:
>> struct xfrm_algo_auth {
>> char alg_name[64];
>> unsigned int alg_key_len; /* in bits */
>> unsigned int alg_trunc_len; /* in bits */
>> char alg_key[0];
>> };
>>
>> So you can let a user chose maclen instead of hard-coding it.
>> Much more extendable than what you propose.
>
> This complicates ABI and implementation with features that are not
> needed. I'd much rather only expose an enum of real-world tcp-ao
> algorithms.
I see it exactly the opposite way: a new enum unnecessary complicates
ABI, instead of passing alg_name[] to crypto engine. No need to add any
support in tcp-ao as the algorithms are already provided by kernel.
That is how it transparently works for ipsec, why not for tcp-ao?
>
>> [..]
>>> +#ifdef CONFIG_TCP_AUTHOPT
>>> + case TCP_AUTHOPT: {
>>> + struct tcp_authopt info;
>>> +
>>> + if (get_user(len, optlen))
>>> + return -EFAULT;
>>> +
>>> + lock_sock(sk);
>>> + tcp_get_authopt_val(sk, &info);
>>> + release_sock(sk);
>>> +
>>> + len = min_t(unsigned int, len, sizeof(info));
>>> + if (put_user(len, optlen))
>>> + return -EFAULT;
>>> + if (copy_to_user(optval, &info, len))
>>> + return -EFAULT;
>>> + return 0;
>>> + }
>>
>> I'm pretty sure it's not a good choice to write partly tcp_authopt.
>> And a user has no way to check what's the correct len on this kernel.
>> Instead of len = min_t(unsigned int, len, sizeof(info)), it should be
>> if (len != sizeof(info))
>> return -EINVAL;
>
> Purpose is to allow sockopts to grow as md5 has grown.
md5 has not grown. See above.
Another issue with your approach
+ /* If userspace optlen is too short fill the rest with zeros */
+ if (optlen > sizeof(opt))
+ return -EINVAL;
+ memset(&opt, 0, sizeof(opt));
+ if (copy_from_sockptr(&opt, optval, optlen))
+ return -EFAULT;
is that userspace compiled with updated/grew structure will fail on
older kernel. So, no extension without breaking something is possible.
Which also reminds me that currently you don't validate (opt.flags) for
unknown by kernel flags.
Extending syscalls is impossible without breaking userspace if ABI is
not designed with extensibility in mind. That was quite a big problem,
and still is. Please, read this carefully:
https://lwn.net/Articles/830666/
That is why I'm suggesting you all those changes that will be harder to
fix when/if your patches get accepted.
As an example how it should work see in copy_clone_args_from_user().
[..]
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 14:31 ` Dmitry Safonov
@ 2021-08-11 17:15 ` David Ahern
2021-08-11 20:12 ` Dmitry Safonov
2021-08-11 19:08 ` Leonard Crestez
1 sibling, 1 reply; 24+ messages in thread
From: David Ahern @ 2021-08-11 17:15 UTC (permalink / raw)
To: Dmitry Safonov, Leonard Crestez
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 8/11/21 8:31 AM, Dmitry Safonov wrote:
> On 8/11/21 9:29 AM, Leonard Crestez wrote:
>> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
> [..]
>>>> + u32 flags;
>>>> + /* Wire identifiers */
>>>> + u8 send_id, recv_id;
>>>> + u8 alg_id;
>>>> + u8 keylen;
>>>> + u8 key[TCP_AUTHOPT_MAXKEYLEN];
>>>> + struct rcu_head rcu;
>>>
>>> This is unaligned and will add padding.
>>
>> Not clear padding matters. Moving rcu_head higher would avoid it, is
>> that what you're suggesting.
>
> Yes.
>
>>> I wonder if it's also worth saving some bytes by something like
>>> struct tcp_ao_key *key;
>>>
>>> With
>>> struct tcp_ao_key {
>>> u8 keylen;
>>> u8 key[0];
>>> };
>>>
>>> Hmm?
>>
>> This increases memory management complexity for very minor gains. Very
>> few tcp_authopt_key will ever be created.
>
> The change doesn't seem to be big, like:
> --- a/net/ipv4/tcp_authopt.c
> +++ b/net/ipv4/tcp_authopt.c
> @@ -422,8 +422,16 @@ int tcp_set_authopt_key(struct sock *sk, sockptr_t
> optval, unsig>
> key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
> if (key_info)
> tcp_authopt_key_del(sk, info, key_info);
> +
> + key = sock_kmalloc(sk, sizeof(*key) + opt.keylen, GFP_KERNEL |
> __GFP_ZERO);
> + if (!key) {
> + tcp_authopt_alg_release(alg);
> + return -ENOMEM;
> + }
> +
> key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL |
> __GFP_ZERO);
> if (!key_info) {
> + sock_kfree_s(sk, key, sizeof(*key) + opt.keylen);
> tcp_authopt_alg_release(alg);
> return -ENOMEM;
> }
>
> I don't know, probably it'll be enough for every user to limit their
> keys by length of 80, but if one day it won't be enough - this ABI will
> be painful to extend.
>
> [..]
>>>> +#define TCP_AUTHOPT 38 /* TCP Authentication
>>>> Option (RFC2385) */
>>>> +#define TCP_AUTHOPT_KEY 39 /* TCP Authentication
>>>> Option update key (RFC2385) */
>>>
>>> RFC2385 is md5 one.
>>> Also, should there be TCP_AUTHOPT_ADD_KEY, TCP_AUTHOPT_DELTE_KEY
>>> instead of using flags inside setsocketopt()? (no hard feelings)
>>
>> Fixed RFC reference.
>>
>> TCP_AUTHOPT_DELETE_KEY could be clearer, I just wanted to avoid bloating
>> the sockopt space. But there's not any scarcity.
>>
>> For reference tcp_md5 handles key deletion based on keylen == 0. This
>> seems wrong to me, an empty key is in fact valid though not realistic.
>>
>> If local_id is removed in favor of "full match on id and binding" then
>> the delete sockopt would still need most of a full struct
>> tcp_authopt_key anyway.
>
> Sounds like a plan.
>
> [..]>> I'm not sure what's the use of enum here, probably: > #define
>>> TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED BIT(2)
>>
>> This is an enum because it looks nice in kernel-doc. I couldn't find a
>> way to attach docs to a macro and include it somewhere else.
>
> Yeah, ok, seems like good justification.
>
>> BTW, the enum gains more members later.
>>
>> As for BIT() it doesn't see to be allowed in uapi and there were recent
>> changes removing such usage.
>
> Ok, I just saw it's still used in include/uapi, but not aware of the
> removal.
>
>>
>>> [..]
>>>> +struct tcp_authopt_key {
>>>> + /** @flags: Combination of &enum tcp_authopt_key_flag */
>>>> + __u32 flags;
>>>> + /** @local_id: Local identifier */
>>>> + __u32 local_id;
>>>> + /** @send_id: keyid value for send */
>>>> + __u8 send_id;
>>>> + /** @recv_id: keyid value for receive */
>>>> + __u8 recv_id;
>>>> + /** @alg: One of &enum tcp_authopt_alg */
>>>> + __u8 alg;
>>>> + /** @keylen: Length of the key buffer */
>>>> + __u8 keylen;
>>>> + /** @key: Secret key */
>>>> + __u8 key[TCP_AUTHOPT_MAXKEYLEN];
>>>> + /**
>>>> + * @addr: Key is only valid for this address
>>>> + *
>>>> + * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
>>>> + */
>>>> + struct __kernel_sockaddr_storage addr;
>>>> +};
>>>
>>> It'll be an ABI if this is accepted. As it is - it can't support
>>> RFC5925 fully.
>>> Extending syscall ABI is painful. I think, even the initial ABI *must*
>>> support
>>> all possible features of the RFC.
>>> In other words, there must be src_addr, src_port, dst_addr and dst_port.
>>> I can see from docs you've written you don't want to support a mix of
>>> different
>>> addr/port MKTs, so you can return -EINVAL or -ENOTSUPP for any value
>>> but zero.
>>> This will create an ABI that can be later extended to fully support RFC.
>>
>> RFC states that MKT connection identifiers can be specified using ranges
>> and wildcards and the details are up to the implementation. Keys are
>> *NOT* just bound to a classical TCP 4-tuple.
>>
>> * src_addr and src_port is implicit in socket binding. Maybe in theory
>> src_addr they could apply for a server socket bound to 0.0.0.0:PORT but
>> userspace can just open more sockets.
>> * dst_port is not supported by MD5 and I can't think of any useful
>> usecase. This is either well known (179 for BGP) or auto-allocated.
>> * tcp_md5 was recently enhanced allow a "prefixlen" for addr and
>> "l3mdev" ifindex binding.
>>
>> This last point shows that the binding features people require can't be
>> easily predicted in advance so it's better to allow the rules to be
>> extended.
>
> Yeah, I see both changes you mention went on easy way as they reused
> existing paddings in the ABI structures.
> Ok, if you don't want to reserve src_addr/src_port/dst_addr/dst_port,
> than how about reserving some space for those instead?
>
>>> The same is about key: I don't think you need to define/use
>>> tcp_authopt_alg.
>>> Just use algo name - that way TCP-AO will automatically be able to use
>>> any algo supported by crypto engine.
>>> See how xfrm does it, e.g.:
>>> struct xfrm_algo_auth {
>>> char alg_name[64];
>>> unsigned int alg_key_len; /* in bits */
>>> unsigned int alg_trunc_len; /* in bits */
>>> char alg_key[0];
>>> };
>>>
>>> So you can let a user chose maclen instead of hard-coding it.
>>> Much more extendable than what you propose.
>>
>> This complicates ABI and implementation with features that are not
>> needed. I'd much rather only expose an enum of real-world tcp-ao
>> algorithms.
>
> I see it exactly the opposite way: a new enum unnecessary complicates
> ABI, instead of passing alg_name[] to crypto engine. No need to add any
> support in tcp-ao as the algorithms are already provided by kernel.
> That is how it transparently works for ipsec, why not for tcp-ao?
>
>>
>>> [..]
>>>> +#ifdef CONFIG_TCP_AUTHOPT
>>>> + case TCP_AUTHOPT: {
>>>> + struct tcp_authopt info;
>>>> +
>>>> + if (get_user(len, optlen))
>>>> + return -EFAULT;
>>>> +
>>>> + lock_sock(sk);
>>>> + tcp_get_authopt_val(sk, &info);
>>>> + release_sock(sk);
>>>> +
>>>> + len = min_t(unsigned int, len, sizeof(info));
>>>> + if (put_user(len, optlen))
>>>> + return -EFAULT;
>>>> + if (copy_to_user(optval, &info, len))
>>>> + return -EFAULT;
>>>> + return 0;
>>>> + }
>>>
>>> I'm pretty sure it's not a good choice to write partly tcp_authopt.
>>> And a user has no way to check what's the correct len on this kernel.
>>> Instead of len = min_t(unsigned int, len, sizeof(info)), it should be
>>> if (len != sizeof(info))
>>> return -EINVAL;
>>
>> Purpose is to allow sockopts to grow as md5 has grown.
>
> md5 has not grown. See above.
MD5 uapi has - e.g., 8917a777be3ba and 6b102db50cdde. We want similar
capabilities for growth with this API.
>
> Another issue with your approach
>
> + /* If userspace optlen is too short fill the rest with zeros */
> + if (optlen > sizeof(opt))
> + return -EINVAL;
> + memset(&opt, 0, sizeof(opt));
> + if (copy_from_sockptr(&opt, optval, optlen))
> + return -EFAULT;
>
> is that userspace compiled with updated/grew structure will fail on
> older kernel. So, no extension without breaking something is possible.
> Which also reminds me that currently you don't validate (opt.flags) for
> unknown by kernel flags.
>
> Extending syscalls is impossible without breaking userspace if ABI is
> not designed with extensibility in mind. That was quite a big problem,
> and still is. Please, read this carefully:
> https://lwn.net/Articles/830666/
>
> That is why I'm suggesting you all those changes that will be harder to
> fix when/if your patches get accepted.
> As an example how it should work see in copy_clone_args_from_user().
>
Look at how TCP_ZEROCOPY_RECEIVE has grown over releases as an example
of how to properly handle this.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 17:15 ` David Ahern
@ 2021-08-11 20:12 ` Dmitry Safonov
2021-08-11 20:23 ` David Ahern
0 siblings, 1 reply; 24+ messages in thread
From: Dmitry Safonov @ 2021-08-11 20:12 UTC (permalink / raw)
To: David Ahern, Leonard Crestez
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
Hi David,
On 8/11/21 6:15 PM, David Ahern wrote:
> On 8/11/21 8:31 AM, Dmitry Safonov wrote:
>> On 8/11/21 9:29 AM, Leonard Crestez wrote:
>>> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
[..]
>>>> I'm pretty sure it's not a good choice to write partly tcp_authopt.
>>>> And a user has no way to check what's the correct len on this kernel.
>>>> Instead of len = min_t(unsigned int, len, sizeof(info)), it should be
>>>> if (len != sizeof(info))
>>>> return -EINVAL;
>>>
>>> Purpose is to allow sockopts to grow as md5 has grown.
>>
>> md5 has not grown. See above.
>
> MD5 uapi has - e.g., 8917a777be3ba and 6b102db50cdde. We want similar
> capabilities for growth with this API.
So, you mean adding a new setsockopt when the struct has to be extended?
Like TCP_AUTHOPT_EXT?
It can work, but sounds like adding a new syscall every time the old one
can't be extended. I can see that with current limitations on TCP-AO RFC
the ABI in these patches will have to be extended.
The second commit started using new cmd.tcpm_flags, where unknown flags
are still at this moment silently ignored by the kernel. So 6b102db50cdd
could have introduced a regression in userspace. By luck and by reason
that md5 isn't probably frequently used it didn't.
Not nice at all example for newer APIs.
>> Another issue with your approach
>>
>> + /* If userspace optlen is too short fill the rest with zeros */
>> + if (optlen > sizeof(opt))
>> + return -EINVAL;
>> + memset(&opt, 0, sizeof(opt));
>> + if (copy_from_sockptr(&opt, optval, optlen))
>> + return -EFAULT;
>>
>> is that userspace compiled with updated/grew structure will fail on
>> older kernel. So, no extension without breaking something is possible.
>> Which also reminds me that currently you don't validate (opt.flags) for
>> unknown by kernel flags.
>>
>> Extending syscalls is impossible without breaking userspace if ABI is
>> not designed with extensibility in mind. That was quite a big problem,
>> and still is. Please, read this carefully:
>> https://lwn.net/Articles/830666/
>>
>> That is why I'm suggesting you all those changes that will be harder to
>> fix when/if your patches get accepted.
>> As an example how it should work see in copy_clone_args_from_user().
>>
>
> Look at how TCP_ZEROCOPY_RECEIVE has grown over releases as an example
> of how to properly handle this.
Exactly.
: switch (len) {
: case offsetofend(...)
: case offsetofend(...)
And than also:
: if (unlikely(len > sizeof(zc))) {
: err = check_zeroed_user(optval + sizeof(zc),
: len - sizeof(zc));
Does it sound similar to what I've written in my ABI review?
And what the LWN article has in it.
Please, look again at the patch I replied to.
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 20:12 ` Dmitry Safonov
@ 2021-08-11 20:23 ` David Ahern
0 siblings, 0 replies; 24+ messages in thread
From: David Ahern @ 2021-08-11 20:23 UTC (permalink / raw)
To: Dmitry Safonov, Leonard Crestez
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 8/11/21 2:12 PM, Dmitry Safonov wrote:
> Hi David,
>
> On 8/11/21 6:15 PM, David Ahern wrote:
>> On 8/11/21 8:31 AM, Dmitry Safonov wrote:
>>> On 8/11/21 9:29 AM, Leonard Crestez wrote:
>>>> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
> [..]
>>>>> I'm pretty sure it's not a good choice to write partly tcp_authopt.
>>>>> And a user has no way to check what's the correct len on this kernel.
>>>>> Instead of len = min_t(unsigned int, len, sizeof(info)), it should be
>>>>> if (len != sizeof(info))
>>>>> return -EINVAL;
>>>>
>>>> Purpose is to allow sockopts to grow as md5 has grown.
>>>
>>> md5 has not grown. See above.
>>
>> MD5 uapi has - e.g., 8917a777be3ba and 6b102db50cdde. We want similar
>> capabilities for growth with this API.
>
> So, you mean adding a new setsockopt when the struct has to be extended?
> Like TCP_AUTHOPT_EXT?
uh, no. That was needed because of failures with the original
implementation wrt checking that all unused bits are 0. If checking is
not done from day 1, that field can never be used in the future.
My point here was only that MD5 uapi was extended.
My second point is more relevant to Leonard as a very recent example of
how to build an extendable struct.
>>
>> Look at how TCP_ZEROCOPY_RECEIVE has grown over releases as an example
>> of how to properly handle this.
>
> Exactly.
>
> : switch (len) {
> : case offsetofend(...)
> : case offsetofend(...)
>
> And than also:
> : if (unlikely(len > sizeof(zc))) {
> : err = check_zeroed_user(optval + sizeof(zc),
> : len - sizeof(zc));
>
> Does it sound similar to what I've written in my ABI review?
> And what the LWN article has in it.
> Please, look again at the patch I replied to.
>
> Thanks,
> Dmitry
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 14:31 ` Dmitry Safonov
2021-08-11 17:15 ` David Ahern
@ 2021-08-11 19:08 ` Leonard Crestez
1 sibling, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-11 19:08 UTC (permalink / raw)
To: Dmitry Safonov, Leonard Crestez, David Ahern
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 11.08.2021 17:31, Dmitry Safonov wrote:
> On 8/11/21 9:29 AM, Leonard Crestez wrote:
>> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
>>> I wonder if it's also worth saving some bytes by something like
>>> struct tcp_ao_key *key;
>>>
>>> With
>>> struct tcp_ao_key {
>>> u8 keylen;
>>> u8 key[0];
>>> };
>>>
>>> Hmm?
>>
>> This increases memory management complexity for very minor gains. Very
>> few tcp_authopt_key will ever be created.
>
> The change doesn't seem to be big, like:
> --- a/net/ipv4/tcp_authopt.c
> +++ b/net/ipv4/tcp_authopt.c
> @@ -422,8 +422,16 @@ int tcp_set_authopt_key(struct sock *sk, sockptr_t
> optval, unsig>
> key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
> if (key_info)
> tcp_authopt_key_del(sk, info, key_info);
> +
> + key = sock_kmalloc(sk, sizeof(*key) + opt.keylen, GFP_KERNEL |
> __GFP_ZERO);
> + if (!key) {
> + tcp_authopt_alg_release(alg);
> + return -ENOMEM;
> + }
> +
> key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL |
> __GFP_ZERO);
> if (!key_info) {
> + sock_kfree_s(sk, key, sizeof(*key) + opt.keylen);
> tcp_authopt_alg_release(alg);
> return -ENOMEM;
> }
>
> I don't know, probably it'll be enough for every user to limit their
> keys by length of 80, but if one day it won't be enough - this ABI will
> be painful to extend.
struct tcp_authopt_key also needs to be modified and a separate
copy_from_user is required. It's not very complicated but not very
useful either.
>>>> +struct tcp_authopt_key {
>>>> + /** @flags: Combination of &enum tcp_authopt_key_flag */
>>>> + __u32 flags;
>>>> + /** @local_id: Local identifier */
>>>> + __u32 local_id;
>>>> + /** @send_id: keyid value for send */
>>>> + __u8 send_id;
>>>> + /** @recv_id: keyid value for receive */
>>>> + __u8 recv_id;
>>>> + /** @alg: One of &enum tcp_authopt_alg */
>>>> + __u8 alg;
>>>> + /** @keylen: Length of the key buffer */
>>>> + __u8 keylen;
>>>> + /** @key: Secret key */
>>>> + __u8 key[TCP_AUTHOPT_MAXKEYLEN];
>>>> + /**
>>>> + * @addr: Key is only valid for this address
>>>> + *
>>>> + * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
>>>> + */
>>>> + struct __kernel_sockaddr_storage addr;
>>>> +};
>>>
>>> It'll be an ABI if this is accepted. As it is - it can't support
>>> RFC5925 fully.
>>> Extending syscall ABI is painful. I think, even the initial ABI *must*
>>> support
>>> all possible features of the RFC.
>>> In other words, there must be src_addr, src_port, dst_addr and dst_port.
>>> I can see from docs you've written you don't want to support a mix of
>>> different
>>> addr/port MKTs, so you can return -EINVAL or -ENOTSUPP for any value
>>> but zero.
>>> This will create an ABI that can be later extended to fully support RFC.
>>
>> RFC states that MKT connection identifiers can be specified using ranges
>> and wildcards and the details are up to the implementation. Keys are
>> *NOT* just bound to a classical TCP 4-tuple.
>>
>> * src_addr and src_port is implicit in socket binding. Maybe in theory
>> src_addr they could apply for a server socket bound to 0.0.0.0:PORT but
>> userspace can just open more sockets.
>> * dst_port is not supported by MD5 and I can't think of any useful
>> usecase. This is either well known (179 for BGP) or auto-allocated.
>> * tcp_md5 was recently enhanced allow a "prefixlen" for addr and
>> "l3mdev" ifindex binding.
>>
>> This last point shows that the binding features people require can't be
>> easily predicted in advance so it's better to allow the rules to be
>> extended.
>
> Yeah, I see both changes you mention went on easy way as they reused
> existing paddings in the ABI structures.
> Ok, if you don't want to reserve src_addr/src_port/dst_addr/dst_port,
> than how about reserving some space for those instead?
My idea was that each additional binding featurs can be added as a new
bit flag in tcp_authopt_key_flag and the structure extended. Older
applications won't pass the flag and kernel will silently accept the
shorter optval and you get compatibility.
As far as I can tell MD5 supports binding in 3 ways:
1) By dst ip address
2) By dst ip address and prefixlen
3) By ifindex for vrfs
Current version of tcp_authopt only supports (1) but I think adding the
other methods isn't actually difficult at all.
I'd rather not guess at future features by adding unused fields.
>>> The same is about key: I don't think you need to define/use
>>> tcp_authopt_alg.
>>> Just use algo name - that way TCP-AO will automatically be able to use
>>> any algo supported by crypto engine.
>>> See how xfrm does it, e.g.:
>>> struct xfrm_algo_auth {
>>> char alg_name[64];
>>> unsigned int alg_key_len; /* in bits */
>>> unsigned int alg_trunc_len; /* in bits */
>>> char alg_key[0];
>>> };
>>>
>>> So you can let a user chose maclen instead of hard-coding it.
>>> Much more extendable than what you propose.
>>
>> This complicates ABI and implementation with features that are not
>> needed. I'd much rather only expose an enum of real-world tcp-ao
>> algorithms.
>
> I see it exactly the opposite way: a new enum unnecessary complicates
> ABI, instead of passing alg_name[] to crypto engine. No need to add any
> support in tcp-ao as the algorithms are already provided by kernel.
> That is how it transparently works for ipsec, why not for tcp-ao?
The TCP Authentication Option standard has been out there for many many
years now and alternative algorithms are not widely used. I think cisco
has a third algorithm? What you're asking for is a large extension of
the IETF standard.
If you look at the rest of this series I had a lot of trouble with
crypto tfm allocation context so I had to create per-cpu pool, similar
to tcp-md5. Should I potentially create a pool for each alg known to
crypto-api?
Letting use control algorithms and traffickey and mac lengths creates
new potential avenues for privilege escalation that need to be checked.
As much as possible I would like to avoid exposing the linux crypto api
through TCP sockopts.
I was also thinking of having a non-namespaced sysctl to disable
tcp_authopt by default for security reasons. Unless you're running a
router you should never let userspace touch these options.
>>> [..]
>>>> +#ifdef CONFIG_TCP_AUTHOPT
>>>> + case TCP_AUTHOPT: {
>>>> + struct tcp_authopt info;
>>>> +
>>>> + if (get_user(len, optlen))
>>>> + return -EFAULT;
>>>> +
>>>> + lock_sock(sk);
>>>> + tcp_get_authopt_val(sk, &info);
>>>> + release_sock(sk);
>>>> +
>>>> + len = min_t(unsigned int, len, sizeof(info));
>>>> + if (put_user(len, optlen))
>>>> + return -EFAULT;
>>>> + if (copy_to_user(optval, &info, len))
>>>> + return -EFAULT;
>>>> + return 0;
>>>> + }
>>>
>>> I'm pretty sure it's not a good choice to write partly tcp_authopt.
>>> And a user has no way to check what's the correct len on this kernel.
>>> Instead of len = min_t(unsigned int, len, sizeof(info)), it should be
>>> if (len != sizeof(info))
>>> return -EINVAL;
>>
>> Purpose is to allow sockopts to grow as md5 has grown.
>
> md5 has not grown. See above.
>
> Another issue with your approach
>
> + /* If userspace optlen is too short fill the rest with zeros */
> + if (optlen > sizeof(opt))
> + return -EINVAL;
> + memset(&opt, 0, sizeof(opt));
> + if (copy_from_sockptr(&opt, optval, optlen))
> + return -EFAULT;
>
> is that userspace compiled with updated/grew structure will fail on
> older kernel. So, no extension without breaking something is possible.
Userspace that needs new features and also compatibility with older
kernels could check something like uname.
I think this is already a problem with md5: passing
TCP_MD5SIG_FLAG_PREFIX on an old kernel simply won't take effect and
that's fine.
The bigger concern is to ensure that old binaries work without changes.
> Which also reminds me that currently you don't validate (opt.flags) for
> unknown by kernel flags.
Not sure what you mean, it is explicitly only known flags that are
copied from userspace. I can make setsockopt return an error on unknown
flags, this will make new apps fail more explicitly on old kernels.
> Extending syscalls is impossible without breaking userspace if ABI is
> not designed with extensibility in mind. That was quite a big problem,
> and still is. Please, read this carefully:
> https://lwn.net/Articles/830666/
>
> That is why I'm suggesting you all those changes that will be harder to
> fix when/if your patches get accepted.
Both of the sockopt structs have a "flags" field and structure expansion
will be accompanied by new flags. Is this not sufficient for compatibility?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
2021-08-11 8:29 ` Leonard Crestez
2021-08-11 13:42 ` David Ahern
2021-08-11 14:31 ` Dmitry Safonov
@ 2021-08-12 19:46 ` Leonard Crestez
2 siblings, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-12 19:46 UTC (permalink / raw)
To: Dmitry Safonov
Cc: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern, Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, open list,
linux-crypto, Network Development, Dmitry Safonov
On 8/11/21 11:29 AM, Leonard Crestez wrote:
> On 8/10/21 11:41 PM, Dmitry Safonov wrote:
>> On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <cdleonard@gmail.com>
>>> + /* If an old value exists for same local_id it is deleted */
>>> + key_info = __tcp_authopt_key_info_lookup(sk, info,
>>> opt.local_id);
>>> + if (key_info)
>>> + tcp_authopt_key_del(sk, info, key_info);
>>> + key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL |
>>> __GFP_ZERO);
>>> + if (!key_info)
>>> + return -ENOMEM;
>>
>> 1. You don't need sock_kmalloc() together with tcp_authopt_key_del().
>> It just frees the memory and allocates it back straight away - no
>> sense in doing that.
>
> The list is scanned in multiple places in later commits using nothing
> but an rcu_read_lock, this means that keys can't be updated in-place.
>
>> 2. I think RFC says you must not allow a user to change an existing key:
>>> MKT parameters are not changed. Instead, new MKTs can be installed,
>>> and a connection
>>> can change which MKT it uses.
>>
>> IIUC, it means that one can't just change an existing MKT, but one can
>> remove and later
>> add MKT with the same (send_id, recv_id, src_addr/port, dst_addr/port).
>>
>> So, a reasonable thing to do:
>> if (key_info)
>> return -EEXIST.
>
> You're right, making the user delete keys explicitly is better.
On a second thought this might be required to mark keys as "send-only"
and "recv-only" atomically.
Separately from RFC5925 some vendors implement a "keychain" model based
on RFC8177 where each key has a distinct "accept-lifetime" and a
"send-lifetime". This could be implemented by adding flags
"expired_for_send" and "expired_for_recv" but requires the ability to
set an expiration mark without the key ever being deleted.
--
Regards,
Leonard
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFCv2 2/9] docs: Add user documentation for tcp_authopt
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
2021-08-09 21:35 ` [RFCv2 1/9] tcp: authopt: Initial support and key management Leonard Crestez
@ 2021-08-09 21:35 ` Leonard Crestez
2021-08-09 21:35 ` [RFCv2 3/9] tcp: authopt: Add crypto initialization Leonard Crestez
` (6 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-09 21:35 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
The .rst documentation contains a brief description of the user
interface and includes kernel-doc generated from uapi header.
Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
Documentation/networking/index.rst | 1 +
Documentation/networking/tcp_authopt.rst | 44 ++++++++++++++++++++++++
2 files changed, 45 insertions(+)
create mode 100644 Documentation/networking/tcp_authopt.rst
diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
index 58bc8cd367c6..f5c324a060d8 100644
--- a/Documentation/networking/index.rst
+++ b/Documentation/networking/index.rst
@@ -100,10 +100,11 @@ Contents:
strparser
switchdev
sysfs-tagging
tc-actions-env-rules
tcp-thin
+ tcp_authopt
team
timestamping
tipc
tproxy
tuntap
diff --git a/Documentation/networking/tcp_authopt.rst b/Documentation/networking/tcp_authopt.rst
new file mode 100644
index 000000000000..484f66f41ad5
--- /dev/null
+++ b/Documentation/networking/tcp_authopt.rst
@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========================
+TCP Authentication Option
+=========================
+
+The TCP Authentication option specified by RFC5925 replaces the TCP MD5
+Signature option. It similar in goals but not compatible in either wire formats
+or ABI.
+
+Interface
+=========
+
+Individual keys can be added to or removed from a TCP socket by using
+TCP_AUTHOPT_KEY setsockopt and a ``struct tcp_authopt_key``. There is no
+support for reading back keys and updates always replace the old key. These
+structures represent "Master Key Tuples (MKTs)" as described by the RFC.
+
+Per-socket options can set or read using the TCP_AUTHOPT sockopt and a ``struct
+tcp_authopt``. This is optional: doing setsockopt TCP_AUTHOPT_KEY is
+sufficient to enable the feature.
+
+Configuration associated with TCP Authentication is indepedently attached to
+each TCP socket. After listen and accept the newly returned socket gets an
+independent copy of relevant settings from the listen socket.
+
+Key binding
+-----------
+
+Keys can be bound to remote addresses in a way that is similar to TCP_MD5.
+
+ * The full address must match (/32 or /128)
+ * Ports are ignored
+ * Address binding is optional, by default keys match all addresses
+
+RFC5925 requires that key ids do not overlap when tcp identifiers (addr/port)
+overlap. This is not enforced by linux, configuring ambiguous keys will result
+in packet drops and lost connections.
+
+ABI Reference
+=============
+
+.. kernel-doc:: include/uapi/linux/tcp.h
+ :identifiers: tcp_authopt tcp_authopt_flag tcp_authopt_key tcp_authopt_key_flag tcp_authopt_alg
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 3/9] tcp: authopt: Add crypto initialization
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
2021-08-09 21:35 ` [RFCv2 1/9] tcp: authopt: Initial support and key management Leonard Crestez
2021-08-09 21:35 ` [RFCv2 2/9] docs: Add user documentation for tcp_authopt Leonard Crestez
@ 2021-08-09 21:35 ` Leonard Crestez
2021-08-09 21:35 ` [RFCv2 4/9] tcp: authopt: Compute packet signatures Leonard Crestez
` (5 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-09 21:35 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
The crypto_shash API is used in order to compute packet signatures. The
API comes with several unfortunate limitations:
1) Allocating a crypto_shash can sleep and must be done in user context.
2) Packet signatures must be computed in softirq context
3) Packet signatures use dynamic "traffic keys" which require exclusive
access to crypto_shash for crypto_setkey.
The solution is to allocate one crypto_shash for each possible cpu for
each algorithm at setsockopt time. The per-cpu tfm is then borrowed from
softirq context, signatures are computed and the tfm is returned.
The pool for each algorithm is reference counted, initialized at
setsockopt time and released in tcp_authopt_key_info's rcu callback
Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
include/net/tcp_authopt.h | 3 +
net/ipv4/tcp_authopt.c | 177 +++++++++++++++++++++++++++++++++++++-
2 files changed, 178 insertions(+), 2 deletions(-)
diff --git a/include/net/tcp_authopt.h b/include/net/tcp_authopt.h
index 458d108bb7a8..bd5ba95e15de 100644
--- a/include/net/tcp_authopt.h
+++ b/include/net/tcp_authopt.h
@@ -2,10 +2,12 @@
#ifndef _LINUX_TCP_AUTHOPT_H
#define _LINUX_TCP_AUTHOPT_H
#include <uapi/linux/tcp.h>
+struct tcp_authopt_alg_imp;
+
/* Representation of a Master Key Tuple as per RFC5925 */
struct tcp_authopt_key_info {
struct hlist_node node;
/* Local identifier */
u32 local_id;
@@ -15,10 +17,11 @@ struct tcp_authopt_key_info {
u8 alg_id;
u8 keylen;
u8 key[TCP_AUTHOPT_MAXKEYLEN];
struct rcu_head rcu;
struct sockaddr_storage addr;
+ struct tcp_authopt_alg_imp *alg;
};
/* Per-socket information regarding tcp_authopt */
struct tcp_authopt_info {
/* List of tcp_authopt_key_info */
diff --git a/net/ipv4/tcp_authopt.c b/net/ipv4/tcp_authopt.c
index 5fa7bce8891b..799607fc076f 100644
--- a/net/ipv4/tcp_authopt.c
+++ b/net/ipv4/tcp_authopt.c
@@ -4,10 +4,161 @@
#include <net/tcp.h>
#include <net/tcp_authopt.h>
#include <crypto/hash.h>
#include <trace/events/tcp.h>
+/* All current algorithms have a mac length of 12 but crypto API digestsize can be larger */
+#define TCP_AUTHOPT_MAXMACBUF 20
+#define TCP_AUTHOPT_MAX_TRAFFIC_KEY_LEN 20
+
+struct tcp_authopt_alg_imp {
+ /* Name of algorithm in crypto-api */
+ const char *alg_name;
+ /* One of the TCP_AUTHOPT_ALG_* constants from uapi */
+ u8 alg_id;
+ /* Length of traffic key */
+ u8 traffic_key_len;
+ /* Length of mac in TCP option */
+ u8 maclen;
+
+ /* shared crypto_shash */
+ spinlock_t lock;
+ int ref_cnt;
+ struct crypto_shash *tfm;
+};
+
+static struct tcp_authopt_alg_imp tcp_authopt_alg_list[] = {
+ {
+ .alg_id = TCP_AUTHOPT_ALG_HMAC_SHA_1_96,
+ .alg_name = "hmac(sha1)",
+ .traffic_key_len = 20,
+ .maclen = 12,
+ .lock = __SPIN_LOCK_UNLOCKED(tcp_authopt_alg_list[0].lock),
+ },
+ {
+ .alg_id = TCP_AUTHOPT_ALG_AES_128_CMAC_96,
+ .alg_name = "cmac(aes)",
+ .traffic_key_len = 16,
+ .maclen = 12,
+ .lock = __SPIN_LOCK_UNLOCKED(tcp_authopt_alg_list[1].lock),
+ },
+};
+
+/* get a pointer to the tcp_authopt_alg instance or NULL if id invalid */
+static inline struct tcp_authopt_alg_imp *tcp_authopt_alg_get(int alg_num)
+{
+ if (alg_num <= 0 || alg_num > 2)
+ return NULL;
+ return &tcp_authopt_alg_list[alg_num - 1];
+}
+
+/* Mark an algorithm as in-use from user context */
+static int tcp_authopt_alg_require(struct tcp_authopt_alg_imp *alg)
+{
+ struct crypto_shash *tfm = NULL;
+ bool need_init = false;
+
+ might_sleep();
+
+ /* If we're the first user then we need to initialize shash but we might lose the race. */
+ spin_lock_bh(&alg->lock);
+ WARN_ON(alg->ref_cnt < 0);
+ if (alg->ref_cnt == 0)
+ need_init = true;
+ else
+ ++alg->ref_cnt;
+ spin_unlock_bh(&alg->lock);
+
+ /* Already initialized */
+ if (!need_init)
+ return 0;
+
+ tfm = crypto_alloc_shash(alg->alg_name, 0, 0);
+ if (IS_ERR(tfm))
+ return PTR_ERR(tfm);
+
+ spin_lock_bh(&alg->lock);
+ if (alg->ref_cnt == 0)
+ /* race won */
+ alg->tfm = tfm;
+ else
+ /* race lost, free tfm later */
+ need_init = false;
+ ++alg->ref_cnt;
+ spin_unlock_bh(&alg->lock);
+
+ if (!need_init)
+ crypto_free_shash(tfm);
+ else
+ pr_info("initialized tcp-ao %s", alg->alg_name);
+
+ return 0;
+}
+
+static void tcp_authopt_alg_release(struct tcp_authopt_alg_imp *alg)
+{
+ struct crypto_shash *tfm_to_free = NULL;
+
+ spin_lock_bh(&alg->lock);
+ --alg->ref_cnt;
+ WARN_ON(alg->ref_cnt < 0);
+ if (alg->ref_cnt == 0) {
+ tfm_to_free = alg->tfm;
+ alg->tfm = NULL;
+ }
+ spin_unlock_bh(&alg->lock);
+
+ if (tfm_to_free) {
+ pr_info("released tcp-ao %s", alg->alg_name);
+ crypto_free_shash(tfm_to_free);
+ }
+}
+
+/* increase reference count on an algorithm that is already in use */
+static void tcp_authopt_alg_incref(struct tcp_authopt_alg_imp *alg)
+{
+ spin_lock_bh(&alg->lock);
+ WARN_ON(alg->ref_cnt <= 0);
+ ++alg->ref_cnt;
+ spin_unlock_bh(&alg->lock);
+}
+
+static struct crypto_shash *tcp_authopt_alg_get_tfm(struct tcp_authopt_alg_imp *alg)
+{
+ spin_lock_bh(&alg->lock);
+ WARN_ON(alg->ref_cnt < 0);
+ return alg->tfm;
+}
+
+static void tcp_authopt_alg_put_tfm(struct tcp_authopt_alg_imp *alg, struct crypto_shash *tfm)
+{
+ WARN_ON(tfm != alg->tfm);
+ spin_unlock_bh(&alg->lock);
+}
+
+static struct crypto_shash *tcp_authopt_get_kdf_shash(struct tcp_authopt_key_info *key)
+{
+ return tcp_authopt_alg_get_tfm(key->alg);
+}
+
+static void tcp_authopt_put_kdf_shash(struct tcp_authopt_key_info *key,
+ struct crypto_shash *tfm)
+{
+ return tcp_authopt_alg_put_tfm(key->alg, tfm);
+}
+
+static struct crypto_shash *tcp_authopt_get_mac_shash(struct tcp_authopt_key_info *key)
+{
+ return tcp_authopt_alg_get_tfm(key->alg);
+}
+
+static void tcp_authopt_put_mac_shash(struct tcp_authopt_key_info *key,
+ struct crypto_shash *tfm)
+{
+ return tcp_authopt_alg_put_tfm(key->alg, tfm);
+}
+
struct tcp_authopt_key_info *__tcp_authopt_key_info_lookup(const struct sock *sk,
struct tcp_authopt_info *info,
int key_id)
{
struct tcp_authopt_key_info *key;
@@ -75,17 +226,25 @@ int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt)
opt->flags = info->flags & TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED;
return 0;
}
+static void tcp_authopt_key_free_rcu(struct rcu_head *rcu)
+{
+ struct tcp_authopt_key_info *key = container_of(rcu, struct tcp_authopt_key_info, rcu);
+
+ tcp_authopt_alg_release(key->alg);
+ kfree(key);
+}
+
static void tcp_authopt_key_del(struct sock *sk,
struct tcp_authopt_info *info,
struct tcp_authopt_key_info *key)
{
hlist_del_rcu(&key->node);
atomic_sub(sizeof(*key), &sk->sk_omem_alloc);
- kfree_rcu(key, rcu);
+ call_rcu(&key->rcu, tcp_authopt_key_free_rcu);
}
/* free info and keys but don't touch tp->authopt_info */
void __tcp_authopt_info_free(struct sock *sk, struct tcp_authopt_info *info)
{
@@ -112,10 +271,12 @@ void tcp_authopt_clear(struct sock *sk)
int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
{
struct tcp_authopt_key opt;
struct tcp_authopt_info *info;
struct tcp_authopt_key_info *key_info;
+ struct tcp_authopt_alg_imp *alg;
+ int err;
/* If userspace optlen is too short fill the rest with zeros */
if (optlen > sizeof(opt))
return -EINVAL;
memset(&opt, 0, sizeof(opt));
@@ -149,22 +310,34 @@ int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
if (opt.flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
if (sk->sk_family != opt.addr.ss_family)
return -EINVAL;
}
+ /* check the algorithm */
+ alg = tcp_authopt_alg_get(opt.alg);
+ if (!alg)
+ return -EINVAL;
+ WARN_ON(alg->alg_id != opt.alg);
+ err = tcp_authopt_alg_require(alg);
+ if (err)
+ return err;
+
/* If an old value exists for same local_id it is deleted */
key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
if (key_info)
tcp_authopt_key_del(sk, info, key_info);
key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL | __GFP_ZERO);
- if (!key_info)
+ if (!key_info) {
+ tcp_authopt_alg_release(alg);
return -ENOMEM;
+ }
key_info->local_id = opt.local_id;
key_info->flags = opt.flags & (TCP_AUTHOPT_KEY_EXCLUDE_OPTS | TCP_AUTHOPT_KEY_ADDR_BIND);
key_info->send_id = opt.send_id;
key_info->recv_id = opt.recv_id;
key_info->alg_id = opt.alg;
+ key_info->alg = alg;
key_info->keylen = opt.keylen;
memcpy(key_info->key, opt.key, opt.keylen);
memcpy(&key_info->addr, &opt.addr, sizeof(key_info->addr));
hlist_add_head_rcu(&key_info->node, &info->head);
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 4/9] tcp: authopt: Compute packet signatures
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
` (2 preceding siblings ...)
2021-08-09 21:35 ` [RFCv2 3/9] tcp: authopt: Add crypto initialization Leonard Crestez
@ 2021-08-09 21:35 ` Leonard Crestez
2021-08-09 21:35 ` [RFCv2 5/9] tcp: authopt: Hook into tcp core Leonard Crestez
` (4 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-09 21:35 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
Computing tcp authopt packet signatures is a two step process:
* traffic key is computed based on tcp 4-tuple, initial sequence numbers
and the secret key.
* packet mac is computed based on traffic key and content of individual
packets.
The traffic key could be cached for established sockets but it is not.
A single code path exists for ipv4/ipv6 and input/output. This keeps the
code short but slightly slower due to lots of conditionals.
On output we read remote IP address from socket members on output, we
can't use skb network header because it's computed after TCP options.
On input we read remote IP address from skb network headers, we can't
use socket binding members because those are not available for SYN.
Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
net/ipv4/tcp_authopt.c | 467 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 467 insertions(+)
diff --git a/net/ipv4/tcp_authopt.c b/net/ipv4/tcp_authopt.c
index 799607fc076f..a42daadf6b7d 100644
--- a/net/ipv4/tcp_authopt.c
+++ b/net/ipv4/tcp_authopt.c
@@ -341,5 +341,472 @@ int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
memcpy(&key_info->addr, &opt.addr, sizeof(key_info->addr));
hlist_add_head_rcu(&key_info->node, &info->head);
return 0;
}
+
+/* feed traffic key into shash */
+static int tcp_authopt_shash_traffic_key(struct shash_desc *desc,
+ struct sock *sk,
+ struct sk_buff *skb,
+ bool input,
+ bool ipv6)
+{
+ struct tcphdr *th = tcp_hdr(skb);
+ int err;
+ __be32 sisn, disn;
+ __be16 digestbits = htons(crypto_shash_digestsize(desc->tfm) * 8);
+
+ // RFC5926 section 3.1.1.1
+ err = crypto_shash_update(desc, "\x01TCP-AO", 7);
+ if (err)
+ return err;
+
+ /* Addresses from packet on input and from socket on output
+ * This is because on output MAC is computed before prepending IP header
+ */
+ if (input) {
+ if (ipv6)
+ err = crypto_shash_update(desc, (u8 *)&ipv6_hdr(skb)->saddr, 32);
+ else
+ err = crypto_shash_update(desc, (u8 *)&ip_hdr(skb)->saddr, 8);
+ if (err)
+ return err;
+ } else {
+ if (ipv6) {
+ struct in6_addr *saddr;
+ struct in6_addr *daddr;
+
+ saddr = &sk->sk_v6_rcv_saddr;
+ daddr = &sk->sk_v6_daddr;
+ err = crypto_shash_update(desc, (u8 *)&sk->sk_v6_rcv_saddr, 16);
+ if (err)
+ return err;
+ err = crypto_shash_update(desc, (u8 *)&sk->sk_v6_daddr, 16);
+ if (err)
+ return err;
+ } else {
+ err = crypto_shash_update(desc, (u8 *)&sk->sk_rcv_saddr, 4);
+ if (err)
+ return err;
+ err = crypto_shash_update(desc, (u8 *)&sk->sk_daddr, 4);
+ if (err)
+ return err;
+ }
+ }
+
+ /* TCP ports from header */
+ err = crypto_shash_update(desc, (u8 *)&th->source, 4);
+ if (err)
+ return err;
+
+ /* special cases for SYN and SYN/ACK */
+ if (th->syn && !th->ack) {
+ sisn = th->seq;
+ disn = 0;
+ } else if (th->syn && th->ack) {
+ sisn = th->seq;
+ disn = htonl(ntohl(th->ack_seq) - 1);
+ } else {
+ struct tcp_authopt_info *authopt_info;
+
+ /* Fetching authopt_info like this means it's possible that authopt_info
+ * was deleted while we were hashing. If that happens we drop the packet
+ * which should be fine.
+ *
+ * A better solution might be to always pass info as a parameter, or
+ * compute traffic_key for established sockets separately.
+ */
+ rcu_read_lock();
+ authopt_info = rcu_dereference(tcp_sk(sk)->authopt_info);
+ if (!authopt_info) {
+ rcu_read_unlock();
+ return -EINVAL;
+ }
+ /* Initial sequence numbers for ESTABLISHED connections from info */
+ if (input) {
+ sisn = htonl(authopt_info->dst_isn);
+ disn = htonl(authopt_info->src_isn);
+ } else {
+ sisn = htonl(authopt_info->src_isn);
+ disn = htonl(authopt_info->dst_isn);
+ }
+ rcu_read_unlock();
+ }
+
+ err = crypto_shash_update(desc, (u8 *)&sisn, 4);
+ if (err)
+ return err;
+ err = crypto_shash_update(desc, (u8 *)&disn, 4);
+ if (err)
+ return err;
+
+ err = crypto_shash_update(desc, (u8 *)&digestbits, 2);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+/* Convert a variable-length key to a 16-byte fixed-length key for AES-CMAC
+ * This is described in RFC5926 section 3.1.1.2
+ */
+static int aes_setkey_derived(struct crypto_shash *tfm, u8 *key, size_t keylen)
+{
+ static const u8 zeros[16] = {0};
+ u8 derived_key[16];
+ int err;
+
+ if (WARN_ON(crypto_shash_digestsize(tfm) != 16))
+ return -EINVAL;
+ err = crypto_shash_setkey(tfm, zeros, sizeof(zeros));
+ if (err)
+ return err;
+ err = crypto_shash_tfm_digest(tfm, key, keylen, derived_key);
+ if (err)
+ return err;
+ return crypto_shash_setkey(tfm, derived_key, sizeof(derived_key));
+}
+
+static int tcp_authopt_get_traffic_key(struct sock *sk,
+ struct sk_buff *skb,
+ struct tcp_authopt_key_info *key,
+ bool input,
+ bool ipv6,
+ u8 *traffic_key)
+{
+ SHASH_DESC_ON_STACK(desc, kdf_tfm);
+ struct crypto_shash *kdf_tfm;
+ int err;
+
+ kdf_tfm = tcp_authopt_get_kdf_shash(key);
+ if (IS_ERR(kdf_tfm))
+ return PTR_ERR(kdf_tfm);
+ if (WARN_ON(crypto_shash_digestsize(kdf_tfm) != key->alg->traffic_key_len)) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ if (key->alg_id == TCP_AUTHOPT_ALG_AES_128_CMAC_96 && key->keylen != 16) {
+ err = aes_setkey_derived(kdf_tfm, key->key, key->keylen);
+ if (err)
+ goto out;
+ } else {
+ err = crypto_shash_setkey(kdf_tfm, key->key, key->keylen);
+ if (err)
+ goto out;
+ }
+
+ desc->tfm = kdf_tfm;
+ err = crypto_shash_init(desc);
+ if (err)
+ goto out;
+
+ err = tcp_authopt_shash_traffic_key(desc, sk, skb, input, ipv6);
+ if (err)
+ goto out;
+
+ err = crypto_shash_final(desc, traffic_key);
+ if (err)
+ goto out;
+ //printk("traffic_key: %*phN\n", 20, traffic_key);
+
+out:
+ tcp_authopt_put_kdf_shash(key, kdf_tfm);
+ return err;
+}
+
+static int crypto_shash_update_zero(struct shash_desc *desc, int len)
+{
+ u8 zero = 0;
+ int i, err;
+
+ for (i = 0; i < len; ++i) {
+ err = crypto_shash_update(desc, &zero, 1);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static int tcp_authopt_hash_tcp4_pseudoheader(struct shash_desc *desc,
+ __be32 saddr,
+ __be32 daddr,
+ int nbytes)
+{
+ struct tcp4_pseudohdr phdr = {
+ .saddr = saddr,
+ .daddr = daddr,
+ .pad = 0,
+ .protocol = IPPROTO_TCP,
+ .len = htons(nbytes)
+ };
+ return crypto_shash_update(desc, (u8 *)&phdr, sizeof(phdr));
+}
+
+static int tcp_authopt_hash_tcp6_pseudoheader(struct shash_desc *desc,
+ struct in6_addr *saddr,
+ struct in6_addr *daddr,
+ u32 plen)
+{
+ int err;
+ u32 buf[2];
+
+ buf[0] = htonl(plen);
+ buf[1] = htonl(IPPROTO_TCP);
+
+ err = crypto_shash_update(desc, (u8 *)saddr, sizeof(*saddr));
+ if (err)
+ return err;
+ err = crypto_shash_update(desc, (u8 *)daddr, sizeof(*daddr));
+ if (err)
+ return err;
+ return crypto_shash_update(desc, (u8 *)&buf, sizeof(buf));
+}
+
+/* TCP authopt as found in header */
+struct tcphdr_authopt {
+ u8 num;
+ u8 len;
+ u8 keyid;
+ u8 rnextkeyid;
+ u8 mac[0];
+};
+
+/* Find TCP_AUTHOPT in header.
+ *
+ * Returns pointer to TCP_AUTHOPT or NULL if not found.
+ */
+static u8 *tcp_authopt_find_option(struct tcphdr *th)
+{
+ int length = (th->doff << 2) - sizeof(*th);
+ u8 *ptr = (u8 *)(th + 1);
+
+ while (length >= 2) {
+ int opcode = *ptr++;
+ int opsize;
+
+ switch (opcode) {
+ case TCPOPT_EOL:
+ return NULL;
+ case TCPOPT_NOP:
+ length--;
+ continue;
+ default:
+ if (length < 2)
+ return NULL;
+ opsize = *ptr++;
+ if (opsize < 2)
+ return NULL;
+ if (opsize > length)
+ return NULL;
+ if (opcode == TCPOPT_AUTHOPT)
+ return ptr - 2;
+ }
+ ptr += opsize - 2;
+ length -= opsize;
+ }
+ return NULL;
+}
+
+/** Hash tcphdr options.
+ * If include_options is false then only the TCPOPT_AUTHOPT option itself is hashed
+ * Maybe we could skip option parsing by assuming the AUTHOPT header is at hash_location-4?
+ */
+static int tcp_authopt_hash_opts(struct shash_desc *desc,
+ struct tcphdr *th,
+ bool include_options)
+{
+ int err;
+ /* start of options */
+ u8 *tcp_opts = (u8 *)(th + 1);
+ /* end of options */
+ u8 *tcp_data = ((u8 *)th) + th->doff * 4;
+ /* pointer to TCPOPT_AUTHOPT */
+ u8 *authopt_ptr = tcp_authopt_find_option(th);
+ u8 authopt_len;
+
+ if (!authopt_ptr)
+ return -EINVAL;
+ authopt_len = *(authopt_ptr + 1);
+
+ if (include_options) {
+ err = crypto_shash_update(desc, tcp_opts, authopt_ptr - tcp_opts + 4);
+ if (err)
+ return err;
+ err = crypto_shash_update_zero(desc, authopt_len - 4);
+ if (err)
+ return err;
+ err = crypto_shash_update(desc,
+ authopt_ptr + authopt_len,
+ tcp_data - (authopt_ptr + authopt_len));
+ if (err)
+ return err;
+ } else {
+ err = crypto_shash_update(desc, authopt_ptr, 4);
+ if (err)
+ return err;
+ err = crypto_shash_update_zero(desc, authopt_len - 4);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static int skb_shash_frags(struct shash_desc *desc,
+ struct sk_buff *skb)
+{
+ struct sk_buff *frag_iter;
+ int err, i;
+
+ for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+ skb_frag_t *f = &skb_shinfo(skb)->frags[i];
+ u32 p_off, p_len, copied;
+ struct page *p;
+ u8 *vaddr;
+
+ skb_frag_foreach_page(f, skb_frag_off(f), skb_frag_size(f),
+ p, p_off, p_len, copied) {
+ vaddr = kmap_atomic(p);
+ err = crypto_shash_update(desc, vaddr + p_off, p_len);
+ kunmap_atomic(vaddr);
+ if (err)
+ return err;
+ }
+ }
+
+ skb_walk_frags(skb, frag_iter) {
+ err = skb_shash_frags(desc, frag_iter);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static int tcp_authopt_hash_packet(struct crypto_shash *tfm,
+ struct sock *sk,
+ struct sk_buff *skb,
+ bool input,
+ bool ipv6,
+ bool include_options,
+ u8 *macbuf)
+{
+ struct tcphdr *th = tcp_hdr(skb);
+ SHASH_DESC_ON_STACK(desc, tfm);
+ int err;
+
+ /* NOTE: SNE unimplemented */
+ __be32 sne = 0;
+
+ desc->tfm = tfm;
+ err = crypto_shash_init(desc);
+ if (err)
+ return err;
+
+ err = crypto_shash_update(desc, (u8 *)&sne, 4);
+ if (err)
+ return err;
+
+ if (ipv6) {
+ struct in6_addr *saddr;
+ struct in6_addr *daddr;
+
+ if (input) {
+ saddr = &ipv6_hdr(skb)->saddr;
+ daddr = &ipv6_hdr(skb)->daddr;
+ } else {
+ saddr = &sk->sk_v6_rcv_saddr;
+ daddr = &sk->sk_v6_daddr;
+ }
+ err = tcp_authopt_hash_tcp6_pseudoheader(desc, saddr, daddr, skb->len);
+ if (err)
+ return err;
+ } else {
+ __be32 saddr;
+ __be32 daddr;
+
+ if (input) {
+ saddr = ip_hdr(skb)->saddr;
+ daddr = ip_hdr(skb)->daddr;
+ } else {
+ saddr = sk->sk_rcv_saddr;
+ daddr = sk->sk_daddr;
+ }
+ err = tcp_authopt_hash_tcp4_pseudoheader(desc, saddr, daddr, skb->len);
+ if (err)
+ return err;
+ }
+
+ // TCP header with checksum set to zero
+ {
+ struct tcphdr hashed_th = *th;
+
+ hashed_th.check = 0;
+ err = crypto_shash_update(desc, (u8 *)&hashed_th, sizeof(hashed_th));
+ if (err)
+ return err;
+ }
+
+ // TCP options
+ err = tcp_authopt_hash_opts(desc, th, include_options);
+ if (err)
+ return err;
+
+ // Rest of SKB->data
+ err = crypto_shash_update(desc, (u8 *)th + th->doff * 4, skb_headlen(skb) - th->doff * 4);
+ if (err)
+ return err;
+
+ err = skb_shash_frags(desc, skb);
+ if (err)
+ return err;
+
+ return crypto_shash_final(desc, macbuf);
+}
+
+int __tcp_authopt_calc_mac(struct sock *sk,
+ struct sk_buff *skb,
+ struct tcp_authopt_key_info *key,
+ bool input,
+ char *macbuf)
+{
+ struct crypto_shash *mac_tfm;
+ u8 traffic_key[TCP_AUTHOPT_MAX_TRAFFIC_KEY_LEN];
+ int err;
+ bool ipv6 = (sk->sk_family != AF_INET);
+
+ if (sk->sk_family != AF_INET && sk->sk_family != AF_INET6)
+ return -EINVAL;
+ if (WARN_ON(key->alg->traffic_key_len > sizeof(traffic_key)))
+ return -ENOBUFS;
+
+ err = tcp_authopt_get_traffic_key(sk, skb, key, input, ipv6, traffic_key);
+ if (err)
+ return err;
+
+ mac_tfm = tcp_authopt_get_mac_shash(key);
+ if (IS_ERR(mac_tfm))
+ return PTR_ERR(mac_tfm);
+ if (crypto_shash_digestsize(mac_tfm) > TCP_AUTHOPT_MAXMACBUF) {
+ err = -EINVAL;
+ goto out;
+ }
+ err = crypto_shash_setkey(mac_tfm, traffic_key, key->alg->traffic_key_len);
+ if (err)
+ goto out;
+
+ err = tcp_authopt_hash_packet(mac_tfm,
+ sk,
+ skb,
+ input,
+ ipv6,
+ !(key->flags & TCP_AUTHOPT_KEY_EXCLUDE_OPTS),
+ macbuf);
+ //printk("mac: %*phN\n", key->maclen, macbuf);
+
+out:
+ tcp_authopt_put_mac_shash(key, mac_tfm);
+ return err;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 5/9] tcp: authopt: Hook into tcp core
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
` (3 preceding siblings ...)
2021-08-09 21:35 ` [RFCv2 4/9] tcp: authopt: Compute packet signatures Leonard Crestez
@ 2021-08-09 21:35 ` Leonard Crestez
2021-08-09 21:35 ` [RFCv2 6/9] tcp: authopt: Add key selection controls Leonard Crestez
` (3 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-09 21:35 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
The tcp_authopt features exposes a minimal interface to the rest of the
TCP stack. Only a few functions are exposed and if the feature is
disabled they return neutral values, avoiding ifdefs in the rest of the
code.
Add calls into tcp authopt from send, receive and accept code.
Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
include/net/tcp_authopt.h | 55 +++++++++
net/ipv4/tcp_authopt.c | 227 ++++++++++++++++++++++++++++++++++++++
net/ipv4/tcp_input.c | 17 +++
net/ipv4/tcp_ipv4.c | 3 +
net/ipv4/tcp_minisocks.c | 2 +
net/ipv4/tcp_output.c | 56 +++++++++-
net/ipv6/tcp_ipv6.c | 4 +
7 files changed, 363 insertions(+), 1 deletion(-)
diff --git a/include/net/tcp_authopt.h b/include/net/tcp_authopt.h
index bd5ba95e15de..28ebc77473a4 100644
--- a/include/net/tcp_authopt.h
+++ b/include/net/tcp_authopt.h
@@ -15,10 +15,11 @@ struct tcp_authopt_key_info {
/* Wire identifiers */
u8 send_id, recv_id;
u8 alg_id;
u8 keylen;
u8 key[TCP_AUTHOPT_MAXKEYLEN];
+ u8 maclen;
struct rcu_head rcu;
struct sockaddr_storage addr;
struct tcp_authopt_alg_imp *alg;
};
@@ -31,15 +32,52 @@ struct tcp_authopt_info {
u32 dst_isn;
struct rcu_head rcu;
};
#ifdef CONFIG_TCP_AUTHOPT
+struct tcp_authopt_key_info *tcp_authopt_select_key(const struct sock *sk,
+ const struct sock *addr_sk,
+ u8 *rnextkeyid);
void tcp_authopt_clear(struct sock *sk);
int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen);
int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *key);
int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen);
+int tcp_authopt_hash(
+ char *hash_location,
+ struct tcp_authopt_key_info *key,
+ struct sock *sk, struct sk_buff *skb);
+int __tcp_authopt_openreq(struct sock *newsk, const struct sock *oldsk, struct request_sock *req);
+static inline int tcp_authopt_openreq(
+ struct sock *newsk,
+ const struct sock *oldsk,
+ struct request_sock *req)
+{
+ if (!rcu_dereference(tcp_sk(oldsk)->authopt_info))
+ return 0;
+ else
+ return __tcp_authopt_openreq(newsk, oldsk, req);
+}
+int __tcp_authopt_inbound_check(
+ struct sock *sk,
+ struct sk_buff *skb,
+ struct tcp_authopt_info *info);
+static inline int tcp_authopt_inbound_check(struct sock *sk, struct sk_buff *skb)
+{
+ struct tcp_authopt_info *info = rcu_dereference(tcp_sk(sk)->authopt_info);
+
+ if (info)
+ return __tcp_authopt_inbound_check(sk, skb, info);
+ else
+ return 0;
+}
#else
+static struct tcp_authopt_key_info *tcp_authopt_select_key(const struct sock *sk,
+ const struct sock *addr_sk,
+ u8 *rnextkeyid);
+{
+ return NULL;
+}
static inline int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen)
{
return -ENOPROTOOPT;
}
static inline int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *key)
@@ -51,8 +89,25 @@ static inline void tcp_authopt_clear(struct sock *sk)
}
static inline int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
{
return -ENOPROTOOPT;
}
+static inline int tcp_authopt_hash(
+ char *hash_location,
+ struct tcp_authopt_key_info *key,
+ struct sock *sk, struct sk_buff *skb)
+{
+ return -EINVAL;
+}
+static inline int tcp_authopt_openreq(struct sock *newsk,
+ const struct sock *oldsk,
+ struct request_sock *req)
+{
+ return 0;
+}
+static inline int tcp_authopt_inbound_check(struct sock *sk, struct sk_buff *skb)
+{
+ return 0;
+}
#endif
#endif /* _LINUX_TCP_AUTHOPT_H */
diff --git a/net/ipv4/tcp_authopt.c b/net/ipv4/tcp_authopt.c
index a42daadf6b7d..493461e46460 100644
--- a/net/ipv4/tcp_authopt.c
+++ b/net/ipv4/tcp_authopt.c
@@ -168,10 +168,66 @@ struct tcp_authopt_key_info *__tcp_authopt_key_info_lookup(const struct sock *sk
return key;
return NULL;
}
+struct tcp_authopt_key_info *tcp_authopt_lookup_send(struct tcp_authopt_info *info,
+ const struct sock *addr_sk,
+ int send_id)
+{
+ struct tcp_authopt_key_info *result = NULL;
+ struct tcp_authopt_key_info *key;
+
+ hlist_for_each_entry_rcu(key, &info->head, node, 0) {
+ if (send_id >= 0 && key->send_id != send_id)
+ continue;
+ if (key->flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
+ if (addr_sk->sk_family == AF_INET) {
+ struct sockaddr_in *key_addr = (struct sockaddr_in *)&key->addr;
+ const struct in_addr *daddr =
+ (const struct in_addr *)&addr_sk->sk_daddr;
+
+ if (WARN_ON(key_addr->sin_family != AF_INET))
+ continue;
+ if (memcmp(daddr, &key_addr->sin_addr, sizeof(*daddr)))
+ continue;
+ }
+ if (addr_sk->sk_family == AF_INET6) {
+ struct sockaddr_in6 *key_addr = (struct sockaddr_in6 *)&key->addr;
+ const struct in6_addr *daddr = &addr_sk->sk_v6_daddr;
+
+ if (WARN_ON(key_addr->sin6_family != AF_INET6))
+ continue;
+ if (memcmp(daddr, &key_addr->sin6_addr, sizeof(*daddr)))
+ continue;
+ }
+ }
+ if (result && net_ratelimit())
+ pr_warn("ambiguous tcp authentication keys configured for send\n");
+ result = key;
+ }
+
+ return result;
+}
+
+/* Select key for sending
+ * addr_sk is the sock used for comparing daddr, it is only different from sk in
+ * the synack case.
+ */
+struct tcp_authopt_key_info *tcp_authopt_select_key(const struct sock *sk,
+ const struct sock *addr_sk,
+ u8 *rnextkeyid)
+{
+ struct tcp_authopt_info *info;
+
+ info = rcu_dereference(tcp_sk(sk)->authopt_info);
+ if (!info)
+ return NULL;
+
+ return tcp_authopt_lookup_send(info, addr_sk, -1);
+}
+
static struct tcp_authopt_info *__tcp_authopt_info_get_or_create(struct sock *sk)
{
struct tcp_sock *tp = tcp_sk(sk);
struct tcp_authopt_info *info;
@@ -336,16 +392,69 @@ int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
key_info->recv_id = opt.recv_id;
key_info->alg_id = opt.alg;
key_info->alg = alg;
key_info->keylen = opt.keylen;
memcpy(key_info->key, opt.key, opt.keylen);
+ key_info->maclen = alg->maclen;
memcpy(&key_info->addr, &opt.addr, sizeof(key_info->addr));
hlist_add_head_rcu(&key_info->node, &info->head);
return 0;
}
+static int tcp_authopt_clone_keys(struct sock *newsk,
+ const struct sock *oldsk,
+ struct tcp_authopt_info *new_info,
+ struct tcp_authopt_info *old_info)
+{
+ struct tcp_authopt_key_info *old_key;
+ struct tcp_authopt_key_info *new_key;
+
+ hlist_for_each_entry_rcu(old_key, &old_info->head, node, lockdep_sock_is_held(sk)) {
+ new_key = sock_kmalloc(newsk, sizeof(*new_key), GFP_ATOMIC);
+ if (!new_key)
+ return -ENOMEM;
+ memcpy(new_key, old_key, sizeof(*new_key));
+ tcp_authopt_alg_incref(old_key->alg);
+ hlist_add_head_rcu(&new_key->node, &new_info->head);
+ }
+
+ return 0;
+}
+
+/** Called to create accepted sockets.
+ *
+ * Need to copy authopt info from listen socket.
+ */
+int __tcp_authopt_openreq(struct sock *newsk, const struct sock *oldsk, struct request_sock *req)
+{
+ struct tcp_authopt_info *old_info;
+ struct tcp_authopt_info *new_info;
+ int err;
+
+ old_info = rcu_dereference(tcp_sk(oldsk)->authopt_info);
+ if (!old_info)
+ return 0;
+
+ new_info = kmalloc(sizeof(*new_info), GFP_ATOMIC | __GFP_ZERO);
+ if (!new_info)
+ return -ENOMEM;
+
+ sk_nocaps_add(newsk, NETIF_F_GSO_MASK);
+ new_info->src_isn = tcp_rsk(req)->snt_isn;
+ new_info->dst_isn = tcp_rsk(req)->rcv_isn;
+ INIT_HLIST_HEAD(&new_info->head);
+ err = tcp_authopt_clone_keys(newsk, oldsk, new_info, old_info);
+ if (err) {
+ __tcp_authopt_info_free(newsk, new_info);
+ return err;
+ }
+ rcu_assign_pointer(tcp_sk(newsk)->authopt_info, new_info);
+
+ return 0;
+}
+
/* feed traffic key into shash */
static int tcp_authopt_shash_traffic_key(struct shash_desc *desc,
struct sock *sk,
struct sk_buff *skb,
bool input,
@@ -808,5 +917,123 @@ int __tcp_authopt_calc_mac(struct sock *sk,
out:
tcp_authopt_put_mac_shash(key, mac_tfm);
return err;
}
+
+int tcp_authopt_hash(char *hash_location,
+ struct tcp_authopt_key_info *key,
+ struct sock *sk,
+ struct sk_buff *skb)
+{
+ /* MAC inside option is truncated to 12 bytes but crypto API needs output
+ * buffer to be large enough so we use a buffer on the stack.
+ */
+ u8 macbuf[TCP_AUTHOPT_MAXMACBUF];
+ int err;
+
+ if (WARN_ON(key->maclen > sizeof(macbuf)))
+ return -ENOBUFS;
+
+ err = __tcp_authopt_calc_mac(sk, skb, key, false, macbuf);
+ if (err) {
+ memset(hash_location, 0, key->maclen);
+ return err;
+ }
+ memcpy(hash_location, macbuf, key->maclen);
+
+ return 0;
+}
+
+static struct tcp_authopt_key_info *tcp_authopt_lookup_recv(struct sock *sk,
+ struct sk_buff *skb,
+ struct tcp_authopt_info *info,
+ int recv_id)
+{
+ struct tcp_authopt_key_info *result = NULL;
+ struct tcp_authopt_key_info *key;
+
+ /* multiple matches will cause occasional failures */
+ hlist_for_each_entry_rcu(key, &info->head, node, 0) {
+ if (recv_id >= 0 && key->recv_id != recv_id)
+ continue;
+ if (key->flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
+ if (sk->sk_family == AF_INET) {
+ struct sockaddr_in *key_addr = (struct sockaddr_in *)&key->addr;
+ struct iphdr *iph = (struct iphdr *)skb_network_header(skb);
+
+ if (WARN_ON(key_addr->sin_family != AF_INET))
+ continue;
+ if (WARN_ON(iph->version != 4))
+ continue;
+ if (memcmp(&iph->saddr, &key_addr->sin_addr, sizeof(iph->saddr)))
+ continue;
+ }
+ if (sk->sk_family == AF_INET6) {
+ struct sockaddr_in6 *key_addr = (struct sockaddr_in6 *)&key->addr;
+ struct ipv6hdr *iph = (struct ipv6hdr *)skb_network_header(skb);
+
+ if (WARN_ON(key_addr->sin6_family != AF_INET6))
+ continue;
+ if (WARN_ON(iph->version != 6))
+ continue;
+ if (memcmp(&iph->saddr, &key_addr->sin6_addr, sizeof(iph->saddr)))
+ continue;
+ }
+ }
+ if (result && net_ratelimit())
+ pr_warn("ambiguous tcp authentication keys configured for receive\n");
+ result = key;
+ }
+
+ return result;
+}
+
+int __tcp_authopt_inbound_check(struct sock *sk, struct sk_buff *skb, struct tcp_authopt_info *info)
+{
+ struct tcphdr *th = (struct tcphdr *)skb_transport_header(skb);
+ struct tcphdr_authopt *opt;
+ struct tcp_authopt_key_info *key;
+ u8 macbuf[16];
+ int err;
+
+ opt = (struct tcphdr_authopt *)tcp_authopt_find_option(th);
+ key = tcp_authopt_lookup_recv(sk, skb, info, opt ? opt->keyid : -1);
+
+ /* nothing found or expected */
+ if (!opt && !key)
+ return 0;
+ if (!opt && key) {
+ net_info_ratelimited("TCP Authentication Missing\n");
+ return -EINVAL;
+ }
+ if (opt && !key) {
+ /* RFC5925 Section 7.3:
+ * A TCP-AO implementation MUST allow for configuration of the behavior
+ * of segments with TCP-AO but that do not match an MKT. The initial
+ * default of this configuration SHOULD be to silently accept such
+ * connections.
+ */
+ if (info->flags & TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED) {
+ net_info_ratelimited("TCP Authentication Unexpected: Rejected\n");
+ return -EINVAL;
+ } else {
+ net_info_ratelimited("TCP Authentication Unexpected: Accepted\n");
+ return 0;
+ }
+ }
+
+ /* bad inbound key len */
+ if (key->maclen + 4 != opt->len)
+ return -EINVAL;
+
+ err = __tcp_authopt_calc_mac(sk, skb, key, true, macbuf);
+ if (err)
+ return err;
+
+ if (memcmp(macbuf, opt->mac, key->maclen)) {
+ net_info_ratelimited("TCP Authentication Failed\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 3f7bd7ae7d7a..e0b51b2f747f 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -70,10 +70,11 @@
#include <linux/sysctl.h>
#include <linux/kernel.h>
#include <linux/prefetch.h>
#include <net/dst.h>
#include <net/tcp.h>
+#include <net/tcp_authopt.h>
#include <net/inet_common.h>
#include <linux/ipsec.h>
#include <asm/unaligned.h>
#include <linux/errqueue.h>
#include <trace/events/tcp.h>
@@ -5967,18 +5968,34 @@ void tcp_init_transfer(struct sock *sk, int bpf_op, struct sk_buff *skb)
if (!icsk->icsk_ca_initialized)
tcp_init_congestion_control(sk);
tcp_init_buffer_space(sk);
}
+static void tcp_authopt_finish_connect(struct sock *sk, struct sk_buff *skb)
+{
+#ifdef CONFIG_TCP_AUTHOPT
+ struct tcp_authopt_info *info;
+
+ info = rcu_dereference_protected(tcp_sk(sk)->authopt_info, lockdep_sock_is_held(sk));
+ if (!info)
+ return;
+
+ info->src_isn = ntohl(tcp_hdr(skb)->ack_seq) - 1;
+ info->dst_isn = ntohl(tcp_hdr(skb)->seq);
+#endif
+}
+
void tcp_finish_connect(struct sock *sk, struct sk_buff *skb)
{
struct tcp_sock *tp = tcp_sk(sk);
struct inet_connection_sock *icsk = inet_csk(sk);
tcp_set_state(sk, TCP_ESTABLISHED);
icsk->icsk_ack.lrcvtime = tcp_jiffies32;
+ tcp_authopt_finish_connect(sk, skb);
+
if (skb) {
icsk->icsk_af_ops->sk_rx_dst_set(sk, skb);
security_inet_conn_established(sk, skb);
sk_mark_napi_id(sk, skb);
}
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 1348615c7576..a1d39183908c 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2060,10 +2060,13 @@ int tcp_v4_rcv(struct sk_buff *skb)
goto discard_and_relse;
if (tcp_v4_inbound_md5_hash(sk, skb, dif, sdif))
goto discard_and_relse;
+ if (tcp_authopt_inbound_check(sk, skb))
+ goto discard_and_relse;
+
nf_reset_ct(skb);
if (tcp_filter(sk, skb))
goto discard_and_relse;
th = (const struct tcphdr *)skb->data;
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 0a4f3f16140a..4d7d86547b0e 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -24,10 +24,11 @@
#include <linux/slab.h>
#include <linux/sysctl.h>
#include <linux/workqueue.h>
#include <linux/static_key.h>
#include <net/tcp.h>
+#include <net/tcp_authopt.h>
#include <net/inet_common.h>
#include <net/xfrm.h>
#include <net/busy_poll.h>
static bool tcp_in_window(u32 seq, u32 end_seq, u32 s_win, u32 e_win)
@@ -539,10 +540,11 @@ struct sock *tcp_create_openreq_child(const struct sock *sk,
#ifdef CONFIG_TCP_MD5SIG
newtp->md5sig_info = NULL; /*XXX*/
if (newtp->af_specific->md5_lookup(sk, newsk))
newtp->tcp_header_len += TCPOLEN_MD5SIG_ALIGNED;
#endif
+ tcp_authopt_openreq(newsk, sk, req);
if (skb->len >= TCP_MSS_DEFAULT + newtp->tcp_header_len)
newicsk->icsk_ack.last_seg_size = skb->len - newtp->tcp_header_len;
newtp->rx_opt.mss_clamp = req->mss;
tcp_ecn_openreq_child(newtp, req);
newtp->fastopen_req = NULL;
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 29553fce8502..0e9ed6578809 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -37,10 +37,11 @@
#define pr_fmt(fmt) "TCP: " fmt
#include <net/tcp.h>
#include <net/mptcp.h>
+#include <net/tcp_authopt.h>
#include <linux/compiler.h>
#include <linux/gfp.h>
#include <linux/module.h>
#include <linux/static_key.h>
@@ -411,10 +412,11 @@ static inline bool tcp_urg_mode(const struct tcp_sock *tp)
#define OPTION_SACK_ADVERTISE (1 << 0)
#define OPTION_TS (1 << 1)
#define OPTION_MD5 (1 << 2)
#define OPTION_WSCALE (1 << 3)
+#define OPTION_AUTHOPT (1 << 4)
#define OPTION_FAST_OPEN_COOKIE (1 << 8)
#define OPTION_SMC (1 << 9)
#define OPTION_MPTCP (1 << 10)
static void smc_options_write(__be32 *ptr, u16 *options)
@@ -435,16 +437,21 @@ static void smc_options_write(__be32 *ptr, u16 *options)
struct tcp_out_options {
u16 options; /* bit field of OPTION_* */
u16 mss; /* 0 to disable */
u8 ws; /* window scale, 0 to disable */
u8 num_sack_blocks; /* number of SACK blocks to include */
- u8 hash_size; /* bytes in hash_location */
u8 bpf_opt_len; /* length of BPF hdr option */
+#ifdef CONFIG_TCP_AUTHOPT
+ u8 authopt_rnextkeyid; /* rnextkey */
+#endif
__u8 *hash_location; /* temporary pointer, overloaded */
__u32 tsval, tsecr; /* need to include OPTION_TS */
struct tcp_fastopen_cookie *fastopen_cookie; /* Fast open cookie */
struct mptcp_out_options mptcp;
+#ifdef CONFIG_TCP_AUTHOPT
+ struct tcp_authopt_key_info *authopt_key;
+#endif
};
static void mptcp_options_write(__be32 *ptr, const struct tcp_sock *tp,
struct tcp_out_options *opts)
{
@@ -617,10 +624,24 @@ static void tcp_options_write(__be32 *ptr, struct tcp_sock *tp,
/* overload cookie hash location */
opts->hash_location = (__u8 *)ptr;
ptr += 4;
}
+#ifdef CONFIG_TCP_AUTHOPT
+ if (unlikely(OPTION_AUTHOPT & options)) {
+ struct tcp_authopt_key_info *key = opts->authopt_key;
+
+ WARN_ON(!key);
+ *ptr++ = htonl((TCPOPT_AUTHOPT << 24) | ((4 + key->maclen) << 16) |
+ (key->send_id << 8) | opts->authopt_rnextkeyid);
+ /* overload cookie hash location */
+ opts->hash_location = (__u8 *)ptr;
+ /* maclen is currently always 12 but try to align nicely anyway. */
+ ptr += (key->maclen + 3) / 4;
+ }
+#endif
+
if (unlikely(opts->mss)) {
*ptr++ = htonl((TCPOPT_MSS << 24) |
(TCPOLEN_MSS << 16) |
opts->mss);
}
@@ -752,10 +773,28 @@ static void mptcp_set_option_cond(const struct request_sock *req,
}
}
}
}
+static int tcp_authopt_init_options(const struct sock *sk,
+ const struct sock *addr_sk,
+ struct tcp_out_options *opts)
+{
+#ifdef CONFIG_TCP_AUTHOPT
+ struct tcp_authopt_key_info *key;
+
+ key = tcp_authopt_select_key(sk, addr_sk, &opts->authopt_rnextkeyid);
+ if (key) {
+ opts->options |= OPTION_AUTHOPT;
+ opts->authopt_key = key;
+ return 4 + key->maclen;
+ }
+#endif
+
+ return 0;
+}
+
/* Compute TCP options for SYN packets. This is not the final
* network wire format yet.
*/
static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb,
struct tcp_out_options *opts,
@@ -774,10 +813,11 @@ static unsigned int tcp_syn_options(struct sock *sk, struct sk_buff *skb,
opts->options |= OPTION_MD5;
remaining -= TCPOLEN_MD5SIG_ALIGNED;
}
}
#endif
+ remaining -= tcp_authopt_init_options(sk, sk, opts);
/* We always get an MSS option. The option bytes which will be seen in
* normal data packets should timestamps be used, must be in the MSS
* advertised. But we subtract them from tp->mss_cache so that
* calculations in tcp_sendmsg are simpler etc. So account for this
@@ -862,10 +902,11 @@ static unsigned int tcp_synack_options(const struct sock *sk,
*/
if (synack_type != TCP_SYNACK_COOKIE)
ireq->tstamp_ok &= !ireq->sack_ok;
}
#endif
+ remaining -= tcp_authopt_init_options(sk, req_to_sk(req), opts);
/* We always send an MSS option. */
opts->mss = mss;
remaining -= TCPOLEN_MSS_ALIGNED;
@@ -930,10 +971,11 @@ static unsigned int tcp_established_options(struct sock *sk, struct sk_buff *skb
opts->options |= OPTION_MD5;
size += TCPOLEN_MD5SIG_ALIGNED;
}
}
#endif
+ size += tcp_authopt_init_options(sk, sk, opts);
if (likely(tp->rx_opt.tstamp_ok)) {
opts->options |= OPTION_TS;
opts->tsval = skb ? tcp_skb_timestamp(skb) + tp->tsoffset : 0;
opts->tsecr = tp->rx_opt.ts_recent;
@@ -1365,10 +1407,17 @@ static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb,
sk_nocaps_add(sk, NETIF_F_GSO_MASK);
tp->af_specific->calc_md5_hash(opts.hash_location,
md5, sk, skb);
}
#endif
+#ifdef CONFIG_TCP_AUTHOPT
+ if (opts.authopt_key) {
+ sk_nocaps_add(sk, NETIF_F_GSO_MASK);
+ err = tcp_authopt_hash(opts.hash_location, opts.authopt_key, sk, skb);
+ WARN_ON(err); // FIXME
+ }
+#endif
/* BPF prog is the last one writing header option */
bpf_skops_write_hdr_opt(sk, skb, NULL, NULL, 0, &opts);
INDIRECT_CALL_INET(icsk->icsk_af_ops->send_check,
@@ -3602,10 +3651,15 @@ struct sk_buff *tcp_make_synack(const struct sock *sk, struct dst_entry *dst,
if (md5)
tcp_rsk(req)->af_specific->calc_md5_hash(opts.hash_location,
md5, req_to_sk(req), skb);
rcu_read_unlock();
#endif
+#ifdef CONFIG_TCP_AUTHOPT
+ /* If signature fails we do nothing */
+ if (opts.authopt_key)
+ tcp_authopt_hash(opts.hash_location, opts.authopt_key, req_to_sk(req), skb);
+#endif
bpf_skops_write_hdr_opt((struct sock *)sk, skb, req, syn_skb,
synack_type, &opts);
skb->skb_mstamp_ns = now;
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 0ce52d46e4f8..51381a9c2bd5 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -40,10 +40,11 @@
#include <linux/icmpv6.h>
#include <linux/random.h>
#include <linux/indirect_call_wrapper.h>
#include <net/tcp.h>
+#include <net/tcp_authopt.h>
#include <net/ndisc.h>
#include <net/inet6_hashtables.h>
#include <net/inet6_connection_sock.h>
#include <net/ipv6.h>
#include <net/transp_v6.h>
@@ -1733,10 +1734,13 @@ INDIRECT_CALLABLE_SCOPE int tcp_v6_rcv(struct sk_buff *skb)
goto discard_and_relse;
if (tcp_v6_inbound_md5_hash(sk, skb, dif, sdif))
goto discard_and_relse;
+ if (tcp_authopt_inbound_check(sk, skb))
+ goto discard_and_relse;
+
if (tcp_filter(sk, skb))
goto discard_and_relse;
th = (const struct tcphdr *)skb->data;
hdr = ipv6_hdr(skb);
tcp_v6_fill_cb(skb, hdr, th);
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 6/9] tcp: authopt: Add key selection controls
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
` (4 preceding siblings ...)
2021-08-09 21:35 ` [RFCv2 5/9] tcp: authopt: Hook into tcp core Leonard Crestez
@ 2021-08-09 21:35 ` Leonard Crestez
2021-08-09 21:35 ` [RFCv2 7/9] tcp: authopt: Add snmp counters Leonard Crestez
` (2 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-09 21:35 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
The RFC requires that TCP can report the keyid and rnextkeyid values
being sent or received, implement this via getsockopt values.
The RFC also requires that user can select the sending key and that the
sending key is automatically switched based on rnextkeyid. These
requirements can conflict so we implement both and add a flag which
specifies if user or peer request takes priority.
Also add an option to control rnextkeyid explicitly from userspace.
Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
Documentation/networking/tcp_authopt.rst | 25 +++++++++++
include/net/tcp_authopt.h | 8 ++++
include/uapi/linux/tcp.h | 31 +++++++++++++
net/ipv4/tcp_authopt.c | 57 ++++++++++++++++++++++--
4 files changed, 117 insertions(+), 4 deletions(-)
diff --git a/Documentation/networking/tcp_authopt.rst b/Documentation/networking/tcp_authopt.rst
index 484f66f41ad5..cded87a70d05 100644
--- a/Documentation/networking/tcp_authopt.rst
+++ b/Documentation/networking/tcp_authopt.rst
@@ -35,10 +35,35 @@ Keys can be bound to remote addresses in a way that is similar to TCP_MD5.
RFC5925 requires that key ids do not overlap when tcp identifiers (addr/port)
overlap. This is not enforced by linux, configuring ambiguous keys will result
in packet drops and lost connections.
+Key selection
+-------------
+
+On getsockopt(TCP_AUTHOPT) information is provided about keyid/rnextkeyid in
+the last send packet and about the keyid/rnextkeyd in the last valid received
+packet.
+
+By default the sending keyid is selected to match the "rnextkeyid" value sent
+by the remote side. If that keyid is not available (or for new connections) a
+random matching key is selected.
+
+If the `TCP_AUTHOPT_LOCK_KEYID` is set then the sending key is selected by the
+`tcp_authopt.send_local_id` field and rnextkeyid is ignored. If no key with
+local_id == send_local_id is configured then a random matching key is
+selected.
+
+The current sending key is cached in the socket and will not change unless
+requested by remote rnextkeyid or by setsockopt.
+
+The rnextkeyid value sent on the wire is usually the recv_id of the current
+key used for sending. If the TCP_AUTHOPT_LOCK_RNEXTKEY flag is set in
+`tcp_authopt.flags` the value of `tcp_authopt.send_rnextkeyid` is send
+instead. This can be used to implement smooth rollover: the peer will switch
+its keyid to the received rnextkeyid when it is available.
+
ABI Reference
=============
.. kernel-doc:: include/uapi/linux/tcp.h
:identifiers: tcp_authopt tcp_authopt_flag tcp_authopt_key tcp_authopt_key_flag tcp_authopt_alg
diff --git a/include/net/tcp_authopt.h b/include/net/tcp_authopt.h
index 28ebc77473a4..759635346874 100644
--- a/include/net/tcp_authopt.h
+++ b/include/net/tcp_authopt.h
@@ -25,11 +25,19 @@ struct tcp_authopt_key_info {
/* Per-socket information regarding tcp_authopt */
struct tcp_authopt_info {
/* List of tcp_authopt_key_info */
struct hlist_head head;
+ /* Current send_key, cached.
+ * Once a key is found it only changes by user or remote request.
+ */
+ struct tcp_authopt_key_info *send_key;
u32 flags;
+ u32 local_send_id;
+ u8 send_rnextkeyid;
+ u8 recv_keyid;
+ u8 recv_rnextkeyid;
u32 src_isn;
u32 dst_isn;
struct rcu_head rcu;
};
diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index bc47664156eb..c04f5166ab33 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -346,10 +346,24 @@ struct tcp_diag_md5sig {
/**
* enum tcp_authopt_flag - flags for `tcp_authopt.flags`
*/
enum tcp_authopt_flag {
+ /**
+ * @TCP_AUTHOPT_FLAG_LOCK_KEYID: keyid controlled by sockopt
+ *
+ * If this is set `tcp_authopt.local_send_id` is used to determined sending
+ * key. Otherwise a key with send_id == recv_rnextkeyid is preferred.
+ */
+ TCP_AUTHOPT_FLAG_LOCK_KEYID = (1 << 0),
+ /**
+ * @TCP_AUTHOPT_FLAG_LOCK_RNEXTKEYID: Override rnextkeyid from userspace
+ *
+ * If this is set then `tcp_authopt.send_rnextkeyid` is sent on outbound
+ * packets. Other the recv_id of the current sending key is sent.
+ */
+ TCP_AUTHOPT_FLAG_LOCK_RNEXTKEYID = (1 << 1),
/**
* @TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED:
* Configure behavior of segments with TCP-AO coming from hosts for which no
* key is configured. The default recommended by RFC is to silently accept
* such connections.
@@ -361,10 +375,27 @@ enum tcp_authopt_flag {
* struct tcp_authopt - Per-socket options related to TCP Authentication Option
*/
struct tcp_authopt {
/** @flags: Combination of &enum tcp_authopt_flag */
__u32 flags;
+ /**
+ * @local_send_id: `tcp_authopt_key.local_id` of preferred send key
+ *
+ * This is only used if `TCP_AUTHOPT_FLAG_LOCK_KEYID` is set
+ */
+ __u32 local_send_id;
+ /**
+ * @send_rnextkeyid: The rnextkeyid to send in packets
+ *
+ * This is controlled by the user iff TCP_AUTHOPT_FLAG_LOCK_RNEXTKEYID is
+ * set. Otherwise rnextkeyid is the recv_id of the current key
+ */
+ __u8 send_rnextkeyid;
+ /** @recv_keyid: A recently-received keyid value. Only for getsockopt. */
+ __u8 recv_keyid;
+ /** @recv_rnextkeyid: A recently-received rnextkeyid value. Only for getsockopt. */
+ __u8 recv_rnextkeyid;
};
/**
* enum tcp_authopt_key_flag - flags for `tcp_authopt.flags`
*
diff --git a/net/ipv4/tcp_authopt.c b/net/ipv4/tcp_authopt.c
index 493461e46460..40412d9ea04e 100644
--- a/net/ipv4/tcp_authopt.c
+++ b/net/ipv4/tcp_authopt.c
@@ -215,17 +215,44 @@ struct tcp_authopt_key_info *tcp_authopt_lookup_send(struct tcp_authopt_info *in
*/
struct tcp_authopt_key_info *tcp_authopt_select_key(const struct sock *sk,
const struct sock *addr_sk,
u8 *rnextkeyid)
{
+ struct tcp_authopt_key_info *key, *new_key;
struct tcp_authopt_info *info;
info = rcu_dereference(tcp_sk(sk)->authopt_info);
if (!info)
return NULL;
- return tcp_authopt_lookup_send(info, addr_sk, -1);
+ key = info->send_key;
+ if (info->flags & TCP_AUTHOPT_FLAG_LOCK_KEYID) {
+ int local_send_id = info->local_send_id;
+
+ if (local_send_id && (!key || key->local_id != local_send_id))
+ new_key = __tcp_authopt_key_info_lookup(sk, info, local_send_id);
+ } else {
+ if (!key || key->send_id != info->recv_rnextkeyid)
+ new_key = tcp_authopt_lookup_send(info, addr_sk, info->recv_rnextkeyid);
+ }
+ if (!key && !new_key)
+ new_key = tcp_authopt_lookup_send(info, addr_sk, -1);
+
+ // Change current key.
+ if (key != new_key && new_key) {
+ key = new_key;
+ info->send_key = key;
+ }
+
+ if (key) {
+ if (info->flags & TCP_AUTHOPT_FLAG_LOCK_RNEXTKEYID)
+ *rnextkeyid = info->send_rnextkeyid;
+ else
+ *rnextkeyid = info->send_rnextkeyid = key->recv_id;
+ }
+
+ return key;
}
static struct tcp_authopt_info *__tcp_authopt_info_get_or_create(struct sock *sk)
{
struct tcp_sock *tp = tcp_sk(sk);
@@ -262,11 +289,17 @@ int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen)
info = __tcp_authopt_info_get_or_create(sk);
if (IS_ERR(info))
return PTR_ERR(info);
- info->flags = opt.flags & TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED);
+ info->flags = opt.flags &
+ (TCP_AUTHOPT_FLAG_LOCK_KEYID |
+ TCP_AUTHOPT_FLAG_LOCK_RNEXTKEYID |
+ TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED);
+ info->local_send_id = opt.local_send_id;
+ if (opt.flags & TCP_AUTHOPT_FLAG_LOCK_RNEXTKEYID)
+ info->send_rnextkeyid = opt.send_rnextkeyid;
return 0;
}
int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt)
@@ -277,11 +310,17 @@ int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt)
WARN_ON(!lockdep_sock_is_held(sk));
memset(opt, 0, sizeof(*opt));
info = rcu_dereference_check(tp->authopt_info, lockdep_sock_is_held(sk));
if (!info)
return -EINVAL;
- opt->flags = info->flags & TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED;
+ if (info->send_key)
+ opt->local_send_id = info->send_key->local_id;
+ else
+ opt->local_send_id = 0;
+ opt->send_rnextkeyid = info->send_rnextkeyid;
+ opt->recv_keyid = info->recv_keyid;
+ opt->recv_rnextkeyid = info->recv_rnextkeyid;
return 0;
}
static void tcp_authopt_key_free_rcu(struct rcu_head *rcu)
@@ -295,10 +334,12 @@ static void tcp_authopt_key_free_rcu(struct rcu_head *rcu)
static void tcp_authopt_key_del(struct sock *sk,
struct tcp_authopt_info *info,
struct tcp_authopt_key_info *key)
{
hlist_del_rcu(&key->node);
+ if (info->send_key == key)
+ info->send_key = NULL;
atomic_sub(sizeof(*key), &sk->sk_omem_alloc);
call_rcu(&key->rcu, tcp_authopt_key_free_rcu);
}
/* free info and keys but don't touch tp->authopt_info */
@@ -440,10 +481,11 @@ int __tcp_authopt_openreq(struct sock *newsk, const struct sock *oldsk, struct r
return -ENOMEM;
sk_nocaps_add(newsk, NETIF_F_GSO_MASK);
new_info->src_isn = tcp_rsk(req)->snt_isn;
new_info->dst_isn = tcp_rsk(req)->rcv_isn;
+ new_info->local_send_id = old_info->local_send_id;
INIT_HLIST_HEAD(&new_info->head);
err = tcp_authopt_clone_keys(newsk, oldsk, new_info, old_info);
if (err) {
__tcp_authopt_info_free(newsk, new_info);
return err;
@@ -1016,11 +1058,11 @@ int __tcp_authopt_inbound_check(struct sock *sk, struct sk_buff *skb, struct tcp
if (info->flags & TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED) {
net_info_ratelimited("TCP Authentication Unexpected: Rejected\n");
return -EINVAL;
} else {
net_info_ratelimited("TCP Authentication Unexpected: Accepted\n");
- return 0;
+ goto accept;
}
}
/* bad inbound key len */
if (key->maclen + 4 != opt->len)
@@ -1033,7 +1075,14 @@ int __tcp_authopt_inbound_check(struct sock *sk, struct sk_buff *skb, struct tcp
if (memcmp(macbuf, opt->mac, key->maclen)) {
net_info_ratelimited("TCP Authentication Failed\n");
return -EINVAL;
}
+accept:
+ /* Doing this for all valid packets will results in keyids temporarily
+ * flipping back and forth if packets are reordered or retransmitted.
+ */
+ info->recv_keyid = opt->keyid;
+ info->recv_rnextkeyid = opt->rnextkeyid;
+
return 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 7/9] tcp: authopt: Add snmp counters
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
` (5 preceding siblings ...)
2021-08-09 21:35 ` [RFCv2 6/9] tcp: authopt: Add key selection controls Leonard Crestez
@ 2021-08-09 21:35 ` Leonard Crestez
2021-08-09 21:35 ` [RFCv2 8/9] selftests: Initial TCP-AO support for nettest Leonard Crestez
2021-08-09 21:35 ` [RFCv2 9/9] selftests: Initial TCP-AO support for fcnal-test Leonard Crestez
8 siblings, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-09 21:35 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
Add LINUX_MIB_TCPAUTHOPTFAILURE and increment on failure. This can be
use by userspace to count the number of failed authentications.
All types of authentication failures are reported under a single
counter.
Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
include/uapi/linux/snmp.h | 1 +
net/ipv4/proc.c | 1 +
net/ipv4/tcp_authopt.c | 3 +++
3 files changed, 5 insertions(+)
diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
index 904909d020e2..1d96030889a1 100644
--- a/include/uapi/linux/snmp.h
+++ b/include/uapi/linux/snmp.h
@@ -290,10 +290,11 @@ enum
LINUX_MIB_TCPDUPLICATEDATAREHASH, /* TCPDuplicateDataRehash */
LINUX_MIB_TCPDSACKRECVSEGS, /* TCPDSACKRecvSegs */
LINUX_MIB_TCPDSACKIGNOREDDUBIOUS, /* TCPDSACKIgnoredDubious */
LINUX_MIB_TCPMIGRATEREQSUCCESS, /* TCPMigrateReqSuccess */
LINUX_MIB_TCPMIGRATEREQFAILURE, /* TCPMigrateReqFailure */
+ LINUX_MIB_TCPAUTHOPTFAILURE, /* TCPAuthOptFailure */
__LINUX_MIB_MAX
};
/* linux Xfrm mib definitions */
enum
diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
index b0d3a09dc84e..61dd06f8389c 100644
--- a/net/ipv4/proc.c
+++ b/net/ipv4/proc.c
@@ -295,10 +295,11 @@ static const struct snmp_mib snmp4_net_list[] = {
SNMP_MIB_ITEM("TcpDuplicateDataRehash", LINUX_MIB_TCPDUPLICATEDATAREHASH),
SNMP_MIB_ITEM("TCPDSACKRecvSegs", LINUX_MIB_TCPDSACKRECVSEGS),
SNMP_MIB_ITEM("TCPDSACKIgnoredDubious", LINUX_MIB_TCPDSACKIGNOREDDUBIOUS),
SNMP_MIB_ITEM("TCPMigrateReqSuccess", LINUX_MIB_TCPMIGRATEREQSUCCESS),
SNMP_MIB_ITEM("TCPMigrateReqFailure", LINUX_MIB_TCPMIGRATEREQFAILURE),
+ SNMP_MIB_ITEM("TCPAuthOptFailure", LINUX_MIB_TCPAUTHOPTFAILURE),
SNMP_MIB_SENTINEL
};
static void icmpmsg_put_line(struct seq_file *seq, unsigned long *vals,
unsigned short *type, int count)
diff --git a/net/ipv4/tcp_authopt.c b/net/ipv4/tcp_authopt.c
index 40412d9ea04e..bee8873423e4 100644
--- a/net/ipv4/tcp_authopt.c
+++ b/net/ipv4/tcp_authopt.c
@@ -1043,10 +1043,11 @@ int __tcp_authopt_inbound_check(struct sock *sk, struct sk_buff *skb, struct tcp
/* nothing found or expected */
if (!opt && !key)
return 0;
if (!opt && key) {
+ NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAUTHOPTFAILURE);
net_info_ratelimited("TCP Authentication Missing\n");
return -EINVAL;
}
if (opt && !key) {
/* RFC5925 Section 7.3:
@@ -1054,10 +1055,11 @@ int __tcp_authopt_inbound_check(struct sock *sk, struct sk_buff *skb, struct tcp
* of segments with TCP-AO but that do not match an MKT. The initial
* default of this configuration SHOULD be to silently accept such
* connections.
*/
if (info->flags & TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED) {
+ NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAUTHOPTFAILURE);
net_info_ratelimited("TCP Authentication Unexpected: Rejected\n");
return -EINVAL;
} else {
net_info_ratelimited("TCP Authentication Unexpected: Accepted\n");
goto accept;
@@ -1071,10 +1073,11 @@ int __tcp_authopt_inbound_check(struct sock *sk, struct sk_buff *skb, struct tcp
err = __tcp_authopt_calc_mac(sk, skb, key, true, macbuf);
if (err)
return err;
if (memcmp(macbuf, opt->mac, key->maclen)) {
+ NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPAUTHOPTFAILURE);
net_info_ratelimited("TCP Authentication Failed\n");
return -EINVAL;
}
accept:
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 8/9] selftests: Initial TCP-AO support for nettest
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
` (6 preceding siblings ...)
2021-08-09 21:35 ` [RFCv2 7/9] tcp: authopt: Add snmp counters Leonard Crestez
@ 2021-08-09 21:35 ` Leonard Crestez
2021-08-09 21:35 ` [RFCv2 9/9] selftests: Initial TCP-AO support for fcnal-test Leonard Crestez
8 siblings, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-09 21:35 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
tools/testing/selftests/net/nettest.c | 43 ++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/nettest.c b/tools/testing/selftests/net/nettest.c
index bd6288302094..48d8f3a6eb27 100644
--- a/tools/testing/selftests/net/nettest.c
+++ b/tools/testing/selftests/net/nettest.c
@@ -100,10 +100,12 @@ struct sock_args {
struct sockaddr_in v4;
struct sockaddr_in6 v6;
} md5_prefix;
unsigned int prefix_len;
+ const char *authopt_password;
+
/* expected addresses and device index for connection */
const char *expected_dev;
const char *expected_server_dev;
int expected_ifindex;
@@ -250,10 +252,36 @@ static int switch_ns(const char *ns)
close(fd);
return ret;
}
+static int tcp_set_authopt(int sd, struct sock_args *args)
+{
+ struct tcp_authopt info;
+ struct tcp_authopt_key key;
+ int rc;
+
+ memset(&info, 0, sizeof(info));
+ info.local_send_id = 1;
+
+ rc = setsockopt(sd, IPPROTO_TCP, TCP_AUTHOPT, &info, sizeof(info));
+ if (rc < 0)
+ log_err_errno("setsockopt(TCP_AUHTOPT_KEY)");
+
+ memset(&key, 0, sizeof(key));
+ strcpy((char *)key.key, args->authopt_password);
+ key.keylen = strlen(args->authopt_password);
+ key.alg = TCP_AUTHOPT_ALG_HMAC_SHA_1_96;
+ key.local_id = 1;
+
+ rc = setsockopt(sd, IPPROTO_TCP, TCP_AUTHOPT_KEY, &key, sizeof(key));
+ if (rc < 0)
+ log_err_errno("setsockopt(TCP_AUHTOPT_KEY)");
+
+ return rc;
+}
+
static int tcp_md5sig(int sd, void *addr, socklen_t alen, struct sock_args *args)
{
int keylen = strlen(args->password);
struct tcp_md5sig md5sig = {};
int opt = TCP_MD5SIG;
@@ -1508,10 +1536,15 @@ static int do_server(struct sock_args *args, int ipc_fd)
if (args->password && tcp_md5_remote(lsd, args)) {
close(lsd);
goto err_exit;
}
+ if (args->authopt_password && tcp_set_authopt(lsd, args)) {
+ close(lsd);
+ goto err_exit;
+ }
+
ipc_write(ipc_fd, 1);
while (1) {
log_msg("waiting for client connection.\n");
FD_ZERO(&rfds);
FD_SET(lsd, &rfds);
@@ -1630,10 +1663,13 @@ static int connectsock(void *addr, socklen_t alen, struct sock_args *args)
goto out;
if (args->password && tcp_md5sig(sd, addr, alen, args))
goto err;
+ if (args->authopt_password && tcp_set_authopt(sd, args))
+ goto err;
+
if (args->bind_test_only)
goto out;
if (connect(sd, addr, alen) < 0) {
if (errno != EINPROGRESS) {
@@ -1819,11 +1855,11 @@ static int ipc_parent(int cpid, int fd, struct sock_args *args)
wait(&status);
return client_status;
}
-#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 GETOPT_STR "sr:l:c:p:t:g:P:DRn:M:X:m:A:d:I:BN:O:SCi6xL:0:1:2:3:Fbq"
static void print_usage(char *prog)
{
printf(
"usage: %s OPTS\n"
@@ -1856,10 +1892,12 @@ static void print_usage(char *prog)
" -n num number of times to send message\n"
"\n"
" -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"
+ " -A password use RFC5925 TCP Authentication option\n"
+ "\n"
" -g grp multicast group (e.g., 239.1.1.1)\n"
" -i interactive mode (default is echo and terminate)\n"
"\n"
" -0 addr Expected local address\n"
" -1 addr Expected remote address\n"
@@ -1970,10 +2008,13 @@ int main(int argc, char *argv[])
args.client_pw = optarg;
break;
case 'm':
args.md5_prefix_str = optarg;
break;
+ case 'A':
+ args.authopt_password = optarg;
+ break;
case 'S':
args.use_setsockopt = 1;
break;
case 'C':
args.use_cmsg = 1;
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RFCv2 9/9] selftests: Initial TCP-AO support for fcnal-test
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
` (7 preceding siblings ...)
2021-08-09 21:35 ` [RFCv2 8/9] selftests: Initial TCP-AO support for nettest Leonard Crestez
@ 2021-08-09 21:35 ` Leonard Crestez
2021-08-11 13:46 ` David Ahern
8 siblings, 1 reply; 24+ messages in thread
From: Leonard Crestez @ 2021-08-09 21:35 UTC (permalink / raw)
To: Eric Dumazet, David S. Miller, Herbert Xu, Kuniyuki Iwashima,
David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
Just test that a correct password is required.
Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
---
tools/testing/selftests/net/fcnal-test.sh | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh
index a8ad92850e63..569c340040f4 100755
--- a/tools/testing/selftests/net/fcnal-test.sh
+++ b/tools/testing/selftests/net/fcnal-test.sh
@@ -788,10 +788,31 @@ ipv4_ping()
}
################################################################################
# IPv4 TCP
+#
+# TCP Authentication Option Tests
+#
+ipv4_tcp_authopt()
+{
+ # basic use case
+ log_start
+ run_cmd nettest -s -A ${MD5_PW} &
+ sleep 1
+ run_cmd_nsb nettest -r ${NSA_IP} -A ${MD5_PW}
+ log_test $? 0 "AO: Simple password"
+
+ # wrong password
+ log_start
+ show_hint "Should timeout since client uses wrong password"
+ run_cmd nettest -s -A ${MD5_PW} &
+ sleep 1
+ run_cmd_nsb nettest -r ${NSA_IP} -A ${MD5_WRONG_PW}
+ log_test $? 2 "AO: Client uses wrong password"
+}
+
#
# MD5 tests without VRF
#
ipv4_tcp_md5_novrf()
{
@@ -1119,10 +1140,11 @@ ipv4_tcp_novrf()
show_hint "Should fail 'Connection refused'"
run_cmd nettest -d ${NSA_DEV} -r ${a}
log_test_addr ${a} $? 1 "No server, device client, local conn"
ipv4_tcp_md5_novrf
+ ipv4_tcp_authopt
}
ipv4_tcp_vrf()
{
local a
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [RFCv2 9/9] selftests: Initial TCP-AO support for fcnal-test
2021-08-09 21:35 ` [RFCv2 9/9] selftests: Initial TCP-AO support for fcnal-test Leonard Crestez
@ 2021-08-11 13:46 ` David Ahern
2021-08-11 19:09 ` Leonard Crestez
0 siblings, 1 reply; 24+ messages in thread
From: David Ahern @ 2021-08-11 13:46 UTC (permalink / raw)
To: Leonard Crestez, Eric Dumazet, David S. Miller, Herbert Xu,
Kuniyuki Iwashima, David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
On 8/9/21 3:35 PM, Leonard Crestez wrote:
> Just test that a correct password is required.
>
This test suite needs to be comprehensive that the UAPI works as
designed and fails when it should - cleanly and with an extack message
as to why some config option fails. Tests should cover the datapath -
that it works properly when it should and fails cleanly when it should
not. If addresses are involved in the configuration, then the tests need
to be written for non VRFs, with VRFs and default VRF since addresses
are relative.
Also, in tree test suites are best for the maintenance of this code
going forward.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFCv2 9/9] selftests: Initial TCP-AO support for fcnal-test
2021-08-11 13:46 ` David Ahern
@ 2021-08-11 19:09 ` Leonard Crestez
0 siblings, 0 replies; 24+ messages in thread
From: Leonard Crestez @ 2021-08-11 19:09 UTC (permalink / raw)
To: David Ahern, Leonard Crestez, Eric Dumazet, David S. Miller,
Herbert Xu, Kuniyuki Iwashima, David Ahern
Cc: Hideaki YOSHIFUJI, Jakub Kicinski, Yuchung Cheng,
Francesco Ruggeri, Mat Martineau, Christoph Paasch,
Ivan Delalande, Priyaranjan Jha, Menglong Dong, linux-kernel,
linux-crypto, netdev
On 11.08.2021 16:46, David Ahern wrote:
> On 8/9/21 3:35 PM, Leonard Crestez wrote:
>> Just test that a correct password is required.
>>
>
> This test suite needs to be comprehensive that the UAPI works as
> designed and fails when it should - cleanly and with an extack message
> as to why some config option fails. Tests should cover the datapath -
> that it works properly when it should and fails cleanly when it should
> not. If addresses are involved in the configuration, then the tests need
> to be written for non VRFs, with VRFs and default VRF since addresses
> are relative.
>
> Also, in tree test suites are best for the maintenance of this code
> going forward.
I can try to integrate my python test suite into kselftest. It's not a
very orthodox choice but a rewrite in C would be much larger.
--
Regards,
Leonard
^ permalink raw reply [flat|nested] 24+ messages in thread