Subject: insufficient validation in ip6_setpktoptions?
To: None <tech-net@netbsd.org>
From: Greg Troxel <gdt@ir.bbn.com>
List: tech-net
Date: 08/12/2004 10:51:53
I just got quagga to do v6 router advertisements on NetBSD/sparc64; it
had been not working due to assuming that padding in control message
structures was only to 4 bytes (i386, vs 8 on sparc and sparc64).
In the process of getting to what's in revision 1.12 of
quagga-cvs:zebra/rtadv.c (much like usr.sbin/rtadvd/rtadvd.c in
behavior), I crashed a NetBSD/sparc64 1.6.2_RC3 box.

Here is the diff from the crashy version to the working version.

-  msg.msg_controllen = CMSG_LEN(sizeof(struct in6_pktinfo));
+  msg.msg_controllen = CMSG_SPACE(sizeof(struct in6_pktinfo));

and the working code:

  msg.msg_name = (void *) &addr;
  msg.msg_namelen = sizeof (struct sockaddr_in6);
  msg.msg_iov = &iov;
  msg.msg_iovlen = 1;
  msg.msg_control = (void *) adata;
  msg.msg_controllen = CMSG_SPACE(sizeof(struct in6_pktinfo));
  msg.msg_flags = 0;
  iov.iov_base = buf;
  iov.iov_len = len;

  cmsgptr = CMSG_FIRSTHDR(&msg);
  cmsgptr->cmsg_len = CMSG_LEN(sizeof(struct in6_pktinfo));
  cmsgptr->cmsg_level = IPPROTO_IPV6;
  cmsgptr->cmsg_type = IPV6_PKTINFO;

  pkt = (struct in6_pktinfo *) CMSG_DATA (cmsgptr);
  memset (&pkt->ipi6_addr, 0, sizeof (struct in6_addr));
  pkt->ipi6_ifindex = ifp->ifindex;


It wasn't clear to me why the control message had to be padded if
there was nothing following it.  In (currentish)
netinet6/ip6_output.c:ip6_setpktoptions(), I see:

	for (; control->m_len; control->m_data += CMSG_ALIGN(cm->cmsg_len),
	    control->m_len -= CMSG_ALIGN(cm->cmsg_len)) {
		cm = mtod(control, struct cmsghdr *);
		if (cm->cmsg_len == 0 || cm->cmsg_len > control->m_len)
			return (EINVAL);
		if (cm->cmsg_level != IPPROTO_IPV6)
			continue;

Here I think I see two problems.  One is that there is no check that
control->m_len is big enough to contain an entire struct cmsghdr, but
the pointer is used.  The second is that the length check is the
cm->cmsg_len fits in the remaining mbuf, but then the pointer is
advanced by CMSG_ALIGN(m->m_len).

So I think 

		if (control->m_len < sizeof(struct cmsghdr))
			return (EINVAL);

is needed before the mtod, and then the next line should be changed to:

		if (cm->cmsg_len == 0
		    || CMSG_ALIGN(cm->cmsg_len) > control->m_len)

but I'm really not familiar enough with this code to be sure I'm not
missing something.