SUIDDIR -> security.bsd.suiddir_enable.

[Bruce Evans]

On Thu, 25 Mar 2004, Pawel Jakub Dawidek wrote:

> On Thu, Mar 25, 2004 at 11:06:38PM +1100, Bruce Evans wrote:
> +> On Thu, 25 Mar 2004, Pawel Jakub Dawidek wrote:
> +>
> +> > Any objection on such exchange?
> +> >
> +> > In p4 pjd_suiddir branch I've a code that replace SUIDDIR kernel option
> +> > with sysctl security.bsd.suiddir_enable sysctl with is turned off by
> +> > default. SUIDDIR option is not removed, but it means now: turn on suiddir
> +> > functionality by default.
> +>
> +> Using SUIDDIR is controlled by the MNT_SUIDDIR mount option, so there
> +> shouldn't be another knob to control it.  If there is a security problem
> +> using MNT_SUIDDIR, then MNT_SUIDDIR should be disallowed up front so
> +> that that all the places that implement SUIDDIR don't have to test
> +> both knobs.
>
> First of all this adds 0 overhead.

Actually, all of it adds a small amount of overhead.  First, making
the SUIDDIR code non-optional requires a test of a variable to decide
if the suiddir case is active.  Second, the sysctl variable requires
another test to decide if the suiddir case is active (this only in the
path where the first test succeeds).  The sysctl variable also gives
the code a different ugly structure, basically from:

% #ifdef SUIDDIR
% 	/* XXX bogus indentation. */
% 	{
% 		if ((dvp->v_mount->mnt_flag & MNT_SUIDDIR)) {
% 			suiddir_case();
% 		} else {
% 			nonsuiddircase();
% 		}
% 	}
% #else /* !SUIDDIR */
% 	nonsuiddircase();
% #endif /* SUIDDIR */

to:

% 	if (suiddir_enable) {
% 		if ((dvp->v_mount->mnt_flag & MNT_SUIDDIR)) {
% 			suiddir_case();
% 		} else {
% 			nonsuiddircase();
% 		}
% 	} else {
% 		nonsuiddircase();
% 	}

(actually much uglier due to nested declarations, nested QUOTA ifdefs, and
duplication of a large block of code for nonsuiddircase()).  It should be
written as:

% 	if ((dvp->v_mount->mnt_flag & MNT_SUIDDIR)) {
% 		suiddir_case();
% 	} else {
% 		nonsuiddircase();
% 	}

> And I think there is a need for additional level of security for such
> functionality, but I see no reason to force people to recompile kernel.

I now think there is no new security problem other than another way
for root to make mistakes, because mounting with MNT_SUIDDIR doesn't
do anything that can't be done by root using chown() and chmod().
There is an old security problem: unprivileged users are permitted to
do MNT_SUIDDIR mounts if they are permitted to do mounts at all.

Bruce