[linux-cifs-client] [PATCH] mount.cifs: properly check for mount being in fstab when running setuid root

Jeff Layton jlayton at redhat.com
Tue May 26 23:51:43 GMT 2009


Ordinarily, I'd consider this a security problem, but I'm not aware of
any distro that ships mount.cifs as setuid binary. Therefore, I'm going
to go ahead and post this publically for discussion.

When mount.cifs is installed setuid root and run as an unprivileged
user, it does some checks to limit how the mount is used. It checks that
the mountpoint is owned by the user doing the mount.

These checks however do not match those that /bin/mount does when it is
called by an unprivileged user. When /bin/mount is called by an
unprivileged user to do a mount, it checks that the mount in question is
in /etc/fstab, that it has the "user" option set, etc.

This means that it's currently not possible to set up user mounts the
standard way (by the admin, in /etc/fstab) and simultaneously protect
from an unprivileged user calling mount.cifs directly to mount a share
on any directory that that user owns.

Fix this by making the checks in mount.cifs match those of /bin/mount
itself. This is a necessary step to make mount.cifs safe to be installed
as a setuid binary, but not sufficient. For that, we'd need to give
mount.cifs a proper security audit.

There is some concern that people are relying on this behavior to
implement unprivileged mounts for their users. My assertion however, is
that this functionality really has no business being in the mount helper
for cifs and should never have been introduced in the first place.

Mount helpers are never intended to be called directly, and shouldn't
offer any "extra" privileges over what /bin/mount allows. Therefore, I'm
proposing that we just change this and suggest that people look to other
solutions (autofs, pam_mount, something new?) for this sort of
functionality.

Signed-off-by: Jeff Layton <jlayton at redhat.com>
---
 source3/client/mount.cifs.c |  152 ++++++++++++++++++++++++++++++++-----------
 1 files changed, 113 insertions(+), 39 deletions(-)

diff --git a/source3/client/mount.cifs.c b/source3/client/mount.cifs.c
index a5d99dc..aba9f86 100644
--- a/source3/client/mount.cifs.c
+++ b/source3/client/mount.cifs.c
@@ -39,6 +39,7 @@
 #include <mntent.h>
 #include <fcntl.h>
 #include <limits.h>
+#include <fstab.h>
 #include "mount.h"
 
 #define MOUNT_CIFS_VERSION_MAJOR "1"
@@ -69,6 +70,10 @@
 #define MS_BIND 4096
 #endif
 
+/* private flags - clear these before passing to kernel */
+#define MS_USERS	0x40000000
+#define MS_USER		0x80000000
+
 #define MAX_UNC_LEN 1024
 
 #define CONST_DISCARD(type, ptr)      ((type) ((void *) (ptr)))
@@ -142,6 +147,77 @@ static size_t strlcat(char *d, const char *s, size_t bufsize)
 }
 #endif
 
+/*
+ * If an unprivileged user is doing the mounting then we need to ensure
+ * that the entry is in /etc/fstab.
+ */
+static int
+check_mountpoint(const char *progname, char *mountpoint)
+{
+	int err;
+	struct stat statbuf;
+
+	/* does mountpoint exist and is it a directory? */
+	err = stat(mountpoint, &statbuf);
+	if (err) {
+		fprintf(stderr, "%s: failed to stat %s: %s\n", progname,
+				mountpoint, strerror(errno));
+		return EX_USAGE;
+	}
+
+	if (!S_ISDIR(statbuf.st_mode)) {
+		fprintf(stderr, "%s: %s is not a directory!", progname,
+				mountpoint);
+		return EX_USAGE;
+	}
+
+	/* FIXME: do we need to check ownership of mountpoint? */
+
+	return 0;
+}
+
+static int
+check_fstab(const char *progname, char *mountpoint, char *devname,
+	    char **options, unsigned long *flags)
+{
+	FILE *fstab;
+	struct mntent *mnt;
+
+	/* make sure this mount is listed in /etc/fstab */
+	fstab = setmntent(_PATH_FSTAB, "r");
+	if (!fstab) {
+		fprintf(stderr, "Couldn't open %s for reading!\n",
+				_PATH_FSTAB);
+		return EX_FILEIO;
+	}
+
+	while((mnt = getmntent(fstab))) {
+		if (!strcmp(mountpoint, mnt->mnt_dir))
+			break;
+	}
+	endmntent(fstab);
+
+	if (mnt == NULL || strcmp(mnt->mnt_fsname, devname)) {
+		fprintf(stderr, "%s: permission denied: no match for "
+				"%s found in %s\n", progname, mountpoint,
+				_PATH_FSTAB);
+		return EX_USAGE;
+	}
+
+	/* unprivileged mounts default to nosuid and nodev */
+	*flags |= MS_NOSUID|MS_NODEV;
+
+	/*
+	 * 'mount' munges the options from fstab before passing them
+	 * to us. It is non-trivial to test that we have the correct
+	 * set of options. We don't want to trust what the user
+	 * gave us, so just take whatever is in /etc/fstab.
+	 */
+	free(*options);
+	*options = strdup(mnt->mnt_opts);
+	return 0;
+}
+
 /* BB finish BB
 
         cifs_umount
@@ -362,7 +438,7 @@ static int get_password_from_file(int file_descript, char * filename)
 	return rc;
 }
 
-static int parse_options(char ** optionsp, int * filesys_flags)
+static int parse_options(char ** optionsp, unsigned long * filesys_flags)
 {
 	const char * data;
 	char * percent_char = NULL;
@@ -415,6 +491,7 @@ static int parse_options(char ** optionsp, int * filesys_flags)
 
 		if (strncmp(data, "users",5) == 0) {
 			if(!value || !*value) {
+				*filesys_flags |= MS_USERS;
 				goto nocopy;
 			}
 		} else if (strncmp(data, "user_xattr",10) == 0) {
@@ -423,10 +500,7 @@ static int parse_options(char ** optionsp, int * filesys_flags)
 
 			if (!value || !*value) {
 				if(data[4] == '\0') {
-					if(verboseflag)
-						printf("\nskipping empty user mount parameter\n");
-					/* remove the parm since it would otherwise be confusing
-					to the kernel code which would think it was a real username */
+					*filesys_flags |= MS_USER;
 					goto nocopy;
 				} else {
 					printf("username specified with no parameter\n");
@@ -1029,7 +1103,7 @@ static void print_cifs_mount_version(void)
 int main(int argc, char ** argv)
 {
 	int c;
-	int flags = MS_MANDLOCK; /* no need to set legacy MS_MGC_VAL */
+	unsigned long flags = MS_MANDLOCK;
 	char * orgoptions = NULL;
 	char * share_name = NULL;
 	const char * ipaddr = NULL;
@@ -1052,7 +1126,6 @@ int main(int argc, char ** argv)
 	size_t current_len;
 	int retry = 0; /* set when we have to retry mount with uppercase */
 	struct addrinfo *addrhead = NULL, *addr;
-	struct stat statbuf;
 	struct utsname sysinfo;
 	struct mntent mountent;
 	struct sockaddr_in *addr4;
@@ -1110,8 +1183,8 @@ int main(int argc, char ** argv)
 		exit(EX_USAGE);
 	}
 
-	/* add sharename in opts string as unc= parm */
 
+	/* add sharename in opts string as unc= parm */
 	while ((c = getopt_long (argc, argv, "afFhilL:no:O:rsSU:vVwt:",
 			 longopts, NULL)) != -1) {
 		switch (c) {
@@ -1249,6 +1322,19 @@ int main(int argc, char ** argv)
 		exit(EX_USAGE);
 	}
 
+	/* make sure mountpoint is legit */
+	rc = check_mountpoint(thisprogram, mountpoint);
+	if (rc)
+		goto mount_exit;
+
+	/* sanity check for unprivileged mounts */
+	if (getuid()) {
+		rc = check_fstab(thisprogram, mountpoint, dev_name,
+				 &orgoptions, &flags);
+		if (rc)
+			goto mount_exit;
+	}
+
 	if (getenv("PASSWD")) {
 		if(mountpassword == NULL)
 			mountpassword = (char *)calloc(MOUNT_PASSWD_SIZE+1,1);
@@ -1266,6 +1352,25 @@ int main(int argc, char ** argv)
                 rc = EX_USAGE;
 		goto mount_exit;
 	}
+
+	if (getuid()) {
+		if (!(flags & (MS_USERS|MS_USER))) {
+			fprintf(stderr, "%s: permission denied\n", thisprogram);
+			rc = EX_USAGE;
+			goto mount_exit;
+		}
+		
+		if (geteuid()) {
+			fprintf(stderr, "%s: not installed setuid - \"user\" "
+					"CIFS mounts not supported.",
+					thisprogram);
+			rc = EX_FAIL;
+			goto mount_exit;
+		}
+	}
+
+	flags &= ~(MS_USERS|MS_USER);
+
 	addrhead = addr = parse_server(&share_name);
 	if((addrhead == NULL) && (got_ip == 0)) {
 		printf("No ip address specified and hostname not found\n");
@@ -1282,37 +1387,6 @@ int main(int argc, char ** argv)
 			mountpoint = resolved_path; 
 		}
 	}
-	if(chdir(mountpoint)) {
-		printf("mount error: can not change directory into mount target %s\n",mountpoint);
-		rc = EX_USAGE;
-		goto mount_exit;
-	}
-
-	if(stat (".", &statbuf)) {
-		printf("mount error: mount point %s does not exist\n",mountpoint);
-		rc = EX_USAGE;
-		goto mount_exit;
-	}
-
-	if (S_ISDIR(statbuf.st_mode) == 0) {
-		printf("mount error: mount point %s is not a directory\n",mountpoint);
-		rc = EX_USAGE;
-		goto mount_exit;
-	}
-
-	if((getuid() != 0) && (geteuid() == 0)) {
-		if((statbuf.st_uid == getuid()) && (S_IRWXU == (statbuf.st_mode & S_IRWXU))) {
-#ifndef CIFS_ALLOW_USR_SUID
-			/* Do not allow user mounts to control suid flag
-			for mount unless explicitly built that way */
-			flags |= MS_NOSUID | MS_NODEV;
-#endif						
-		} else {
-			printf("mount error: permission denied or not superuser and mount.cifs not installed SUID\n"); 
-			exit(EX_USAGE);
-		}
-	}
-
 	if(got_user == 0) {
 		/* Note that the password will not be retrieved from the
 		   USER env variable (ie user%password form) as there is
-- 
1.6.0.6



More information about the linux-cifs-client mailing list