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