PERFORCE change 32072 for review

[Adam Migus]

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.
>
>  
>
>><quote who="Robert Watson">
>>    
>>
>>>http://perforce.freebsd.org/chv.cgi?CH=32072
>>>
>>>Change 32072 by rwatson at 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)



To Unsubscribe: send mail to majordomo at trustedbsd.org
with "unsubscribe trustedbsd-cvs" in the body of the message