Fix userland tools that don't check the format of routing socket

messages before accessing message fields that may not be present,
removing dead/duplicate/misleading code along the way.

Document the message format for each routing socket message in
route.h.

Fix a bug in usr.bin/netstat introduced in r287351 that resulted in
pointer computation with essentially random 16-bit offsets and
dereferencing of the results.

Reviewed by:	ae
MFC after:	1 month
Differential Revision:	https://reviews.freebsd.org/D10330

contrib/traceroute/findsaddr-socket.c

@@ -156,7 +156,8 @@ findsaddr(register const struct sockaddr_in *to,
 			return (errbuf);
 		}
 
-	} while (rp->rtm_seq != seq || rp->rtm_pid != pid);
+	} while (rp->rtm_type != RTM_GET || rp->rtm_seq != seq ||
+	    rp->rtm_pid != pid);
 	close(s);
 
 

sbin/route/route.c

@@ -1497,10 +1497,7 @@ rtmsg(int cmd, int flags, int fib)
 
 #define NEXTADDR(w, u)							\
 	if (rtm_addrs & (w)) {						\
-		l = (((struct sockaddr *)&(u))->sa_len == 0) ?		\
-		    sizeof(long) :					\
-		    1 + ((((struct sockaddr *)&(u))->sa_len - 1)	\
-			| (sizeof(long) - 1));				\
+		l = SA_SIZE(&(u));					\
 		memmove(cp, (char *)&(u), l);				\
 		cp += l;						\
 		if (verbose)						\
@@ -1564,7 +1561,8 @@ rtmsg(int cmd, int flags, int fib)
 		do {
 			l = read(s, (char *)&m_rtmsg, sizeof(m_rtmsg));
 		} while (l > 0 && stop_read == 0 &&
-		    (rtm.rtm_seq != rtm_seq || rtm.rtm_pid != pid));
+		    (rtm.rtm_type != RTM_GET || rtm.rtm_seq != rtm_seq ||
+			rtm.rtm_pid != pid));
 		if (stop_read != 0) {
 			warnx("read from routing socket timed out");
 			return (-1);
@@ -1706,10 +1704,13 @@ print_rtmsg(struct rt_msghdr *rtm, size_t msglen)
 		break;
 
 	default:
-		printf("pid: %ld, seq %d, errno %d, flags:",
-			(long)rtm->rtm_pid, rtm->rtm_seq, rtm->rtm_errno);
-		printb(rtm->rtm_flags, routeflags);
-		pmsg_common(rtm, msglen);
+		if (rtm->rtm_type <= RTM_RESOLVE) {
+			printf("pid: %ld, seq %d, errno %d, flags:",
+			    (long)rtm->rtm_pid, rtm->rtm_seq, rtm->rtm_errno);
+			printb(rtm->rtm_flags, routeflags);
+			pmsg_common(rtm, msglen);
+		} else
+			printf("type: %u, len: %zu\n", rtm->rtm_type, msglen);
 	}
 
 	return;

sbin/routed/table.c

@@ -1233,6 +1233,15 @@ read_rt(void)
 		if (m.r.rtm.rtm_type <= RTM_CHANGE)
 			strp += sprintf(strp," from pid %d",m.r.rtm.rtm_pid);
 
+		/*
+		 * Only messages that use the struct rt_msghdr format are
+		 * allowed beyond this point.
+		 */
+		if (m.r.rtm.rtm_type > RTM_RESOLVE) {
+			trace_act("ignore %s", str);
+			continue;
+		}
+		
 		rt_xaddrs(&info, m.r.addrs, &m.r.addrs[RTAX_MAX],
 			  m.r.rtm.rtm_addrs);
 

sys/net/route.h

@@ -265,25 +265,35 @@ struct rt_msghdr {
 
 /*
  * Message types.
+ *
+ * The format for each message is annotated below using the following
+ * identifiers:
+ *
+ * (1) struct rt_msghdr
+ * (2) struct ifa_msghdr
+ * (3) struct if_msghdr
+ * (4) struct ifma_msghdr
+ * (5) struct if_announcemsghdr
+ *
  */
-#define RTM_ADD		0x1	/* Add Route */
-#define RTM_DELETE	0x2	/* Delete Route */
-#define RTM_CHANGE	0x3	/* Change Metrics or flags */
-#define RTM_GET		0x4	/* Report Metrics */
-#define RTM_LOSING	0x5	/* Kernel Suspects Partitioning */
-#define RTM_REDIRECT	0x6	/* Told to use different route */
-#define RTM_MISS	0x7	/* Lookup failed on this address */
-#define RTM_LOCK	0x8	/* fix specified metrics */
+#define	RTM_ADD		0x1	/* (1) Add Route */
+#define	RTM_DELETE	0x2	/* (1) Delete Route */
+#define	RTM_CHANGE	0x3	/* (1) Change Metrics or flags */
+#define	RTM_GET		0x4	/* (1) Report Metrics */
+#define	RTM_LOSING	0x5	/* (1) Kernel Suspects Partitioning */
+#define	RTM_REDIRECT	0x6	/* (1) Told to use different route */
+#define	RTM_MISS	0x7	/* (1) Lookup failed on this address */
+#define	RTM_LOCK	0x8	/* (1) fix specified metrics */
 		    /*	0x9  */
 		    /*	0xa  */
-#define RTM_RESOLVE	0xb	/* req to resolve dst to LL addr */
-#define RTM_NEWADDR	0xc	/* address being added to iface */
-#define RTM_DELADDR	0xd	/* address being removed from iface */
-#define RTM_IFINFO	0xe	/* iface going up/down etc. */
-#define	RTM_NEWMADDR	0xf	/* mcast group membership being added to if */
-#define	RTM_DELMADDR	0x10	/* mcast group membership being deleted */
-#define	RTM_IFANNOUNCE	0x11	/* iface arrival/departure */
-#define	RTM_IEEE80211	0x12	/* IEEE80211 wireless event */
+#define	RTM_RESOLVE	0xb	/* (1) req to resolve dst to LL addr */
+#define	RTM_NEWADDR	0xc	/* (2) address being added to iface */
+#define	RTM_DELADDR	0xd	/* (2) address being removed from iface */
+#define	RTM_IFINFO	0xe	/* (3) iface going up/down etc. */
+#define	RTM_NEWMADDR	0xf	/* (4) mcast group membership being added to if */
+#define	RTM_DELMADDR	0x10	/* (4) mcast group membership being deleted */
+#define	RTM_IFANNOUNCE	0x11	/* (5) iface arrival/departure */
+#define	RTM_IEEE80211	0x12	/* (5) IEEE80211 wireless event */
 
 /*
  * Bitmask values for rtm_inits and rmx_locks.
@@ -342,11 +352,10 @@ struct rt_addrinfo {
  * This macro returns the size of a struct sockaddr when passed
  * through a routing socket. Basically we round up sa_len to
  * a multiple of sizeof(long), with a minimum of sizeof(long).
- * The check for a NULL pointer is just a convenience, probably never used.
  * The case sa_len == 0 should only apply to empty structures.
  */
 #define SA_SIZE(sa)						\
-    (  (!(sa) || ((struct sockaddr *)(sa))->sa_len == 0) ?	\
+    (  (((struct sockaddr *)(sa))->sa_len == 0) ?		\
 	sizeof(long)		:				\
 	1 + ( (((struct sockaddr *)(sa))->sa_len - 1) | (sizeof(long) - 1) ) )
 

usr.bin/netstat/route.c

@@ -360,9 +360,10 @@ p_rtentry_sysctl(const char *name, struct rt_msghdr *rtm)
 	xo_open_instance(name);
 	sa = (struct sockaddr *)(rtm + 1);
 	for (i = 0; i < RTAX_MAX; i++) {
-		if (rtm->rtm_addrs & (1 << i))
+		if (rtm->rtm_addrs & (1 << i)) {
 			addr[i] = sa;
-		sa = (struct sockaddr *)((char *)sa + SA_SIZE(sa));
+			sa = (struct sockaddr *)((char *)sa + SA_SIZE(sa));
+		}
 	}
 
 	protrusion = p_sockaddr("destination", addr[RTAX_DST],

usr.sbin/arp/arp.c

@@ -404,7 +404,7 @@ set(int argc, char **argv)
 	 * the prefix route covering the local end of the
 	 * PPP link should be returned, on which ARP applies.
 	 */
-	rtm = rtmsg(RTM_GET, dst, &sdl_m);
+	rtm = rtmsg(RTM_GET, dst, NULL);
 	if (rtm == NULL) {
 		xo_warn("%s", host);
 		return (1);
@@ -466,7 +466,6 @@ delete(char *host)
 	struct sockaddr_in *addr, *dst;
 	struct rt_msghdr *rtm;
 	struct sockaddr_dl *sdl;
-	struct sockaddr_dl sdl_m;
 
 	dst = getaddr(host);
 	if (dst == NULL)
@@ -477,17 +476,8 @@ delete(char *host)
 	 */
 	flags &= ~RTF_ANNOUNCE;
 
-	/*
-	 * setup the data structure to notify the kernel
-	 * it is the ARP entry the RTM_GET is interested
-	 * in
-	 */
-	bzero(&sdl_m, sizeof(sdl_m));
-	sdl_m.sdl_len = sizeof(sdl_m);
-	sdl_m.sdl_family = AF_LINK;
-
 	for (;;) {	/* try twice */
-		rtm = rtmsg(RTM_GET, dst, &sdl_m);
+		rtm = rtmsg(RTM_GET, dst, NULL);
 		if (rtm == NULL) {
 			xo_warn("%s", host);
 			return (1);
@@ -511,7 +501,7 @@ delete(char *host)
 		}
 
 		/*
-		 * Regualar entry delete failed, now check if there
+		 * Regular entry delete failed, now check if there
 		 * is a proxy-arp entry to remove.
 		 */
 		if (flags & RTF_ANNOUNCE) {
@@ -815,7 +805,8 @@ rtmsg(int cmd, struct sockaddr_in *dst, struct sockaddr_dl *sdl)
 	}
 	do {
 		l = read(s, (char *)&m_rtmsg, sizeof(m_rtmsg));
-	} while (l > 0 && (rtm->rtm_seq != seq || rtm->rtm_pid != pid));
+	} while (l > 0 && (rtm->rtm_type != cmd || rtm->rtm_seq != seq ||
+	    rtm->rtm_pid != pid));
 	if (l < 0)
 		xo_warn("read from routing socket");
 	return (rtm);

usr.sbin/ndp/ndp.c

@@ -888,7 +888,8 @@ rtmsg(int cmd)
 	}
 	do {
 		l = read(s, (char *)&m_rtmsg, sizeof(m_rtmsg));
-	} while (l > 0 && (rtm->rtm_seq != seq || rtm->rtm_pid != pid));
+	} while (l > 0 && (rtm->rtm_type != cmd || rtm->rtm_seq != seq ||
+	    rtm->rtm_pid != pid));
 	if (l < 0)
 		(void) fprintf(stderr, "ndp: read from routing socket: %s\n",
 		    strerror(errno));

usr.sbin/rarpd/rarpd.c

@@ -755,7 +755,8 @@ update_arptab(u_char *ep, in_addr_t ipaddr)
 	}
 	do {
 		cc = read(r, rt, sizeof(rtmsg));
-	} while (cc > 0 && (rt->rtm_seq != seq || rt->rtm_pid != pid));
+	} while (cc > 0 && (rt->rtm_type != RTM_GET || rt->rtm_seq != seq ||
+	    rt->rtm_pid != pid));
 	if (cc == -1) {
 		logmsg(LOG_ERR, "rtmsg get read: %m");
 		close(r);
@@ -803,7 +804,8 @@ update_arptab(u_char *ep, in_addr_t ipaddr)
 	}
 	do {
 		cc = read(r, rt, sizeof(rtmsg));
-	} while (cc > 0 && (rt->rtm_seq != seq || rt->rtm_pid != pid));
+	} while (cc > 0 && (rt->rtm_type != RTM_ADD || rt->rtm_seq != seq ||
+	    rt->rtm_pid != pid));
 	close(r);
 	if (cc == -1) {
 		logmsg(LOG_ERR, "rtmsg add read: %m");

usr.sbin/route6d/route6d.c

@@ -1768,14 +1768,23 @@ rtrecv(void)
 			break;
 		default:
 			rtm = (struct rt_msghdr *)(void *)p;
-			addrs = rtm->rtm_addrs;
-			q = (char *)(rtm + 1);
 			if (rtm->rtm_version != RTM_VERSION) {
 				trace(1, "unexpected rtmsg version %d "
 					"(should be %d)\n",
 					rtm->rtm_version, RTM_VERSION);
 				continue;
 			}
+			/*
+			 * Only messages that use the struct rt_msghdr
+			 * format are allowed beyond this point.
+			 */
+			if (rtm->rtm_type > RTM_RESOLVE) {
+				trace(1, "rtmsg type %d ignored\n",
+					rtm->rtm_type);
+				continue;
+			}
+			addrs = rtm->rtm_addrs;
+			q = (char *)(rtm + 1);
 			if (rtm->rtm_pid == pid) {
 #if 0
 				trace(1, "rtmsg looped back to me, ignored\n");
@@ -2973,7 +2982,8 @@ getroute(struct netinfo6 *np, struct in6_addr *gw)
 			exit(1);
 		}
 		rtm = (struct rt_msghdr *)(void *)buf;
-	} while (rtm->rtm_seq != myseq || rtm->rtm_pid != pid);
+	} while (rtm->rtm_type != RTM_GET || rtm->rtm_seq != myseq ||
+	    rtm->rtm_pid != pid);
 	sin6 = (struct sockaddr_in6 *)(void *)&buf[sizeof(struct rt_msghdr)];
 	if (rtm->rtm_addrs & RTA_DST) {
 		sin6 = (struct sockaddr_in6 *)(void *)