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

Groups > linux.kernel > #1437142 > unrolled thread

[PATCH 0/5][RFC] Overlayfs SELinux Support

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5][RFC] Overlayfs SELinux Support Vivek Goyal <vgoyal@redhat.com> - 2016-07-05 18:00 +0200
    [PATCH 3/5] selinux: Pass security pointer to determine_inode_label() Vivek Goyal <vgoyal@redhat.com> - 2016-07-05 18:00 +0200
      Re: [PATCH 3/5] selinux: Pass security pointer to  determine_inode_label() Casey Schaufler <casey@schaufler-ca.com> - 2016-07-05 22:30 +0200
        Re: [PATCH 3/5] selinux: Pass security pointer to  determine_inode_label() Vivek Goyal <vgoyal@redhat.com> - 2016-07-05 23:10 +0200
    [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Vivek Goyal <vgoyal@redhat.com> - 2016-07-05 18:00 +0200
      Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Casey Schaufler <casey@schaufler-ca.com> - 2016-07-05 22:40 +0200
        Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Vivek Goyal <vgoyal@redhat.com> - 2016-07-05 23:20 +0200
          Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Miklos Szeredi <miklos@szeredi.hu> - 2016-07-06 06:40 +0200
            Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Vivek Goyal <vgoyal@redhat.com> - 2016-07-06 13:00 +0200
              Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Miklos Szeredi <miklos@szeredi.hu> - 2016-07-06 17:00 +0200
                Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Vivek Goyal <vgoyal@redhat.com> - 2016-07-07 20:40 +0200
                  Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Miklos Szeredi <miklos@szeredi.hu> - 2016-07-08 09:10 +0200
                    Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode Casey Schaufler <casey@schaufler-ca.com> - 2016-07-08 17:30 +0200

#1437142 — [PATCH 0/5][RFC] Overlayfs SELinux Support

Hi,

Following are RFC patches to support SELinux with overlayfs. I started
with David Howells's latest posting on this topic and started modifying
patches. These patches apply on top of overlayfs-next branch of miklos
vfs git tree.

git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git overlayfs-next

These patches can be pulled from my branch too.

https://github.com/rhvgoyal/linux/commits/overlayfs-selinux-mounter-next

Thanks to Dan Walsh, Stephen Smalley and Miklos Szeredi for numerous
conversation and ideas in helping figuring out what one reasonable
implementation might look like.

Dan Walsh has been writing tests for selinux overlayfs in selinux-testsuite.
These patches pass those tests except one. I think that test/policy need
to be fixed. 

https://github.com/rhatdan/selinux-testsuite/commits/master

Posting these patches for review and comments.

These patches introduce 3 new security hooks.

- security_inode_copy_up(), is called when a file is copied up. This hook
  prepares a new set of cred which is used for copy up operation. And
  new set of creds are prepared so that ->create_sid can be set appropriately
  and newly created file is labeled properly. 

  When a file is copied up, label of lower file is retained except for the
  case of context= mount where new file gets the label from context= option.

- security_inode_copy_up_xattr(), is called when xattrs of a file are
  being copied up. Before this we already called security_inode_copy_up()
  and created new file and copied up data. That means file already got
  labeled properly and there is no need to take SELINUX xattr of lower
  file and overwrite the upper file xattr. So this hook is used to avoid
  copying up of SELINUX xattr.

- dentry_create_files_as(), is called when a new file is about to be created.
  This hook determines what the label of the file should be if task had
  created that file in upper/ and sets create_sid accordingly in the passed
  in creds.

  Normal transition rules don't work for the case of context mounts as
  underlying file system is not aware of context option which only overlay
  layer is aware of. For non-context mounts, creation can happen in work/
  dir first and then file might be renamed into upper/, and it might get
  label based on work/ dir. So this hooks helps avoiding all these issues.

  When a new file is created in upper/, it gets its label based on transition
  rules. For the case of context mount, it gets the label from context=
  option.

Apart from hooks, also changed overlay code to not do getxattr checks on
underlying inode so that overlay inode selinux label does not fail
initializaiton.
 
Any feedback is welcome.

Thanks
Vivek

Vivek Goyal (5):
  security, overlayfs: provide copy up security hook for unioned files
  security,overlayfs: Provide security hook for copy up of xattrs for
    overlay file
  selinux: Pass security pointer to determine_inode_label()
  overlayfs: Correctly label newly created file over whiteout
  overlayfs: Use vfs_getxattr_noperm() for real inode

 fs/overlayfs/copy_up.c    | 16 ++++++++++
 fs/overlayfs/dir.c        | 10 ++++++
 fs/overlayfs/inode.c      |  7 +----
 fs/xattr.c                | 28 +++++++++++------
 include/linux/lsm_hooks.h | 41 ++++++++++++++++++++++++
 include/linux/security.h  | 28 +++++++++++++++++
 include/linux/xattr.h     |  1 +
 security/security.c       | 28 +++++++++++++++++
 security/selinux/hooks.c  | 80 ++++++++++++++++++++++++++++++++++++++++++-----
 9 files changed, 216 insertions(+), 23 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1437143 — [PATCH 3/5] selinux: Pass security pointer to determine_inode_label()

Right now selinux_determine_inode_label() works on security pointer of
current task. Soon I need this to work on a security pointer retrieved
from a set of creds. So start passing in a pointer and caller can decide
where to fetch security pointer from.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 security/selinux/hooks.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c68223c..86a07ed 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1785,13 +1785,13 @@ out:
 /*
  * Determine the label for an inode that might be unioned.
  */
-static int selinux_determine_inode_label(struct inode *dir,
-					 const struct qstr *name,
-					 u16 tclass,
+static int selinux_determine_inode_label(const void *security,
+					 struct inode *dir,
+					 const struct qstr *name, u16 tclass,
 					 u32 *_new_isid)
 {
 	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
-	const struct task_security_struct *tsec = current_security();
+	const struct task_security_struct *tsec = security;
 
 	if ((sbsec->flags & SE_SBINITIALIZED) &&
 	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
@@ -1834,8 +1834,8 @@ static int may_create(struct inode *dir,
 	if (rc)
 		return rc;
 
-	rc = selinux_determine_inode_label(dir, &dentry->d_name, tclass,
-					   &newsid);
+	rc = selinux_determine_inode_label(current_security(), dir,
+					   &dentry->d_name, tclass, &newsid);
 	if (rc)
 		return rc;
 
@@ -2815,7 +2815,8 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
 	u32 newsid;
 	int rc;
 
-	rc = selinux_determine_inode_label(d_inode(dentry->d_parent), name,
+	rc = selinux_determine_inode_label(current_security(),
+					   d_inode(dentry->d_parent), name,
 					   inode_mode_to_security_class(mode),
 					   &newsid);
 	if (rc)
@@ -2840,7 +2841,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 	sid = tsec->sid;
 	newsid = tsec->create_sid;
 
-	rc = selinux_determine_inode_label(
+	rc = selinux_determine_inode_label(current_security(),
 		dir, qstr,
 		inode_mode_to_security_class(inode->i_mode),
 		&newsid);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1437261 — Re: [PATCH 3/5] selinux: Pass security pointer to determine_inode_label()

On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> Right now selinux_determine_inode_label() works on security pointer of
> current task. Soon I need this to work on a security pointer retrieved
> from a set of creds. So start passing in a pointer and caller can decide
> where to fetch security pointer from.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  security/selinux/hooks.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c68223c..86a07ed 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1785,13 +1785,13 @@ out:
>  /*
>   * Determine the label for an inode that might be unioned.
>   */
> -static int selinux_determine_inode_label(struct inode *dir,
> -					 const struct qstr *name,
> -					 u16 tclass,
> +static int selinux_determine_inode_label(const void *security,

You know the type. Why not use it?

	static int selinux_determine_inode_label(const struct task_security_struct *tsec,

> +					 struct inode *dir,
> +					 const struct qstr *name, u16 tclass,
>  					 u32 *_new_isid)
>  {
>  	const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
> -	const struct task_security_struct *tsec = current_security();
> +	const struct task_security_struct *tsec = security;
>  
>  	if ((sbsec->flags & SE_SBINITIALIZED) &&
>  	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> @@ -1834,8 +1834,8 @@ static int may_create(struct inode *dir,
>  	if (rc)
>  		return rc;
>  
> -	rc = selinux_determine_inode_label(dir, &dentry->d_name, tclass,
> -					   &newsid);
> +	rc = selinux_determine_inode_label(current_security(), dir,
> +					   &dentry->d_name, tclass, &newsid);
>  	if (rc)
>  		return rc;
>  
> @@ -2815,7 +2815,8 @@ static int selinux_dentry_init_security(struct dentry *dentry, int mode,
>  	u32 newsid;
>  	int rc;
>  
> -	rc = selinux_determine_inode_label(d_inode(dentry->d_parent), name,
> +	rc = selinux_determine_inode_label(current_security(),
> +					   d_inode(dentry->d_parent), name,
>  					   inode_mode_to_security_class(mode),
>  					   &newsid);
>  	if (rc)
> @@ -2840,7 +2841,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
>  	sid = tsec->sid;
>  	newsid = tsec->create_sid;
>  
> -	rc = selinux_determine_inode_label(
> +	rc = selinux_determine_inode_label(current_security(),
>  		dir, qstr,
>  		inode_mode_to_security_class(inode->i_mode),
>  		&newsid);

[toc] | [prev] | [next] | [standalone]

#1437284 — Re: [PATCH 3/5] selinux: Pass security pointer to determine_inode_label()

On Tue, Jul 05, 2016 at 01:25:22PM -0700, Casey Schaufler wrote:
> On 7/5/2016 8:50 AM, Vivek Goyal wrote:
> > Right now selinux_determine_inode_label() works on security pointer of
> > current task. Soon I need this to work on a security pointer retrieved
> > from a set of creds. So start passing in a pointer and caller can decide
> > where to fetch security pointer from.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  security/selinux/hooks.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index c68223c..86a07ed 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1785,13 +1785,13 @@ out:
> >  /*
> >   * Determine the label for an inode that might be unioned.
> >   */
> > -static int selinux_determine_inode_label(struct inode *dir,
> > -					 const struct qstr *name,
> > -					 u16 tclass,
> > +static int selinux_determine_inode_label(const void *security,
> 
> You know the type. Why not use it?
> 
> 	static int selinux_determine_inode_label(const struct task_security_struct *tsec,

Will change it. All callers use current_security() to fetch this pointer
and it returns void * and I guess I assumed that compiler will complain
but it does not seem to complain. 

Vivek

[toc] | [prev] | [next] | [standalone]

#1437144 — [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

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

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

[toc] | [prev] | [next] | [standalone]

#1437264 — Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

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

[toc] | [prev] | [next] | [standalone]

#1437290 — Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

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

[toc] | [prev] | [next] | [standalone]

#1437407 — Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

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

[toc] | [prev] | [next] | [standalone]

#1437634 — Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

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

[toc] | [prev] | [next] | [standalone]

#1437784 — Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

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

[toc] | [prev] | [next] | [standalone]

#1438794 — Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

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

[toc] | [prev] | [next] | [standalone]

#1439144 — Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

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

[toc] | [prev] | [next] | [standalone]

#1439566 — Re: [PATCH 5/5] overlayfs: Use vfs_getxattr_noperm() for real inode

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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web