Re: [patch 1/3] libsemanage: genhomedircon replacement

[James Antill <jantill@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3037 bytes --]

On Thu, 2007-05-24 at 10:04 -0400, Mark Goldman wrote:
> <snip>
> > > +static int write_home_dir_context(FILE * out, list_t * tpl, char *user,
> > > +				  char *seuser, char *home, char *prefix)
> > > +{
> > > +	char *temp;
> > > +	char *line;
> > > +
> > > +	fprintf(out, "\n\n#\n# Home Context for user %s\n#\n\n", user);
> > > +	for (; tpl; tpl = tpl->next) {
> > > +		temp = semanage_sed(tpl->data, "HOME_DIR", home);
> > 
> > Wow - I understand that this string handling in C is painful, but is
> > this really the best alternative? Perhaps parsing the input to make the
> > substitution easier would be better? Perhaps we should look for some
> > library code to do this?
> I am not really sure what the problem with this is.  We need to replace
> arbitrary substrings.  I searched in the C libraries for something that
> might do what I want.  I couldn't find anything, so I wrote a function
> to do it.

 I feel your pain, doing anything non-trivial with strings in C is
always a major PITA.
 As a personal project I've almost got ustr[1] in a 1.0 release, which I
hope could be used in the SELinux libraries (being able to easily
integrate it into libraries, is one of it's main points), and would
solve this problem and others in the SELinux libs. (like the strdup() +
a bit more data).

> As for parsing the input to make the substitution easier, I think it
> would be a lot more work to write or use a full fledged parser.  What
> we want in this situation is what I wrote in my opinion, although I am
> open to suggestions.

 Saying that, there were more than a few problems with semanage_sed()
even if you discount the genericness of the name:

1. You pre-allocate a bad amount of space:

+       srclen = strlen(src);
+       maxdest = 2 * srclen;
+       dest = malloc(maxdest);

...consider:

HOME_DIR        -d      system_u:object_r:user_home_dir_t:s0

strlen("HOME_DIR") == 8, *2 == 16
strlen("/home/kmacmillan") == 16

...I think, you really want something like:

+       maxdest = srclen + (replen - vallen) * 2 + 1;


2. You pre-reallocate if there isn't enough room to insert a new val,
even if it's not possible to insert a new val:

+               min_increment = (dindex + replen + 1) - maxdest;
+               if (min_increment >= 0) {

...consider:

HOME_DIR/.+     system_u:object_r:user_home_t:s0

strlen("HOME_DIR/.+") == 11, *2 == 22
strlen("/home/james") == 11

...now when you are at '.' in the above substitution, you'll have
dindex=11 and replen=11 which will trigger a realloc() even though you
only have 2 bytes left in src (so HOME_DIR will never match, obviously).


3. You should probably be calling strstr() instead of walking the
string.


4. All the sizes are signed int's, and no overflow/underflow checking is
done. This might be safe due to current usage, but it seems like a bad
idea to leave it this way in a core security lib.


[1] http://www.and.org/ustr/

-- 
James Antill <jantill@redhat.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]