[5/5] overlayfs: Use vfs_getxattr_noperm() for real inode
diff mbox

Message ID 1467733854-6314-6-git-send-email-vgoyal@redhat.com
State New
Headers show

Commit Message

Vivek Goyal July 5, 2016, 3:50 p.m. UTC
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(-)

Comments

Casey Schaufler July 5, 2016, 8:29 p.m. UTC | #1
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-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivek Goyal July 5, 2016, 9:16 p.m. UTC | #2
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-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Miklos Szeredi July 6, 2016, 4:36 a.m. UTC | #3
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-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivek Goyal July 6, 2016, 10:54 a.m. UTC | #4
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-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Miklos Szeredi July 6, 2016, 2:58 p.m. UTC | #5
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-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vivek Goyal July 7, 2016, 6:35 p.m. UTC | #6
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-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Miklos Szeredi July 8, 2016, 7:06 a.m. UTC | #7
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-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Casey Schaufler July 8, 2016, 3:28 p.m. UTC | #8
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-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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);