LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Dmitry Safonov <0x7f454c46@gmail.com>
To: Leonard Crestez <cdleonard@gmail.com>,
	David Ahern <dsahern@kernel.org>, Shuah Khan <shuah@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Kuniyuki Iwashima <kuniyu@amazon.co.jp>,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	Jakub Kicinski <kuba@kernel.org>,
	Yuchung Cheng <ycheng@google.com>,
	Francesco Ruggeri <fruggeri@arista.com>,
	Mat Martineau <mathew.j.martineau@linux.intel.com>,
	Christoph Paasch <cpaasch@apple.com>,
	Ivan Delalande <colona@arista.com>,
	Priyaranjan Jha <priyarjha@google.com>,
	Menglong Dong <dong.menglong@zte.com.cn>,
	netdev@vger.kernel.org, linux-crypto@vger.kernel.org,
	linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFCv3 01/15] tcp: authopt: Initial support and key management
Date: Tue, 31 Aug 2021 20:04:20 +0100	[thread overview]
Message-ID: <b93d21fa-13aa-20d9-0747-c443ccf2c5d5@gmail.com> (raw)
In-Reply-To: <090898ac6f3345ec02999858c65c2ebb8cd274a8.1629840814.git.cdleonard@gmail.com>

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

  reply	other threads:[~2021-08-31 19:04 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 21:34 [RFCv3 00/15] tcp: Initial support for RFC5925 auth option Leonard Crestez
2021-08-24 21:34 ` [RFCv3 01/15] tcp: authopt: Initial support and key management Leonard Crestez
2021-08-31 19:04   ` Dmitry Safonov [this message]
2021-09-03 14:26     ` Leonard Crestez
2021-08-24 21:34 ` [RFCv3 02/15] docs: Add user documentation for tcp_authopt Leonard Crestez
2021-08-24 21:34 ` [RFCv3 03/15] selftests: Initial tcp_authopt test module Leonard Crestez
2021-08-24 21:34 ` [RFCv3 04/15] selftests: tcp_authopt: Initial sockopt manipulation Leonard Crestez
2021-08-24 21:34 ` [RFCv3 05/15] tcp: authopt: Add crypto initialization Leonard Crestez
2021-08-24 23:02   ` Eric Dumazet
2021-08-24 23:34   ` Eric Dumazet
2021-08-25  8:08     ` Herbert Xu
2021-08-25 14:55       ` Eric Dumazet
2021-08-25 16:04       ` Ard Biesheuvel
2021-08-25 16:31         ` Leonard Crestez
2021-08-25 16:35     ` Leonard Crestez
2021-08-25 17:55       ` Eric Dumazet
2021-08-25 18:56         ` Leonard Crestez
2021-08-24 21:34 ` [RFCv3 06/15] tcp: authopt: Compute packet signatures Leonard Crestez
2021-08-24 21:34 ` [RFCv3 07/15] tcp: authopt: Hook into tcp core Leonard Crestez
2021-08-24 22:59   ` Eric Dumazet
2021-08-25 16:32     ` Leonard Crestez
2021-08-24 21:34 ` [RFCv3 08/15] tcp: authopt: Add snmp counters Leonard Crestez
2021-08-24 21:34 ` [RFCv3 09/15] selftests: tcp_authopt: Test key address binding Leonard Crestez
2021-08-25  5:18   ` David Ahern
2021-08-25 16:37     ` Leonard Crestez
2021-08-24 21:34 ` [RFCv3 10/15] selftests: tcp_authopt: Capture and verify packets Leonard Crestez
2021-08-24 21:34 ` [RFCv3 11/15] selftests: Initial tcp_authopt support for nettest Leonard Crestez
2021-08-24 21:34 ` [RFCv3 12/15] selftests: Initial tcp_authopt support for fcnal-test Leonard Crestez
2021-08-24 21:34 ` [RFCv3 13/15] selftests: Add -t tcp_authopt option for fcnal-test.sh Leonard Crestez
2021-08-24 21:34 ` [RFCv3 14/15] tcp: authopt: Add key selection controls Leonard Crestez
2021-08-24 21:34 ` [RFCv3 15/15] selftests: tcp_authopt: Add tests for rollover Leonard Crestez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b93d21fa-13aa-20d9-0747-c443ccf2c5d5@gmail.com \
    --to=0x7f454c46@gmail.com \
    --cc=cdleonard@gmail.com \
    --cc=colona@arista.com \
    --cc=cpaasch@apple.com \
    --cc=davem@davemloft.net \
    --cc=dong.menglong@zte.com.cn \
    --cc=dsahern@kernel.org \
    --cc=edumazet@google.com \
    --cc=fruggeri@arista.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=kuba@kernel.org \
    --cc=kuniyu@amazon.co.jp \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mathew.j.martineau@linux.intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=priyarjha@google.com \
    --cc=shuah@kernel.org \
    --cc=ycheng@google.com \
    --cc=yoshfuji@linux-ipv6.org \
    --subject='Re: [RFCv3 01/15] tcp: authopt: Initial support and key management' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).