Thread (31 messages) 31 messages, 7 authors, 2021-09-03

Re: [RFCv3 01/15] tcp: authopt: Initial support and key management

flat view

From: Dmitry Safonov <hidden>
Date: 2021-08-31 19:04:30
Also in: linux-crypto, linux-kselftest, lkml

Hi Leonard,

On 8/24/21 10:34 PM, Leonard Crestez wrote:
[..]
quoted hunk ↗ jump to hunk
--- /dev/null
+++ b/include/net/tcp_authopt.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _LINUX_TCP_AUTHOPT_H
+#define _LINUX_TCP_AUTHOPT_H
+
+#include <uapi/linux/tcp.h>
+
+/**
+ * struct tcp_authopt_key_info - Representation of a Master Key Tuple as per RFC5925
+ *
+ * Key structure lifetime is only protected by RCU so readers needs to hold a
+ * single rcu_read_lock until they're done with the key.
+ */
+struct tcp_authopt_key_info {
+	struct hlist_node node;
+	struct rcu_head rcu;
+	/* Local identifier */
+	u32 local_id;
It's unused now, can be removed.

[..]
quoted hunk ↗ jump to hunk
+
+/**
+ * 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.
                                              ^
By send_id and recv_id.
Also, tcp_authopt_key_match_exact() seems to check
TCP_AUTHOPT_KEY_ADDR_BIND. I wounder if that makes sense to relax it in
case of TCP_AUTHOPT_KEY_DEL to match only send_id/recv_id if addr isn't
specified (no hard feelings about it, though).

[..]
quoted hunk ↗ jump to hunk
+#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;
Failed tcp_get_authopt_val() lookup in:
:       if (!info)
:               return -EINVAL;

will leak uninitialized kernel memory from stack.
ASLR guys defeated.

[..]
quoted hunk ↗ jump to hunk
+#define TCP_AUTHOPT_KNOWN_FLAGS ( \
+	TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED)
+
+int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen)
+{
+	struct tcp_authopt opt;
+	struct tcp_authopt_info *info;
+
+	sock_owned_by_me(sk);
+
+	/* If userspace optlen is too short fill the rest with zeros */
+	if (optlen > sizeof(opt))
+		return -EINVAL;
More like
:	if (unlikely(len > sizeof(opt))) {
:		err = check_zeroed_user(optval + sizeof(opt),
:					len - sizeof(opt));
:		if (err < 1)
:			return err == 0 ? -EINVAL : err;
:		len = sizeof(opt);
:		if (put_user(len, optlen))
:			return -EFAULT;
:	}
quoted hunk ↗ jump to hunk
+	memset(&opt, 0, sizeof(opt));
+	if (copy_from_sockptr(&opt, optval, optlen))
+		return -EFAULT;
+
+	if (opt.flags & ~TCP_AUTHOPT_KNOWN_FLAGS)
+		return -EINVAL;
+
+	info = __tcp_authopt_info_get_or_create(sk);
+	if (IS_ERR(info))
+		return PTR_ERR(info);
+
+	info->flags = opt.flags & TCP_AUTHOPT_KNOWN_FLAGS;
+
+	return 0;
+}
[..]
quoted hunk ↗ jump to hunk
+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;
+
+	sock_owned_by_me(sk);
+
+	/* If userspace optlen is too short fill the rest with zeros */
+	if (optlen > sizeof(opt))
+		return -EINVAL;
Ditto
quoted hunk ↗ jump to hunk
+	memset(&opt, 0, sizeof(opt));
+	if (copy_from_sockptr(&opt, optval, optlen))
+		return -EFAULT;
+
+	if (opt.flags & ~TCP_AUTHOPT_KEY_KNOWN_FLAGS)
+		return -EINVAL;
+
+	if (opt.keylen > TCP_AUTHOPT_MAXKEYLEN)
+		return -EINVAL;
+
+	/* Delete is a special case: */
+	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_lookup_exact(sk, info, &opt);
+		if (!key_info)
+			return -ENOENT;
+		tcp_authopt_key_del(sk, info, key_info);
Doesn't seem to be safe together with tcp_authopt_select_key().
A key can be in use at this moment - you have to add checks for it.
quoted hunk ↗ jump to hunk
+		return 0;
+	}
+
+	/* check key family */
+	if (opt.flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
+		if (sk->sk_family != opt.addr.ss_family)
+			return -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);
+
+	/* If an old key exists with exact ID then remove and replace.
+	 * RCU-protected readers might observe both and pick any.
+	 */
+	key_info = tcp_authopt_key_lookup_exact(sk, info, &opt);
+	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;
So, you may end up without any key.
Also, replacing a key is not at all safe: you may receive old segments
which you in turn will discard and reset the connection.

I think the limitation RFC puts on removing keys in use and replacing
existing keys are actually reasonable. Probably, it'd be better to
enforce "key in use => desired key is different (or key_outdated flag)
=> key not in use => key may be removed" life-cycle of MKT.

Thanks,
            Dmitry
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help