netdev - Re: [PATCH] tcp: md5: Fix overlap between vrf and non-vrf keys

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 9 Oct 2021 11:19:22 -0600
From:   David Ahern <dsahern@...il.com>
To:     Leonard Crestez <cdleonard@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        David Ahern <dsahern@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
        Yonghong Song <yhs@...com>,
        Alexander Duyck <alexanderduyck@...com>,
        Florian Westphal <fw@...len.de>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tcp: md5: Fix overlap between vrf and non-vrf keys

On 10/8/21 9:51 AM, Leonard Crestez wrote:
> On 07.10.2021 21:27, David Ahern wrote:
>> On 10/7/21 12:41 AM, Leonard Crestez wrote:
>>> On 07.10.2021 04:14, David Ahern wrote:
>>>> On 10/6/21 11:48 AM, Leonard Crestez wrote:
>>>>> @@ -1103,11 +1116,11 @@ static struct tcp_md5sig_key
>>>>> *tcp_md5_do_lookup_exact(const struct sock *sk,
>>>>>    #endif
>>>>>        hlist_for_each_entry_rcu(key, &md5sig->head, node,
>>>>>                     lockdep_sock_is_held(sk)) {
>>>>>            if (key->family != family)
>>>>>                continue;
>>>>> -        if (key->l3index && key->l3index != l3index)
>>>>> +        if (key->l3index != l3index)
>>>>
>>>> That seems like the bug fix there. The L3 reference needs to match for
>>>> new key and existing key. I think the same change is needed in
>>>> __tcp_md5_do_lookup.
>>>
>>> Current behavior is that keys added without tcpm_ifindex will match
>>> connections both inside and outside VRFs. Changing this might break real
>>> applications, is it really OK to claim that this behavior was a bug all
>>> along?
>>
>> no.
>>
>> It's been a few years. I need to refresh on the logic and that is not
>> going to happen before this weekend.
> 
> It seems that always doing a strict key->l3index != l3index condition
> inside of __tcp_md5_do_lookup breaks the usecase of binding one listener
> to each VRF and not specifying the ifindex for each key.
> 
> This is a very valid usecase, maybe the most common way to use md5 with
> vrf.
> 
> Ways to fix this:
> * Make this comparison only take effect if TCP_MD5SIG_FLAG_IFINDEX is set.
> * Make this comparison only take effect if tcp_l3mdev_accept=1
> * Add a new flag?
> 
> Right now passing TCP_MD5SIG_FLAG_IFINDEX and ifindex == 0 results in an
> error but maybe it should be accepted to mean "key applies only for
> default VRF".
> 

I think I remember the history now: prior to the set
98c8147648fa..5cad8bce26e0 MD5 lookups for VRF and default VRF both
succeed on an address or prefix match because the L3 domain was not
checked. That set did not want to break the legacy behavior which is why
the change is based on db key having l3index set and matching the
ingress domain.

That means the limitation (hole depending on perspective) is a default
VRF and a VRF having overlap addresses with a key installed with the L3
index set (can't since default VRF does not have one) - which I believe
is your point.

So, yes, one option is to have a flag that indicates strict checking to
close the legacy path.