Re: [PATCH 17/19] mount.cifs: guard against signals by unprivileged users

On Thu, 01 Apr 2010 15:43:18 -0400
simo <[email protected]> wrote:

> On Thu, 2010-04-01 at 15:32 -0400, Jeff Layton wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > On Sun, 28 Mar 2010 09:34:10 -0400
> > Jeff Layton <[email protected]> wrote:
> > 
> > > On Fri, 26 Mar 2010 10:25:40 -0400
> > > Jeff Layton <[email protected]> wrote:
> > > 
> > > > From: Jeff Layton <[email protected]>
> > > > 
> > > > If mount.cifs is setuid root, then the unprivileged user who runs the
> > > > program can send the mount.cifs process a signal and kill it. This is
> > > > not a huge problem unless we happen to be updating the mtab at the
> > > > time, in which case the mtab lockfiles might not get cleaned up.
> > > > 
> > > > To remedy this, have the privileged mount.cifs process set its real
> > > > uid to the effective uid (usually, root). This prevents unprivileged
> > > > users from being able to signal the process.
> > > > 
> > > > While we're at it, also mask off signals while we're updating the
> > > > mtab. This leaves a SIGKILL by root as the only way to interrupt the
> > > > mtab update, but there's really nothing we can do about that.
> > > > 
> > > > Signed-off-by: Jeff Layton <[email protected]>
> > >
> > > A little self-review on this patch...
> > > 
> > > It's probably better not to change the real uid until the mtab is set
> > > to be updated, so I'm moving that piece into add_mtab. Doing so very
> > > early on like this means that the kernel loses the ability to get the
> > > real uid of the user running the mount command.
> > > 
> > > Replacement patch attached...
> > 
> > Simo pointed out another problem with this patch. It's possible for
> > getpwuid to block for quite a while if using something like NIS, SSSD,
> > etc...
> > 
> > Thus, it's better to move the getusername() call out of the section
> > where we hold the mtab lock. New patch attached.
> 
> With this change the whole patch set looks good to me.
> I haven't had a chance to actually test it though.
> 
> Simo.
> 

Thanks for the review. I've tested it quite a bit and also think it's
an improvement. I've gone ahead and pushed these patches into the repo.

Cheers,
-- 
Jeff Layton <[email protected]>