Re: [RFCv3 07/15] tcp: authopt: Hook into tcp core

[Leonard Crestez <cdleonard@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.