From owner-freebsd-bugs@FreeBSD.ORG Mon Oct 25 14:20:12 2010 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 861341065675 for ; Mon, 25 Oct 2010 14:20:12 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 599D98FC12 for ; Mon, 25 Oct 2010 14:20:12 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id o9PEKCCn091241 for ; Mon, 25 Oct 2010 14:20:12 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.4/8.14.4/Submit) id o9PEKCsM091240; Mon, 25 Oct 2010 14:20:12 GMT (envelope-from gnats) Date: Mon, 25 Oct 2010 14:20:12 GMT Message-Id: <201010251420.o9PEKCsM091240@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Gleb Smirnoff Cc: Subject: Re: bin/151664: [PATCH] sbin/route/route.c: Incorrect array bounds checking X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Gleb Smirnoff List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Oct 2010 14:20:12 -0000 The following reply was made to PR bin/151664; it has been noted by GNATS. From: Gleb Smirnoff To: Alexey Illarionov Cc: freebsd-gnats-submit@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--