| Message ID | 1244163031-9177-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 4 Jun 2009 20:50:31 -0400 Jeff Layton <jlayton@redhat.com> wrote: > This is the third attempt to clean up the checks when a setuid > mount.cifs is run by an unprivileged user. The main difference in this > patch from the last one is that it fixes a bug where the mount might > have failed if unnecessarily if CIFS_LEGACY_SETUID_CHECK was set. > > 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. > > Since some users may be depending on the legacy behavior, this patch > also adds the ability to build mount.cifs with the older behavior. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > client/mount.cifs.c | 202 +++++++++++++++++++++++++++++++++++++++++---------- > 1 files changed, 162 insertions(+), 40 deletions(-) > > diff --git a/client/mount.cifs.c b/client/mount.cifs.c > index 1b94486..f53bcf1 100644 > --- a/client/mount.cifs.c > +++ b/client/mount.cifs.c > @@ -39,10 +39,11 @@ > #include <mntent.h> > #include <fcntl.h> > #include <limits.h> > +#include <fstab.h> > #include "mount.h" > > #define MOUNT_CIFS_VERSION_MAJOR "1" > -#define MOUNT_CIFS_VERSION_MINOR "12" > +#define MOUNT_CIFS_VERSION_MINOR "13" > > #ifndef MOUNT_CIFS_VENDOR_SUFFIX > #ifdef _SAMBA_BUILD_ > @@ -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))) > @@ -83,6 +88,27 @@ > /* currently maximum length of IPv6 address string */ > #define MAX_ADDRESS_LEN INET6_ADDRSTRLEN > > +/* > + * 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 > + * mount.cifs is setuid root. > + * > + * Older versions of mount.cifs however were "looser" in this regard. When > + * made setuid root, a user could run mount.cifs directly and mount any share > + * on a directory owned by that user. > + * > + * The legacy behavior is now disabled by default. To reenable it, set the > + * following #define to true. > + */ > +#define CIFS_LEGACY_SETUID_CHECK 0 > + > +/* > + * When an unprivileged user runs a setuid mount.cifs, we set certain mount > + * flags by default. These defaults can be changed here. > + */ > +#define CIFS_SETUID_FLAGS (MS_NOSUID|MS_NODEV) > + > const char *thisprogram; > int verboseflag = 0; > int fakemnt = 0; > @@ -142,6 +168,99 @@ 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; > + } > + > +#if CIFS_LEGACY_SETUID_CHECK > + /* do extra checks on mountpoint for legacy setuid behavior */ > + if (!getuid() || geteuid()) > + return 0; > + > + if (statbuf.st_uid != getuid()) { > + fprintf(stderr, "%s: %s is not owned by user\n", progname, > + mountpoint); > + return EX_USAGE; > + } > + > + if ((statbuf.st_mode & S_IRWXU) != S_IRWXU) { > + fprintf(stderr, "%s: invalid permissions on %s\n", progname, > + mountpoint); > + return EX_USAGE; > + } > +#endif /* CIFS_LEGACY_SETUID_CHECK */ > + > + return 0; > +} > + > +#if CIFS_LEGACY_SETUID_CHECK > +static int > +check_fstab(const char *progname, char *mountpoint, char *devname, > + char **options) > +{ > + return 0; > +} > +#else /* CIFS_LEGACY_SETUID_CHECK */ > +static int > +check_fstab(const char *progname, char *mountpoint, char *devname, > + char **options) > +{ > + 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; > + } > + > + /* > + * '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; > +} > +#endif /* CIFS_LEGACY_SETUID_CHECK */ > + > /* BB finish BB > > cifs_umount > @@ -362,7 +481,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 +534,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 +543,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 +1146,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 +1169,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 +1226,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 +1365,22 @@ 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); > + if (rc) > + goto mount_exit; > + > + /* enable any default user mount flags */ > + flags |= CIFS_SETUID_FLAGS; > + } > + > if (getenv("PASSWD")) { > if(mountpassword == NULL) > mountpassword = (char *)calloc(MOUNT_PASSWD_SIZE+1,1); > @@ -1266,6 +1398,27 @@ int main(int argc, char ** argv) > rc = EX_USAGE; > goto mount_exit; > } > + > + if (getuid()) { > +#if !CIFS_LEGACY_SETUID_CHECK > + if (!(flags & (MS_USERS|MS_USER))) { > + fprintf(stderr, "%s: permission denied\n", thisprogram); > + rc = EX_USAGE; > + goto mount_exit; > + } > +#endif /* !CIFS_LEGACY_SETUID_CHECK */ > + > + 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 +1435,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 > -- No comments on last couple of these patches. I've gone ahead and pushed it to master.
On Thu, Jun 4, 2009 at 7:50 PM, Jeff Layton<jlayton@redhat.com> wrote: > This is the third attempt to clean up the checks when a setuid > mount.cifs is run by an unprivileged user. The main difference in this > patch from the last one is that it fixes a bug where the mount might > have failed if unnecessarily if CIFS_LEGACY_SETUID_CHECK was set. > > 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. > > Since some users may be depending on the legacy behavior, this patch > also adds the ability to build mount.cifs with the older behavior. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > Â client/mount.cifs.c | Â 202 +++++++++++++++++++++++++++++++++++++++++---------- > Â 1 files changed, 162 insertions(+), 40 deletions(-) > > diff --git a/client/mount.cifs.c b/client/mount.cifs.c > index 1b94486..f53bcf1 100644 > --- a/client/mount.cifs.c > +++ b/client/mount.cifs.c > @@ -39,10 +39,11 @@ > Â #include <mntent.h> > Â #include <fcntl.h> > Â #include <limits.h> > +#include <fstab.h> > Â #include "mount.h" > > Â #define MOUNT_CIFS_VERSION_MAJOR "1" > -#define MOUNT_CIFS_VERSION_MINOR "12" > +#define MOUNT_CIFS_VERSION_MINOR "13" > > Â #ifndef MOUNT_CIFS_VENDOR_SUFFIX > Â #ifdef _SAMBA_BUILD_ > @@ -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))) > @@ -83,6 +88,27 @@ > Â /* currently maximum length of IPv6 address string */ > Â #define MAX_ADDRESS_LEN INET6_ADDRSTRLEN > > +/* > + * 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 > + * mount.cifs is setuid root. > + * > + * Older versions of mount.cifs however were "looser" in this regard. When > + * made setuid root, a user could run mount.cifs directly and mount any share > + * on a directory owned by that user. > + * > + * The legacy behavior is now disabled by default. To reenable it, set the > + * following #define to true. > + */ > +#define CIFS_LEGACY_SETUID_CHECK 0 > + > +/* > + * When an unprivileged user runs a setuid mount.cifs, we set certain mount > + * flags by default. These defaults can be changed here. > + */ > +#define CIFS_SETUID_FLAGS (MS_NOSUID|MS_NODEV) > + > Â const char *thisprogram; > Â int verboseflag = 0; > Â int fakemnt = 0; > @@ -142,6 +168,99 @@ 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; > + Â Â Â } > + > +#if CIFS_LEGACY_SETUID_CHECK > + Â Â Â /* do extra checks on mountpoint for legacy setuid behavior */ > + Â Â Â if (!getuid() || geteuid()) > + Â Â Â Â Â Â Â return 0; > + > + Â Â Â if (statbuf.st_uid != getuid()) { > + Â Â Â Â Â Â Â fprintf(stderr, "%s: %s is not owned by user\n", progname, > + Â Â Â Â Â Â Â Â Â Â Â mountpoint); > + Â Â Â Â Â Â Â return EX_USAGE; > + Â Â Â } > + > + Â Â Â if ((statbuf.st_mode & S_IRWXU) != S_IRWXU) { > + Â Â Â Â Â Â Â fprintf(stderr, "%s: invalid permissions on %s\n", progname, > + Â Â Â Â Â Â Â Â Â Â Â mountpoint); > + Â Â Â Â Â Â Â return EX_USAGE; > + Â Â Â } > +#endif /* CIFS_LEGACY_SETUID_CHECK */ > + > + Â Â Â return 0; > +} > + > +#if CIFS_LEGACY_SETUID_CHECK > +static int > +check_fstab(const char *progname, char *mountpoint, char *devname, > + Â Â Â Â Â char **options) > +{ > + Â Â Â return 0; > +} > +#else /* CIFS_LEGACY_SETUID_CHECK */ > +static int > +check_fstab(const char *progname, char *mountpoint, char *devname, > + Â Â Â Â Â char **options) > +{ > + Â Â Â 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; > + Â Â Â } > + > + Â Â Â /* > + Â Â Â Â * '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; > +} > +#endif /* CIFS_LEGACY_SETUID_CHECK */ > + > Â /* BB finish BB > > Â Â Â Â cifs_umount > @@ -362,7 +481,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 +534,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 +543,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 +1146,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 +1169,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 +1226,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 +1365,22 @@ 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); > + Â Â Â Â Â Â Â if (rc) > + Â Â Â Â Â Â Â Â Â Â Â goto mount_exit; > + > + Â Â Â Â Â Â Â /* enable any default user mount flags */ > + Â Â Â Â Â Â Â flags |= CIFS_SETUID_FLAGS; > + Â Â Â } > + > Â Â Â Â if (getenv("PASSWD")) { > Â Â Â Â Â Â Â Â if(mountpassword == NULL) > Â Â Â Â Â Â Â Â Â Â Â Â mountpassword = (char *)calloc(MOUNT_PASSWD_SIZE+1,1); > @@ -1266,6 +1398,27 @@ int main(int argc, char ** argv) > Â Â Â Â Â Â Â Â rc = EX_USAGE; > Â Â Â Â Â Â Â Â goto mount_exit; > Â Â Â Â } > + > + Â Â Â if (getuid()) { > +#if !CIFS_LEGACY_SETUID_CHECK > + Â Â Â Â Â Â Â if (!(flags & (MS_USERS|MS_USER))) { > + Â Â Â Â Â Â Â Â Â Â Â fprintf(stderr, "%s: permission denied\n", thisprogram); > + Â Â Â Â Â Â Â Â Â Â Â rc = EX_USAGE; > + Â Â Â Â Â Â Â Â Â Â Â goto mount_exit; > + Â Â Â Â Â Â Â } > +#endif /* !CIFS_LEGACY_SETUID_CHECK */ > + > + Â Â Â Â Â Â Â 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 +1435,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 > > _______________________________________________ > linux-cifs-client mailing list > linux-cifs-client@lists.samba.org > https://lists.samba.org/mailman/listinfo/linux-cifs-client > Jeff, late comments... but as mentioned in Samba bugzilla bug 5118, it is confusing to use user (to allow user mounts) and user=<username> mount options in an entry in /etc/fstab. I do not see any other way but to allow/continue using them as they are. They are handled correctly, just that the same mount option getting used twice for different purposes can be confusing!
On Wed, Jul 22, 2009 at 12:01 PM, Jeff Layton<jlayton@redhat.com> wrote: > On Wed, 2009-07-22 at 11:42 -0500, Shirish Pargaonkar wrote: >> Jeff, >> >> late comments... but as mentioned in Samba bugzilla bug 5118, it is >> confusing to use >> user (to allow user mounts) and user=<username> mount options in an >> entry in /etc/fstab. >> I do not see any other way but to allow/continue using them as they are. >> They are handled correctly, just that the same mount option getting >> used twice for >> different purposes can be confusing! > > Agreed. The decision to use "user=" for specifying the username was not > a good one. I think smbfs used "username=", and I believe it was for > that reason. > > Unfortunately, we're sort of stuck with it for the time being. We could > start transitioning away from it but we'll need to come up with a > replacement option name (maybe username=) and set it up as an alias for > user=. username option is handled right now. > Then set it up so that user= generates a warning  at mount time > so that people can start moving their options over. Eventually we'd > deprecate that option altogether (make it cause the mount to error out). > > That'll probably take some time since we'll have to phase it in > gradually, probably over a year or more. > > -- > Jeff Layton <jlayton@redhat.com> > >
diff --git a/client/mount.cifs.c b/client/mount.cifs.c index 1b94486..f53bcf1 100644 --- a/client/mount.cifs.c +++ b/client/mount.cifs.c @@ -39,10 +39,11 @@ #include <mntent.h> #include <fcntl.h> #include <limits.h> +#include <fstab.h> #include "mount.h" #define MOUNT_CIFS_VERSION_MAJOR "1" -#define MOUNT_CIFS_VERSION_MINOR "12" +#define MOUNT_CIFS_VERSION_MINOR "13" #ifndef MOUNT_CIFS_VENDOR_SUFFIX #ifdef _SAMBA_BUILD_ @@ -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))) @@ -83,6 +88,27 @@ /* currently maximum length of IPv6 address string */ #define MAX_ADDRESS_LEN INET6_ADDRSTRLEN +/* + * 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 + * mount.cifs is setuid root. + * + * Older versions of mount.cifs however were "looser" in this regard. When + * made setuid root, a user could run mount.cifs directly and mount any share + * on a directory owned by that user. + * + * The legacy behavior is now disabled by default. To reenable it, set the + * following #define to true. + */ +#define CIFS_LEGACY_SETUID_CHECK 0 + +/* + * When an unprivileged user runs a setuid mount.cifs, we set certain mount + * flags by default. These defaults can be changed here. + */ +#define CIFS_SETUID_FLAGS (MS_NOSUID|MS_NODEV) + const char *thisprogram; int verboseflag = 0; int fakemnt = 0; @@ -142,6 +168,99 @@ 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; + } + +#if CIFS_LEGACY_SETUID_CHECK + /* do extra checks on mountpoint for legacy setuid behavior */ + if (!getuid() || geteuid()) + return 0; + + if (statbuf.st_uid != getuid()) { + fprintf(stderr, "%s: %s is not owned by user\n", progname, + mountpoint); + return EX_USAGE; + } + + if ((statbuf.st_mode & S_IRWXU) != S_IRWXU) { + fprintf(stderr, "%s: invalid permissions on %s\n", progname, + mountpoint); + return EX_USAGE; + } +#endif /* CIFS_LEGACY_SETUID_CHECK */ + + return 0; +} + +#if CIFS_LEGACY_SETUID_CHECK +static int +check_fstab(const char *progname, char *mountpoint, char *devname, + char **options) +{ + return 0; +} +#else /* CIFS_LEGACY_SETUID_CHECK */ +static int +check_fstab(const char *progname, char *mountpoint, char *devname, + char **options) +{ + 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; + } + + /* + * '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; +} +#endif /* CIFS_LEGACY_SETUID_CHECK */ + /* BB finish BB cifs_umount @@ -362,7 +481,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 +534,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 +543,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 +1146,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 +1169,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 +1226,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 +1365,22 @@ 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); + if (rc) + goto mount_exit; + + /* enable any default user mount flags */ + flags |= CIFS_SETUID_FLAGS; + } + if (getenv("PASSWD")) { if(mountpassword == NULL) mountpassword = (char *)calloc(MOUNT_PASSWD_SIZE+1,1); @@ -1266,6 +1398,27 @@ int main(int argc, char ** argv) rc = EX_USAGE; goto mount_exit; } + + if (getuid()) { +#if !CIFS_LEGACY_SETUID_CHECK + if (!(flags & (MS_USERS|MS_USER))) { + fprintf(stderr, "%s: permission denied\n", thisprogram); + rc = EX_USAGE; + goto mount_exit; + } +#endif /* !CIFS_LEGACY_SETUID_CHECK */ + + 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 +1435,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
This is the third attempt to clean up the checks when a setuid mount.cifs is run by an unprivileged user. The main difference in this patch from the last one is that it fixes a bug where the mount might have failed if unnecessarily if CIFS_LEGACY_SETUID_CHECK was set. 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. Since some users may be depending on the legacy behavior, this patch also adds the ability to build mount.cifs with the older behavior. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- client/mount.cifs.c | 202 +++++++++++++++++++++++++++++++++++++++++---------- 1 files changed, 162 insertions(+), 40 deletions(-)