| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 Aug 2021 19:32:44 +0300
From: Leonard Crestez <cdleonard@...il.com>
To: Eric Dumazet <eric.dumazet@...il.com>,
Dmitry Safonov <0x7f454c46@...il.com>,
David Ahern <dsahern@...nel.org>
Cc: Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>,
Herbert Xu <herbert@...dor.apana.org.au>,
Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
Jakub Kicinski <kuba@...nel.org>,
Yuchung Cheng <ycheng@...gle.com>,
Francesco Ruggeri <fruggeri@...sta.com>,
Mat Martineau <mathew.j.martineau@...ux.intel.com>,
Christoph Paasch <cpaasch@...le.com>,
Ivan Delalande <colona@...sta.com>,
Priyaranjan Jha <priyarjha@...gle.com>,
Menglong Dong <dong.menglong@....com.cn>,
netdev@...r.kernel.org, linux-crypto@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
Shuah Khan <shuah@...nel.org>
Subject: Re: [RFCv3 07/15] tcp: authopt: Hook into tcp core
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@...il.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.
Powered by blists - more mailing lists