From owner-svn-src-all@FreeBSD.ORG Sun Jun 21 02:47:21 2009 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 54BC71065673; Sun, 21 Jun 2009 02:47:21 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail08.syd.optusnet.com.au (mail08.syd.optusnet.com.au [211.29.132.189]) by mx1.freebsd.org (Postfix) with ESMTP id E1D1B8FC0A; Sun, 21 Jun 2009 02:47:20 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-159-184.carlnfd1.nsw.optusnet.com.au (c122-106-159-184.carlnfd1.nsw.optusnet.com.au [122.106.159.184]) by mail08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n5L2lHYL011826 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 21 Jun 2009 12:47:18 +1000 Date: Sun, 21 Jun 2009 12:47:17 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Brooks Davis In-Reply-To: <20090620183728.GA72393@lor.one-eyed-alien.net> Message-ID: <20090621123046.V30192@delplex.bde.org> References: <200906191552.n5JFqZcG047705@svn.freebsd.org> <20090620130238.N29302@delplex.bde.org> <20090620183728.GA72393@lor.one-eyed-alien.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans Subject: Re: svn commit: r194493 - head/usr.bin/catman X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 21 Jun 2009 02:47:21 -0000 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