Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1232769

Re: [PATCH v4 3/7] fs: Verify access of user towards block device file when mounting

Show all headers | View raw

On Thu, Sep 24, 2015 at 04:53:11PM -0500, Eric W. Biederman wrote:
> Seth Forshee <seth.forshee@canonical.com> writes:
> 
> > When mounting a filesystem on a block device there is currently
> > no verification that the user has appropriate access to the
> > device file passed to mount. This has not been an issue so far
> > since the user in question has always been root, but this must
> > be changed before allowing unprivileged users to mount in user
> > namespaces.
> >
> > To do this, a new version of lookup_bdev() is added named
> > lookup_bdev_perm(). Both of these functions become wrappers
> > around a common inner fucntion. The behavior of lookup_bdev() is
> > unchanged, but calling lookup_bdev_perm() will fail if the user
> > does not have the specified access rights to the supplied path.
> > The permission check is skipped if the user has CAP_SYS_ADMIN to
> > avoid any possible regressions in behavior.
> >
> > blkdev_get_by_path() is updated to use lookup_bdev_perm(). This
> > is used by mount_bdev() and mount_mtd(), so this will cause
> > mounts on block devices to fail when the user lacks the required
> > permissions. Other calls to blkdev_get_by_path() will all happen
> > with root privileges, so these calls will be unaffected.
> 
> Good but buggy patch.
> 
> In the mtd bits the flags are super flags, not file mode bits,
> which makes testing them against FMODE_READ and FMODE_WRITE is
> incorrect.

Bah, yes. Fixed.

> Further it looks like quite a few more possibly all of the lookup_bdev
> instances could use inode level permission checking.
> 
> Certainly code such as quotactl makes me wonder.

I opted to stick to places related to mounting, but let's take a look at
the other callers.

bcache calls it in the context of sysfs writes, and those attributes are
writable only by root. In that case the inode permission check will be
skipped anyway, so it makes no difference either way.

Device mapper calls it in dm_get_device, which is called from a bunch of
places. I had started trying to walk back through all the callers of
dm_get_device, but that rabbit hole got really deep really quickly so I
didn't feel confident that changing it wouldn't break anyone.

quotactl gave me pause, as it seems to have done for you too. I was
surprised that inode permissions aren't checked, but
check_quotactl_permission does get called before actually doing
anything. I fear that adding a check of inode permissions could end up
breaking someone.

> Ugh.  Your code needs to be using __inode_permission and not
> inode_permission.
> 
> The readability or writability of a device node has little or nothing to
> do with the readability/writability of the superblock.   So I don't see
> any reason we should be checking that.

Makes sense, fixed.

Thanks,
Seth

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v4 0/7] Initial support for user namespace owned mounts Seth Forshee <seth.forshee@canonical.com> - 2015-09-23 22:20 +0200
  [PATCH v4 4/7] fs: Limit file caps to the user namespace of the super block Seth Forshee <seth.forshee@canonical.com> - 2015-09-23 22:20 +0200
    Re: [PATCH v4 4/7] fs: Limit file caps to the user namespace of the super block ebiederm@xmission.com (Eric W. Biederman) - 2015-09-25 00:10 +0200
      Re: [PATCH v4 4/7] fs: Limit file caps to the user namespace of the  super block Seth Forshee <seth.forshee@canonical.com> - 2015-09-25 14:50 +0200
        Re: [PATCH v4 4/7] fs: Limit file caps to the user namespace of the super block ebiederm@xmission.com (Eric W. Biederman) - 2015-09-25 20:10 +0200
  [PATCH v4 5/7] fs: Treat foreign mounts as nosuid Seth Forshee <seth.forshee@canonical.com> - 2015-09-23 22:20 +0200
  [PATCH v4 3/7] fs: Verify access of user towards block device file when mounting Seth Forshee <seth.forshee@canonical.com> - 2015-09-23 22:20 +0200
    Re: [PATCH v4 3/7] fs: Verify access of user towards block device file when mounting ebiederm@xmission.com (Eric W. Biederman) - 2015-09-25 00:10 +0200
      Re: [PATCH v4 3/7] fs: Verify access of user towards block device  file when mounting Seth Forshee <seth.forshee@canonical.com> - 2015-09-25 14:50 +0200
        Re: [PATCH v4 3/7] fs: Verify access of user towards block device file when mounting ebiederm@xmission.com (Eric W. Biederman) - 2015-09-25 19:30 +0200
          Re: [PATCH v4 3/7] fs: Verify access of user towards block device  file when mounting Seth Forshee <seth.forshee@canonical.com> - 2015-09-25 19:40 +0200
            Re: [PATCH v4 3/7] fs: Verify access of user towards block device file when mounting ebiederm@xmission.com (Eric W. Biederman) - 2015-09-25 20:00 +0200
  [PATCH v4 7/7] selinux: Add support for unprivileged mounts from user namespaces Seth Forshee <seth.forshee@canonical.com> - 2015-09-23 22:20 +0200
  [PATCH v4 1/7] fs: Add user namesapace member to struct super_block Seth Forshee <seth.forshee@canonical.com> - 2015-09-23 22:20 +0200
    Re: [PATCH v4 1/7] fs: Add user namesapace member to struct super_block ebiederm@xmission.com (Eric W. Biederman) - 2015-09-24 23:30 +0200
      Re: [PATCH v4 1/7] fs: Add user namesapace member to struct  super_block Seth Forshee <seth.forshee@canonical.com> - 2015-09-25 15:00 +0200
        Re: [PATCH v4 1/7] fs: Add user namesapace member to struct super_block ebiederm@xmission.com (Eric W. Biederman) - 2015-09-25 19:40 +0200
  [PATCH v4 6/7] Smack: Add support for unprivileged mounts from user namespaces Seth Forshee <seth.forshee@canonical.com> - 2015-09-23 22:20 +0200
    Re: [PATCH v4 6/7] Smack: Add support for unprivileged mounts from user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2015-09-25 00:30 +0200
    Re: [PATCH v4 6/7] Smack: Add support for unprivileged mounts from  user namespaces Casey Schaufler <casey@schaufler-ca.com> - 2015-09-25 00:40 +0200
    Re: [PATCH v4 6/7] Smack: Add support for unprivileged mounts from user namespaces ebiederm@xmission.com (Eric W. Biederman) - 2015-09-27 21:40 +0200
      Re: [PATCH v4 6/7] Smack: Add support for unprivileged mounts from  user namespaces Seth Forshee <seth.forshee@canonical.com> - 2015-09-28 21:50 +0200

csiph-web