From: Otto Moerbeek <otto@drijf.net>
Subject: Re: ntpd: avoid duplicate addresses from pools
To: tech@openbsd.org
Date: Mon, 29 Jan 2024 11:58:58 +0100

On Thu, Jan 18, 2024 at 08:36:45AM +0100, Otto Moerbeek wrote:

> Hi,
> 
> Currenlty, if you specify "servers" that resolve to the same
> addresses, you'll see peers that have duplicate peer addresses. This
> avoids that by re-resolving a pool and assuming the pool changes over
> time.
> 
> It does not avoid all duplciate cases. If two "server" clauses
> resolve to the same address, duplciates cna still occur.
> 
> See somewhat recent discussion on bugs.
> 
> OK?

Ping...

> 	-Otto
> 
> Index: client.c
> ===================================================================
> RCS file: /home/cvs/src/usr.sbin/ntpd/client.c,v
> diff -u -p -r1.118 client.c
> --- client.c	20 Dec 2023 15:36:36 -0000	1.118
> +++ client.c	18 Jan 2024 07:22:25 -0000
> @@ -108,6 +108,7 @@ client_nextaddr(struct ntp_peer *p)
>  		return (-1);
>  
>  	if (p->addr_head.a == NULL) {
> +		log_debug("kicking query for %s",  p->addr_head.name);
>  		priv_dns(IMSG_HOST_DNS, p->addr_head.name, p->id);
>  		p->state = STATE_DNS_INPROGRESS;
>  		return (-1);
> @@ -145,7 +146,15 @@ client_query(struct ntp_peer *p)
>  	}
>  
>  	if (p->state < STATE_DNS_DONE || p->addr == NULL)
> -		return (-1);
> +		return (0);
> +
> +	/* if we're a pool member and a peer has taken our address, signal */
> +	if (p->addr_head.pool != 0 && addr_already_used(&p->addr->ss, p->addr_head.pool)) {
> +		log_debug("pool addr %s used by peer, will reresolve pool",
> +		    log_sockaddr((struct sockaddr *)&p->addr->ss));
> +		p->senderrors++;
> +		return (0);
> +	}
>  
>  	if (p->query.fd == -1) {
>  		struct sockaddr *sa = (struct sockaddr *)&p->addr->ss;
> Index: ntp.c
> ===================================================================
> RCS file: /home/cvs/src/usr.sbin/ntpd/ntp.c,v
> diff -u -p -r1.172 ntp.c
> --- ntp.c	20 Dec 2023 15:36:36 -0000	1.172
> +++ ntp.c	18 Jan 2024 07:22:25 -0000
> @@ -54,8 +54,6 @@ int	ntp_dispatch_imsg(void);
>  int	ntp_dispatch_imsg_dns(void);
>  void	peer_add(struct ntp_peer *);
>  void	peer_remove(struct ntp_peer *);
> -int	inpool(struct sockaddr_storage *,
> -	    struct sockaddr_storage[MAX_SERVERS_DNS], size_t);
>  
>  void
>  ntp_sighdlr(int sig)
> @@ -519,23 +517,52 @@ ntp_dispatch_imsg(void)
>  	return (0);
>  }
>  
> -int
> +static int
> +addr_equal(struct sockaddr_storage *a, struct sockaddr_storage *b)
> +{
> +	if (a->ss_family != b->ss_family)
> +		return 0;
> +	if (a->ss_family == AF_INET) {
> +		if (((struct sockaddr_in *)a)->sin_addr.s_addr ==
> +		    ((struct sockaddr_in *)b)->sin_addr.s_addr)
> +			return 1;
> +	} else if (memcmp(&((struct sockaddr_in6 *)a)->sin6_addr,
> +	    &((struct sockaddr_in6 *)b)->sin6_addr,
> +	    sizeof(struct sockaddr_in6)) == 0) {
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +static int
>  inpool(struct sockaddr_storage *a,
>      struct sockaddr_storage old[MAX_SERVERS_DNS], size_t n)
>  {
>  	size_t i;
>  
>  	for (i = 0; i < n; i++) {
> -		if (a->ss_family != old[i].ss_family)
> +		if (addr_equal(a, &old[i]))
> +			return 1;
> +	}
> +	return 0;
> +}
> +
> +/* check to see af an address is already listed
> +   in that case, it does not get added to a pool */
> +int
> +addr_already_used(struct sockaddr_storage *a, int skippool)
> +{
> +	struct ntp_peer *peer;
> +	struct ntp_addr *addr;
> +
> +	TAILQ_FOREACH(peer, &conf->ntp_peers, entry) {
> +		if (peer->addr_head.pool == skippool)
>  			continue;
> -		if (a->ss_family == AF_INET) {
> -			if (((struct sockaddr_in *)a)->sin_addr.s_addr ==
> -			    ((struct sockaddr_in *)&old[i])->sin_addr.s_addr)
> +		addr = peer->addr_head.a;
> +		while (addr != NULL) {
> +			if (addr_equal(a, &addr->ss))
>  				return 1;
> -		} else if (memcmp(&((struct sockaddr_in6 *)a)->sin6_addr,
> -		    &((struct sockaddr_in6 *)&old[i])->sin6_addr,
> -		    sizeof(struct sockaddr_in6)) == 0) {
> -			return 1;
> +			addr = addr->next;
>  		}
>  	}
>  	return 0;
> @@ -623,8 +650,11 @@ ntp_dispatch_imsg_dns(void)
>  						free(h);
>  						continue;
>  					}
> -					if (inpool(&h->ss, existing,
> -					    n)) {
> +					if (addr_already_used(&h->ss, peer->addr_head.pool) ||
> +					    inpool(&h->ss, existing, n)) {
> +						log_debug("skipping address %s for %s",
> +						    log_sockaddr((struct sockaddr *)
> +						    &h->ss), peer->addr_head.name);
>  						free(h);
>  						continue;
>  					}
> @@ -654,8 +684,12 @@ ntp_dispatch_imsg_dns(void)
>  			}
>  			if (dlen != 0)
>  				fatalx("IMSG_HOST_DNS: dlen != 0");
> -			if (peer->addr_head.pool)
> -				peer_remove(peer);
> +			if (peer->addr_head.pool) {
> +				if (peercount > addrcount)
> +					peer_remove(peer);
> +				else
> +					peer->state = STATE_NONE;
> +			}
>  			else
>  				client_addr_init(peer);
>  			break;
> Index: ntpd.h
> ===================================================================
> RCS file: /home/cvs/src/usr.sbin/ntpd/ntpd.h,v
> diff -u -p -r1.153 ntpd.h
> --- ntpd.h	20 Dec 2023 15:36:36 -0000	1.153
> +++ ntpd.h	18 Jan 2024 07:22:25 -0000
> @@ -371,6 +371,7 @@ int	server_dispatch(int, struct ntpd_con
>  int	client_peer_init(struct ntp_peer *);
>  int	client_addr_init(struct ntp_peer *);
>  int	client_nextaddr(struct ntp_peer *);
> +int	addr_already_used(struct sockaddr_storage *, int);
>  int	client_query(struct ntp_peer *);
>  int	client_dispatch(struct ntp_peer *, u_int8_t, u_int8_t);
>  void	client_log_error(struct ntp_peer *, const char *, int);
>