From: Otto Moerbeek <otto@drijf.net>
Subject: ntpd: avoid duplicate addresses from pools
To: tech@openbsd.org
Date: Thu, 18 Jan 2024 08:36:45 +0100

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?
	-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);