From patchwork Sun Mar 28 13:34:10 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 48779 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.samba.org (fn.samba.org [216.83.154.106]) by ozlabs.org (Postfix) with ESMTP id 2E912B7CED for ; Mon, 29 Mar 2010 00:34:19 +1100 (EST) Received: from fn.samba.org (localhost [127.0.0.1]) by lists.samba.org (Postfix) with ESMTP id E7BB1AD1D7; Sun, 28 Mar 2010 07:34:18 -0600 (MDT) X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on fn.samba.org X-Spam-Level: X-Spam-Status: No, score=-1.4 required=3.8 tests=AWL, BAYES_00, NO_MORE_FUNN, SPF_NEUTRAL autolearn=no version=3.2.5 X-Original-To: linux-cifs-client@lists.samba.org Delivered-To: linux-cifs-client@lists.samba.org Received: from cdptpa-omtalb.mail.rr.com (cdptpa-omtalb.mail.rr.com [75.180.132.121]) by lists.samba.org (Postfix) with ESMTP id 50E87AD11F for ; Sun, 28 Mar 2010 07:34:13 -0600 (MDT) X-Authority-Analysis: v=1.0 c=1 a=x8hHiHbFPtQA:10 a=hGzw-44bAAAA:8 a=20KFwNOVAAAA:8 a=vhpdd7w497lKl4EGWKsA:9 a=-JOXYazsRJ_sWQOD4s0A:7 a=0JWP1vphobk7lL47aO3nIR80qKkA:4 a=CjuIK1q_8ugA:10 a=dowx1zmaLagA:10 a=jEp0ucaQiEUA:10 a=MdrFE29ugB6E9VW3gtgA:9 a=seWJjxEtD56ioQtKylsA:7 a=0efHgnJ7YebCpX6dPJsDdf9BJRsA:4 X-Cloudmark-Score: 0 X-Originating-IP: 71.70.153.3 Received: from [71.70.153.3] ([71.70.153.3:51445] helo=mail.poochiereds.net) by cdptpa-oedge04.mail.rr.com (envelope-from ) (ecelerity 2.2.2.39 r()) with ESMTP id F6/F9-19488-3DA5FAB4; Sun, 28 Mar 2010 13:34:12 +0000 Received: from tlielax.poochiereds.net (tlielax.poochiereds.net [192.168.1.3]) by mail.poochiereds.net (Postfix) with ESMTPS id 5C2C358057 for ; Sun, 28 Mar 2010 09:34:11 -0400 (EDT) Date: Sun, 28 Mar 2010 09:34:10 -0400 From: Jeff Layton To: linux-cifs-client@lists.samba.org Message-ID: <20100328093410.507ee18b@tlielax.poochiereds.net> In-Reply-To: <1269613542-6402-18-git-send-email-jlayton@samba.org> References: <1269613542-6402-1-git-send-email-jlayton@samba.org> <1269613542-6402-18-git-send-email-jlayton@samba.org> X-Mailer: Claws Mail 3.7.5 (GTK+ 2.19.7; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Subject: Re: [linux-cifs-client] [PATCH 17/19] mount.cifs: guard against signals by unprivileged users X-BeenThere: linux-cifs-client@lists.samba.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: The Linux CIFS VFS client List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-cifs-client-bounces@lists.samba.org Errors-To: linux-cifs-client-bounces@lists.samba.org On Fri, 26 Mar 2010 10:25:40 -0400 Jeff Layton wrote: > From: Jeff Layton > > 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 > --- > 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... From 591ee3ccb5821a4dbc20b4ffd37f000c5c48c417 Mon Sep 17 00:00:00 2001 From: Jeff Layton Date: Sun, 28 Mar 2010 09:16:10 -0400 Subject: [PATCH] mount.cifs: guard against signals by unprivileged users 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 --- mount.cifs.c | 38 +++++++++++++++++++++++++++++++++----- 1 files changed, 33 insertions(+), 5 deletions(-) diff --git a/mount.cifs.c b/mount.cifs.c index f4aea01..4cbcc61 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; @@ -1054,10 +1054,37 @@ static int add_mtab(char *devname, char *mountpoint, unsigned long flags) { int rc = 0; + uid_t uid; char *mount_user; struct mntent mountent; FILE *pmntfile; + sigset_t mask, oldmask; + /* + * 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_FILEIO; + } + + 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 +1121,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 +1136,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 +1232,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; } -- 1.6.6.1