[linux-cifs-client] [PATCH 17/19] mount.cifs: guard against signals by unprivileged users

[Jeff Layton]

On Fri, 26 Mar 2010 10:25:40 -0400
Jeff Layton <jlayton at samba.org> wrote:

> From: Jeff Layton <jlayton at 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 at redhat.com>
> ---
>  mount.cifs.c |   42 +++++++++++++++++++++++++++++++++++-------
>  1 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/mount.cifs.c b/mount.cifs.c
> index f4aea01..e292e40 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;
> @@ -1051,13 +1051,26 @@ static int check_mtab(const char *progname, const char *devname,
>  }
>  
>  static int
> -add_mtab(char *devname, char *mountpoint, unsigned long flags)
> +add_mtab(char *devname, char *mountpoint, unsigned long flags, uid_t uid)
>  {
>  	int rc = 0;
>  	char *mount_user;
>  	struct mntent mountent;
>  	FILE *pmntfile;
> +	sigset_t mask, oldmask;
>  
> +	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();
>  	if (rc) {
> @@ -1094,9 +1107,9 @@ 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 (uid != 0) {
>  			strlcat(mountent.mnt_opts, ",user=", MTAB_OPTIONS_LEN);
> -			mount_user = getusername();
> +			mount_user = getusername(uid);
>  			if (mount_user)
>  				strlcat(mountent.mnt_opts, mount_user,
>  					MTAB_OPTIONS_LEN);
> @@ -1109,6 +1122,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 +1218,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;
>  	}
> @@ -1259,6 +1273,7 @@ int main(int argc, char **argv)
>  	size_t dev_len;
>  	struct parsed_mount_info *parsed_info = NULL;
>  	pid_t pid;
> +	uid_t uid;
>  
>  	if (check_setuid())
>  		return EX_USAGE;
> @@ -1390,6 +1405,19 @@ int main(int argc, char **argv)
>  			goto mount_exit;
>  	}
>  
> +	/*
> +	 * 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.
> +	 */
> +	uid = getuid();
> +	rc = setreuid(geteuid(), -1);
> +	if (rc != 0) {
> +		fprintf(stderr, "Unable to set real uid to effective uid: %s\n",
> +				strerror(errno));
> +		rc = EX_USAGE;
> +	}
> +
>  	options = calloc(options_size, 1);
>  	if (!options) {
>  		fprintf(stderr, "Unable to allocate memory.\n");
> @@ -1500,7 +1528,7 @@ mount_retry:
>  	}
>  
>  	if (!parsed_info->nomtab)
> -		rc = add_mtab(dev_name, mountpoint, parsed_info->flags);
> +		rc = add_mtab(dev_name, mountpoint, parsed_info->flags, uid);
>  
>  mount_exit:
>  	if (parsed_info) {

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...
-- 
Jeff Layton <jlayton at samba.org>