LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Leonard Crestez <cdleonard@gmail.com>
To: Eric Dumazet <eric.dumazet@gmail.com>,
	Dmitry Safonov <0x7f454c46@gmail.com>,
	David Ahern <dsahern@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,
	Shuah Khan <shuah@kernel.org>
Subject: Re: [RFCv3 07/15] tcp: authopt: Hook into tcp core
Date: Wed, 25 Aug 2021 19:32:44 +0300	[thread overview]
Message-ID: <07c44a66-1db3-1136-8894-731dafb0d2d7@gmail.com> (raw)
In-Reply-To: <3fc7b060-0ed9-eb73-92c0-0765fe4cb414@gmail.com>

On 25.08.2021 01:59, Eric Dumazet wrote:
> On 8/24/21 2:34 PM, Leonard Crestez wrote:
>> The tcp_authopt features exposes a minimal interface to the rest of the
>> TCP stack. Only a few functions are exposed and if the feature is
>> disabled they return neutral values, avoiding ifdefs in the rest of the
>> code.
>>
>> Add calls into tcp authopt from send, receive and accept code.
>>
>> Signed-off-by: Leonard Crestez <cdleonard@gmail.com>
>> ---
>>   include/net/tcp_authopt.h |  56 +++++++++
>>   net/ipv4/tcp_authopt.c    | 246 ++++++++++++++++++++++++++++++++++++++
>>   net/ipv4/tcp_input.c      |  17 +++
>>   net/ipv4/tcp_ipv4.c       |   3 +
>>   net/ipv4/tcp_minisocks.c  |   2 +
>>   net/ipv4/tcp_output.c     |  74 +++++++++++-
>>   net/ipv6/tcp_ipv6.c       |   4 +
>>   7 files changed, 401 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/tcp_authopt.h b/include/net/tcp_authopt.h
>> index c9ee2059b442..61db268f36f8 100644
>> --- a/include/net/tcp_authopt.h
>> +++ b/include/net/tcp_authopt.h
>> @@ -21,10 +21,11 @@ struct tcp_authopt_key_info {
>>   	/* Wire identifiers */
>>   	u8 send_id, recv_id;
>>   	u8 alg_id;
>>   	u8 keylen;
>>   	u8 key[TCP_AUTHOPT_MAXKEYLEN];
>> +	u8 maclen;
> 
> I do not see maclen being enforced to 12, or a multiple of 4 ?

For both current algorithms the maclen value is 12. I just implemented 
RFC5926, there is no way to control this from userspace.

> This means that later [2], tcp_authopt_hash() will leave up to 3
> unitialized bytes in the TCP options, sent to the wire.
> 
> This is a  security issue, since we will leak kernel memory.

Filling the remainder with zeroes does make sense, or at least 
WARN_ON(maclen != 4) so that it's obvious to anyone who attempts to 
extend the algorithms.

>> +struct tcp_authopt_key_info *tcp_authopt_lookup_send(struct tcp_authopt_info *info,
>> +						     const struct sock *addr_sk,
>> +						     int send_id)
>> +{
>> +	struct tcp_authopt_key_info *result = NULL;
>> +	struct tcp_authopt_key_info *key;
>> +
>> +	hlist_for_each_entry_rcu(key, &info->head, node, 0) {
>> +		if (send_id >= 0 && key->send_id != send_id)
>> +			continue;
>> +		if (key->flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
>> +			if (addr_sk->sk_family == AF_INET) {
>> +				struct sockaddr_in *key_addr = (struct sockaddr_in *)&key->addr;
>> +				const struct in_addr *daddr =
>> +					(const struct in_addr *)&addr_sk->sk_daddr;
> 
> Why a cast is needed ? sk_daddr is a __be32, no need to cast it to in_addr
>> +
>> +				if (WARN_ON(key_addr->sin_family != AF_INET))
> 
> Why a WARN_ON() is used ? If we expect this to trigger, then at minimumum WARN_ON_ONCE() please.
> 
>> +					continue;
>> +				if (memcmp(daddr, &key_addr->sin_addr, sizeof(*daddr)))
>> +					continue;
> 
> Using memcmp() to compare two __be32 is overkill.
> 
>> +			}
>> +			if (addr_sk->sk_family == AF_INET6) {
>> +				struct sockaddr_in6 *key_addr = (struct sockaddr_in6 *)&key->addr;
>> +				const struct in6_addr *daddr = &addr_sk->sk_v6_daddr;
> 
> Not sure why a variable is used, you need it once.
> 
>> +
>> +				if (WARN_ON(key_addr->sin6_family != AF_INET6))
>> +					continue;
>> +				if (memcmp(daddr, &key_addr->sin6_addr, sizeof(*daddr)))
> 
> ipv6_addr_equal() should be faster.

OK, I will replace the comparisons.

Checking address family is mostly paranoia on my part, I don't know if a 
real scenario exists for AF mismatch. Still need to check ipv4-mapped 
ipv6 addresses, not sure if those can receive ipv4 skbs on an ipv6 socket.

>> +struct tcp_authopt_key_info *tcp_authopt_select_key(const struct sock *sk,
>> +						    const struct sock *addr_sk,
>> +						    u8 *rnextkeyid)
>> +{
>> +	struct tcp_authopt_info *info;
>> +
>> +	info = rcu_dereference(tcp_sk(sk)->authopt_info);
> 
> distro kernels will have CONFIG_TCP_AUTHOPT set, meaning
> that we will add a cache line miss for every incoming TCP packet
> even on hosts not using any RFC5925 TCP flow.
> 
> For TCP MD5 we are using a static key, to avoid this extra cost.

OK, will add a static_key.

The check for "does socket have tcp_authopt" also belongs in an inline 
wrapper, similar to inbound check

>> +int __tcp_authopt_openreq(struct sock *newsk, const struct sock *oldsk, struct request_sock *req)
>> +{
>> +	struct tcp_authopt_info *old_info;
>> +	struct tcp_authopt_info *new_info;
>> +	int err;
>> +
>> +	old_info = rcu_dereference(tcp_sk(oldsk)->authopt_info);
>> +	if (!old_info)
>> +		return 0;
>> +
>> +	new_info = kmalloc(sizeof(*new_info), GFP_ATOMIC | __GFP_ZERO);
> 
> kzalloc() is your friend. (same remark for your other patches, where you are using __GFP_ZERO)
> Also see additional comment [1]

OK
> 
>> +	if (!new_info)
>> +		return -ENOMEM;
>> +
>> +	sk_nocaps_add(newsk, NETIF_F_GSO_MASK);
>> +	new_info->src_isn = tcp_rsk(req)->snt_isn;
>> +	new_info->dst_isn = tcp_rsk(req)->rcv_isn;
>> +	INIT_HLIST_HEAD(&new_info->head);
>> +	err = tcp_authopt_clone_keys(newsk, oldsk, new_info, old_info);
>> +	if (err) {
>> +		__tcp_authopt_info_free(newsk, new_info);
> 
> 		Are we leaving in place old value of newsk->authopt_info ?
> 		If this is copied from the listener, I think you need
> 		to add a tcp_sk(newsk)->authopt_info = NULL;
> 		before the kzalloc() call done above.

Yes, authopt_info should be set to NULL on error because keeping the 
listen socket's value is wrong and dangerous (double free).

Leaving authopt_info NULL or malloc failure is still possible dangerous 
because it means all keys are ignored and accepted. Not clear how we 
could cause tcp_create_openreq_child to fail instead.

This is a problem in a few other parts: if cryptography fails the 
outbound MAC is filled with zeros because there's not obvious way to 
make TX fail at that point.

>> +	err = __tcp_authopt_calc_mac(sk, skb, key, false, macbuf);
>> +	if (err) {
>> +		/* If mac calculation fails and caller doesn't handle the error
>> +		 * try to make it obvious inside the packet.
>> +		 */
>> +		memset(hash_location, 0, key->maclen);
>> +		return err;
>> +	}
>> +	memcpy(hash_location, macbuf, key->maclen);
> 
> 
> [2]
> This is the place were we do not make sure to clear the padding bytes
> (if key->maclen is not a multiple of 4)

Yes. It might make sense to fix in caller because it's the caller which 
decides to align options.

  reply	other threads:[~2021-08-25 16:32 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
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 [this message]
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=07c44a66-1db3-1136-8894-731dafb0d2d7@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=eric.dumazet@gmail.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 07/15] tcp: authopt: Hook into tcp core' \
    /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).