bin/151664: [PATCH] sbin/route/route.c: Incorrect array bounds checking

[Gleb Smirnoff]

The following reply was made to PR bin/151664; it has been noted by GNATS.

From: Gleb Smirnoff <glebius at FreeBSD.org>
To: Alexey Illarionov <littlesavage at rambler.ru>
Cc: freebsd-gnats-submit at FreeBSD.org
Subject: Re: bin/151664: [PATCH] sbin/route/route.c: Incorrect array bounds
 checking
Date: Mon, 25 Oct 2010 18:12:06 +0400

 --G6nVm6DDWH/FONJq
 Content-Type: text/plain; charset=koi8-r
 Content-Disposition: inline
 
   Hello!
 
   Another patch, that also checks msglen even deeper down, when printing sockaddrs.
 
 -- 
 Totus tuus, Glebius.
 
 --G6nVm6DDWH/FONJq
 Content-Type: text/x-diff; charset=koi8-r
 Content-Disposition: attachment; filename="route.c.diff"
 
 Index: route.c
 ===================================================================
 --- route.c	(revision 213832)
 +++ route.c	(working copy)
 @@ -115,11 +115,11 @@
  static void	monitor(void);
  static const char	*netname(struct sockaddr *);
  static void	newroute(int, char **);
 -static void	pmsg_addrs(char *, int);
 -static void	pmsg_common(struct rt_msghdr *);
 +static void	pmsg_addrs(char *, int, size_t);
 +static void	pmsg_common(struct rt_msghdr *, size_t);
  static int	prefixlen(const char *);
  static void	print_getmsg(struct rt_msghdr *, int);
 -static void	print_rtmsg(struct rt_msghdr *, int);
 +static void	print_rtmsg(struct rt_msghdr *, size_t);
  static const char	*routename(struct sockaddr *);
  static int	rtmsg(int, int);
  static void	set_metric(char *, int);
 @@ -1306,7 +1306,7 @@
  	"RTM_NEWMADDR: new multicast group membership on iface",
  	"RTM_DELMADDR: multicast group membership removed from iface",
  	"RTM_IFANNOUNCE: interface arrival/departure",
 -	0,
 +	"RTM_IEEE80211: IEEE 802.11 wireless event",
  };
  
  char metricnames[] =
 @@ -1325,7 +1325,7 @@
  "\1DST\2GATEWAY\3NETMASK\4GENMASK\5IFP\6IFA\7AUTHOR\010BRD";
  
  static void
 -print_rtmsg(struct rt_msghdr *rtm, int msglen __unused)
 +print_rtmsg(struct rt_msghdr *rtm, size_t msglen)
  {
  	struct if_msghdr *ifm;
  	struct ifa_msghdr *ifam;
 @@ -1342,13 +1342,22 @@
  		    rtm->rtm_version);
  		return;
  	}
 -	if (msgtypes[rtm->rtm_type] != NULL)
 +	if (rtm->rtm_type < sizeof(msgtypes)/sizeof(msgtypes[0]))
  		(void)printf("%s: ", msgtypes[rtm->rtm_type]);
  	else
  		(void)printf("#%d: ", rtm->rtm_type);
  	(void)printf("len %d, ", rtm->rtm_msglen);
 +
 +#define	REQUIRE(x)	do {		\
 +	if (msglen < sizeof(x))		\
 +		goto badlen;		\
 +	else				\
 +		msglen -= sizeof(x);	\
 +	} while (0)
 +
  	switch (rtm->rtm_type) {
  	case RTM_IFINFO:
 +		REQUIRE(struct if_msghdr);
  		ifm = (struct if_msghdr *)rtm;
  		(void) printf("if# %d, ", ifm->ifm_index);
  		switch (ifm->ifm_data.ifi_link_state) {
 @@ -1364,23 +1373,26 @@
  		}
  		(void) printf("link: %s, flags:", state);
  		bprintf(stdout, ifm->ifm_flags, ifnetflags);
 -		pmsg_addrs((char *)(ifm + 1), ifm->ifm_addrs);
 +		pmsg_addrs((char *)(ifm + 1), ifm->ifm_addrs, msglen);
  		break;
  	case RTM_NEWADDR:
  	case RTM_DELADDR:
 +		REQUIRE(struct ifa_msghdr);
  		ifam = (struct ifa_msghdr *)rtm;
  		(void) printf("metric %d, flags:", ifam->ifam_metric);
  		bprintf(stdout, ifam->ifam_flags, routeflags);
 -		pmsg_addrs((char *)(ifam + 1), ifam->ifam_addrs);
 +		pmsg_addrs((char *)(ifam + 1), ifam->ifam_addrs, msglen);
  		break;
  #ifdef RTM_NEWMADDR
  	case RTM_NEWMADDR:
  	case RTM_DELMADDR:
 +		REQUIRE(struct ifma_msghdr);
  		ifmam = (struct ifma_msghdr *)rtm;
 -		pmsg_addrs((char *)(ifmam + 1), ifmam->ifmam_addrs);
 +		pmsg_addrs((char *)(ifmam + 1), ifmam->ifmam_addrs, msglen);
  		break;
  #endif
  	case RTM_IFANNOUNCE:
 +		REQUIRE(struct if_announcemsghdr);
  		ifan = (struct if_announcemsghdr *)rtm;
  		(void) printf("if# %d, what: ", ifan->ifan_index);
  		switch (ifan->ifan_what) {
 @@ -1401,8 +1413,13 @@
  		(void) printf("pid: %ld, seq %d, errno %d, flags:",
  			(long)rtm->rtm_pid, rtm->rtm_seq, rtm->rtm_errno);
  		bprintf(stdout, rtm->rtm_flags, routeflags);
 -		pmsg_common(rtm);
 +		pmsg_common(rtm, msglen);
  	}
 +
 +	return;
 +
 +badlen:
 +	(void)printf("bad message length %u\n", msglen);
  }
  
  static void
 @@ -1490,7 +1507,7 @@
  #undef msec
  #define	RTA_IGN	(RTA_DST|RTA_GATEWAY|RTA_NETMASK|RTA_IFP|RTA_IFA|RTA_BRD)
  	if (verbose)
 -		pmsg_common(rtm);
 +		pmsg_common(rtm, msglen);
  	else if (rtm->rtm_addrs &~ RTA_IGN) {
  		(void) printf("sockaddrs: ");
  		bprintf(stdout, rtm->rtm_addrs, addrnames);
 @@ -1500,17 +1517,21 @@
  }
  
  static void
 -pmsg_common(struct rt_msghdr *rtm)
 +pmsg_common(struct rt_msghdr *rtm, size_t msglen)
  {
  	(void) printf("\nlocks: ");
  	bprintf(stdout, rtm->rtm_rmx.rmx_locks, metricnames);
  	(void) printf(" inits: ");
  	bprintf(stdout, rtm->rtm_inits, metricnames);
 -	pmsg_addrs(((char *)(rtm + 1)), rtm->rtm_addrs);
 +	if (msglen > sizeof(struct rt_msghdr))
 +		pmsg_addrs(((char *)(rtm + 1)), rtm->rtm_addrs,
 +		    msglen - sizeof(struct rt_msghdr));
 +	else
 +		(void) fflush(stdout);
  }
  
  static void
 -pmsg_addrs(char *cp, int addrs)
 +pmsg_addrs(char *cp, int addrs, size_t len)
  {
  	struct sockaddr *sa;
  	int i;
 @@ -1524,8 +1545,17 @@
  	(void) putchar('\n');
  	for (i = 1; i != 0; i <<= 1)
  		if (i & addrs) {
 +			if (len < sizeof(struct sockaddr)) {
 +				(void) printf("bad message length\n");
 +				break;
 +			}
  			sa = (struct sockaddr *)cp;
  			(void) printf(" %s", routename(sa));
 +			if (len < SA_SIZE(sa)) {
 +				(void) printf("bad message length\n");
 +				break;
 +			}
 +			len -= SA_SIZE(sa);
  			cp += SA_SIZE(sa);
  		}
  	(void) putchar('\n');
 
 --G6nVm6DDWH/FONJq--