diff mbox series

[Openvpn-devel,net,3/5] ovpn: reject multipeer peers without VPN addresses

Message ID 17ced9a7caee691e602e3c02f5e399aa9a35067c.1785338921.git.ralf@mandelbit.com
State Changes Requested
Headers
Series ovpn: validate peer VPN addresses |

Commit Message

Ralf Lici July 29, 2026, 3:37 p.m. UTC 
  In MP mode, ovpn uses the peer VPN addresses to select the peer for
outgoing tunnel packets. Peer creation currently requires a VPN IPv4 or
IPv6 attribute, but it only checks for the presence of the attribute and
not for a usable address value.

This allows userspace to create an MP peer with only unspecified VPN
addresses, or to update an existing peer so that both VPN address
families become unspecified. Such a peer cannot be selected through the
VPN address hash tables.

Reject MP peer creation or update when the resulting peer would not have
at least one VPN address configured.

Fixes: 1d36a36f6d53 ("ovpn: implement peer add/get/dump/delete via netlink")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
 drivers/net/ovpn/netlink.c | 47 +++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 13 deletions(-)

Comments

Sabrina Dubroca Aug. 14, 2026, 10 a.m. UTC | #1 
2026-07-29, 17:37:41 +0200, Ralf Lici wrote:
> In MP mode, ovpn uses the peer VPN addresses to select the peer for
> outgoing tunnel packets. Peer creation currently requires a VPN IPv4 or
> IPv6 attribute, but it only checks for the presence of the attribute and
> not for a usable address value.
> 
> This allows userspace to create an MP peer with only unspecified VPN
> addresses, or to update an existing peer so that both VPN address
> families become unspecified. Such a peer cannot be selected through the
> VPN address hash tables.
> 
> Reject MP peer creation or update when the resulting peer would not have
> at least one VPN address configured.

Same comment about "this has never worked".

> @@ -371,11 +373,19 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
>  		return -EINVAL;
>  
>  	/* in MP mode VPN IPs are required for selecting the right peer */
> -	if (ovpn->mode == OVPN_MODE_MP && !attrs[OVPN_A_PEER_VPN_IPV4] &&
> -	    !attrs[OVPN_A_PEER_VPN_IPV6]) {
> -		NL_SET_ERR_MSG_FMT_MOD(info->extack,
> -				       "VPN IP must be provided in MP mode");
> -		return -EINVAL;
> +	if (ovpn->mode == OVPN_MODE_MP) {
> +		if (attrs[OVPN_A_PEER_VPN_IPV4])
> +			vpn_addr4.s_addr =
> +				nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
> +		if (attrs[OVPN_A_PEER_VPN_IPV6])
> +			vpn_addr6 =
> +				nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
> +
> +		if (!vpn_addr4.s_addr && ipv6_addr_any(&vpn_addr6)) {

minor nit, only if you end up resending this patch: the inconsistency
between !s_addr and != htonl(INADDR_ANY) isn't great. (but I don't
think it's confusing anyone)
Ralf Lici Aug. 14, 2026, 3:34 p.m. UTC | #2 
On Fri, 14 Aug 2026 12:00:25 +0200, Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2026-07-29, 17:37:41 +0200, Ralf Lici wrote:
> > In MP mode, ovpn uses the peer VPN addresses to select the peer for
> > outgoing tunnel packets. Peer creation currently requires a VPN IPv4 or
> > IPv6 attribute, but it only checks for the presence of the attribute and
> > not for a usable address value.
> > 
> > This allows userspace to create an MP peer with only unspecified VPN
> > addresses, or to update an existing peer so that both VPN address
> > families become unspecified. Such a peer cannot be selected through the
> > VPN address hash tables.
> > 
> > Reject MP peer creation or update when the resulting peer would not have
> > at least one VPN address configured.
>
> Same comment about "this has never worked".
>

ACK.

> > @@ -371,11 +373,19 @@ int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
> >  		return -EINVAL;
> >  
> >  	/* in MP mode VPN IPs are required for selecting the right peer */
> > -	if (ovpn->mode == OVPN_MODE_MP && !attrs[OVPN_A_PEER_VPN_IPV4] &&
> > -	    !attrs[OVPN_A_PEER_VPN_IPV6]) {
> > -		NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > -				       "VPN IP must be provided in MP mode");
> > -		return -EINVAL;
> > +	if (ovpn->mode == OVPN_MODE_MP) {
> > +		if (attrs[OVPN_A_PEER_VPN_IPV4])
> > +			vpn_addr4.s_addr =
> > +				nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
> > +		if (attrs[OVPN_A_PEER_VPN_IPV6])
> > +			vpn_addr6 =
> > +				nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
> > +
> > +		if (!vpn_addr4.s_addr && ipv6_addr_any(&vpn_addr6)) {
>
> minor nit, only if you end up resending this patch: the inconsistency
> between !s_addr and != htonl(INADDR_ANY) isn't great. (but I don't
> think it's confusing anyone)
>

Ah right, I missed this one. I'll resend the series anyway so I'll make
sure to use != htonl(INADDR_ANY) here as well.
diff mbox series

Patch

diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index 79775af26fda..43e6c7a29a6f 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -346,8 +346,10 @@  static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
 
 int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
+	struct in_addr vpn_addr4 = { .s_addr = INADDR_ANY };
+	struct in6_addr vpn_addr6 = IN6ADDR_ANY_INIT;
 	struct ovpn_priv *ovpn = info->user_ptr[0];
+	struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
 	struct ovpn_socket *ovpn_sock;
 	struct socket *sock = NULL;
 	struct ovpn_peer *peer;
@@ -371,11 +373,19 @@  int ovpn_nl_peer_new_doit(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 
 	/* in MP mode VPN IPs are required for selecting the right peer */
-	if (ovpn->mode == OVPN_MODE_MP && !attrs[OVPN_A_PEER_VPN_IPV4] &&
-	    !attrs[OVPN_A_PEER_VPN_IPV6]) {
-		NL_SET_ERR_MSG_FMT_MOD(info->extack,
-				       "VPN IP must be provided in MP mode");
-		return -EINVAL;
+	if (ovpn->mode == OVPN_MODE_MP) {
+		if (attrs[OVPN_A_PEER_VPN_IPV4])
+			vpn_addr4.s_addr =
+				nla_get_in_addr(attrs[OVPN_A_PEER_VPN_IPV4]);
+		if (attrs[OVPN_A_PEER_VPN_IPV6])
+			vpn_addr6 =
+				nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
+
+		if (!vpn_addr4.s_addr && ipv6_addr_any(&vpn_addr6)) {
+			NL_SET_ERR_MSG_FMT_MOD(info->extack,
+					       "VPN IP must be provided in MP mode");
+			return -EINVAL;
+		}
 	}
 
 	peer_id = nla_get_u32(attrs[OVPN_A_PEER_ID]);
@@ -534,13 +544,24 @@  int ovpn_nl_peer_set_doit(struct sk_buff *skb, struct genl_info *info)
 	if (attrs[OVPN_A_PEER_VPN_IPV6])
 		vpn_addr6 = nla_get_in6_addr(attrs[OVPN_A_PEER_VPN_IPV6]);
 
-	/* reject peer with conflicting VPN address */
-	if ((attrs[OVPN_A_PEER_VPN_IPV4] || attrs[OVPN_A_PEER_VPN_IPV6]) &&
-	    ovpn_peer_vpn_addr_conflict(ovpn, peer, &vpn_addr4, &vpn_addr6)) {
-		NL_SET_ERR_MSG_FMT_MOD(info->extack,
-				       "VPN IP is already assigned to another peer");
-		ret = -EADDRINUSE;
-		goto unlock;
+	/* in MP mode VPN IPs are required for selecting the right peer */
+	if (ovpn->mode == OVPN_MODE_MP &&
+	    (attrs[OVPN_A_PEER_VPN_IPV4] || attrs[OVPN_A_PEER_VPN_IPV6])) {
+		if (!vpn_addr4.s_addr && ipv6_addr_any(&vpn_addr6)) {
+			NL_SET_ERR_MSG_FMT_MOD(info->extack,
+					       "MP peer must have at least one valid VPN IP");
+			ret = -EINVAL;
+			goto unlock;
+		}
+
+		/* reject peer with conflicting VPN address */
+		if (ovpn_peer_vpn_addr_conflict(ovpn, peer, &vpn_addr4,
+						&vpn_addr6)) {
+			NL_SET_ERR_MSG_FMT_MOD(info->extack,
+					       "VPN IP is already assigned to another peer");
+			ret = -EADDRINUSE;
+			goto unlock;
+		}
 	}
 
 	ret = ovpn_nl_peer_modify(peer, info, attrs);