Date: Tue, 17 Apr 2007 09:35:53 +0300 From: Mike Makonnen <mtm@FreeBSD.Org> To: Max Laier <max@love2party.net> Cc: freebsd-net@freebsd.org Subject: Re: ping6 extension headers bounds checking Message-ID: <20070417063553.GA2053@rogue.navcom.lan> In-Reply-To: <200704162226.08931.max@love2party.net> References: <20070416101609.GA2137@rogue.navcom.lan> <200704162226.08931.max @love2party.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--LZvS9be/3tNcYl/X Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Apr 16, 2007 at 10:25:59PM +0200, Max Laier wrote: > > I think it'd be better to supply the print functions with the rest of the > bufferlen instead of an offset. This way only the caller has to know the > size of the buffer - Ok, Done. See attached patch. >btw, do we get a result back i.e. how much buffer > was used. In addition you could check if the offset in the for-loop of > the caller is within bounds, before even attempting to call further. On the first part: no and yes :). The cmsg structure tells you the length, including the cmsg header, which is fine if the buffer is big enough. If the buffer is too small, the stated length in the cmsg structure stays the same but MSG_CTRUNC is appended to msg_flags to tell you that dome data was discarded at the end because it didn't fit in the buffer. On the second part: Done. see attached patch. 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 --LZvS9be/3tNcYl/X 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 -r1.29 ping6.c --- sbin/ping6/ping6.c 10 Feb 2005 09:19:32 -0000 1.29 +++ sbin/ping6/ping6.c 17 Apr 2007 06:07:53 -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,33 @@ pr_exthdrs(mhdr) struct msghdr *mhdr; { - struct cmsghdr *cm; + struct cmsghdr *cm, *cm1; + int bufsize; - for (cm = (struct cmsghdr *)CMSG_FIRSTHDR(mhdr); cm; - cm = (struct cmsghdr *)CMSG_NXTHDR(mhdr, cm)) { + bufsize = 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; + bufsize = CONTROLLEN - ((caddr_t)CMSG_DATA(cm) - (caddr_t)cm1); + if (bufsize <= 0) + return; switch (cm->cmsg_type) { case IPV6_HOPOPTS: printf(" HbH Options: "); - pr_ip6opt(CMSG_DATA(cm)); + pr_ip6opt(CMSG_DATA(cm), (unsigned int)bufsize); break; case IPV6_DSTOPTS: #ifdef IPV6_RTHDRDSTOPTS case IPV6_RTHDRDSTOPTS: #endif printf(" Dst Options: "); - pr_ip6opt(CMSG_DATA(cm)); + pr_ip6opt(CMSG_DATA(cm), (unsigned int)bufsize); break; case IPV6_RTHDR: printf(" Routing: "); - pr_rthdr(CMSG_DATA(cm)); + pr_rthdr(CMSG_DATA(cm), (unsigned int)bufsize); break; } } @@ -1764,12 +1777,12 @@ #ifdef USE_RFC2292BIS void -pr_ip6opt(void *extbuf) +pr_ip6opt(void *extbuf, unsigned int bufsize) { 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 +1793,18 @@ 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: + * subtract the size of a cmsg structure from the buffer size. + */ + if (bufsize < (extlen + CMSG_SPACE(0))) { + origextlen = extlen; + extlen = bufsize - CMSG_SPACE(0); + 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 +1841,7 @@ #else /* !USE_RFC2292BIS */ /* ARGSUSED */ void -pr_ip6opt(void *extbuf) +pr_ip6opt(void *extbuf, unsigned int bufsize __unused) { putchar('\n'); return; @@ -1825,21 +1850,43 @@ #ifdef USE_RFC2292BIS void -pr_rthdr(void *extbuf) +pr_rthdr(void *extbuf, unsigned int bufsize) { 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 size of the cmsg structure + * - The size of one segment (the size of one Type 0 route header) + * - 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 (bufsize < (rthsize + CMSG_SPACE(0))) { + origsegs = segments; + size0 = inet6_rth_space(IPV6_RTHDR_TYPE_0, 0); + size1 = inet6_rth_space(IPV6_RTHDR_TYPE_0, 1); + segments -= (rthsize - (bufsize - 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 +1907,7 @@ #else /* !USE_RFC2292BIS */ /* ARGSUSED */ void -pr_rthdr(void *extbuf) +pr_rthdr(void *extbuf, unsigned int bufsize __unused) { putchar('\n'); return; --LZvS9be/3tNcYl/X--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070417063553.GA2053>