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

[Sabrina Dubroca <sd@queasysnail.net>]

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.

> 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.


> > 
> >      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)

(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)

-- 
Sabrina