PERFORCE change 32072 for review

[Robert Watson]

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

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