Date:   Tue, 31 Aug 2021 20:04:20 +0100
From:   Dmitry Safonov <0x7f454c46@...il.com>
To:     Leonard Crestez <cdleonard@...il.com>,
        David Ahern <dsahern@...nel.org>, Shuah Khan <shuah@...nel.org>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Yuchung Cheng <ycheng@...gle.com>,
        Francesco Ruggeri <fruggeri@...sta.com>,
        Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        Christoph Paasch <cpaasch@...le.com>,
        Ivan Delalande <colona@...sta.com>,
        Priyaranjan Jha <priyarjha@...gle.com>,
        Menglong Dong <dong.menglong@....com.cn>,
        netdev@...r.kernel.org, linux-crypto@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFCv3 01/15] tcp: authopt: Initial support and key management

Hi Leonard,

On 8/24/21 10:34 PM, Leonard Crestez wrote:
[..]
> --- /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.

[..]
> +
> +/**
> + * 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).

[..]
> +#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.

[..]
> +#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;
:	}

> +	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;
> +}

[..]
> +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

> +	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.

> +		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

Powered by blists - more mailing lists