From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1B6D3C43331 for ; Tue, 12 Nov 2019 23:25:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D595521925 for ; Tue, 12 Nov 2019 23:25:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="WVfBl5Z8" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727022AbfKLXZ0 (ORCPT ); Tue, 12 Nov 2019 18:25:26 -0500 Received: from mail-pg1-f194.google.com ([209.85.215.194]:43265 "EHLO mail-pg1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726910AbfKLXZ0 (ORCPT ); Tue, 12 Nov 2019 18:25:26 -0500 Received: by mail-pg1-f194.google.com with SMTP id l24so14956pgh.10 for ; Tue, 12 Nov 2019 15:25:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=v/05ewFGIu4TLNi9D2OL0wDmLlrmscOb/YagM09gAds=; b=WVfBl5Z8fLpT/F7hpsoQIz/hJisIbSVPjS10XwLIRzWXm3MQ1kPy0fTIHhjTFBbjoS icV23ZNFHmpydn/hnhXxmB3q4f4ZInOJ772AccCmjb8sc8u4v1LS8cPpS1UZ63887C2/ Jj4N5qkGo/1svgbFeMSpc6RsMo9iFpoLBHM+Y= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=v/05ewFGIu4TLNi9D2OL0wDmLlrmscOb/YagM09gAds=; b=kTcv5rimtR0VvrHyjg6taYEqCk2uYt9tSXkIXVd+mWfu6VYbYKyO14TBzhBOP3CxE1 iGyxKSbhgju1whSphEq3I8aBiNGHHuGNmoQNs+2dvZocS4so+uTomtnVvA7j3rL19PXZ folcCyGUcCNIVzmrTrNRZdG5SR1K5EyR6tCzsO0ULvWOLf3HGH94l2ywlxVIbz3z79jG 7YE/wjvO69rW4IhitDv71xSxFhq0/WHYi+Mkoph3BhSZ/WFxekNDT6lnF1/dN0bg79I4 L/Dpr14OGE+5beT/uJn0mM8AOMay5L9TEwwdueA/ciW4o7Z7X9cu4zYueNQL82m4Wu/m IEhA== X-Gm-Message-State: APjAAAUYscVFqyszqtCC4mG6yQ/FbcZVkAMB3YRb2wwB3vA6pIW23iee nFrYkartN08CeFGF9nsF3mhnPw== X-Google-Smtp-Source: APXvYqyBx5mL5/sNTTkpUmdCOW/egLPUqOpRV5xLG78w/QrLUZolYDx4HfZ4l10HpUCpqdk4kHuVrQ== X-Received: by 2002:a17:90a:86c3:: with SMTP id y3mr524359pjv.102.1573601125368; Tue, 12 Nov 2019 15:25:25 -0800 (PST) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id x2sm70930pge.76.2019.11.12.15.25.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Nov 2019 15:25:24 -0800 (PST) Date: Tue, 12 Nov 2019 15:25:23 -0800 From: Kees Cook To: Topi Miettinen Cc: Luis Chamberlain , Alexey Dobriyan , "linux-kernel@vger.kernel.org" , "open list:FILESYSTEMS (VFS and infrastructure)" , "Eric W. Biederman" Subject: Re: [PATCH] proc: Allow restricting permissions in /proc/sys Message-ID: <201911121523.9C097E7D2C@keescook> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Ah! I see the v2 here now. :) Can you please include that in your Subject next time, as "[PATCH v2] proc: Allow restricting permissions in /proc/sys"? Also, can you adjust your MUA to not send a duplicate attachment? The patch inline is fine. Please CC akpm as well, since I think this should likely go through the -mm tree. Eric, do you have any other thoughts on this? Thanks! -Kees On Mon, Nov 04, 2019 at 02:07:29PM +0200, Topi Miettinen wrote: > Several items in /proc/sys need not be accessible to unprivileged > tasks. Let the system administrator change the permissions, but only > to more restrictive modes than what the sysctl tables allow. > > Signed-off-by: Topi Miettinen > --- > v2: actually keep track of changed permissions instead of relying on inode > cache > --- > fs/proc/proc_sysctl.c | 42 ++++++++++++++++++++++++++++++++++++++---- > include/linux/sysctl.h | 1 + > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index d80989b6c344..1f75382c49fd 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -818,6 +818,10 @@ static int proc_sys_permission(struct inode *inode, int > mask) > if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode)) > return -EACCES; > > + error = generic_permission(inode, mask); > + if (error) > + return error; > + > head = grab_header(inode); > if (IS_ERR(head)) > return PTR_ERR(head); > @@ -835,17 +839,46 @@ static int proc_sys_permission(struct inode *inode, > int mask) > static int proc_sys_setattr(struct dentry *dentry, struct iattr *attr) > { > struct inode *inode = d_inode(dentry); > + struct ctl_table_header *head = grab_header(inode); > + struct ctl_table *table = PROC_I(inode)->sysctl_entry; > int error; > > - if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) > + if (attr->ia_valid & (ATTR_UID | ATTR_GID)) > return -EPERM; > > + if (attr->ia_valid & ATTR_MODE) { > + umode_t max_mode = 0777; /* Only these bits may change */ > + > + if (IS_ERR(head)) > + return PTR_ERR(head); > + > + if (!table) /* global root - r-xr-xr-x */ > + max_mode &= ~0222; > + else /* > + * Don't allow permissions to become less > + * restrictive than the sysctl table entry > + */ > + max_mode &= table->mode; > + > + /* Execute bits only allowed for directories */ > + if (!S_ISDIR(inode->i_mode)) > + max_mode &= ~0111; > + > + if (attr->ia_mode & ~S_IFMT & ~max_mode) > + return -EPERM; > + } > + > error = setattr_prepare(dentry, attr); > if (error) > return error; > > setattr_copy(inode, attr); > mark_inode_dirty(inode); > + > + if (table) > + table->current_mode = inode->i_mode; > + sysctl_head_finish(head); > + > return 0; > } > > @@ -861,7 +894,7 @@ static int proc_sys_getattr(const struct path *path, > struct kstat *stat, > > generic_fillattr(inode, stat); > if (table) > - stat->mode = (stat->mode & S_IFMT) | table->mode; > + stat->mode = (stat->mode & S_IFMT) | table->current_mode; > > sysctl_head_finish(head); > return 0; > @@ -981,7 +1014,7 @@ static struct ctl_dir *new_dir(struct ctl_table_set > *set, > memcpy(new_name, name, namelen); > new_name[namelen] = '\0'; > table[0].procname = new_name; > - table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO; > + table[0].current_mode = table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO; > init_header(&new->header, set->dir.header.root, set, node, table); > > return new; > @@ -1155,6 +1188,7 @@ static int sysctl_check_table(const char *path, struct > ctl_table *table) > if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode) > err |= sysctl_err(path, table, "bogus .mode 0%o", > table->mode); > + table->current_mode = table->mode; > } > return err; > } > @@ -1192,7 +1226,7 @@ static struct ctl_table_header *new_links(struct > ctl_dir *dir, struct ctl_table > int len = strlen(entry->procname) + 1; > memcpy(link_name, entry->procname, len); > link->procname = link_name; > - link->mode = S_IFLNK|S_IRWXUGO; > + link->current_mode = link->mode = S_IFLNK|S_IRWXUGO; > link->data = link_root; > link_name += len; > } > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index 6df477329b76..7c519c35bf9c 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -126,6 +126,7 @@ struct ctl_table > void *data; > int maxlen; > umode_t mode; > + umode_t current_mode; > struct ctl_table *child; /* Deprecated */ > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; > -- > 2.24.0.rc1 > > From 3cde64e0aa2734c335355ee6d0d9f12c1f1e8a87 Mon Sep 17 00:00:00 2001 > From: Topi Miettinen > Date: Sun, 3 Nov 2019 16:36:43 +0200 > Subject: [PATCH] proc: Allow restricting permissions in /proc/sys > > Several items in /proc/sys need not be accessible to unprivileged > tasks. Let the system administrator change the permissions, but only > to more restrictive modes than what the sysctl tables allow. > > Signed-off-by: Topi Miettinen > --- > fs/proc/proc_sysctl.c | 42 ++++++++++++++++++++++++++++++++++++++---- > include/linux/sysctl.h | 1 + > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index d80989b6c344..1f75382c49fd 100644 > --- a/fs/proc/proc_sysctl.c > +++ b/fs/proc/proc_sysctl.c > @@ -818,6 +818,10 @@ static int proc_sys_permission(struct inode *inode, int mask) > if ((mask & MAY_EXEC) && S_ISREG(inode->i_mode)) > return -EACCES; > > + error = generic_permission(inode, mask); > + if (error) > + return error; > + > head = grab_header(inode); > if (IS_ERR(head)) > return PTR_ERR(head); > @@ -835,17 +839,46 @@ static int proc_sys_permission(struct inode *inode, int mask) > static int proc_sys_setattr(struct dentry *dentry, struct iattr *attr) > { > struct inode *inode = d_inode(dentry); > + struct ctl_table_header *head = grab_header(inode); > + struct ctl_table *table = PROC_I(inode)->sysctl_entry; > int error; > > - if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) > + if (attr->ia_valid & (ATTR_UID | ATTR_GID)) > return -EPERM; > > + if (attr->ia_valid & ATTR_MODE) { > + umode_t max_mode = 0777; /* Only these bits may change */ > + > + if (IS_ERR(head)) > + return PTR_ERR(head); > + > + if (!table) /* global root - r-xr-xr-x */ > + max_mode &= ~0222; > + else /* > + * Don't allow permissions to become less > + * restrictive than the sysctl table entry > + */ > + max_mode &= table->mode; > + > + /* Execute bits only allowed for directories */ > + if (!S_ISDIR(inode->i_mode)) > + max_mode &= ~0111; > + > + if (attr->ia_mode & ~S_IFMT & ~max_mode) > + return -EPERM; > + } > + > error = setattr_prepare(dentry, attr); > if (error) > return error; > > setattr_copy(inode, attr); > mark_inode_dirty(inode); > + > + if (table) > + table->current_mode = inode->i_mode; > + sysctl_head_finish(head); > + > return 0; > } > > @@ -861,7 +894,7 @@ static int proc_sys_getattr(const struct path *path, struct kstat *stat, > > generic_fillattr(inode, stat); > if (table) > - stat->mode = (stat->mode & S_IFMT) | table->mode; > + stat->mode = (stat->mode & S_IFMT) | table->current_mode; > > sysctl_head_finish(head); > return 0; > @@ -981,7 +1014,7 @@ static struct ctl_dir *new_dir(struct ctl_table_set *set, > memcpy(new_name, name, namelen); > new_name[namelen] = '\0'; > table[0].procname = new_name; > - table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO; > + table[0].current_mode = table[0].mode = S_IFDIR|S_IRUGO|S_IXUGO; > init_header(&new->header, set->dir.header.root, set, node, table); > > return new; > @@ -1155,6 +1188,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table) > if ((table->mode & (S_IRUGO|S_IWUGO)) != table->mode) > err |= sysctl_err(path, table, "bogus .mode 0%o", > table->mode); > + table->current_mode = table->mode; > } > return err; > } > @@ -1192,7 +1226,7 @@ static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table > int len = strlen(entry->procname) + 1; > memcpy(link_name, entry->procname, len); > link->procname = link_name; > - link->mode = S_IFLNK|S_IRWXUGO; > + link->current_mode = link->mode = S_IFLNK|S_IRWXUGO; > link->data = link_root; > link_name += len; > } > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index 6df477329b76..7c519c35bf9c 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -126,6 +126,7 @@ struct ctl_table > void *data; > int maxlen; > umode_t mode; > + umode_t current_mode; > struct ctl_table *child; /* Deprecated */ > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; > -- > 2.24.0.rc1 > -- Kees Cook