| 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
| ||
|
Message-ID: <28447817-028e-4c5c-ae7e-3ffa86de9bf1@openvpn.net>
Date: Wed, 20 Nov 2024 12:34:08 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
Shuah Khan <shuah@...nel.org>, ryazanov.s.a@...il.com,
Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 18/23] ovpn: implement peer
add/get/dump/delete via netlink
On 20/11/2024 12:12, Sabrina Dubroca wrote:
> 2024-11-14, 10:21:18 +0100, Antonio Quartulli wrote:
>> On 13/11/2024 17:56, Sabrina Dubroca wrote:
>>> 2024-11-12, 15:19:50 +0100, Antonio Quartulli wrote:
>>>> On 04/11/2024 16:14, Sabrina Dubroca wrote:
>>>>> 2024-10-29, 11:47:31 +0100, Antonio Quartulli wrote:
>>>>>> +static int ovpn_nl_peer_precheck(struct ovpn_struct *ovpn,
>>>>>> + struct genl_info *info,
>>>>>> + struct nlattr **attrs)
>>>>>> +{
>>>>>> + if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_PEER], attrs,
>>>>>> + OVPN_A_PEER_ID))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + if (attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6]) {
>>>>>> + NL_SET_ERR_MSG_MOD(info->extack,
>>>>>> + "cannot specify both remote IPv4 or IPv6 address");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
>>>>>> + !attrs[OVPN_A_PEER_REMOTE_IPV6] && attrs[OVPN_A_PEER_REMOTE_PORT]) {
>>>>>> + NL_SET_ERR_MSG_MOD(info->extack,
>>>>>> + "cannot specify remote port without IP address");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
>>>>>> + attrs[OVPN_A_PEER_LOCAL_IPV4]) {
>>>>>> + NL_SET_ERR_MSG_MOD(info->extack,
>>>>>> + "cannot specify local IPv4 address without remote");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
>>>>>> + attrs[OVPN_A_PEER_LOCAL_IPV6]) {
>>>>>
>>>>> I think these consistency checks should account for v4mapped
>>>>> addresses. With remote=v4mapped and local=v6 we'll end up with an
>>>>> incorrect ipv4 "local" address (taken out of the ipv6 address's first
>>>>> 4B by ovpn_peer_reset_sockaddr). With remote=ipv6 and local=v4mapped,
>>>>> we'll pass the last 4B of OVPN_A_PEER_LOCAL_IPV6 to
>>>>> ovpn_peer_reset_sockaddr and try to read 16B (the full ipv6 address)
>>>>> out of that.
>>>>
>>>> Right, a v4mapped address would fool this check.
>>>> How about checking if both or none addresses are v4mapped? This way we
>>>> should prevent such cases.
>>>
>>> I don't know when userspace would use v4mapped addresses,
>>
>> It happens when listening on [::] with a v6 socket that has no "IPV6_V6ONLY"
>> set to true (you can check ipv6(7) for more details).
>> This socket can receive IPv4 connections, which are implemented using
>> v4mapped addresses. In this case both remote and local are going to be
>> v4mapped.
>
> I'm familiar with v4mapped addresses, but I wasn't sure the userspace
> part would actually passed them as peer. But I guess it would when the
> peer connects over ipv4 on an ipv6 socket.
>
> So the combination of PEER_IPV4 with LOCAL_IPV6(v4mapped) should never
> happen? In that case I guess we just need to check that we got 2
> attributes of the same type (both _IPV4 or both _IPV6) and if we got
> _IPV6, that they're either both v4mapped or both not. Might be a tiny
> bit simpler than what I was suggesting below.
Exactly - this is what I was originally suggesting, but your solution is
just a bit cleaner imho.
>
>> However, the sanity check should make sure nobody can inject bogus
>> combinations.
>>
>>> but treating
>>> a v4mapped address as a "proper" ipv4 address should work with the
>>> rest of the code, since you already have the conversion in
>>> ovpn_nl_attr_local_ip and ovpn_nl_attr_sockaddr_remote. So maybe you
>>> could do something like (rough idea and completely untested):
>>>
>>> static int get_family(attr_v4, attr_v6)
>>> {
>>> if (attr_v4)
>>> return AF_INET;
>>> if (attr_v6) {
>>> if (ipv6_addr_v4mapped(attr_v6)
>>> return AF_INET;
>>> return AF_INET6;
>>> }
>>> return AF_UNSPEC;
>>> }
>>>
>>>
>>> // in _precheck:
>>> // keep the attrs[OVPN_A_PEER_REMOTE_IPV4] && attrs[OVPN_A_PEER_REMOTE_IPV6] check
>>> // maybe add a similar one for LOCAL_IPV4 && LOCAL_IPV6
>>
>> the latter is already covered by:
>>
>> 192 if (!attrs[OVPN_A_PEER_REMOTE_IPV4] &&
>> 193 attrs[OVPN_A_PEER_LOCAL_IPV4]) {
>> 194 NL_SET_ERR_MSG_MOD(info->extack,
>> 195 "cannot specify local IPv4 address
>> without remote");
>> 196 return -EINVAL;
>> 197 }
>> 198
>> 199 if (!attrs[OVPN_A_PEER_REMOTE_IPV6] &&
>> 200 attrs[OVPN_A_PEER_LOCAL_IPV6]) {
>> 201 NL_SET_ERR_MSG_MOD(info->extack,
>> 202 "cannot specify local IPV6 address
>> without remote");
>> 203 return -EINVAL;
>> 204 }
>
> LOCAL_IPV4 combined with REMOTE_IPV6 should be fine if the remote is
> v4mapped. And conversely, LOCAL_IPV6 combined with REMOTE_IPV6 isn't
> ok if remote is v4mapped. So those checks should go away and be
> replaced with the "get_family" thing, but that requires at most one of
> the _IPV4/_IPV6 attributes to be present to behave consistently.
I don't expect to receive a mix of _IPV4 and _IPV6, because the
assumption is that either both addresses are v4mapped or none.
Userspace fetches the addresses from the received packet, so I presume
they will both be exposed as v4mapped if we are in this special case.
Hence, I don't truly want to allow combining them.
Does it make sense?
>
>
>>>
>>> remote_family = get_family(attrs[OVPN_A_PEER_REMOTE_IPV4], attrs[OVPN_A_PEER_REMOTE_IPV6]);
>>> local_family = get_family(attrs[OVPN_A_PEER_LOCAL_IPV4], attrs[OVPN_A_PEER_LOCAL_IPV6]);
>>> if (remote_family != local_family) {
>>> extack "incompatible address families";
>>> return -EINVAL;
>>> }
>>>
>>> That would mirror the conversion that
>>> ovpn_nl_attr_local_ip/ovpn_nl_attr_sockaddr_remote do.
>>
>> Yeah, pretty much what I was suggested, but in a more explicit manner.
>> I like it.
>
> Cool.
>
> BTW, I guess scope_id should only be used when it's not a v4mapped address?
> So the "cannot specify scope id without remote IPv6 address" check
> should probably use:
>
> if (remote_family != AF_INET6)
Right!
>
> (or split it into !attrs[OVPN_A_PEER_REMOTE_IPV6] and remote_family !=
> AF_INET6 to have a fully specific extack message, but maybe that's
> overkill)
Yeah, maybe splitting works better.
Thanks a lot!
Regards,
>
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists