Date: Fri, 26 Mar 2004 00:50:09 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Pawel Jakub Dawidek <pjd@FreeBSD.org> Cc: freebsd-arch@FreeBSD.org Subject: Re: SUIDDIR -> security.bsd.suiddir_enable. Message-ID: <20040326000115.R37325@gamplex.bde.org> In-Reply-To: <20040325123554.GZ8930@darkness.comp.waw.pl> References: <20040324235120.GU8930@darkness.comp.waw.pl> <20040325123554.GZ8930@darkness.comp.waw.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040326000115.R37325>