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

login
register
mail settings
Submitter Jeff Layton
Date April 1, 2010, 7:32 p.m.
Message ID <20100401153241.6ffdbed6@corrin.poochiereds.net>
Download mbox | patch
Permalink /patch/49237/
State New
Headers show

Comments

Jeff Layton - April 1, 2010, 7:32 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Sun, 28 Mar 2010 09:34:10 -0400
Jeff Layton <jlayton@samba.org> wrote:

> On Fri, 26 Mar 2010 10:25:40 -0400

> Jeff Layton <jlayton@samba.org> wrote:

> 

> > From: Jeff Layton <jlayton@redhat.com>

> > 

> > 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 <jlayton@redhat.com>

>

> 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.

- -- 
Jeff Layton <jlayton@samba.org>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.14 (GNU/Linux)

iEYEARECAAYFAku09N0ACgkQyP0gxQMdzIBRDwCeKMwXGMBFUsdoFDxprCDrnjJe
va8An3wRc/6Es+S5/HVQk2lCISoUgnEK
=Z6vK
-----END PGP SIGNATURE-----
Simo Sorce - April 1, 2010, 7:43 p.m.
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 <jlayton@samba.org> wrote:
> 
> > On Fri, 26 Mar 2010 10:25:40 -0400
> > Jeff Layton <jlayton@samba.org> wrote:
> > 
> > > From: Jeff Layton <jlayton@redhat.com>
> > > 
> > > 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 <jlayton@redhat.com>
> >
> > 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.
Jeff Layton - April 2, 2010, 1:35 a.m.
On Thu, 01 Apr 2010 15:43:18 -0400
simo <idra@samba.org> 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 <jlayton@samba.org> wrote:
> > 
> > > On Fri, 26 Mar 2010 10:25:40 -0400
> > > Jeff Layton <jlayton@samba.org> wrote:
> > > 
> > > > From: Jeff Layton <jlayton@redhat.com>
> > > > 
> > > > 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 <jlayton@redhat.com>
> > >
> > > 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,

Patch

From 810f7e4e0f2dbcbee0294d9b371071cb08268200 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
Date: Thu, 1 Apr 2010 15:28:54 -0400
Subject: [PATCH] mount.cifs: guard against signals by unprivileged users

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 <jlayton@redhat.com>
---
 mount.cifs.c |   47 ++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index f4aea01..aa5c9f9 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -267,10 +267,10 @@  static int set_password(struct parsed_mount_info *parsed_info, const char *src)
 }
 
 /* caller frees username if necessary */
-static char *getusername(void)
+static char *getusername(uid_t uid)
 {
 	char *username = NULL;
-	struct passwd *password = getpwuid(getuid());
+	struct passwd *password = getpwuid(uid);
 
 	if (password)
 		username = password->pw_name;
@@ -1054,9 +1054,39 @@  static int
 add_mtab(char *devname, char *mountpoint, unsigned long flags)
 {
 	int rc = 0;
-	char *mount_user;
+	uid_t uid;
+	char *mount_user = NULL;
 	struct mntent mountent;
 	FILE *pmntfile;
+	sigset_t mask, oldmask;
+
+	uid = getuid();
+	if (uid != 0)
+		mount_user = getusername(uid);
+
+	/*
+	 * Set the real uid to the effective uid. This prevents unprivileged
+	 * users from sending signals to this process, though ^c on controlling
+	 * terminal should still work.
+	 */
+	rc = setreuid(geteuid(), -1);
+	if (rc != 0) {
+		fprintf(stderr, "Unable to set real uid to effective uid: %s\n",
+				strerror(errno));
+		rc = EX_FILEIO;
+	}
+
+	rc = sigfillset(&mask);
+	if (rc) {
+		fprintf(stderr, "Unable to set filled signal mask\n");
+		return EX_FILEIO;
+	}
+
+	rc = sigprocmask(SIG_SETMASK, &mask, &oldmask);
+	if (rc) {
+		fprintf(stderr, "Unable to make process ignore signals\n");
+		return EX_FILEIO;
+	}
 
 	atexit(unlock_mtab);
 	rc = lock_mtab();
@@ -1094,12 +1124,10 @@  add_mtab(char *devname, char *mountpoint, unsigned long flags)
 			strlcat(mountent.mnt_opts, ",nodev", MTAB_OPTIONS_LEN);
 		if (flags & MS_SYNCHRONOUS)
 			strlcat(mountent.mnt_opts, ",sync", MTAB_OPTIONS_LEN);
-		if (getuid() != 0) {
+		if (mount_user) {
 			strlcat(mountent.mnt_opts, ",user=", MTAB_OPTIONS_LEN);
-			mount_user = getusername();
-			if (mount_user)
-				strlcat(mountent.mnt_opts, mount_user,
-					MTAB_OPTIONS_LEN);
+			strlcat(mountent.mnt_opts, mount_user,
+				MTAB_OPTIONS_LEN);
 		}
 	}
 	mountent.mnt_freq = 0;
@@ -1109,6 +1137,7 @@  add_mtab(char *devname, char *mountpoint, unsigned long flags)
 	unlock_mtab();
 	SAFE_FREE(mountent.mnt_opts);
 add_mtab_exit:
+	sigprocmask(SIG_SETMASK, &oldmask, NULL);
 	if (rc) {
 		fprintf(stderr, "unable to add mount entry to mtab\n");
 		rc = EX_FILEIO;
@@ -1204,7 +1233,7 @@  assemble_mountinfo(struct parsed_mount_info *parsed_info,
 			strlcpy(parsed_info->username, getenv("USER"),
 				sizeof(parsed_info->username));
 		else
-			strlcpy(parsed_info->username, getusername(),
+			strlcpy(parsed_info->username, getusername(getuid()),
 				sizeof(parsed_info->username));
 		parsed_info->got_user = 1;
 	}
-- 
1.6.6.1