PERFORCE change 32072 for review

[Adam Migus]

Robert,
My memory of all this is sketchy at best but I think my proposed
changes to the layers above the code in question would have
eliminated the possibility of this error occuring.  At any rate
sorry for the confusion.

Adam

Robert Watson wrote:

>On Thu, 29 May 2003, Adam Migus wrote:
>
>  
>
>>Sorry I failed to address a point.  I assumed when writing the code that
>>the caller would pass in a large enough string, as that assumption is
>>made elsewhere in code called at this layer in the framework.  To be
>>explicit, I think this change is therefor rendundant.  Also I am
>>curious, asside from intentionally passing in a string that's too short
>>to hold the result can you make this code panic? 
>>    
>>
>
>By passing too small a buffer in from user space test program, I am able
>to trigger kernel memory corruption resulting in a panic.  To do this, I
>set the buffer in user space to an artificially small value after setting
>the Biba or MLS label to be something that renders to quite a large
>string.  It appears that the '\0' generally gets plopped in recently
>free'd memory, and so I pick it up with INVARIANTS on, since under
>INVARIANTS, the kernel malloc code checks that memory hasn't been modified
>after a free().
>
>Obviously, there's not a 100% certainty that this particular program is
>triggering the panic, it could be triggered by another bug along the same
>code path, but after my change, I'm unable to reproduce the panic after
>about three hours of running the test program and exercising the kernel
>memory allocator.
>
>Robert N M Watson             FreeBSD Core Team, TrustedBSD Projects
>robert at fledge.watson.org      Network Associates Laboratories
>
>
>  
>
>>Adam
>>
>><quote who="Adam Migus">
>>    
>>
>>>Robert,
>>>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?
>>>
>>>Adam
>>>
>>><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)
>>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)