From owner-p4-projects@FreeBSD.ORG Thu May 29 20:52:08 2003 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id DA22B37B405; Thu, 29 May 2003 20:52:07 -0700 (PDT) Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5804137B401; Thu, 29 May 2003 20:52:07 -0700 (PDT) Received: from garple.migus.org (pcp243563pcs.howard01.md.comcast.net [68.55.84.59]) by mx1.FreeBSD.org (Postfix) with ESMTP id 7C07843F75; Thu, 29 May 2003 20:52:06 -0700 (PDT) (envelope-from amigus@migus.org) Received: from migus.org (ganyopa.migus.org [192.168.4.2]) by garple.migus.org (8.12.7/8.12.7) with ESMTP id h4U3q4Z3006865; Thu, 29 May 2003 23:52:04 -0400 (EDT) (envelope-from amigus@migus.org) Message-ID: <3ED6D53E.7040207@migus.org> Date: Thu, 29 May 2003 23:51:26 -0400 From: Adam Migus Organization: The FreeBSD Project User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.3b) Gecko/20030302 X-Accept-Language: en-us, en MIME-Version: 1.0 To: Robert Watson References: In-Reply-To: Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, hits=-31.9 required=5.0 tests=EMAIL_ATTRIBUTION,IN_REP_TO,QUOTED_EMAIL_TEXT,REFERENCES, REPLY_WITH_QUOTES,USER_AGENT_MOZILLA_UA version=2.50 X-Spam-Checker-Version: SpamAssassin 2.50 (1.173-2003-02-20-exp) cc: Adam Migus cc: perforce@freebsd.org Subject: Re: PERFORCE change 32072 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 May 2003 03:52:09 -0000 Robert, So my mistake was essentially neglecting to account for an intentionally inappropriate action by a userland process. My apologies. Sometimes I overlook these things. Perhaps a better fix might be checking if left goes negative and barfing if it does? Adam Robert Watson wrote: >On Thu, 29 May 2003, Adam Migus wrote: > > > >>len can never exceed size which is passed in as the size of the string. >>Therefor the NULL will always land inside the string. How exactly does >>snprintf() fail? >> >> > >snprintf() in the kernel appears to be able to operate in two ways: > >(1) There is enough space in the buffer, and the return value is the size > of the space in the buffer consumed. I.e., 'len' <= 'size'. > >(2) There is not enough space in the buffer, and the return value is the > size of the space that snprintf() would like to have consumed. In > this case, 'size' is > 'len'. > >In this code, you initialize 'left' to the size argument passed into the >string conversion routine. In each call to snprintf(), you store the >return value of snprintf() in 'len', then you subtract 'len' from 'left' >and store the result in 'left'. If there's enough room in the buffer, in >each pass 'left' will decrease a bit more, and end as either 0 ("just >enough room") or a positive value ("more than enough room). However, at >some point you run out of room, 'left' will become negative, which causes >the following code to misbehave: > > len = size - left - 1; > string[len] = '\0'; > return (len); > >So that will drop a '\0' somewhere in kernel memory, probably close to the >buffer, but potentially fairly far (whatever the maximum size of a Biba >label string can be, minus a few characters of filler). > >The size of the buffer in question is set by the user process, since the >kernel buffer size is set to the size of the user process buffer passed in >via the system call; we place a maximum bound on that buffer size, but the >minum size is determined by the length of the list of labels they ask for. >In the libc/posix1e/mac code, that buffer size is set to the max buffer >length available to the kernel, leaving a lot of room for label text. >However, if I manually invoke the system call from userland code, I can >use a much smaller buffer; for MLS, four bytes ("mls" + nul termination). > >Note that the kern_mac.c code does an extra check for each call to >snprintf() to handle this case: > > if (first) { \ > len = snprintf(curptr, left, "%s/", \ > element_name); \ > first = 0; \ > } else \ > len = snprintf(curptr, left, ",%s/", \ > element_name); \ > if (len >= left) { \ > error = EINVAL; /* XXXMAC: E2BIG */ \ > break; \ > } > >I.e., it checks to make sure the resulting string fit in the buffer before >continuing, and generates a failure if not. The error code here is >arguably incorrect, hence the comment. The reason this came up in the >compartment version of the code and not the non-compartment version is >that in the non-compartment version, there was only one invocation of >snprintf() in this helper function, so the return value was handed up and >the caller was responsible for checking desired/consumed buffer length vs. >remaining buffer length. When there are multiple snprintf() calls, >special handling has to be introduced. > >The problem with the fix I committed is that I think it may not properly >generate the failure case to the caller, so the best thing to do may be to >avoid a recalculation of 'len' based on potentially incorrect values (in >fact, I may not have fully corrected the overflow case for this reason). > >I'll try to take a look at it again in detail tonight once I finish up >another contract deliverable we're working on. A far better answer is >probably to use the kernel sbuf code, although that's probably a post-5.1 >thing due to the complexity of that change. > > > >> >> >> >>>http://perforce.freebsd.org/chv.cgi?CH=32072 >>> >>>Change 32072 by rwatson@rwatson_tislabs on >>>2003/05/29 16:14:02 >>> >>> Temporary work-around for overflows in >>>externalization of >>> compartment strings in the Biba and MLS policies. >>>Validate >>> that the nul we slap down in fact lands inside the >>>string. >>> This code generally needs cleaning up, since it >>>fails to >>> handle failures by snprintf(). If the provided >>>string is >>> too short, this result is preferable to kernel >>>panics, etc. >>> >>>Affected files ... >>> >>>.. >>>//depot/projects/trustedbsd/mac/sys/security/mac_biba/mac_biba.c#209 >>>edit .. >>>//depot/projects/trustedbsd/mac/sys/security/mac_mls/mac_mls.c#167 >>>edit >>> >>>Differences ... >>> >>>==== >>>//depot/projects/trustedbsd/mac/sys/security/mac_biba/mac_biba.c#209 >>>(text+ko) ==== >>> >>>@@ -583,7 +583,11 @@ >>> } while(bit <= MAC_BIBA_MAX_COMPARTMENTS); >>> >>> len = size - left - 1; >>>- string[len] = '\0'; >>>+ if (len > 0 && len < size) >>>+ string[len] = '\0'; >>>+ else >>>+ string[0] = '\0'; >>>+ >>> return (len); >>> >>> default: >>> >>>==== >>>//depot/projects/trustedbsd/mac/sys/security/mac_mls/mac_mls.c#167 >>>(text+ko) ==== >>> >>>@@ -547,7 +547,10 @@ >>> } while(bit <= MAC_MLS_MAX_COMPARTMENTS); >>> >>> len = size - left - 1; >>>- string[len] = '\0'; >>>+ if (len > 0 && len < size) >>>+ string[len] = '\0'; >>>+ else >>>+ string[0] = '\0'; >>> return (len); >>> >>> default: >>> >>> >>-- >>Adam Migus - Research Scientist >>Network Associates Laboratories (http://www.nailabs.com) >>FreeBSD (http://www.freebsd.org) >>God may be subtle, but He isn't plain mean. -- Albert >>Einstein >> >> >> >> >> >> > > > -- Adam Migus - Research Scientist Network Associates Laboratories (http://www.nailabs.com)