[linux-cifs-client] [PATCH] mount.cifs: properly check for mount being in fstab when running setuid root (try#3)
Shirish Pargaonkar
shirishpargaonkar at gmail.com
Wed Jul 22 10:42:16 MDT 2009
On Thu, Jun 4, 2009 at 7:50 PM, Jeff Layton<jlayton at 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 at 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 at 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!
More information about the linux-cifs-client
mailing list