Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 10 Feb 2013 06:35:50 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Eitan Adler <eadler@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org
Subject:   Re: svn commit: r246593 - head/lib/libc/sys
Message-ID:  <20130210051121.L3760@besplex.bde.org>
In-Reply-To: <CAF6rxgnD0oK%2B%2B8csMXmxKzYtjbAXk=VD=-bf=eFTunNjzBSqdw@mail.gmail.com>
References:  <201302091713.r19HDqv6006148@svn.freebsd.org> <CAF6rxgnD0oK%2B%2B8csMXmxKzYtjbAXk=VD=-bf=eFTunNjzBSqdw@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 9 Feb 2013, Eitan Adler wrote:

> On 9 February 2013 12:13, Eitan Adler <eadler@freebsd.org> 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 <chris+freebsd@chrullrich.net>
>>   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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130210051121.L3760>