ldpd: buffer underflow, thread safety (PVS-Studio)

This commit fixes two issues:
- memcpy() using containers of different sizes when using addr2sa(), mixing
  'struct sockaddr_storage' and 'union sockunion'.
- addr2sa() function not being thread safe (using a local static variable as
  container.

Signed-off-by: F. Aragon <paco@voltanet.io>

6 files changed, 46 insertions, 51 deletions

diff --git a/ldpd/ldpd.h b/ldpd/ldpd.h
index ed0a5407b2..9113505581 100644
--- a/ldpd/ldpd.h
+++ b/ldpd/ldpd.h
@@ -691,7 +691,8 @@ void		 embedscope(struct sockaddr_in6 *);
 void		 recoverscope(struct sockaddr_in6 *);
 void		 addscope(struct sockaddr_in6 *, uint32_t);
 void		 clearscope(struct in6_addr *);
-struct sockaddr	*addr2sa(int af, union ldpd_addr *, uint16_t);
+void		 addr2sa(int af, const union ldpd_addr *, uint16_t,
+		    union sockunion *su);
 void		 sa2addr(struct sockaddr *, int *, union ldpd_addr *,
 		    in_port_t *);
 socklen_t	 sockaddr_len(struct sockaddr *);
diff --git a/ldpd/neighbor.c b/ldpd/neighbor.c
index 1c3f650dff..78a6131ca4 100644
--- a/ldpd/neighbor.c
+++ b/ldpd/neighbor.c
@@ -584,8 +584,8 @@ nbr_connect_cb(struct thread *thread)
 int
 nbr_establish_connection(struct nbr *nbr)
 {
-	struct sockaddr_storage	 local_sa;
-	struct sockaddr_storage	 remote_sa;
+	union sockunion		 local_su;
+	union sockunion		 remote_su;
 	struct adj		*adj;
 	struct nbr_params	*nbrp;
 #ifdef __OpenBSD__
@@ -619,16 +619,14 @@ nbr_establish_connection(struct nbr *nbr)
 #endif
 	}
 
-	memcpy(&local_sa, addr2sa(nbr->af, &nbr->laddr, 0), sizeof(local_sa));
-	memcpy(&remote_sa, addr2sa(nbr->af, &nbr->raddr, LDP_PORT),
-	    sizeof(local_sa));
+	addr2sa(nbr->af, &nbr->laddr, 0, &local_su);
+	addr2sa(nbr->af, &nbr->raddr, LDP_PORT, &remote_su);
 	if (nbr->af == AF_INET6 && nbr->raddr_scope)
-		addscope((struct sockaddr_in6 *)&remote_sa, nbr->raddr_scope);
+		addscope(&remote_su.sin6, nbr->raddr_scope);
 
-	if (bind(nbr->fd, (struct sockaddr *)&local_sa,
-	    sockaddr_len((struct sockaddr *)&local_sa)) == -1) {
+	if (bind(nbr->fd, &local_su.sa, sockaddr_len(&local_su.sa)) == -1) {
 		log_warn("%s: error while binding socket to %s", __func__,
-		    log_sockaddr((struct sockaddr *)&local_sa));
+			 log_sockaddr(&local_su.sa));
 		close(nbr->fd);
 		return (-1);
 	}
@@ -646,15 +644,15 @@ nbr_establish_connection(struct nbr *nbr)
 		send_hello(adj->source.type, adj->source.link.ia,
 		    adj->source.target);
 
-	if (connect(nbr->fd, (struct sockaddr *)&remote_sa,
-	    sockaddr_len((struct sockaddr *)&remote_sa)) == -1) {
+	if (connect(nbr->fd, &remote_su.sa, sockaddr_len(&remote_su.sa))
+	    == -1) {
 		if (errno == EINPROGRESS) {
 			thread_add_write(master, nbr_connect_cb, nbr, nbr->fd,
 					 &nbr->ev_connect);
 			return (0);
 		}
 		log_warn("%s: error while connecting to %s", __func__,
-		    log_sockaddr((struct sockaddr *)&remote_sa));
+			 log_sockaddr(&remote_su.sa));
 		close(nbr->fd);
 		return (-1);
 	}
diff --git a/ldpd/packet.c b/ldpd/packet.c
index b0f9c5eb14..8ca90841de 100644
--- a/ldpd/packet.c
+++ b/ldpd/packet.c
@@ -70,7 +70,7 @@ int
 send_packet(int fd, int af, union ldpd_addr *dst, struct iface_af *ia,
     void *pkt, size_t len)
 {
-	struct sockaddr		*sa;
+	union sockunion su;
 
 	switch (af) {
 	case AF_INET:
@@ -97,10 +97,10 @@ send_packet(int fd, int af, union ldpd_addr *dst, struct iface_af *ia,
 		fatalx("send_packet: unknown af");
 	}
 
-	sa = addr2sa(af, dst, LDP_PORT);
-	if (sendto(fd, pkt, len, 0, sa, sockaddr_len(sa)) == -1) {
+	addr2sa(af, dst, LDP_PORT, &su);
+	if (sendto(fd, pkt, len, 0, &su.sa, sockaddr_len(&su.sa)) == -1) {
 		log_warn("%s: error sending packet to %s", __func__,
-		    log_sockaddr(sa));
+			 log_sockaddr(&su.sa));
 		return (-1);
 	}
 
diff --git a/ldpd/pfkey.c b/ldpd/pfkey.c
index a1a79dabfd..906737217c 100644
--- a/ldpd/pfkey.c
+++ b/ldpd/pfkey.c
@@ -64,17 +64,17 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
 	ssize_t			n;
 	int			len = 0;
 	int			iov_cnt;
-	struct sockaddr_storage	ssrc, sdst, smask, dmask;
-	struct sockaddr		*saptr;
+	struct sockaddr_storage	smask, dmask;
+	union sockunion		su_src, su_dst;
 
 	if (!pid)
 		pid = getpid();
 
 	/* we need clean sockaddr... no ports set */
-	memset(&ssrc, 0, sizeof(ssrc));
 	memset(&smask, 0, sizeof(smask));
-	if ((saptr = addr2sa(af, src, 0)))
-		memcpy(&ssrc, saptr, sizeof(ssrc));
+
+	addr2sa(af, src, 0, &su_src);
+
 	switch (af) {
 	case AF_INET:
 		memset(&((struct sockaddr_in *)&smask)->sin_addr, 0xff, 32/8);
@@ -86,13 +86,13 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
 	default:
 		return (-1);
 	}
-	smask.ss_family = ssrc.ss_family;
-	smask.ss_len = ssrc.ss_len;
+	smask.ss_family = su_src.sa.sa_family;
+	smask.ss_len = sockaddr_len(&su_src.sa);
 
-	memset(&sdst, 0, sizeof(sdst));
 	memset(&dmask, 0, sizeof(dmask));
-	if ((saptr = addr2sa(af, dst, 0)))
-		memcpy(&sdst, saptr, sizeof(sdst));
+
+	addr2sa(af, dst, 0, &su_dst);
+
 	switch (af) {
 	case AF_INET:
 		memset(&((struct sockaddr_in *)&dmask)->sin_addr, 0xff, 32/8);
@@ -104,8 +104,8 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
 	default:
 		return (-1);
 	}
-	dmask.ss_family = sdst.ss_family;
-	dmask.ss_len = sdst.ss_len;
+	dmask.ss_family = su_dst.sa.sa_family;
+	dmask.ss_len = sockaddr_len(&su_dst.sa);
 
 	memset(&smsg, 0, sizeof(smsg));
 	smsg.sadb_msg_version = PF_KEY_V2;
@@ -138,11 +138,13 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
 
 	memset(&sa_src, 0, sizeof(sa_src));
 	sa_src.sadb_address_exttype = SADB_EXT_ADDRESS_SRC;
-	sa_src.sadb_address_len = (sizeof(sa_src) + ROUNDUP(ssrc.ss_len)) / 8;
+	sa_src.sadb_address_len =
+		(sizeof(sa_src) + ROUNDUP(sockaddr_len(&su_src.sa))) / 8;
 
 	memset(&sa_dst, 0, sizeof(sa_dst));
 	sa_dst.sadb_address_exttype = SADB_EXT_ADDRESS_DST;
-	sa_dst.sadb_address_len = (sizeof(sa_dst) + ROUNDUP(sdst.ss_len)) / 8;
+	sa_dst.sadb_address_len =
+		(sizeof(sa_dst) + ROUNDUP(sockaddr_len(&su_dst.sa))) / 8;
 
 	sa.sadb_sa_auth = aalg;
 	sa.sadb_sa_encrypt = SADB_X_EALG_AES; /* XXX */
@@ -195,8 +197,8 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
 	iov[iov_cnt].iov_base = &sa_dst;
 	iov[iov_cnt].iov_len = sizeof(sa_dst);
 	iov_cnt++;
-	iov[iov_cnt].iov_base = &sdst;
-	iov[iov_cnt].iov_len = ROUNDUP(sdst.ss_len);
+	iov[iov_cnt].iov_base = &su_dst;
+	iov[iov_cnt].iov_len = ROUNDUP(sockaddr_len(&su_dst.sa));
 	smsg.sadb_msg_len += sa_dst.sadb_address_len;
 	iov_cnt++;
 
@@ -204,8 +206,8 @@ pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
 	iov[iov_cnt].iov_base = &sa_src;
 	iov[iov_cnt].iov_len = sizeof(sa_src);
 	iov_cnt++;
-	iov[iov_cnt].iov_base = &ssrc;
-	iov[iov_cnt].iov_len = ROUNDUP(ssrc.ss_len);
+	iov[iov_cnt].iov_base = &su_src;
+	iov[iov_cnt].iov_len = ROUNDUP(sockaddr_len(&su_src.sa));
 	smsg.sadb_msg_len += sa_src.sadb_address_len;
 	iov_cnt++;
 
diff --git a/ldpd/socket.c b/ldpd/socket.c
index eaea9973a0..aefa3461a8 100644
--- a/ldpd/socket.c
+++ b/ldpd/socket.c
@@ -37,7 +37,7 @@ ldp_create_socket(int af, enum socket_type type)
 {
 	int			 fd, domain, proto;
 	union ldpd_addr		 addr;
-	struct sockaddr_storage	 local_sa;
+	union sockunion		 local_su;
 #ifdef __OpenBSD__
 	int			 opt;
 #endif
@@ -70,14 +70,12 @@ ldp_create_socket(int af, enum socket_type type)
 	case LDP_SOCKET_DISC:
 		/* listen on all addresses */
 		memset(&addr, 0, sizeof(addr));
-		memcpy(&local_sa, addr2sa(af, &addr, LDP_PORT),
-		    sizeof(local_sa));
+		addr2sa(af, &addr, LDP_PORT, &local_su);
 		break;
 	case LDP_SOCKET_EDISC:
 	case LDP_SOCKET_SESSION:
 		addr = (ldp_af_conf_get(ldpd_conf, af))->trans_addr;
-		memcpy(&local_sa, addr2sa(af, &addr, LDP_PORT),
-		    sizeof(local_sa));
+		addr2sa(af, &addr, LDP_PORT, &local_su);
 		/* ignore any possible error */
 		sock_set_bindany(fd, 1);
 		break;
@@ -90,8 +88,7 @@ ldp_create_socket(int af, enum socket_type type)
 		close(fd);
 		return (-1);
 	}
-	if (bind(fd, (struct sockaddr *)&local_sa,
-	    sockaddr_len((struct sockaddr *)&local_sa)) == -1) {
+	if (bind(fd, &local_su.sa, sockaddr_len(&local_su.sa)) == -1) {
 		save_errno = errno;
 		if (ldpd_privs.change(ZPRIVS_LOWER))
 			log_warn("%s: could not lower privs", __func__);
@@ -307,7 +304,7 @@ sock_set_md5sig(int fd, int af, union ldpd_addr *addr, const char *password)
 	if (fd == -1)
 		return (0);
 #if HAVE_DECL_TCP_MD5SIG
-	memcpy(&su, addr2sa(af, addr, 0), sizeof(su));
+	addr2sa(af, addr, 0, &su);
 
 	if (ldpe_privs.change(ZPRIVS_RAISE)) {
 		log_warn("%s: could not raise privs", __func__);
diff --git a/ldpd/util.c b/ldpd/util.c
index e735263f5f..12f9cb0ccf 100644
--- a/ldpd/util.c
+++ b/ldpd/util.c
@@ -305,14 +305,13 @@ clearscope(struct in6_addr *in6)
 	}
 }
 
-struct sockaddr *
-addr2sa(int af, union ldpd_addr *addr, uint16_t port)
+void
+addr2sa(int af, const union ldpd_addr *addr, uint16_t port, union sockunion *su)
 {
-	static struct sockaddr_storage	 ss;
-	struct sockaddr_in		*sa_in = (struct sockaddr_in *)&ss;
-	struct sockaddr_in6		*sa_in6 = (struct sockaddr_in6 *)&ss;
+	struct sockaddr_in		*sa_in = &su->sin;
+	struct sockaddr_in6		*sa_in6 = &su->sin6;
 
-	memset(&ss, 0, sizeof(ss));
+	memset(su, 0, sizeof(*su));
 	switch (af) {
 	case AF_INET:
 		sa_in->sin_family = AF_INET;
@@ -333,8 +332,6 @@ addr2sa(int af, union ldpd_addr *addr, uint16_t port)
 	default:
 		fatalx("addr2sa: unknown af");
 	}
-
-	return ((struct sockaddr *)&ss);
 }
 
 void