| Message ID | 1467733854-6314-6-git-send-email-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > 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. That's not going to fly. A slighly better, but still quite ugly solution would be to add a "flags" arg to the current ->getxattr() callback indicating whether the caller wants permission checking inside the call or not. But we already have the current->creds. Can't that be used to control the permission checking done by the callback? Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote: > On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > > 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. > > That's not going to fly. A slighly better, but still quite ugly > solution would be to add a "flags" arg to the current ->getxattr() > callback indicating whether the caller wants permission checking > inside the call or not. > Ok, will try that. > But we already have the current->creds. Can't that be used to control > the permission checking done by the callback? Sorry, did not get how to use current->creds to control permission checking. Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 6, 2016 at 12:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote: >> On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote: >> > 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. >> >> That's not going to fly. A slighly better, but still quite ugly >> solution would be to add a "flags" arg to the current ->getxattr() >> callback indicating whether the caller wants permission checking >> inside the call or not. >> > > Ok, will try that. > >> But we already have the current->creds. Can't that be used to control >> the permission checking done by the callback? > > Sorry, did not get how to use current->creds to control permission > checking. I'm not sure about the details either. But current->creds *is* the context provided for the VFS and filesystems to check permissions. It might make sense to use that to indicate to overlayfs that permission should not be checked. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 06, 2016 at 04:58:37PM +0200, Miklos Szeredi wrote: > On Wed, Jul 6, 2016 at 12:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > > On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote: > >> On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > >> > 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. > >> > >> That's not going to fly. A slighly better, but still quite ugly > >> solution would be to add a "flags" arg to the current ->getxattr() > >> callback indicating whether the caller wants permission checking > >> inside the call or not. > >> > > > > Ok, will try that. > > > >> But we already have the current->creds. Can't that be used to control > >> the permission checking done by the callback? > > > > Sorry, did not get how to use current->creds to control permission > > checking. > > I'm not sure about the details either. But current->creds *is* the > context provided for the VFS and filesystems to check permissions. It > might make sense to use that to indicate to overlayfs that permission > should not be checked. That sounds like raising capabilities of task temporarily to do getxattr(). But AFAIK, there is no cap which will override SELinux checks. I am taking a step back re-thinking about the problem. - For context mounts this is not a problem at all as overlay inode will get its label from context= mount option and we will not call into ovl_getxattr(). - For non-context mounts this is a problem only if mounter is not privileged enough to do getattr. And that's not going to be a common case either. IOW, this does not look like a common case. And if getxattr() fails, SELinux already seems to mark inode as unlabeled_t. And my understanding is that task can't access unlabeled_t anyway, so there is no information leak. So for now, why not leave it as it is. Only side affect I seem to see is following warnings on console. SELinux: inode_doinit_with_dentry: getxattr returned 13 for dev=overlay ino=29147 This is for information purposes only and given getxattr() can fail in stacked configuration, I think we can change this to KERN_DEBUG instead of KERN_WARNING. Thanks Vivek -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 7, 2016 at 8:35 PM, Vivek Goyal <vgoyal@redhat.com> wrote: > On Wed, Jul 06, 2016 at 04:58:37PM +0200, Miklos Szeredi wrote: >> On Wed, Jul 6, 2016 at 12:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote: >> > On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote: >> >> On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote: >> >> > 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. >> >> >> >> That's not going to fly. A slighly better, but still quite ugly >> >> solution would be to add a "flags" arg to the current ->getxattr() >> >> callback indicating whether the caller wants permission checking >> >> inside the call or not. >> >> >> > >> > Ok, will try that. >> > >> >> But we already have the current->creds. Can't that be used to control >> >> the permission checking done by the callback? >> > >> > Sorry, did not get how to use current->creds to control permission >> > checking. >> >> I'm not sure about the details either. But current->creds *is* the >> context provided for the VFS and filesystems to check permissions. It >> might make sense to use that to indicate to overlayfs that permission >> should not be checked. > > That sounds like raising capabilities of task temporarily to do > getxattr(). But AFAIK, there is no cap which will override SELinux checks. So a new capability can be invented for this purpose? > I am taking a step back re-thinking about the problem. > > - For context mounts this is not a problem at all as overlay inode will > get its label from context= mount option and we will not call into > ovl_getxattr(). > > - For non-context mounts this is a problem only if mounter is not > privileged enough to do getattr. And that's not going to be a common > case either. > > IOW, this does not look like a common case. And if getxattr() fails, > SELinux already seems to mark inode as unlabeled_t. And my understanding > is that task can't access unlabeled_t anyway, so there is no information > leak. > > So for now, why not leave it as it is. Only side affect I seem to see > is following warnings on console. > > SELinux: inode_doinit_with_dentry: getxattr returned 13 for dev=overlay ino=29147 > > This is for information purposes only and given getxattr() can fail in > stacked configuration, I think we can change this to KERN_DEBUG instead > of KERN_WARNING. I'm fine with this as well. Thanks, Miklos -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/8/2016 12:06 AM, Miklos Szeredi wrote: > On Thu, Jul 7, 2016 at 8:35 PM, Vivek Goyal <vgoyal@redhat.com> wrote: >> On Wed, Jul 06, 2016 at 04:58:37PM +0200, Miklos Szeredi wrote: >>> On Wed, Jul 6, 2016 at 12:54 PM, Vivek Goyal <vgoyal@redhat.com> wrote: >>>> On Wed, Jul 06, 2016 at 06:36:49AM +0200, Miklos Szeredi wrote: >>>>> On Tue, Jul 5, 2016 at 11:16 PM, Vivek Goyal <vgoyal@redhat.com> wrote: >>>>>> 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. >>>>> That's not going to fly. A slighly better, but still quite ugly >>>>> solution would be to add a "flags" arg to the current ->getxattr() >>>>> callback indicating whether the caller wants permission checking >>>>> inside the call or not. >>>>> >>>> Ok, will try that. >>>> >>>>> But we already have the current->creds. Can't that be used to control >>>>> the permission checking done by the callback? >>>> Sorry, did not get how to use current->creds to control permission >>>> checking. >>> I'm not sure about the details either. But current->creds *is* the >>> context provided for the VFS and filesystems to check permissions. It >>> might make sense to use that to indicate to overlayfs that permission >>> should not be checked. >> That sounds like raising capabilities of task temporarily to do >> getxattr(). But AFAIK, there is no cap which will override SELinux checks. > So a new capability can be invented for this purpose? SELinux does not use capabilities as an override mechanism. The capability you would want if it did is CAP_MAC_OVERRIDE, which is used by Smack. > >> I am taking a step back re-thinking about the problem. >> >> - For context mounts this is not a problem at all as overlay inode will >> get its label from context= mount option and we will not call into >> ovl_getxattr(). >> >> - For non-context mounts this is a problem only if mounter is not >> privileged enough to do getattr. And that's not going to be a common >> case either. >> >> IOW, this does not look like a common case. And if getxattr() fails, >> SELinux already seems to mark inode as unlabeled_t. And my understanding >> is that task can't access unlabeled_t anyway, so there is no information >> leak. >> >> So for now, why not leave it as it is. Only side affect I seem to see >> is following warnings on console. >> >> SELinux: inode_doinit_with_dentry: getxattr returned 13 for dev=overlay ino=29147 >> >> This is for information purposes only and given getxattr() can fail in >> stacked configuration, I think we can change this to KERN_DEBUG instead >> of KERN_WARNING. > I'm fine with this as well. > > Thanks, > Miklos > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(). Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/overlayfs/inode.c | 7 +------ fs/xattr.c | 28 +++++++++++++++++++--------- include/linux/xattr.h | 1 + 3 files changed, 21 insertions(+), 15 deletions(-)