Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup.

Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup.

[Vivek Goyal]

CCing linux-fsdevel as it is a wider issue.


On Wed, Feb 24, 2016 at 02:55:52PM +0100, Ignacy Gawędzki wrote:
> Add alternate lookup_one_len_check function to fs/namei.c which does
> what lookup_one_len did until now with a boolean argument telling
> whether to check that the base directory is traversable.  Modify
> original lookup_one_len function to call the former with true as the
> last argument.
> 
> In function ovl_lookup_real, file fs/overlayfs/super.c, call
> lookup_one_len_check with false as the last argument, so that failure
> to traverse the base directory does not return -EACCES.  This should
> make lookup resolution work properly in the following setup
> 
>   drwxr-xr-x lower/
>   drwx------ lower/foo/
>   drw-r--r-- lower/boo/bar
>   drwxr-xr-x upper/
>   drwxr-xr-x upper/foo/
> 
> when any user not being the owner of lower/foo is trying to access
> foo/bar in the mounted overlay.

So what's the problem we are trying to solve. Why should we able to
override the DAC checks of lower layer if same directory in upper
is searchable for user but it is not searchable in lower layer. 

Thanks
Vivek

> 
> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
> ---
>  fs/namei.c            | 29 +++++++++++++++++++++++++----
>  fs/overlayfs/super.c  |  2 +-
>  include/linux/namei.h |  1 +
>  3 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index f624d13..f9486da 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2278,6 +2278,25 @@ EXPORT_SYMBOL(vfs_path_lookup);
>   */
>  struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
>  {
> +	return lookup_one_len_check(name, base, len, true);
> +}
> +EXPORT_SYMBOL(lookup_one_len);
> +
> +/**
> + * lookup_one_len_check - filesystem helper to lookup single pathname component
> + * @name:	pathname component to lookup
> + * @base:	base directory to lookup from
> + * @len:	maximum length @len should be interpreted to
> + * @check:	whether to check that @base is itself traversable.
> + *
> + * Note that this routine is purely a helper for filesystem usage and should
> + * not be called by generic code.
> + *
> + * The caller must hold base->i_mutex.
> + */
> +struct dentry *lookup_one_len_check(const char *name, struct dentry *base,
> +				    int len, bool check_base)
> +{
>  	struct qstr this;
>  	unsigned int c;
>  	int err;
> @@ -2310,13 +2329,15 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
>  			return ERR_PTR(err);
>  	}
>  
> -	err = inode_permission(base->d_inode, MAY_EXEC);
> -	if (err)
> -		return ERR_PTR(err);
> +	if (check_base) {
> +		err = inode_permission(base->d_inode, MAY_EXEC);
> +		if (err)
> +			return ERR_PTR(err);
> +	}
>  
>  	return __lookup_hash(&this, base, 0);
>  }
> -EXPORT_SYMBOL(lookup_one_len);
> +EXPORT_SYMBOL(lookup_one_len_check);
>  
>  /**
>   * lookup_one_len_unlocked - filesystem helper to lookup single pathname component
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 8d826bd..965c5dd 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -376,7 +376,7 @@ static inline struct dentry *ovl_lookup_real(struct dentry *dir,
>  	struct dentry *dentry;
>  
>  	inode_lock(dir->d_inode);
> -	dentry = lookup_one_len(name->name, dir, name->len);
> +	dentry = lookup_one_len_check(name->name, dir, name->len, false);
>  	inode_unlock(dir->d_inode);
>  
>  	if (IS_ERR(dentry)) {
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index d0f25d8..0af6775 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -77,6 +77,7 @@ extern struct dentry *kern_path_locked(const char *, struct path *);
>  extern int kern_path_mountpoint(int, const char *, struct path *, unsigned int);
>  
>  extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
> +extern struct dentry *lookup_one_len_check(const char *, struct dentry *, int, bool);
>  extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int);
>  
>  extern int follow_down_one(struct path *);
> -- 
> 2.5.0
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup.

[Nazarov Sergey]

26.02.2016, 22:41, "Vivek Goyal" <vgoyal@redhat.com>:
>
> So what's the problem we are trying to solve. Why should we able to
> override the DAC checks of lower layer if same directory in upper
> is searchable for user but it is not searchable in lower layer.
>

If I right, this is a one of the main feature of overlayfs - upper layer has priority over lower ones.
Override AC checks necessary for lookup operation only. Lower layer files access AC checks
remain, so this should not be a security problem.
Sergey.

Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup.

[Vivek Goyal]

On Sat, Feb 27, 2016 at 01:40:02PM +0300, Nazarov Sergey wrote:
> 26.02.2016, 22:41, "Vivek Goyal" <vgoyal@redhat.com>:
> >
> > So what's the problem we are trying to solve. Why should we able to
> > override the DAC checks of lower layer if same directory in upper
> > is searchable for user but it is not searchable in lower layer.
> >
> 
> If I right, this is a one of the main feature of overlayfs - upper layer has priority over lower ones.
> Override AC checks necessary for lookup operation only. Lower layer files access AC checks
> remain, so this should not be a security problem.

So a directory which is not searchable by user in lower layer suddenly
becomes searchable once union is created. In fact there are two cases.
If upper layer directory does exist, then it is not searchable. If 
upper layer direcotry does exist with user having search permissions, then
lower layer directory should become searchable? May be, I don't know.

Thanks
Vivek

Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup.

[Ignacy Gawędzki]

On Fri, Feb 26, 2016 at 02:41:43PM -0500, thus spake Vivek Goyal:
> CCing linux-fsdevel as it is a wider issue.
> 
> 
> On Wed, Feb 24, 2016 at 02:55:52PM +0100, Ignacy Gaw�dzki wrote:
> > Add alternate lookup_one_len_check function to fs/namei.c which does
> > what lookup_one_len did until now with a boolean argument telling
> > whether to check that the base directory is traversable.  Modify
> > original lookup_one_len function to call the former with true as the
> > last argument.
> > 
> > In function ovl_lookup_real, file fs/overlayfs/super.c, call
> > lookup_one_len_check with false as the last argument, so that failure
> > to traverse the base directory does not return -EACCES.  This should
> > make lookup resolution work properly in the following setup
> > 
> >   drwxr-xr-x lower/
> >   drwx------ lower/foo/
> >   drw-r--r-- lower/boo/bar
> >   drwxr-xr-x upper/
> >   drwxr-xr-x upper/foo/
> > 
> > when any user not being the owner of lower/foo is trying to access
> > foo/bar in the mounted overlay.
> 
> So what's the problem we are trying to solve. Why should we able to
> override the DAC checks of lower layer if same directory in upper
> is searchable for user but it is not searchable in lower layer. 

My point is that an overlay filesystem should have consistent
semantics.  The current state of affairs fails in this regard on two
points.  First, suppose you have the same lower as above, but upper is
empty.  Then initially, foo/ has permissions 700 and shouldn't be
traversable by anyone but user root and the owner of foo/.  But if user
root or the owner changes foo's permissions to 755, a foo directory is
created in the upper layer and we arrive at the exact same
configuration as above.  In this case, I don't think anyone would
expect other users be denied traversal of foo to access bar.  Second,
the state of the cache shouldn't have any influence on the way access
rights are enforced.  In the present case, traversal of foo is granted
to other users depending on whether the owner or root already accessed
bar.

Besides, according to Documentation/fs/overlay.txt:

  Only the lists of names from directories are merged.  Other content
  such as metadata and extended attributes are reported for the upper
  directory only.  These attributes of the lower directory are hidden.

which implies that lower's permissions of foo should be ignored if foo
exists in upper as well.

Cheers,

Ignacy

-- 
Ignacy Gaw�dzki
R&D Engineer
Green Communications

Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup.

[Vivek Goyal]

On Sun, Feb 28, 2016 at 12:09:42PM +0100, Ignacy Gawędzki wrote:
> On Fri, Feb 26, 2016 at 02:41:43PM -0500, thus spake Vivek Goyal:
> > CCing linux-fsdevel as it is a wider issue.
> > 
> > 
> > On Wed, Feb 24, 2016 at 02:55:52PM +0100, Ignacy Gawędzki wrote:
> > > Add alternate lookup_one_len_check function to fs/namei.c which does
> > > what lookup_one_len did until now with a boolean argument telling
> > > whether to check that the base directory is traversable.  Modify
> > > original lookup_one_len function to call the former with true as the
> > > last argument.
> > > 
> > > In function ovl_lookup_real, file fs/overlayfs/super.c, call
> > > lookup_one_len_check with false as the last argument, so that failure
> > > to traverse the base directory does not return -EACCES.  This should
> > > make lookup resolution work properly in the following setup
> > > 
> > >   drwxr-xr-x lower/
> > >   drwx------ lower/foo/
> > >   drw-r--r-- lower/boo/bar
> > >   drwxr-xr-x upper/
> > >   drwxr-xr-x upper/foo/
> > > 
> > > when any user not being the owner of lower/foo is trying to access
> > > foo/bar in the mounted overlay.
> > 
> > So what's the problem we are trying to solve. Why should we able to
> > override the DAC checks of lower layer if same directory in upper
> > is searchable for user but it is not searchable in lower layer. 
> 
> My point is that an overlay filesystem should have consistent
> semantics.  The current state of affairs fails in this regard on two
> points.  First, suppose you have the same lower as above, but upper is
> empty.  Then initially, foo/ has permissions 700 and shouldn't be
> traversable by anyone but user root and the owner of foo/.  But if user
> root or the owner changes foo's permissions to 755, a foo directory is
> created in the upper layer and we arrive at the exact same
> configuration as above.  In this case, I don't think anyone would
> expect other users be denied traversal of foo to access bar.

I agree that semantics should be more consistent. I don't know that
if upper layer should override lower layer checks or not.

One could also argue that if root did chown, then changes effectively
happened in upper layer and anything in upper layer should become
visible to unpriviliged user but not the one in lower layer. 

I just don't know. I guess those who have more background on this
could pitch in and clarify that was is supposed to be the design
intention.


> Second,
> the state of the cache shouldn't have any influence on the way access
> rights are enforced.  In the present case, traversal of foo is granted
> to other users depending on whether the owner or root already accessed
> bar.

If we decide not to override checks from lower layer, then this is
an isue at caching level and requires fixing at that level.

> 
> Besides, according to Documentation/fs/overlay.txt:
> 
>   Only the lists of names from directories are merged.  Other content
>   such as metadata and extended attributes are reported for the upper
>   directory only.  These attributes of the lower directory are hidden.
> 
> which implies that lower's permissions of foo should be ignored if foo
> exists in upper as well.

Right, but it does not say anything about what happens to DAC checks
at lower layer. IOW, it does not say that if lower directory owner
is different then whether files from that directory will become searchable
or not.

Thanks
Vivek

Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup.

[Ignacy Gawędzki]

On Mon, Feb 29, 2016 at 11:25:46AM -0500, thus spake Vivek Goyal:
> I agree that semantics should be more consistent. I don't know that
> if upper layer should override lower layer checks or not.
> 
> One could also argue that if root did chown, then changes effectively
> happened in upper layer and anything in upper layer should become
> visible to unpriviliged user but not the one in lower layer. 
> 
> I just don't know. I guess those who have more background on this
> could pitch in and clarify that was is supposed to be the design
> intention.
> 
> [...]
> 
> Right, but it does not say anything about what happens to DAC checks
> at lower layer. IOW, it does not say that if lower directory owner
> is different then whether files from that directory will become searchable
> or not.

I suppose that looking at these questions from the perspective of the
primary application of OverlayFS, i.e. embedded systems with lower
being some read-only SquashFS and upper being read-write, may give
some good intuition on how this should work.  If the root user changes
access rights to some directories, then it is natural that permissions in
upper are less restrictive than permissions in lower and this in no
way breaks any security.  If you're thinking about what happens if
some overlay is mounted where the more permissive directory in upper
shadows a less permissive one in lower, then well, the only user able
to mount such an overlay, i.e. root, should know what she's doing.

Anyway, DAC checks should be consistent from the standpoint of
userland, first and foremost.

-- 
Ignacy Gaw�dzki
R&D Engineer
Green Communications

Re: [PATCH v2 1/1] OverlayFS: Fix checking permissions during lookup.

[Miklos Szeredi]

On Mon, Feb 29, 2016 at 05:54:40PM +0100, Ignacy Gawędzki wrote:
> On Mon, Feb 29, 2016 at 11:25:46AM -0500, thus spake Vivek Goyal:
> > I agree that semantics should be more consistent. I don't know that
> > if upper layer should override lower layer checks or not.
> > 
> > One could also argue that if root did chown, then changes effectively
> > happened in upper layer and anything in upper layer should become
> > visible to unpriviliged user but not the one in lower layer. 
> > 
> > I just don't know. I guess those who have more background on this
> > could pitch in and clarify that was is supposed to be the design
> > intention.

I haven't thought about this aspect yet.

[thinking]

We cannot actually respect lower layer search permissions in a merged directory,
because we cannot selectively allow or deny directory search permissions on
individual directory entries (there's simply not enough information in the
->permission() call).

The only sane thing to do is to override search permission by the upper layer
permissions.

We can do that with the cap_raise(... CAP_DAC_OVERRIDE) thing, which is what
overlayfs usually does when it needs elevated permissions to do something.

Or we can a special lookup_one_len() type helper.  But we already have the
dentry hash in the overlay dentry, so no need to recalculate that (we've also
made sure that underlying fs doesnt have ->d_hash()).  The sanity checks on
'name' are also unnecessary.  And that leaves us with __lookup_hash() where we
can just get rid of the underscores and export to modules.

Attaching folded patch.  Will post split out ones for review.

Thanks,
Miklos


diff --git a/fs/namei.c b/fs/namei.c
index 9c590e0f66e9..80fe2a68f361 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1447,7 +1447,7 @@ static int follow_dotdot(struct nameidata *nd)
  *
  * dir->d_inode->i_mutex must be held
  */
-static struct dentry *lookup_dcache(struct qstr *name, struct dentry *dir,
+static struct dentry *lookup_dcache(const struct qstr *name, struct dentry *dir,
 				    unsigned int flags, bool *need_lookup)
 {
 	struct dentry *dentry;
@@ -1506,8 +1506,8 @@ static struct dentry *lookup_real(struct inode *dir, struct dentry *dentry,
 	return dentry;
 }
 
-static struct dentry *__lookup_hash(struct qstr *name,
-		struct dentry *base, unsigned int flags)
+struct dentry *lookup_hash(const struct qstr *name,
+			   struct dentry *base, unsigned int flags)
 {
 	bool need_lookup;
 	struct dentry *dentry;
@@ -1518,6 +1518,7 @@ static struct dentry *__lookup_hash(struct qstr *name,
 
 	return lookup_real(base->d_inode, dentry, flags);
 }
+EXPORT_SYMBOL(lookup_hash);
 
 /*
  *  It's more convoluted than I'd like it to be, but... it's still fairly
@@ -1630,7 +1631,7 @@ static int lookup_slow(struct nameidata *nd, struct path *path)
 	BUG_ON(nd->inode != parent->d_inode);
 
 	inode_lock(parent->d_inode);
-	dentry = __lookup_hash(&nd->last, parent, nd->flags);
+	dentry = lookup_hash(&nd->last, parent, nd->flags);
 	inode_unlock(parent->d_inode);
 	if (IS_ERR(dentry))
 		return PTR_ERR(dentry);
@@ -2235,7 +2236,7 @@ struct dentry *kern_path_locked(const char *name, struct path *path)
 		return ERR_PTR(-EINVAL);
 	}
 	inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
-	d = __lookup_hash(&last, path->dentry, 0);
+	d = lookup_hash(&last, path->dentry, 0);
 	if (IS_ERR(d)) {
 		inode_unlock(path->dentry->d_inode);
 		path_put(path);
@@ -2319,7 +2320,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
 	if (err)
 		return ERR_PTR(err);
 
-	return __lookup_hash(&this, base, 0);
+	return lookup_hash(&this, base, 0);
 }
 EXPORT_SYMBOL(lookup_one_len);
 
@@ -2386,7 +2387,7 @@ struct dentry *lookup_one_len_unlocked(const char *name,
 		return ret;
 
 	inode_lock(base->d_inode);
-	ret =  __lookup_hash(&this, base, 0);
+	ret =  lookup_hash(&this, base, 0);
 	inode_unlock(base->d_inode);
 	return ret;
 }
@@ -3498,7 +3499,7 @@ static struct dentry *filename_create(int dfd, struct filename *name,
 	 */
 	lookup_flags |= LOOKUP_CREATE | LOOKUP_EXCL;
 	inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
-	dentry = __lookup_hash(&last, path->dentry, lookup_flags);
+	dentry = lookup_hash(&last, path->dentry, lookup_flags);
 	if (IS_ERR(dentry))
 		goto unlock;
 
@@ -3803,7 +3804,7 @@ retry:
 		goto exit1;
 
 	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
-	dentry = __lookup_hash(&last, path.dentry, lookup_flags);
+	dentry = lookup_hash(&last, path.dentry, lookup_flags);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto exit2;
@@ -3925,7 +3926,7 @@ retry:
 		goto exit1;
 retry_deleg:
 	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
-	dentry = __lookup_hash(&last, path.dentry, lookup_flags);
+	dentry = lookup_hash(&last, path.dentry, lookup_flags);
 	error = PTR_ERR(dentry);
 	if (!IS_ERR(dentry)) {
 		/* Why not before? Because we want correct error value */
@@ -4443,7 +4444,7 @@ retry:
 retry_deleg:
 	trap = lock_rename(new_path.dentry, old_path.dentry);
 
-	old_dentry = __lookup_hash(&old_last, old_path.dentry, lookup_flags);
+	old_dentry = lookup_hash(&old_last, old_path.dentry, lookup_flags);
 	error = PTR_ERR(old_dentry);
 	if (IS_ERR(old_dentry))
 		goto exit3;
@@ -4451,7 +4452,7 @@ retry_deleg:
 	error = -ENOENT;
 	if (d_is_negative(old_dentry))
 		goto exit4;
-	new_dentry = __lookup_hash(&new_last, new_path.dentry, lookup_flags | target_flags);
+	new_dentry = lookup_hash(&new_last, new_path.dentry, lookup_flags | target_flags);
 	error = PTR_ERR(new_dentry);
 	if (IS_ERR(new_dentry))
 		goto exit4;
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d894e7cd9a86..0cf45a0e904e 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -220,8 +220,7 @@ static int ovl_copy_up_locked(struct dentry *workdir, struct dentry *upperdir,
 	if (IS_ERR(newdentry))
 		goto out;
 
-	upper = lookup_one_len(dentry->d_name.name, upperdir,
-			       dentry->d_name.len);
+	upper = lookup_hash(&dentry->d_name, upperdir, 0);
 	err = PTR_ERR(upper);
 	if (IS_ERR(upper))
 		goto out1;
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 52f6de5d40a9..888b5fe29fdf 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -168,8 +168,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	int err;
 
 	inode_lock_nested(udir, I_MUTEX_PARENT);
-	newdentry = lookup_one_len(dentry->d_name.name, upperdir,
-				   dentry->d_name.len);
+	newdentry = lookup_hash(&dentry->d_name, upperdir, 0);
 	err = PTR_ERR(newdentry);
 	if (IS_ERR(newdentry))
 		goto out_unlock;
@@ -337,8 +336,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	if (IS_ERR(newdentry))
 		goto out_unlock;
 
-	upper = lookup_one_len(dentry->d_name.name, upperdir,
-			       dentry->d_name.len);
+	upper = lookup_hash(&dentry->d_name, upperdir, 0);
 	err = PTR_ERR(upper);
 	if (IS_ERR(upper))
 		goto out_dput;
@@ -547,8 +545,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, bool is_dir)
 
 	upper = ovl_dentry_upper(dentry);
 	if (!upper) {
-		upper = lookup_one_len(dentry->d_name.name, upperdir,
-				       dentry->d_name.len);
+		upper = lookup_hash(&dentry->d_name, upperdir, 0);
 		err = PTR_ERR(upper);
 		if (IS_ERR(upper))
 			goto kill_whiteout;
@@ -851,8 +848,7 @@ static int ovl_rename2(struct inode *olddir, struct dentry *old,
 		}
 	} else {
 		new_create = true;
-		newdentry = lookup_one_len(new->d_name.name, new_upperdir,
-					   new->d_name.len);
+		newdentry = lookup_hash(&new->d_name, new_upperdir, 0);
 		err = PTR_ERR(newdentry);
 		if (IS_ERR(newdentry))
 			goto out_unlock;
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 619ad4b016d2..c16eeb3e5021 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -379,7 +379,7 @@ static inline struct dentry *ovl_lookup_real(struct dentry *dir,
 	struct dentry *dentry;
 
 	inode_lock(dir->d_inode);
-	dentry = lookup_one_len(name->name, dir, name->len);
+	dentry = lookup_hash(name, dir, 0);
 	inode_unlock(dir->d_inode);
 
 	if (IS_ERR(dentry)) {
diff --git a/include/linux/namei.h b/include/linux/namei.h
index d0f25d81b46a..22cc2cece000 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -78,6 +78,8 @@ extern int kern_path_mountpoint(int, const char *, struct path *, unsigned int);
 
 extern struct dentry *lookup_one_len(const char *, struct dentry *, int);
 extern struct dentry *lookup_one_len_unlocked(const char *, struct dentry *, int);
+struct qstr;
+extern struct dentry *lookup_hash(const struct qstr *, struct dentry *, unsigned int);
 
 extern int follow_down_one(struct path *);
 extern int follow_down(struct path *);