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