On Tue, Jul 05, 2016 at 01:29:39PM -0700, Casey Schaufler wrote:
> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> > ovl_getxattr() currently uses vfs_getxattr() on realinode. This fails
> > if mounter does not have DAC/MAC permission to access getxattr.
> >
> > Specifically this becomes a problem when selinux is trying to initialize
> > overlay inode and does ->getxattr(overlay_inode). A task might trigger
> > initialization of overlay inode and we will access real inode xattr in the
> > context of mounter and if mounter does not have permissions, then inode
> > selinux context initialization fails and inode is labeled as unlabeled_t.
> >
> > One way to deal with it is to let SELinux do getxattr checks both on
> > overlay inode and underlying inode and overlay can call vfs_getxattr_noperm()
> > to make sure when selinux is trying to initialize label on inode, it does
> > not go through checks on lower levels and initialization is successful.
> > And after inode initialization, SELinux will make sure task has getatttr
> > permission.
> >
> > One issue with this approach is that it does not work for directories as
> > d_real() returns the overlay dentry for directories and not the underlying
> > directory dentry.
> >
> > Another way to deal with it to introduce another function pointer in
> > inode_operations, say getxattr_noperm(), which is responsible to get
> > xattr without any checks. SELinux initialization code will call this
> > first if it is available on inode. So user space code path will call
> > ->getxattr() and that will go through checks and SELinux internal
> > initialization will call ->getxattr_noperm() and that will not
> > go through checks.
> >
> > For now, I am just converting ovl_getxattr() to get xattr without
> > any checks on underlying inode. That means it is possible for
> > a task to get xattr of a file/dir on lower/upper through overlay mount
> > while it is not possible outside overlay mount.
> >
> > If this is a major concern, I can look into implementing getxattr_noperm().
>
> This is a major concern.
Hmm.., In that case I will write patch to provide another inode operation
getxattr_noperm() and a wrapper which falls back to getxattr() if noperm
variant is not defined. That should take care of this issue.
Thanks
Vivek
>
> >
> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> > ---
> > fs/overlayfs/inode.c | 7 +------
> > fs/xattr.c | 28 +++++++++++++++++++---------
> > include/linux/xattr.h | 1 +
> > 3 files changed, 21 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> > index 36dfd86..a5d3320 100644
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -233,16 +233,11 @@ ssize_t ovl_getxattr(struct dentry *dentry, struct inode *inode,
> > const char *name, void *value, size_t size)
> > {
> > struct dentry *realdentry = ovl_dentry_real(dentry);
> > - ssize_t sz;
> > - const struct cred *old_cred;
> >
> > if (ovl_is_private_xattr(name))
> > return -ENODATA;
> >
> > - old_cred = ovl_override_creds(dentry->d_sb);
> > - sz = vfs_getxattr(realdentry, name, value, size);
> > - revert_creds(old_cred);
> > - return size;
> > + return vfs_getxattr_noperm(realdentry, name, value, size);
> > }
> >
> > ssize_t ovl_listxattr(struct dentry *dentry, char *list, size_t size)
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index 4beafc4..973e18c 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -209,19 +209,11 @@ vfs_getxattr_alloc(struct dentry *dentry, const char *name, char **xattr_value,
> > }
> >
> > ssize_t
> > -vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> > +vfs_getxattr_noperm(struct dentry *dentry, const char *name, void *value, size_t size)
> > {
> > struct inode *inode = dentry->d_inode;
> > int error;
> >
> > - error = xattr_permission(inode, name, MAY_READ);
> > - if (error)
> > - return error;
> > -
> > - error = security_inode_getxattr(dentry, name);
> > - if (error)
> > - return error;
> > -
> > if (!strncmp(name, XATTR_SECURITY_PREFIX,
> > XATTR_SECURITY_PREFIX_LEN)) {
> > const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> > @@ -242,6 +234,24 @@ nolsm:
> >
> > return error;
> > }
> > +EXPORT_SYMBOL_GPL(vfs_getxattr_noperm);
> > +
> > +ssize_t
> > +vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
> > +{
> > + struct inode *inode = dentry->d_inode;
> > + int error;
> > +
> > + error = xattr_permission(inode, name, MAY_READ);
> > + if (error)
> > + return error;
> > +
> > + error = security_inode_getxattr(dentry, name);
> > + if (error)
> > + return error;
> > +
> > + return vfs_getxattr_noperm(dentry, name, value, size);
> > +}
> > EXPORT_SYMBOL_GPL(vfs_getxattr);
> >
> > ssize_t
> > diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> > index 94079ba..832a6b6 100644
> > --- a/include/linux/xattr.h
> > +++ b/include/linux/xattr.h
> > @@ -47,6 +47,7 @@ struct xattr {
> >
> > ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
> > ssize_t vfs_getxattr(struct dentry *, const char *, void *, size_t);
> > +ssize_t vfs_getxattr_noperm(struct dentry *, const char *, void *, size_t);
> > ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
> > int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, size_t, int);
> > int vfs_setxattr(struct dentry *, const char *, const void *, size_t, int);
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
