LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Leonard Crestez <cdleonard@gmail.com>
To: Dmitry Safonov <0x7f454c46@gmail.com>,
	David Ahern <dsahern@kernel.org>,
	Eric Dumazet <edumazet@google.com>
Cc: "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,
	Shuah Khan <shuah@kernel.org>
Subject: Re: [RFCv3 01/15] tcp: authopt: Initial support and key management
Date: Fri, 3 Sep 2021 17:26:47 +0300	[thread overview]
Message-ID: <5c7dbf82-b176-dd95-4fd4-dd1ee69d962d@gmail.com> (raw)
In-Reply-To: <b93d21fa-13aa-20d9-0747-c443ccf2c5d5@gmail.com>

On 31.08.2021 22:04, Dmitry Safonov wrote:
> Hi Leonard,
> On 8/24/21 10:34 PM, Leonard Crestez wrote:
>> +/**
>> + * 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.

Yes

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

Yes. The identifying fields are documented on struct tcp_authopt_key so 
I will abbreviate this.

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

Same send_id/recv_id can overlap between different remote peers.

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

tcp_get_authopt_val clears *info before all checks so this will return 
zeros to userspace.

I do need to propagate the return value from tcp_get_authopt_val.

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

If (optlen > sizeof(opt)) means userspace is attempting to use newer 
ABI. Current behavior is to return an error which seems very reasonable.

You seem to be suggesting that we check that the rest of option is 
zeroes and if it is to continue. That seems potentially dangerous but it 
could work if we forever ensure that zeroes always mean "no effect".

This would make it easier for new apps to run on old kernels: unless 
they specifically use new features they don't need to do anything.

Also, setsockopt can't report a new length back and there's no 
getsockopt for keys.

>> +	memset(&opt, 0, sizeof(opt));
>> +	if (copy_from_sockptr(&opt, optval, optlen))
>> +		return -EFAULT;
>> +
>> +	if (opt.flags & ~TCP_AUTHOPT_KNOWN_FLAGS)
>> +		return -EINVAL;

Here if the user requests unrecognized flags an error is reported. My 
intention is that new fields will be accompanied by new flags.

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

tcp_authopt_key_del does kfree_rcu. As far as I understand this means 
that if select_key can see the key it is guaranteed to live until the 
next grace period, which shouldn't be until after the packet is signed.

I will attempt to document this restriction on tcp_authopt_select_key: 
you can't do anything with the key except give it to tcp_authopt_hash 
before an RCU grace period.

I'm not confident this is correct in all cases. It's inspired by what 
MD5 does but apparently those key lists are protected by a combination 
of sk_lock and rcu?

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

Moving the sock_kmalloc higher should fix this, there would be no effect 
on alloc failure.

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

Userspace breaking its own connections seems fine, it can already do 
this in many ways.

If the current key is removed the kernel will just switch to another 
valid key. If no valid keys exist then I expect it will switch to 
unsigned packets which is possibly quite dangerous.

Maybe it should be possible to insert a "marker" key which just says 
"don't do any unsigned traffic with this peer"?

--
Regards,
Leonard

  reply	other threads:[~2021-09-03 14:26 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
2021-09-03 14:26     ` Leonard Crestez [this message]
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=5c7dbf82-b176-dd95-4fd4-dd1ee69d962d@gmail.com \
    --to=cdleonard@gmail.com \
    --cc=0x7f454c46@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).