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.