Re: [PATCH] Allow restricting permissions in /proc/sys

6 messages, 4 authors, 2019-11-13 · open the first message on its own page

Re: [PATCH] Allow restricting permissions in /proc/sys

From: Christian Brauner <hidden>
Date: 2019-11-12 23:22:50 

[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:
quoted hunk
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 <redacted>
---
 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

Re: [PATCH] Allow restricting permissions in /proc/sys

From: Andy Lutomirski <luto@kernel.org>
Date: 2019-11-13 04:51:07 

On Tue, Nov 12, 2019 at 3:22 PM Christian Brauner
[off-list ref] 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:
quoted
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 <redacted>
---
 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.
quoted

+       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.
quoted
+
+               sysctl_head_finish(head);
+
+               /* Execute bits only allowed for directories */
+               if (!S_ISDIR(inode->i_mode))
+                       max_mode &= ~0111;
Why is this needed?

Re: [PATCH] Allow restricting permissions in /proc/sys

From: Topi Miettinen <hidden>
Date: 2019-11-13 10:52:08 

On 13.11.2019 6.50, Andy Lutomirski wrote:
On Tue, Nov 12, 2019 at 3:22 PM Christian Brauner
[off-list ref] wrote:
quoted
[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:
quoted
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 <redacted>
---
  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.
quoted
quoted

+       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.
quoted
quoted
+
+               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

Re: [PATCH] Allow restricting permissions in /proc/sys

From: Jann Horn <jannh@google.com>
Date: 2019-11-13 16:16:47 

On Wed, Nov 13, 2019 at 12:22 AM Christian Brauner
[off-list ref] wrote:
On Sun, Nov 03, 2019 at 04:55:48PM +0200, Topi Miettinen wrote:
quoted
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 <redacted>
---
 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?

Re: [PATCH] Allow restricting permissions in /proc/sys

From: Topi Miettinen <hidden>
Date: 2019-11-13 16:19:25 

On 13.11.2019 18.00, Jann Horn wrote:
On Wed, Nov 13, 2019 at 12:22 AM Christian Brauner
[off-list ref] wrote:
quoted
On Sun, Nov 03, 2019 at 04:55:48PM +0200, Topi Miettinen wrote:
quoted
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 <redacted>
---
  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

Re: [PATCH] Allow restricting permissions in /proc/sys

From: Jann Horn <jannh@google.com>
Date: 2019-11-13 16:41:07 

On Wed, Nov 13, 2019 at 5:19 PM Topi Miettinen [off-list ref] wrote:
On 13.11.2019 18.00, Jann Horn wrote:
quoted
On Wed, Nov 13, 2019 at 12:22 AM Christian Brauner
[off-list ref] wrote:
quoted
On Sun, Nov 03, 2019 at 04:55:48PM +0200, Topi Miettinen wrote:
quoted
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.
[...]
quoted
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help