svn commit: r194493 - head/usr.bin/catman

[Bruce Evans]

On Sat, 20 Jun 2009, Brooks Davis wrote:

> On Sat, Jun 20, 2009 at 01:57:07PM +1000, Bruce Evans wrote:
>> On Fri, 19 Jun 2009, Brooks Davis wrote:
>>
>>> Log:
>>>  When checking if we can write to a file, use access() instead of a
>>>  manual permission check based on stat output.  Also, get rid of the
>>>  executability check since it is not used.
>>
>> This seems to add some security holes.  From "man assert | col -bx":
>>
>> %%%
>> SECURITY CONSIDERATIONS
>>      The access() system call is a potential security hole due to race condi-
>>      tions and should never be used.  Set-user-ID and set-group-ID applica-
>> ...
>> %%%
>
> The code I replaced was effectivly a hand rolled version of access() based
> on examining stat(1) results.  I think both are equivalently wrong from
> a security perspective.

Not really.  It is impossible to hand-roll access using only stat()
results, since ACL/MAC/other results might also be needed, and it is
difficult to hand-roll it since it is difficult even to know which
results are needed.  The hand-rolling that you removed didn't even
know about the immutable flags, which _are_ in the stat() results.

This both are inequivalently wrong from a security perspective.

catman would probably be better from a non-secuity persepective if it
didn't use access().  It can only use the results to avoid some errors
in advance.  It does this, but the advantages of doing this are small
at best.  man(1) doesn't do this.  It just attempts to open a temporary
cat file in the cat directory.  When this fails, it assumes that this
is intended and doesn't print a warning.  If this succeeds, than it later
attempts to rename the temporary fail.  If this fails (e.g., due to someone
settting uchg on the source or target file), then it assumes that this is
not intended and prints a warning.

>>> Modified: head/usr.bin/catman/catman.c
>>> ==============================================================================
>>> --- head/usr.bin/catman/catman.c	Fri Jun 19 15:31:40 2009	(r194492)
>>> +++ head/usr.bin/catman/catman.c	Fri Jun 19 15:52:35 2009	(r194493)
>>> @@ -759,14 +742,6 @@ main(int argc, char **argv)
>>> {
>>> 	int opt;
>>>
>>> -	if ((uid = getuid()) == 0) {
>>> -		fprintf(stderr, "don't run %s as root, use:\n   echo", argv[0]);
>>> -		for (optind = 0; optind < argc; optind++) {
>>> -			fprintf(stderr, " %s", argv[optind]);
>>> -		}
>>> -		fprintf(stderr, " | nice -5 su -m man\n");
>>> -		exit(1);
>>> -	}
>>> 	while ((opt = getopt(argc, argv, "vnfLrh")) != -1) {
>>> 		switch (opt) {
>>> 		case 'f':
>>
>> Surely it is wrong to remove the enforcement of not running as root?
>
> Yes that was an error, I've restored that check.  Thanks for the catch.

The named `uid' variable should be removed now, since it is no longer
needed outside this function and was never needed inside this function.

Bruce