From owner-freebsd-bugs@FreeBSD.ORG Fri Apr 6 10:00:25 2007 Return-Path: X-Original-To: freebsd-bugs@hub.freebsd.org Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 82FBF16A404 for ; Fri, 6 Apr 2007 10:00:25 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [69.147.83.40]) by mx1.freebsd.org (Postfix) with ESMTP id 6FE2A13C44B for ; Fri, 6 Apr 2007 10:00:25 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.4/8.13.4) with ESMTP id l36A0PKI030306 for ; Fri, 6 Apr 2007 10:00:25 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.4/8.13.4/Submit) id l36A0PfO030305; Fri, 6 Apr 2007 10:00:25 GMT (envelope-from gnats) Date: Fri, 6 Apr 2007 10:00:25 GMT Message-Id: <200704061000.l36A0PfO030305@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Mike Makonnen Cc: Subject: kern/99425: [ipv6] ping6 memory leak due to IPV6_RTHDR setsockopt b ehavior and inet6_rth_* functions. X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Mike Makonnen List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Apr 2007 10:00:25 -0000 The following reply was made to PR kern/99425; it has been noted by GNATS. From: Mike Makonnen To: bug-followup@FreeBSD.org, clemun@gmail.com Cc: ume@freebsd.org, gnn@freebsd.org Subject: kern/99425: [ipv6] ping6 memory leak due to IPV6_RTHDR setsockopt b ehavior and inet6_rth_* functions. Date: Fri, 6 Apr 2007 13:02:50 +0300 --9amGYk9869ThD9tj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline [cc'ing our resident IPv6 gurus] Hi, Ok, it seems the problem is that there are 2 problems here: 1. The static buffer that ping6(8) uses to hold the control data it gets from recvmsg(2) is too small. 2. When it outputs the extra header information it doesn't confirm that its static buffer is indeed big enough to hold all the data that the headers tell it is there. The attached patch solves both problems. First, the buffer to hold the control data is increased to 10240 bytes because RFC3542 section 20.1 says that the buffer to hold the control data must be a minimum of 10420 bytes. Second, pr_ip6opt() and pr_rthdr() are modified to check that the size of the buffer, allocated in main(), to hold the control data is big enough to hold the data that the headers say is included. If it is NOT big enough then they make sure not to read outside of the allocated buffer. Third, a warning is printed if the buffer is not big enough to hold all the data (by checking if recvmsg(8) has appened MSG_CTRUNC to the msghdr flags). Also, as stated in the PR the inet6_rth_* functions do NOT check that the number of segments is within the limits set by RFC3542, but that is a separate issue and will be fixed separately. There is no problem when sending because the buffer to hold the control data is dynamically allocated to the correct size. It also correctly checks the return value of inet6_rth_space() to make sure there are no problems with routing header type or number. The problem is that inet6_rth_space() doesn't do the proper checking. But, as I stated in the previous paragraph that's a separate issue and I'll fix it in a separate commit (I'm working on the patch now). Please apply the attached patch. Let me know if it doesn't behave as advertised :-) Cheers. -- Mike Makonnen | GPG-KEY: http://people.freebsd.org/~mtm/mtm.asc mmakonnen @ gmail.com | AC7B 5672 2D11 F4D0 EBF8 5279 5359 2B82 7CD4 1F55 mtm @ FreeBSD.Org | FreeBSD - http://www.freebsd.org --9amGYk9869ThD9tj Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="ping6.diff" Index: sbin/ping6/ping6.c =================================================================== RCS file: /home/ncvs/src/sbin/ping6/ping6.c,v retrieving revision 1.29 diff -u -u -r1.29 ping6.c --- sbin/ping6/ping6.c 10 Feb 2005 09:19:32 -0000 1.29 +++ sbin/ping6/ping6.c 5 Apr 2007 22:32:52 -0000 @@ -150,6 +150,7 @@ #define ICMP6ECHOLEN 8 /* icmp echo header len excluding time */ #define ICMP6ECHOTMLEN sizeof(struct tv32) #define ICMP6_NIQLEN (ICMP6ECHOLEN + 8) +# define CONTROLLEN 10240 /* ancillary data buffer size RFC3542 20.1 */ /* FQDN case, 64 bits of nonce + 32 bits ttl */ #define ICMP6_NIRLEN (ICMP6ECHOLEN + 12) #define EXTRA 256 /* for AH and various other headers. weird. */ @@ -269,8 +270,8 @@ char *, size_t); void pr_pack(u_char *, int, struct msghdr *); void pr_exthdrs(struct msghdr *); -void pr_ip6opt(void *); -void pr_rthdr(void *); +void pr_ip6opt(void *, unsigned int); +void pr_rthdr(void *, unsigned int); int pr_bitrange(u_int32_t, int, int); void pr_retip(struct ip6_hdr *, u_char *); void summary(void); @@ -307,6 +308,7 @@ char *e, *target, *ifname = NULL, *gateway = NULL; int ip6optlen = 0; struct cmsghdr *scmsgp = NULL; + struct cmsghdr *cm; #if defined(SO_SNDBUF) && defined(SO_RCVBUF) u_long lsockbufsize; int sockbufsize = 0; @@ -1057,10 +1059,13 @@ seeninfo = 0; #endif + /* For control (ancillary) data received from recvmsg() */ + cm = (struct cmsghdr *)malloc(CONTROLLEN); + if (cm == NULL) + err(1, "malloc"); + for (;;) { struct msghdr m; - struct cmsghdr *cm; - u_char buf[1024]; struct iovec iov[2]; /* signal handling */ @@ -1127,9 +1132,9 @@ iov[0].iov_len = packlen; m.msg_iov = iov; m.msg_iovlen = 1; - cm = (struct cmsghdr *)buf; - m.msg_control = (caddr_t)buf; - m.msg_controllen = sizeof(buf); + memset(cm, 0, CONTROLLEN); + m.msg_control = (void *)cm; + m.msg_controllen = CONTROLLEN; cc = recvmsg(s, &m, 0); if (cc < 0) { @@ -1493,6 +1498,9 @@ pr_addr(from, fromlen)); return; } + if (((mhdr->msg_flags & MSG_CTRUNC) != 0) && + (options & F_VERBOSE) != 0) + warnx("some control data discarded, insufficient buffer size"); icp = (struct icmp6_hdr *)buf; ni = (struct icmp6_nodeinfo *)buf; off = 0; @@ -1735,28 +1743,31 @@ pr_exthdrs(mhdr) struct msghdr *mhdr; { - struct cmsghdr *cm; + struct cmsghdr *cm, *cm1; + unsigned int offset; - for (cm = (struct cmsghdr *)CMSG_FIRSTHDR(mhdr); cm; - cm = (struct cmsghdr *)CMSG_NXTHDR(mhdr, cm)) { + offset = 0; + cm1 = (struct cmsghdr *)CMSG_FIRSTHDR(mhdr); + for (cm = cm1; cm; cm = (struct cmsghdr *)CMSG_NXTHDR(mhdr, cm)) { if (cm->cmsg_level != IPPROTO_IPV6) continue; + offset = (caddr_t)CMSG_DATA(cm) - (caddr_t)cm1; switch (cm->cmsg_type) { case IPV6_HOPOPTS: printf(" HbH Options: "); - pr_ip6opt(CMSG_DATA(cm)); + pr_ip6opt(CMSG_DATA(cm), offset); break; case IPV6_DSTOPTS: #ifdef IPV6_RTHDRDSTOPTS case IPV6_RTHDRDSTOPTS: #endif printf(" Dst Options: "); - pr_ip6opt(CMSG_DATA(cm)); + pr_ip6opt(CMSG_DATA(cm), offset); break; case IPV6_RTHDR: printf(" Routing: "); - pr_rthdr(CMSG_DATA(cm)); + pr_rthdr(CMSG_DATA(cm), offset); break; } } @@ -1764,12 +1775,12 @@ #ifdef USE_RFC2292BIS void -pr_ip6opt(void *extbuf) +pr_ip6opt(void *extbuf, unsigned int cmoffset) { struct ip6_hbh *ext; int currentlen; u_int8_t type; - socklen_t extlen, len; + socklen_t extlen, len, origextlen; void *databuf; size_t offset; u_int16_t value2; @@ -1780,6 +1791,21 @@ printf("nxt %u, len %u (%lu bytes)\n", ext->ip6h_nxt, (unsigned int)ext->ip6h_len, (unsigned long)extlen); + /* + * Bounds checking on the ancillary data buffer. When calculating + * the number of items to show keep in mind: + * - The offset, in the ancillary data buffer, of this header + * - The size of the cmsg structure + * - The size of one unit (8 octets) + */ + if (CONTROLLEN < (extlen + CMSG_SPACE(0) + offset)) { + origextlen = extlen; + extlen -= (extlen - (CONTROLLEN - offset - CMSG_SPACE(0))) / 8; + warnx("options truncated, showing only %u (total=%u)", + (unsigned int)(extlen / 8 - 1), + (unsigned int)(ext->ip6h_len)); + } + currentlen = 0; while (1) { currentlen = inet6_opt_next(extbuf, extlen, currentlen, @@ -1816,7 +1842,7 @@ #else /* !USE_RFC2292BIS */ /* ARGSUSED */ void -pr_ip6opt(void *extbuf) +pr_ip6opt(void *extbuf, unsigned int cmoffset __unused) { putchar('\n'); return; @@ -1825,21 +1851,44 @@ #ifdef USE_RFC2292BIS void -pr_rthdr(void *extbuf) +pr_rthdr(void *extbuf, unsigned int offset) { struct in6_addr *in6; char ntopbuf[INET6_ADDRSTRLEN]; struct ip6_rthdr *rh = (struct ip6_rthdr *)extbuf; - int i, segments; + int i, segments, origsegs, rthsize, size0, size1; /* print fixed part of the header */ printf("nxt %u, len %u (%d bytes), type %u, ", rh->ip6r_nxt, rh->ip6r_len, (rh->ip6r_len + 1) << 3, rh->ip6r_type); - if ((segments = inet6_rth_segments(extbuf)) >= 0) + if ((segments = inet6_rth_segments(extbuf)) >= 0) { printf("%d segments, ", segments); - else + printf("%d left\n", rh->ip6r_segleft); + } else { printf("segments unknown, "); - printf("%d left\n", rh->ip6r_segleft); + printf("%d left\n", rh->ip6r_segleft); + return; + } + + /* + * Bounds checking on the ancillary data buffer. When calculating + * the number of items to show keep in mind: + * - The offset, in the ancillary data buffer, of this header + * - The size of the cmsg structure + * - The size of one segment (the size of one IPv6 address) + * - When dividing add a fudge factor of one in case the + * dividend is not evenly divisible by the divisor + */ + rthsize = (rh->ip6r_len + 1) * 8; + if (CONTROLLEN < (rthsize + CMSG_SPACE(0) + offset)) { + origsegs = segments; + size0 = inet6_rth_space(IPV6_RTHDR_TYPE_0, 0); + size1 = inet6_rth_space(IPV6_RTHDR_TYPE_0, 1); + segments -= (rthsize - (CONTROLLEN - offset - CMSG_SPACE(0))) / + (size1 - size0) + 1; + warnx("segments truncated, showing only %d (total=%d)", + segments, origsegs); + } for (i = 0; i < segments; i++) { in6 = inet6_rth_getaddr(extbuf, i); @@ -1860,7 +1909,7 @@ #else /* !USE_RFC2292BIS */ /* ARGSUSED */ void -pr_rthdr(void *extbuf) +pr_rthdr(void *extbuf, unsigned int offset __unused) { putchar('\n'); return; --9amGYk9869ThD9tj--