From owner-svn-src-head@FreeBSD.ORG Sat Feb 9 21:35:55 2013 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id D1BC81DD; Sat, 9 Feb 2013 21:35:55 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx08.syd.optusnet.com.au (fallbackmx08.syd.optusnet.com.au [211.29.132.10]) by mx1.freebsd.org (Postfix) with ESMTP id E7533942; Sat, 9 Feb 2013 21:35:54 +0000 (UTC) Received: from mail12.syd.optusnet.com.au (mail12.syd.optusnet.com.au [211.29.132.193]) by fallbackmx08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r19JZsRF020687; Sun, 10 Feb 2013 06:35:54 +1100 Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail12.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r19JZoi4025212 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 10 Feb 2013 06:35:51 +1100 Date: Sun, 10 Feb 2013 06:35:50 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eitan Adler Subject: Re: svn commit: r246593 - head/lib/libc/sys In-Reply-To: Message-ID: <20130210051121.L3760@besplex.bde.org> References: <201302091713.r19HDqv6006148@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=Uug7rJMB c=1 sm=1 a=cAXCqpZEShQA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=uiiGxAOqq18A:10 a=6I5d2MoRAAAA:8 a=ZFVg263UAAAA:8 a=NV3LF1PM_9Jh4ooLTqgA:9 a=CjuIK1q_8ugA:10 a=SV7veod9ZcQA:10 a=o-NGsO4tlvUA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 09 Feb 2013 21:35:55 -0000 On Sat, 9 Feb 2013, Eitan Adler wrote: > On 9 February 2013 12:13, Eitan Adler wrote: >> Author: eadler >> Date: Sat Feb 9 17:13:51 2013 >> New Revision: 246593 >> URL: http://svnweb.freebsd.org/changeset/base/246593 >> >> Log: >> Fix logic inversion. >> >> PR: docs/174966 http://www.FreeBSD.org/cgi/query-pr.cgi?pr=174966 >> Submitted by: Christian Ullrich >> Approved by: bcr (mentor) >> >> Modified: >> head/lib/libc/sys/chflags.2 >> >> Modified: head/lib/libc/sys/chflags.2 >> ============================================================================== >> --- head/lib/libc/sys/chflags.2 Sat Feb 9 13:31:59 2013 (r246592) >> +++ head/lib/libc/sys/chflags.2 Sat Feb 9 17:13:51 2013 (r246593) >> @@ -98,7 +98,7 @@ If one of >> or >> .Dv SF_NOUNLINK >> is set a non-super-user cannot change any flags and even the super-user >> -can change flags only if securelevel is greater than 0. >> +can change flags only if securelevel is 0. > > I was reminded the default -1, not 0. I'll fix this shortly. Also, this seems to be wrong about changing flags. Old versions say that only clearing flags is prevented by securelevel > 0, and the code still seems to do this: Old man page: @ The ``SF_IMMUTABLE'', ``SF_APPEND'', ``SF_NOUNLINK'', and ``SF_ARCHIVED'' @ flags may only be set or unset by the super-user. Attempts by the non- @ super-user to set the super-user only flags are silently ignored. These @ ^^^^^ @ flags may be set at any time, but normally may only be unset when the @ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @ system is in single-user mode. (See init(8) for details.) Current ffs code: @ /* @ * Unprivileged processes are not permitted to unset system @ * flags, or modify flags if any system flags are set. @ ^^^^^^^^^^^^^^^^ @ * Privileged non-jail processes may not modify system flags @ ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @ * if securelevel > 0 and any existing system flags are set. @ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^. @ ||| ||| @ * Privileged jail processes behave like privileged non-jail @ * processes if the security.jail.chflags_allowed sysctl is @ * is non-zero; otherwise, they behave like unprivileged @ * processes. @ */ "any" is also wrong here. Only 3 flags system flags prevent unprivileged processes from modifying (user) flags. Only the same 3 system flags prevent privileged processes from modifying flags if securelevel > 0. These 3 flags are listed correctly in the man page, except they are listed 3 times (in the part that you changed, and in the [EPERM] description for each of chflags(2) and chflags(3)). Triplicating this is a good source of errors. There is no logic inversion or off-by-1 error in the [EPERM] parts. @ if (!priv_check_cred(cred, PRIV_VFS_SYSFLAGS, 0)) { The man page still uses the old term "super-user", while the code uses "privileged" (in comments too). We get here if we have privilege. The logic for the test is obfuscated uses boolean '!' for a non-boolean. @ if (ip->i_flags & @ (SF_NOUNLINK | SF_IMMUTABLE | SF_APPEND)) { @ error = securelevel_gt(cred, 0); @ if (error) @ return (error); @ } This checks for flags already set (in the inode). Not ones that we are trying to set. When none is set, we don't even look at securelevel. We get here even if securelevel > 0 if none of the 3 flags is already set. Same as in 4.4BSD-Lite1. The old man page is correct. @ /* The snapshot flag cannot be toggled. */ @ if ((vap->va_flags ^ ip->i_flags) & SF_SNAPSHOT) @ return (EPERM); After this, all changes to the flags are permitted. When securelevel > 0, the toggling that they do must actually be setting for the 3 special flags. A final toggling of the other flags is possible in the same step as setting one of the 3 special flags. @ } else { 3 special flags is 1 or 2 too many. In my version, SF_NOUNLINK doesn't give immutability for flags. It just prevents unlink(), rename() and rmdir(). In all versions, it doesn't prevent chown(), chmod(), utimes(), link() or truncating the file, so why should it prevent chflags()? If you want immutability then you should use SF_IMMUTABLE. Perhaps similarly for SF_APPEND, but since it is a positive logic flag its semantics can be read as append *only*, and that is in fact what it does. It prevents chown(), chmod(), utimes(), link() and truncating the file (at least for [f]truncate(2) on regular files). The man page doesn't give many details of what append and nounlink flags do. It says that *F_APPEND means that the file may only be appended to. This covers all of the above preventions, but also literally says that the file may not be read, or cntled, or even opened... It says that *F_NOUNLINK means that the file may not be renamed or deleted, then (mis)describes how it it also gives immutability of the flags. This seems to cover all the things that it prevents. Nearby I noticed bad wording for SF_ARCHIVED. It says "may". For all the other descriptions, "may" means permission. Here it means that no one knows what it means, but it might mean that an old version of the file might have been archived. msdosfs clears this flag when the file is changed (at least for write()), but ffs doesn't, so you can never trust it for ffs. Bruce