Re: kern/29360: vfs.generic.usermount and mount(8) general questions

[Antti Kantee <pooka%cs.hut.fi@localhost>]

On Sun Sep 06 2009 at 11:20:09 +0200, Manuel Bouyer wrote:
> On Sun, Sep 06, 2009 at 02:01:44AM -0400, Elad Efrat wrote:
> > Hi,
> > 
> > I just came across this PR.
> > 
> > The check that a non-root user owns the mount-point directory was
> > introduced way before vfs.generic.usermount. In fact, it seems that it
> > actually removed the root check, and allowed non-root users to freely
> > mount file-systems:
> > 
> >     
> > http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/kern/vfs_syscalls.c.diff?r1=1.42&r2=1.43&f=h
> 
> Yes, vfs.generic.usermount was introduced later, because of security issues
> that usermounts could cause. AFAIK the know security issues with
> usermounts are fixed, but still it's better to have it disabled on systems
> where it's not needed.

Really?  If you are going to claim a fixed security issue, please provide
some reference to the issue you are talking about.  As I recall, it was
added because mounting enough file systems (I used kernfs for testing back
then) would cause the kernel to run out of memory and the system to panic.

> > With something like the following:
> > 
> > /* Ensure that the user can write to the mount-point. */
> > if ((error = VOP_ACCESS(vp, VWRITE, l->l_cred)) != 0)
> >     return error;
> > 
> > Does anyone see any drawbacks to this approach? If not, I'll change
> > the relevant code.
> 
> Yes, that would mean a user could mount his own FS over e.g. /tmp, or
> /var/mail. that's bad.
> I think that checking the user owns the mount point is the right thing to do.

I agree that ownership is the right check.

> I think a sysctl to control whenever to check for group ownerchip instead
> of user ownerchip would work, though. It's up to the admin to carefully
> choose a group for devices and mount points :)

I am opposed to adding a kernel switch with confusing security
implications.  Especially since the issue in the PR is corner-case (IMHO,
of course) and can be solved easily at user-level with a wrapper without
affecting everyone.

(at the very least, you'd need to check owner || (group && write).
and even then, there are difficult-to-foresee consequences e.g. a
sticky-bitty group-shared working directory or group +wx "drop site"
directory)