Date: Mon, 25 Oct 2010 14:20:12 GMT From: Gleb Smirnoff <glebius@FreeBSD.org> To: freebsd-bugs@FreeBSD.org Subject: Re: bin/151664: [PATCH] sbin/route/route.c: Incorrect array bounds checking Message-ID: <201010251420.o9PEKCsM091240@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/151664; it has been noted by GNATS.
From: Gleb Smirnoff <glebius@FreeBSD.org>
To: Alexey Illarionov <littlesavage@rambler.ru>
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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201010251420.o9PEKCsM091240>