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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87d0e8g5f4.fsf@x220.int.ebiederm.org>
Date:   Sun, 03 Nov 2019 12:50:55 -0600
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Topi Miettinen <toiwoton@...il.com>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list\:FILESYSTEMS \(VFS and infrastructure\)" 
        <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] Allow restricting permissions in /proc/sys

Topi Miettinen <toiwoton@...il.com> writes:

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

This looks quite buggy.  You neither update table->mode nor
do you ever read from table->mode to initialize the inode.
I am missing something in my quick reading of your patch?

The not updating table->mode almost certainly means that as soon as the
cached inode is invalidated the mode changes will disappear.  Not to
mention they will fail to propogate between  different instances of
proc.

Loosing all of your changes at cache invalidation seems to make this a
useless feature.

Eric


> Signed-off-by: Topi Miettinen <toiwoton@...il.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;
>  }