NetBSD Problem Report #29199

From gdt@ir.bbn.com  Wed Feb  2 15:11:31 2005
Return-Path: <gdt@ir.bbn.com>
Received: from fnord.ir.bbn.com (fnord.ir.bbn.com [192.1.100.210])
	by narn.netbsd.org (Postfix) with ESMTP id 545F163B400
	for <gnats-bugs@gnats.NetBSD.org>; Wed,  2 Feb 2005 15:11:30 +0000 (UTC)
Message-Id: <20050202151118.E8C3C1F6F@fnord.ir.bbn.com>
Date: Wed,  2 Feb 2005 10:11:18 -0500 (EST)
From: gdt@ir.bbn.com
Reply-To: gdt@ir.bbn.com
To: gnats-bugs@netbsd.org
Subject: msghdr parsing incorrect in kernel for v6 options
X-Send-Pr-Version: 3.95

>Number:         29199
>Category:       kern
>Synopsis:       msghdr parsing incorrect in kernel for v6 options
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Feb 02 15:12:00 +0000 2005
>Originator:     Greg Troxel
>Release:        NetBSD 2.0_RC4
>Organization:
        Greg Troxel <gdt@ir.bbn.com>
>Environment:


System: NetBSD fnord.ir.bbn.com 2.0_RC4 NetBSD 2.0_RC4 (SINEW) #44: Mon Jan 10 19:54:05 EST 2005 root@call-cc.ir.bbn.com:/n0/obj/sinew/gdt/i386/sys/arch/i386/compile/SINEW i386
Architecture: i386
Machine: i386
>Description:
In netinet/ip6_output:ip6_setpktoptions, an mbuf with control data is
looped over to parse all the options.  However, the code assumes that
the mbuf is sized appropriately for the data, and can crash the system
if bad data is passed by the user.

	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);

There are two problems here.  One is that no check is made to prevent
control->m_len from going negative, since it is quite possible that
the mbuf is only CMSG_LEN rather than CMSG_SPACE for the data.  The
second is that it is not verified that the entire header is present
before the mtod and dereferencing of cm.

The code does check for cm->cmsg_len > control->m_len, but it's
possible for that to fail but still have CMSG_ALIGN(cm->cmsg_len) be
greater, for example if cmsg_len is 20 on sparc.

>How-To-Repeat:

While debugging quagga's sending of router advertisements, I caused
crashes on both sparc and sparc64, I think 1.6.2.  My memory is
slightly fuzzy and I hvae lost the kernel patch, but the problem was
sending a control message with the length being CMSG_LEN.  I then
changed to use SPACE, as rtadvd does.

Inspect source code, and see that the above conditions are possible.

>Fix:

(not tested, from memory)

Add at start of for body:

if (cm != NULL && control->m_data == CMSG_LEN(cm->cmsg_len))
	/* Control message exactly filled mbuf. */
	break;
if (control->m_len < sizeof(struct cmsghdr))
	/* Junk, other than proper padding, at end of control message. */
	return (EINVAL);

Or, move iteration step to end of for, and check for exact match and
overrun there.

>Unformatted:

Home	PR Database Search