lkml.org 
[lkml]   [2021]   [Aug]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [RFCv3 07/15] tcp: authopt: Hook into tcp core
Date
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.

\
 
 \ /
  Last update: 2021-08-25 18:34    [W:0.056 / U:0.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site