* Re: [PATCH] Allow restricting permissions in /proc/sys [not found] <74a91362-247c-c749-5200-7bdce704ed9e@gmail.com> @ 2019-11-12 23:22 ` Christian Brauner 2019-11-13 4:50 ` Andy Lutomirski 2019-11-13 16:00 ` Jann Horn 0 siblings, 2 replies; 6+ messages in thread From: Christian Brauner @ 2019-11-12 23:22 UTC (permalink / raw) To: Topi Miettinen Cc: Luis Chamberlain, Kees Cook, Alexey Dobriyan, linux-kernel@vger.kernel.org, open list:FILESYSTEMS (VFS and infrastructure), linux-api [Cc+ linux-api@vger.kernel.org] since that's potentially relevant to quite a few people. On Sun, Nov 03, 2019 at 04:55:48PM +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 <toiwoton@gmail.com> > --- > fs/proc/proc_sysctl.c | 41 +++++++++++++++++++++++++++++++---------- > 1 file changed, 31 insertions(+), 10 deletions(-) > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > index d80989b6c344..88c4ca7d2782 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); > @@ -837,9 +841,35 @@ static int proc_sys_setattr(struct dentry *dentry, > struct iattr *attr) > struct inode *inode = d_inode(dentry); > 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) { > + struct ctl_table_header *head = grab_header(inode); > + struct ctl_table *table = PROC_I(inode)->sysctl_entry; > + 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; > + > + sysctl_head_finish(head); > + > + /* 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; > @@ -853,17 +883,8 @@ static int proc_sys_getattr(const struct path *path, > struct kstat *stat, > u32 request_mask, unsigned int query_flags) > { > struct inode *inode = d_inode(path->dentry); > - struct ctl_table_header *head = grab_header(inode); > - struct ctl_table *table = PROC_I(inode)->sysctl_entry; > - > - if (IS_ERR(head)) > - return PTR_ERR(head); > > generic_fillattr(inode, stat); > - if (table) > - stat->mode = (stat->mode & S_IFMT) | table->mode; > - > - sysctl_head_finish(head); > return 0; > } > > -- > 2.24.0.rc1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Allow restricting permissions in /proc/sys 2019-11-12 23:22 ` [PATCH] Allow restricting permissions in /proc/sys Christian Brauner @ 2019-11-13 4:50 ` Andy Lutomirski 2019-11-13 10:52 ` Topi Miettinen 2019-11-13 16:00 ` Jann Horn 1 sibling, 1 reply; 6+ messages in thread From: Andy Lutomirski @ 2019-11-13 4:50 UTC (permalink / raw) To: Christian Brauner Cc: Topi Miettinen, Luis Chamberlain, Kees Cook, Alexey Dobriyan, linux-kernel@vger.kernel.org, open list:FILESYSTEMS (VFS and infrastructure), Linux API On Tue, Nov 12, 2019 at 3:22 PM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > [Cc+ linux-api@vger.kernel.org] > > since that's potentially relevant to quite a few people. > > On Sun, Nov 03, 2019 at 04:55:48PM +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 <toiwoton@gmail.com> > > --- > > fs/proc/proc_sysctl.c | 41 +++++++++++++++++++++++++++++++---------- > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > > index d80989b6c344..88c4ca7d2782 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); > > @@ -837,9 +841,35 @@ static int proc_sys_setattr(struct dentry *dentry, > > struct iattr *attr) > > struct inode *inode = d_inode(dentry); > > int error; > > > > - if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) > > + if (attr->ia_valid & (ATTR_UID | ATTR_GID)) > > return -EPERM; Supporting at least ATTR_GID would make this much more useful. > > > > + if (attr->ia_valid & ATTR_MODE) { > > + struct ctl_table_header *head = grab_header(inode); > > + struct ctl_table *table = PROC_I(inode)->sysctl_entry; > > + 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; Style nit: please put braces around multi-line if and else branches, even if they're only multi-line because of comments. > > + > > + sysctl_head_finish(head); > > + > > + /* Execute bits only allowed for directories */ > > + if (!S_ISDIR(inode->i_mode)) > > + max_mode &= ~0111; Why is this needed? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Allow restricting permissions in /proc/sys 2019-11-13 4:50 ` Andy Lutomirski @ 2019-11-13 10:52 ` Topi Miettinen 0 siblings, 0 replies; 6+ messages in thread From: Topi Miettinen @ 2019-11-13 10:52 UTC (permalink / raw) To: Andy Lutomirski, Christian Brauner Cc: Luis Chamberlain, Kees Cook, Alexey Dobriyan, linux-kernel@vger.kernel.org, open list:FILESYSTEMS (VFS and infrastructure), Linux API, Andrew Morton On 13.11.2019 6.50, Andy Lutomirski wrote: > On Tue, Nov 12, 2019 at 3:22 PM Christian Brauner > <christian.brauner@ubuntu.com> wrote: >> >> [Cc+ linux-api@vger.kernel.org] >> >> since that's potentially relevant to quite a few people. >> >> On Sun, Nov 03, 2019 at 04:55:48PM +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 <toiwoton@gmail.com> >>> --- >>> fs/proc/proc_sysctl.c | 41 +++++++++++++++++++++++++++++++---------- >>> 1 file changed, 31 insertions(+), 10 deletions(-) >>> >>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c >>> index d80989b6c344..88c4ca7d2782 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); >>> @@ -837,9 +841,35 @@ static int proc_sys_setattr(struct dentry *dentry, >>> struct iattr *attr) >>> struct inode *inode = d_inode(dentry); >>> int error; >>> >>> - if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) >>> + if (attr->ia_valid & (ATTR_UID | ATTR_GID)) >>> return -EPERM; > > Supporting at least ATTR_GID would make this much more useful. Yes, also XATTR/ACL support would be useful. But so far I've tried to allow only tightening of permissions. >>> >>> + if (attr->ia_valid & ATTR_MODE) { >>> + struct ctl_table_header *head = grab_header(inode); >>> + struct ctl_table *table = PROC_I(inode)->sysctl_entry; >>> + 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; > > Style nit: please put braces around multi-line if and else branches, > even if they're only multi-line because of comments. OK, thanks. >>> + >>> + sysctl_head_finish(head); >>> + >>> + /* Execute bits only allowed for directories */ >>> + if (!S_ISDIR(inode->i_mode)) >>> + max_mode &= ~0111; > > Why is this needed? > In general, /proc/sys does not allow executable permissions for the files, so I've continued this policy. -Topi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Allow restricting permissions in /proc/sys 2019-11-12 23:22 ` [PATCH] Allow restricting permissions in /proc/sys Christian Brauner 2019-11-13 4:50 ` Andy Lutomirski @ 2019-11-13 16:00 ` Jann Horn 2019-11-13 16:19 ` Topi Miettinen 1 sibling, 1 reply; 6+ messages in thread From: Jann Horn @ 2019-11-13 16:00 UTC (permalink / raw) To: Topi Miettinen Cc: Luis Chamberlain, Kees Cook, Alexey Dobriyan, linux-kernel@vger.kernel.org, open list:FILESYSTEMS (VFS and infrastructure), Linux API, Christian Brauner On Wed, Nov 13, 2019 at 12:22 AM Christian Brauner <christian.brauner@ubuntu.com> wrote: > On Sun, Nov 03, 2019 at 04:55:48PM +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 <toiwoton@gmail.com> > > --- > > fs/proc/proc_sysctl.c | 41 +++++++++++++++++++++++++++++++---------- > > 1 file changed, 31 insertions(+), 10 deletions(-) > > > > diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c > > index d80989b6c344..88c4ca7d2782 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; In kernel/ucount.c, the ->permissions handler set_permissions() grants access based on whether the caller has CAP_SYS_RESOURCE. And in net/sysctl_net.c, the handler net_ctl_permissions() grants access based on whether the caller has CAP_NET_ADMIN. This added check is going to break those, right? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Allow restricting permissions in /proc/sys 2019-11-13 16:00 ` Jann Horn @ 2019-11-13 16:19 ` Topi Miettinen 2019-11-13 16:40 ` Jann Horn 0 siblings, 1 reply; 6+ messages in thread From: Topi Miettinen @ 2019-11-13 16:19 UTC (permalink / raw) To: Jann Horn Cc: Luis Chamberlain, Kees Cook, Alexey Dobriyan, linux-kernel@vger.kernel.org, open list:FILESYSTEMS (VFS and infrastructure), Linux API, Christian Brauner On 13.11.2019 18.00, Jann Horn wrote: > On Wed, Nov 13, 2019 at 12:22 AM Christian Brauner > <christian.brauner@ubuntu.com> wrote: >> On Sun, Nov 03, 2019 at 04:55:48PM +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 <toiwoton@gmail.com> >>> --- >>> fs/proc/proc_sysctl.c | 41 +++++++++++++++++++++++++++++++---------- >>> 1 file changed, 31 insertions(+), 10 deletions(-) >>> >>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c >>> index d80989b6c344..88c4ca7d2782 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; > > In kernel/ucount.c, the ->permissions handler set_permissions() grants > access based on whether the caller has CAP_SYS_RESOURCE. And in > net/sysctl_net.c, the handler net_ctl_permissions() grants access > based on whether the caller has CAP_NET_ADMIN. This added check is > going to break those, right? > Right. The comment above seems then a bit misleading: /* * sysctl entries that are not writeable, * are _NOT_ writeable, capabilities or not. */ -Topi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Allow restricting permissions in /proc/sys 2019-11-13 16:19 ` Topi Miettinen @ 2019-11-13 16:40 ` Jann Horn 0 siblings, 0 replies; 6+ messages in thread From: Jann Horn @ 2019-11-13 16:40 UTC (permalink / raw) To: Topi Miettinen Cc: Luis Chamberlain, Kees Cook, Alexey Dobriyan, linux-kernel@vger.kernel.org, open list:FILESYSTEMS (VFS and infrastructure), Linux API, Christian Brauner On Wed, Nov 13, 2019 at 5:19 PM Topi Miettinen <toiwoton@gmail.com> wrote: > On 13.11.2019 18.00, Jann Horn wrote: > > On Wed, Nov 13, 2019 at 12:22 AM Christian Brauner > > <christian.brauner@ubuntu.com> wrote: > >> On Sun, Nov 03, 2019 at 04:55:48PM +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. [...] > > In kernel/ucount.c, the ->permissions handler set_permissions() grants > > access based on whether the caller has CAP_SYS_RESOURCE. And in > > net/sysctl_net.c, the handler net_ctl_permissions() grants access > > based on whether the caller has CAP_NET_ADMIN. This added check is > > going to break those, right? > > > > Right. The comment above seems then a bit misleading: > /* > * sysctl entries that are not writeable, > * are _NOT_ writeable, capabilities or not. > */ I don't see the problem. Those handlers never make a file writable that doesn't have one of the three write bits (0222) set. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-13 16:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <74a91362-247c-c749-5200-7bdce704ed9e@gmail.com>
2019-11-12 23:22 ` [PATCH] Allow restricting permissions in /proc/sys Christian Brauner
2019-11-13 4:50 ` Andy Lutomirski
2019-11-13 10:52 ` Topi Miettinen
2019-11-13 16:00 ` Jann Horn
2019-11-13 16:19 ` Topi Miettinen
2019-11-13 16:40 ` Jann Horn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).