[SCM] Samba Shared Repository - branch master updated

Jeff Layton jlayton at samba.org
Tue Jan 26 06:16:11 MST 2010


The branch, master has been updated
       via  a0c31ec... mount.cifs: don't allow it to be run as setuid root program
       via  a065c17... mount.cifs: check for invalid characters in device name and mountpoint
       via  3ae5dac... mount.cifs: take extra care that mountpoint isn't changed during mount
      from  7148eff... s4-smbtorture: also test smbc_getOptionUseCCache

http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master


- Log -----------------------------------------------------------------
commit a0c31ec1c8d1220a5884e40d9ba6b191a04a24d5
Author: Jeff Layton <jlayton at redhat.com>
Date:   Tue Jan 26 08:15:41 2010 -0500

    mount.cifs: don't allow it to be run as setuid root program
    
    mount.cifs has been the subject of several "security" fire drills due to
    distributions installing it as a setuid root program. This program has
    not been properly audited for security and the Samba team highly
    recommends that it not be installed as a setuid root program at this
    time.
    
    To make that abundantly clear, this patch forcibly disables the ability
    for mount.cifs to run as a setuid root program. People are welcome to
    trivially patch this out, but they do so at their own peril.
    
    A security audit and redesign of this program is in progress and we hope
    that we'll be able to remove this in the near future.
    
    Signed-off-by: Jeff Layton <jlayton at redhat.com>

commit a065c177dfc8f968775593ba00dffafeebb2e054
Author: Jeff Layton <jlayton at redhat.com>
Date:   Tue Jan 26 08:15:41 2010 -0500

    mount.cifs: check for invalid characters in device name and mountpoint
    
    It's apparently possible to corrupt the mtab if you pass embedded
    newlines to addmntent. Apparently tabs are also a problem with certain
    earlier glibc versions. Backslashes are also a minor issue apparently,
    but we can't reasonably filter those.
    
    Make sure that neither the devname or mountpoint contain any problematic
    characters before allowing the mount to proceed.
    
    Signed-off-by: Jeff Layton <jlayton at redhat.com>

commit 3ae5dac462c4ed0fb2cd94553583c56fce2f9d80
Author: Jeff Layton <jlayton at redhat.com>
Date:   Tue Jan 26 08:15:41 2010 -0500

    mount.cifs: take extra care that mountpoint isn't changed during mount
    
    It's possible to trick mount.cifs into mounting onto the wrong directory
    by replacing the mountpoint with a symlink to a directory. mount.cifs
    attempts to check the validity of the mountpoint, but there's still a
    possible race between those checks and the mount(2) syscall.
    
    To guard against this, chdir to the mountpoint very early, and only deal
    with it as "." from then on out.
    
    Signed-off-by: Jeff Layton <jlayton at redhat.com>

-----------------------------------------------------------------------

Summary of changes:
 client/mount.cifs.c |  107 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 98 insertions(+), 9 deletions(-)


Changeset truncated at 500 lines:

diff --git a/client/mount.cifs.c b/client/mount.cifs.c
index 459a9f3..9044184 100644
--- a/client/mount.cifs.c
+++ b/client/mount.cifs.c
@@ -43,7 +43,7 @@
 #include "mount.h"
 
 #define MOUNT_CIFS_VERSION_MAJOR "1"
-#define MOUNT_CIFS_VERSION_MINOR "13"
+#define MOUNT_CIFS_VERSION_MINOR "14"
 
 #ifndef MOUNT_CIFS_VENDOR_SUFFIX
  #ifdef _SAMBA_BUILD_
@@ -87,6 +87,17 @@
 #define MAX_ADDRESS_LEN INET6_ADDRSTRLEN
 
 /*
+ * mount.cifs has been the subject of many "security" bugs that have arisen
+ * because of users and distributions installing it as a setuid root program.
+ * mount.cifs has not been audited for security. Thus, we strongly recommend
+ * that it not be installed setuid root. To make that abundantly clear,
+ * mount.cifs now check whether it's running setuid root and exit with an
+ * error if it is. If you wish to disable this check, then set the following
+ * #define to 1, but please realize that you do so at your own peril.
+ */
+#define CIFS_DISABLE_SETUID_CHECK 0
+
+/*
  * By default, mount.cifs follows the conventions set forth by /bin/mount
  * for user mounts. That is, it requires that the mount be listed in
  * /etc/fstab with the "user" option when run as an unprivileged user and
@@ -178,7 +189,7 @@ check_mountpoint(const char *progname, char *mountpoint)
 	struct stat statbuf;
 
 	/* does mountpoint exist and is it a directory? */
-	err = stat(mountpoint, &statbuf);
+	err = stat(".", &statbuf);
 	if (err) {
 		fprintf(stderr, "%s: failed to stat %s: %s\n", progname,
 				mountpoint, strerror(errno));
@@ -212,6 +223,29 @@ check_mountpoint(const char *progname, char *mountpoint)
 	return 0;
 }
 
+#if CIFS_DISABLE_SETUID_CHECK
+static int
+check_setuid(void)
+{
+	return 0;
+}
+#else /* CIFS_DISABLE_SETUID_CHECK */
+static int
+check_setuid(void)
+{
+	if (getuid() && !geteuid()) {
+		printf("This mount.cifs program has been built with the "
+			"ability to run as a setuid root program disabled.\n"
+			"mount.cifs has not been well audited for security "
+			"holes. Therefore the Samba team does not recommend "
+			"installing it as a setuid root program.\n");
+		return 1;
+	}
+
+	return 0;
+}
+#endif /* CIFS_DISABLE_SETUID_CHECK */
+
 #if CIFS_LEGACY_SETUID_CHECK
 static int
 check_fstab(const char *progname, char *mountpoint, char *devname,
@@ -1164,6 +1198,36 @@ static void print_cifs_mount_version(void)
 		MOUNT_CIFS_VENDOR_SUFFIX);
 }
 
+/*
+ * This function borrowed from fuse-utils...
+ *
+ * glibc's addmntent (at least as of 2.10 or so) doesn't properly encode
+ * newlines embedded within the text fields. To make sure no one corrupts
+ * the mtab, fail the mount if there are embedded newlines.
+ */
+static int check_newline(const char *progname, const char *name)
+{
+    char *s;
+    for (s = "\n"; *s; s++) {
+        if (strchr(name, *s)) {
+            fprintf(stderr, "%s: illegal character 0x%02x in mount entry\n",
+                    progname, *s);
+            return EX_USAGE;
+        }
+    }
+    return 0;
+}
+
+static int check_mtab(const char *progname, const char *devname,
+			const char *dir)
+{
+	if (check_newline(progname, devname) == -1 ||
+	    check_newline(progname, dir) == -1)
+		return EX_USAGE;
+	return 0;
+}
+
+
 int main(int argc, char ** argv)
 {
 	int c;
@@ -1196,6 +1260,9 @@ int main(int argc, char ** argv)
 	struct sockaddr_in6 *addr6;
 	FILE * pmntfile;
 
+	if (check_setuid())
+		return EX_USAGE;
+
 	/* setlocale(LC_ALL, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
 	textdomain(PACKAGE); */
@@ -1377,6 +1444,14 @@ int main(int argc, char ** argv)
 	}
 
 	/* make sure mountpoint is legit */
+	rc = chdir(mountpoint);
+	if (rc) {
+		fprintf(stderr, "Couldn't chdir to %s: %s\n", mountpoint,
+				strerror(errno));
+		rc = EX_USAGE;
+		goto mount_exit;
+	}
+
 	rc = check_mountpoint(thisprogram, mountpoint);
 	if (rc)
 		goto mount_exit;
@@ -1439,13 +1514,23 @@ int main(int argc, char ** argv)
 	
 	/* BB save off path and pop after mount returns? */
 	resolved_path = (char *)malloc(PATH_MAX+1);
-	if(resolved_path) {
-		/* Note that if we can not canonicalize the name, we get
-		another chance to see if it is valid when we chdir to it */
-		if (realpath(mountpoint, resolved_path)) {
-			mountpoint = resolved_path; 
-		}
+	if (!resolved_path) {
+		fprintf(stderr, "Unable to allocate memory.\n");
+		rc = EX_SYSERR;
+		goto mount_exit;
+	}
+
+	/* Note that if we can not canonicalize the name, we get
+	   another chance to see if it is valid when we chdir to it */
+	if(!realpath(".", resolved_path)) {
+		fprintf(stderr, "Unable to resolve %s to canonical path: %s\n",
+				mountpoint, strerror(errno));
+		rc = EX_SYSERR;
+		goto mount_exit;
 	}
+
+	mountpoint = resolved_path; 
+
 	if(got_user == 0) {
 		/* Note that the password will not be retrieved from the
 		   USER env variable (ie user%password form) as there is
@@ -1589,7 +1674,11 @@ mount_retry:
 	if (verboseflag)
 		fprintf(stderr, "\n");
 
-	if (!fakemnt && mount(dev_name, mountpoint, cifs_fstype, flags, options)) {
+	rc = check_mtab(thisprogram, dev_name, mountpoint);
+	if (rc)
+		goto mount_exit;
+
+	if (!fakemnt && mount(dev_name, ".", cifs_fstype, flags, options)) {
 		switch (errno) {
 		case ECONNREFUSED:
 		case EHOSTUNREACH:


-- 
Samba Shared Repository


More information about the samba-cvs mailing list