PERFORCE change 32072 for review

[Robert Watson]

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