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
