netdev - Re: [PATCH v2 net-next] wireguard: allowedips: Add WGALLOWEDIP_F_REMOVE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241127232133.3928793-1-jrife@google.com>
Date: Wed, 27 Nov 2024 23:21:33 +0000
From: Jordan Rife <jrife@...gle.com>
To: jrife@...gle.com
Cc: Jason@...c4.com, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	linux-kselftest@...r.kernel.org, netdev@...r.kernel.org, pabeni@...hat.com, 
	shuah@...nel.org, wireguard@...ts.zx2c4.com
Subject: Re: [PATCH v2 net-next] wireguard: allowedips: Add
 WGALLOWEDIP_F_REMOVE_ME flag

> I think the challenge with WGALLOWEDIP_A_FLAGS in particular is that
> because it didn't exist since the beginning like WGPEER_A_FLAGS, there
> are kernels out there that have no knowledge of it and wouldn't have
> this check in place. While I think it's a good idea to replicate this
> check for WGALLOWEDIP_A_FLAGS as well for future compatibility, we
> still need some way for clients to probe whether or not this feature
> is supported in case they're running on an older kernel. If we want to
> keep the version number as-is, I see a few alternatives:

Forget about all of this actually. I was under the mistaken impression
that an unrecognized attribute would be silently ignored by an older
kernel, but it seems that validation is strict.

	if (attrs[WGPEER_A_ALLOWEDIPS]) {
		struct nlattr *attr, *allowedip[WGALLOWEDIP_A_MAX + 1];
		int rem;

		nla_for_each_nested(attr, attrs[WGPEER_A_ALLOWEDIPS], rem) {
			ret = nla_parse_nested(allowedip, WGALLOWEDIP_A_MAX,
					       attr, allowedip_policy, NULL);
			if (ret < 0)
				goto out;
			ret = set_allowedip(peer, allowedip);
			if (ret < 0)
				goto out;
		}
	}

nla_parse_nested() uses NL_VALIDATE_STRICT which sets
NL_VALIDATE_MAXTYPE, causing __nla_validate_parse() in this case to
check that no attribute types are greater than WGALLOWEDIP_A_MAX.

The WG_CMD_SET_DEVICE operation simply returns EINVAL if you try to use
WGALLOWEDIP_A_FLAGS on a kernel that doesn't support it. I tested this
using a patched version of wg that sets the WGALLOWEDIP_F_REMOVE_ME
attribute when using an argument I added called "allowed-ips-patch".

Kernel With WGALLOWEDIP_A_FLAGS
==================================
jordan@t14:~/code/wireguard-tools/src$ sudo ./wg set wg0 peer xK7O/YnTb8W/fgPA4dwAshEV06rMPMqqmy3zZN0NPS4= allowed-ips-patch 192.168.0.3/32
jordan@t14:~/code/wireguard-tools/src$ sudo ./wg
interface: wg0

peer: xK7O/YnTb8W/fgPA4dwAshEV06rMPMqqmy3zZN0NPS4=
  allowed ips: 192.168.0.3/32
jordan@t14:~/code/wireguard-tools/src$ sudo ./wg set wg0 peer xK7O/YnTb8W/fgPA4dwAshEV06rMPMqqmy3zZN0NPS4= allowed-ips-patch -192.168.0.3/32
jordan@t14:~/code/wireguard-tools/src$ sudo ./wg
interface: wg0

peer: xK7O/YnTb8W/fgPA4dwAshEV06rMPMqqmy3zZN0NPS4=
  allowed ips: (none)
jordan@t14:~/code/wireguard-tools/src$ 

Kernel Without WGALLOWEDIP_A_FLAGS
==================================
jordan@t14:~/code/wireguard-tools/src$ sudo ./wg set wg0 peer xK7O/YnTb8W/fgPA4dwAshEV06rMPMqqmy3zZN0NPS4= allowed-ips-patch 192.168.0.3/32
jordan@t14:~/code/wireguard-tools/src$ sudo ./wg
interface: wg0

peer: xK7O/YnTb8W/fgPA4dwAshEV06rMPMqqmy3zZN0NPS4=
  allowed ips: 192.168.0.3/32
jordan@t14:~/code/wireguard-tools/src$ sudo ./wg set wg0 peer xK7O/YnTb8W/fgPA4dwAshEV06rMPMqqmy3zZN0NPS4= allowed-ips-patch -192.168.0.3/32
Unable to modify interface: Invalid argument
jordan@t14:~/code/wireguard-tools/src$ 

The second command fails with "Invalid argument" (EINVAL) on the
unpatched kernel. This simplifies things, as there's no need for
clients to explicitly probe to see if this attribute is supported. I
will do the following:

1. Revert WG_GENL_VERSION back to 1.
2. Add a check for new flags similar to the one you mentioned for
   WGPEER_A_FLAGS.

        if (attrs[WGPEER_A_FLAGS])
                flags = nla_get_u32(attrs[WGPEER_A_FLAGS]);
        ret = -EOPNOTSUPP;
        if (flags & ~__WGPEER_F_ALL)
                goto out;

This should be sufficient. We might want to consider how best to bubble
this error up to users. In the case of wg, "Invalid argument" may not be
very helpful in determining where you went wrong. We could always detect
when EINVAL is returned in response to an operation that sets
WGALLOWEDIP_A_FLAGS and print something more helpful like "Operation not
supported on this kernel". However, these are details that can be worked
out.

Sorry for the confusion!

-Jordan