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] [thread-next>] [day] [month] [year] [list]
Message-ID: <86fea40c-6b8b-4ac3-bb14-4a24c63cf167@openvpn.net>
Date: Tue, 25 Mar 2025 00:15:48 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Donald Hunter <donald.hunter@...il.com>, Shuah Khan <shuah@...nel.org>,
 ryazanov.s.a@...il.com, Andrew Lunn <andrew+netdev@...n.ch>,
 Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Xiao Liang <shaw.leon@...il.com>
Subject: Re: [PATCH net-next v24 18/23] ovpn: implement peer
 add/get/dump/delete via netlink

On 24/03/2025 11:48, Sabrina Dubroca wrote:
> Hello Antonio,
> 
> A few questions wrt the API:
> 
> 2025-03-18, 02:40:53 +0100, Antonio Quartulli wrote:
>> +static bool ovpn_nl_attr_sockaddr_remote(struct nlattr **attrs,
>> +					 struct sockaddr_storage *ss)
>> +{
>> +	struct sockaddr_in6 *sin6;
>> +	struct sockaddr_in *sin;
>> +	struct in6_addr *in6;
>> +	__be16 port = 0;
>> +	__be32 *in;
>> +
>> +	ss->ss_family = AF_UNSPEC;
>> +
>> +	if (attrs[OVPN_A_PEER_REMOTE_PORT])
>> +		port = nla_get_be16(attrs[OVPN_A_PEER_REMOTE_PORT]);
> 
> What's the expected behavior if REMOTE_PORT isn't provided? We'll send
> packets do port 0 (which I'm guessing will get dropped on the other
> side) until we get a message from the peer and float sets the correct
> port/address?

I have never seen a packet going out with port 0 :)
But being dropped is most likely what's going to happen.

I'd say this is not something that we expect the user to do:
if the remote address if specified, the user should specify a non-zero 
port too.

We could add a check to ensure that a port is always specified if the 
remote address is there too, just to avoid the user to shoot himself in 
the foot.
But we expect the user to pass an addr:port where the peer is listening 
to (and that can't be a 0 port).

> 
> 
>> +static int ovpn_nl_peer_modify(struct ovpn_peer *peer, struct genl_info *info,
>> +			       struct nlattr **attrs)
>> +{
> [...]
>> +	/* when setting the keepalive, both parameters have to be configured */
>> +	if (attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL] &&
>> +	    attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]) {
>> +		interv = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_INTERVAL]);
>> +		timeout = nla_get_u32(attrs[OVPN_A_PEER_KEEPALIVE_TIMEOUT]);
>> +		ovpn_peer_keepalive_set(peer, interv, timeout);
> 
> Should we interpret OVPN_A_PEER_KEEPALIVE_INTERVAL = 0 &&
> OVPN_A_PEER_KEEPALIVE_TIMEOUT == 0 as "disable keepalive/timeout" on
> an active peer?  And maybe "one set to 0, the other set to some
> non-zero value" as invalid?  Setting either value to 0 doesn't seem
> very useful (timeout = 0 will probably kill the peer immediately, and
> I suspect interval = 0 would be quite spammy).
> 

Considering "0" as "disable keepalive" is the current intention.

In ovpn_peer_keepalive_work_single() you can see that if either one if 
0, we just skip the peer:

1217         /* we expect both timers to be configured at the same time,
1218          * therefore bail out if either is not set
1219          */
1220         if (!peer->keepalive_timeout || !peer->keepalive_interval) {
1221                 spin_unlock_bh(&peer->lock);
1222                 return 0;
1223         }

does it make sense?

Regards,

-- 
Antonio Quartulli
OpenVPN Inc.


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ