lkml.org 
[lkml]   [2021]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH v5 13/16] ima: Move some IMA policy and filesystem related variables into ima_namespace
    From

    On 12/13/21 10:50, Christian Brauner wrote:
    > On Mon, Dec 13, 2021 at 10:33:40AM -0500, Stefan Berger wrote:
    >> On 12/11/21 04:50, Christian Brauner wrote:
    >>> On Fri, Dec 10, 2021 at 08:57:11AM -0500, Stefan Berger wrote:
    >>>>
    >>>> there anything that would prevent us from setns()'ing to that target user
    >>>> namespace so that we would now see that of a user namespace that we are not
    >>>> allowed to see?
    >>> If you're really worried about someone being able to access a securityfs
    >>> instance whose userns doesn't match the userns the securityfs instance
    >>> was mounted in there are multiple ways to fix it. The one that I tend to
    >>> prefer is:
    >>>
    >>> From e0ff6a8dcc573763568e685dd70d1547efd68df9 Mon Sep 17 00:00:00 2001
    >>> From: Christian Brauner <christian.brauner@ubuntu.com>
    >>> Date: Fri, 10 Dec 2021 11:47:37 +0100
    >>> Subject: !!!! HERE BE DRAGONS - COMPLETELY UNTESTED !!!!
    >>>
    >>> securityfs: only allow access to securityfs from within same namespace
    >>>
    >>> Limit opening of securityfs files to callers located in the same namespace.
    >>>
    >>> ---
    >>> security/inode.c | 33 +++++++++++++++++++++++++++++++--
    >>> 1 file changed, 31 insertions(+), 2 deletions(-)
    >>>
    >>> diff --git a/security/inode.c b/security/inode.c
    >>> index eaccba7017d9..9eaf757c08cb 100644
    >>> --- a/security/inode.c
    >>> +++ b/security/inode.c
    >>> @@ -80,6 +80,35 @@ static struct file_system_type fs_type = {
    >>> .fs_flags = FS_USERNS_MOUNT,
    >>> };
    >>> +static int securityfs_permission(struct user_namespace *mnt_userns,
    >>> + struct inode *inode, int mask)
    >>> +{
    >>> + int err;
    >>> +
    >>> + err = generic_permission(&init_user_ns, inode, mask);
    >>> + if (!err) {
    >>> + if (inode->i_sb->s_user_ns != current_user_ns())
    >>> + err = -EACCES;
    >>> + }
    >>> +
    >>> + return err;
    >>> +}
    >>> +
    >>> +const struct inode_operations securityfs_dir_inode_operations = {
    >>> + .permission = securityfs_permission,
    >>> + .lookup = simple_lookup,
    >>> +};
    >>> +
    >>> +const struct file_operations securityfs_dir_operations = {
    >>> + .permission = securityfs_permission,
    >>
    >> This interface function on file operations doesn't exist.
    > It's almost as if the subject line of this patch warned about its draft
    > character. That was supposed for regular files.
    >
    >> I'll use the inode_operations and also hook it to the root dentry of the
    >> super_block. Then there's no need to have this check on symlinks and
    >> files...
    > Don't special case the inode_operations for the root inode!

    I modified the inode_operations *also* for the root node, since that one
    is initialized with &simple_dir_inode_operationsby simple_fill_super, so
    I didn't want to miss it...


    > If a privileged process opens an fd refering to a struct file for the
    > root inode and leaks it to an unprivileged process by accident the
    > unprivileged process can open any file or directory beneath via openat()
    > and friends.
    >
    > Symlinks don't need a .permission handler anyway because they just
    > contain the name of another file and that is checked for .permission
    > unless you have a separate .getlink handler where you want to restrict
    > link contents further.
    >
    > But regular files need to have a .permission method see openat().

    So here's what I have now for the hardening.


    diff --git a/security/inode.c b/security/inode.c
    index fee01ff4d831..a0d9f086e3d5 100644
    --- a/security/inode.c
    +++ b/security/inode.c
    @@ -26,6 +26,29 @@
     static struct vfsmount *init_securityfs_mount;
     static int init_securityfs_mount_count;

    +static int securityfs_permission(struct user_namespace *mnt_userns,
    +                                struct inode *inode, int mask)
    +{
    +       int err;
    +
    +       err = generic_permission(&init_user_ns, inode, mask);
    +       if (!err) {
    +               if (inode->i_sb->s_user_ns != current_user_ns())
    +                       err = -EACCES;
    +       }
    +
    +       return err;
    +}
    +
    +const struct inode_operations securityfs_dir_inode_operations = {
    +       .permission     = securityfs_permission,
    +       .lookup         = simple_lookup,
    +};
    +
    +const struct inode_operations securityfs_file_inode_operations = {
    +       .permission     = securityfs_permission,
    +};
    +
     static void securityfs_free_inode(struct inode *inode)
     {
            if (S_ISLNK(inode->i_mode))
    @@ -41,20 +64,25 @@ static const struct super_operations
    securityfs_super_operations = {
     static int securityfs_fill_super(struct super_block *sb, struct
    fs_context *fc)
     {
            static const struct tree_descr files[] = {{""}};
    +       struct user_namespace *ns = fc->user_ns;
            int error;

    +       if (WARN_ON(ns != current_user_ns()))
    +               return -EINVAL;
    +
            error = simple_fill_super(sb, SECURITYFS_MAGIC, files);
            if (error)
                    return error;

            sb->s_op = &securityfs_super_operations;
    +       sb->s_root->d_inode->i_op = &securityfs_dir_inode_operations;

            return 0;
     }
    [...]
    @@ -157,7 +186,7 @@ static struct dentry *securityfs_create_dentry(const
    char *name, umode_t mode,
            inode->i_atime = inode->i_mtime = inode->i_ctime =
    current_time(inode);
            inode->i_private = data;
            if (S_ISDIR(mode)) {
    -               inode->i_op = &simple_dir_inode_operations;
    +               inode->i_op = &securityfs_dir_inode_operations;
                    inode->i_fop = &simple_dir_operations;
                    inc_nlink(inode);
                    inc_nlink(dir);
    @@ -165,10 +194,10 @@ static struct dentry
    *securityfs_create_dentry(const char *name, umode_t mode,
                    inode->i_op = iops ? iops :
    &simple_symlink_inode_operations;
                    inode->i_link = data;
            } else {
    +               inode->i_op = &securityfs_file_inode_operations;
                    inode->i_fop = fops;
            }
            d_instantiate(dentry, inode);

    \
     
     \ /
      Last update: 2021-12-13 17:28    [W:3.680 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site