Re: [PATCH net-next v11 18/23] ovpn: implement peer add/get/dump/delete via netlink

[Antonio Quartulli <antonio@openvpn.net>]

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.