[PATCH 1/1] overlayfs: apply device cgroup and security permissions to overlay files

[PATCH 1/1] overlayfs: apply device cgroup and security permissions to overlay files

[Andy Whitcroft]

When checking permissions on an overlayfs inode we do not take into
account either device cgroup restrictions nor security permissions.
This allows a user to mount an overlayfs layer over a restricted device
directory and by pass those permissions to open otherwise restricted
files.

Use devcgroup_inode_permission() and security_inode_permission() against
the underlying inodes when calculating ovl_permission().

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/overlayfs/inode.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

	Not sure whether you saw this the first time round.  This was shown
	up in testing with containers, specifically with device cgroups.
	It seems we should be re-checking here else users can simply
	bypass the containers device permissions by mounting an overlayfs
	fs over them.

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index ba1a777..1145a76 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -10,6 +10,8 @@
 #include <linux/fs.h>
 #include <linux/slab.h>
 #include <linux/xattr.h>
+#include <linux/device_cgroup.h>
+#include <linux/security.h>
 #include "overlayfs.h"
 
 int ovl_setattr(struct dentry *dentry, struct iattr *attr)
@@ -118,6 +120,11 @@ int ovl_permission(struct inode *inode, int mask)
 		err = realinode->i_op->permission(realinode, mask);
 	else
 		err = generic_permission(realinode, mask);
+
+	if (!err)
+		err = devcgroup_inode_permission(realinode, mask);
+	if (!err)
+		err = security_inode_permission(realinode, mask);
 out_dput:
 	dput(alias);
 	return err;
-- 
1.7.9

Re: [PATCH 1/1] overlayfs: apply device cgroup and security permissions to overlay files

[Miklos Szeredi]

Andy Whitcroft <apw@canonical.com> writes:

> When checking permissions on an overlayfs inode we do not take into
> account either device cgroup restrictions nor security permissions.
> This allows a user to mount an overlayfs layer over a restricted device
> directory and by pass those permissions to open otherwise restricted
> files.
>
> Use devcgroup_inode_permission() and security_inode_permission() against
> the underlying inodes when calculating ovl_permission().

Andy,

Thanks for the patch.

__devcgroup_inode_permission() and security_inode_permission() are not
exported to modules, so this will not work if overlayfs is a module.

We could export those but I think a better solution is to split out the
part of inode_permission() that doesn't check for a read-only fs and
export that.

Thanks,
Miklos

>
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  fs/overlayfs/inode.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> 	Not sure whether you saw this the first time round.  This was shown
> 	up in testing with containers, specifically with device cgroups.
> 	It seems we should be re-checking here else users can simply
> 	bypass the containers device permissions by mounting an overlayfs
> 	fs over them.
>
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index ba1a777..1145a76 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -10,6 +10,8 @@
>  #include <linux/fs.h>
>  #include <linux/slab.h>
>  #include <linux/xattr.h>
> +#include <linux/device_cgroup.h>
> +#include <linux/security.h>
>  #include "overlayfs.h"
>  
>  int ovl_setattr(struct dentry *dentry, struct iattr *attr)
> @@ -118,6 +120,11 @@ int ovl_permission(struct inode *inode, int mask)
>  		err = realinode->i_op->permission(realinode, mask);
>  	else
>  		err = generic_permission(realinode, mask);
> +
> +	if (!err)
> +		err = devcgroup_inode_permission(realinode, mask);
> +	if (!err)
> +		err = security_inode_permission(realinode, mask);
>  out_dput:
>  	dput(alias);
>  	return err;

Re: [PATCH 1/1] overlayfs: apply device cgroup and security permissions to overlay files

[Andy Whitcroft]

On Mon, Mar 05, 2012 at 06:18:23PM +0100, Miklos Szeredi wrote:
> Andy Whitcroft <apw@canonical.com> writes:
> 
> > When checking permissions on an overlayfs inode we do not take into
> > account either device cgroup restrictions nor security permissions.
> > This allows a user to mount an overlayfs layer over a restricted device
> > directory and by pass those permissions to open otherwise restricted
> > files.
> >
> > Use devcgroup_inode_permission() and security_inode_permission() against
> > the underlying inodes when calculating ovl_permission().
> 
> Andy,
> 
> Thanks for the patch.
> 
> __devcgroup_inode_permission() and security_inode_permission() are not
> exported to modules, so this will not work if overlayfs is a module.
> 
> We could export those but I think a better solution is to split out the
> part of inode_permission() that doesn't check for a read-only fs and
> export that.

Yeah that makes much more sense.  I got caught out by some exports
required for aufs3 which was also applied in my test environment.

How about the following pair of patches?

-apw

[PATCH 1/2] inode_only_permission: export inode level permissions checks

[Andy Whitcroft]

We need to be able to check inode permissions (but not filesystem implied
permissions) for stackable filesystems.  Now that permissions involve
checking with the security LSM, cgroups and basic inode permissions it is
easy to miss a key permission check and introduce a security vunerability.
Expose a new interface for these checks.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/namei.c         |   34 +++++++++++++++++++++++++---------
 include/linux/fs.h |    1 +
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index e2ba628..16c77a4 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -328,6 +328,30 @@ static inline int do_inode_permission(struct inode *inode, int mask)
 }
 
 /**
+ * inode_only_permission  -  check access rights to a given inode only
+ * @inode:	inode to check permissions on
+ * @mask:	right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC, ...)
+ *
+ * Uses to check read/write/execute permissions on an inode directly, we do
+ * not check filesystem permissions.
+ */
+int inode_only_permission(struct inode *inode, int mask)
+{
+	int retval;
+
+	retval = do_inode_permission(inode, mask);
+	if (retval)
+		return retval;
+
+	retval = devcgroup_inode_permission(inode, mask);
+	if (retval)
+		return retval;
+
+	return security_inode_permission(inode, mask);
+}
+EXPORT_SYMBOL(inode_only_permission);
+
+/**
  * inode_permission  -  check for access rights to a given inode
  * @inode:	inode to check permission on
  * @mask:	right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC, ...)
@@ -360,15 +384,7 @@ int inode_permission(struct inode *inode, int mask)
 			return -EACCES;
 	}
 
-	retval = do_inode_permission(inode, mask);
-	if (retval)
-		return retval;
-
-	retval = devcgroup_inode_permission(inode, mask);
-	if (retval)
-		return retval;
-
-	return security_inode_permission(inode, mask);
+	return inode_only_permission(inode, mask);
 }
 
 /**
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 871c87f..b06a3b4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2228,6 +2228,7 @@ extern sector_t bmap(struct inode *, sector_t);
 #endif
 extern int notify_change(struct dentry *, struct iattr *);
 extern int inode_permission(struct inode *, int);
+extern int inode_only_permission(struct inode *, int);
 extern int generic_permission(struct inode *, int);
 
 static inline bool execute_ok(struct inode *inode)
-- 
1.7.9

[PATCH 2/2] overlayfs: switch to use inode_only_permissions

[Andy Whitcroft]

When checking permissions on an overlayfs inode we do not take into
account either device cgroup restrictions nor security permissions.
This allows a user to mount an overlayfs layer over a restricted device
directory and by pass those permissions to open otherwise restricted
files.

Switch over to the newly introduced inode_only_permissions.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 fs/overlayfs/inode.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index ba1a777..1e3d157 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -114,10 +114,7 @@ int ovl_permission(struct inode *inode, int mask)
 			goto out_dput;
 	}
 
-	if (realinode->i_op->permission)
-		err = realinode->i_op->permission(realinode, mask);
-	else
-		err = generic_permission(realinode, mask);
+	err = inode_only_permission(realinode, mask);
 out_dput:
 	dput(alias);
 	return err;
-- 
1.7.9

Re: [PATCH 1/2] inode_only_permission: export inode level permissions checks

[Miklos Szeredi]

Andy Whitcroft <apw@canonical.com> writes:

> We need to be able to check inode permissions (but not filesystem implied
> permissions) for stackable filesystems.  Now that permissions involve
> checking with the security LSM, cgroups and basic inode permissions it is
> easy to miss a key permission check and introduce a security vunerability.
> Expose a new interface for these checks.
>
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  fs/namei.c         |   34 +++++++++++++++++++++++++---------
>  include/linux/fs.h |    1 +
>  2 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index e2ba628..16c77a4 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -328,6 +328,30 @@ static inline int do_inode_permission(struct inode *inode, int mask)
>  }
>  
>  /**
> + * inode_only_permission  -  check access rights to a given inode only
> + * @inode:	inode to check permissions on
> + * @mask:	right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC, ...)
> + *
> + * Uses to check read/write/execute permissions on an inode directly, we do
> + * not check filesystem permissions.
> + */
> +int inode_only_permission(struct inode *inode, int mask)
> +{
> +	int retval;
> +


IS_IMMUTABLE() is per-inode.  So I think only the IS_RDONLY() check
needs to be left out.

Thanks,
Miklos


> +	retval = do_inode_permission(inode, mask);
> +	if (retval)
> +		return retval;
> +
> +	retval = devcgroup_inode_permission(inode, mask);
> +	if (retval)
> +		return retval;
> +
> +	return security_inode_permission(inode, mask);
> +}
> +EXPORT_SYMBOL(inode_only_permission);
> +
> +/**
>   * inode_permission  -  check for access rights to a given inode
>   * @inode:	inode to check permission on
>   * @mask:	right to check for (%MAY_READ, %MAY_WRITE, %MAY_EXEC, ...)
> @@ -360,15 +384,7 @@ int inode_permission(struct inode *inode, int mask)
>  			return -EACCES;
>  	}
>  
> -	retval = do_inode_permission(inode, mask);
> -	if (retval)
> -		return retval;
> -
> -	retval = devcgroup_inode_permission(inode, mask);
> -	if (retval)
> -		return retval;
> -
> -	return security_inode_permission(inode, mask);
> +	return inode_only_permission(inode, mask);
>  }
>  
>  /**
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 871c87f..b06a3b4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2228,6 +2228,7 @@ extern sector_t bmap(struct inode *, sector_t);
>  #endif
>  extern int notify_change(struct dentry *, struct iattr *);
>  extern int inode_permission(struct inode *, int);
> +extern int inode_only_permission(struct inode *, int);
>  extern int generic_permission(struct inode *, int);
>  
>  static inline bool execute_ok(struct inode *inode)

Re: [PATCH 1/1] overlayfs: apply device cgroup and security permissions to overlay files

[Sedat Dilek]

On Tue, Mar 6, 2012 at 5:12 PM, Andy Whitcroft <apw@canonical.com> wrote:
> On Mon, Mar 05, 2012 at 06:18:23PM +0100, Miklos Szeredi wrote:
>> Andy Whitcroft <apw@canonical.com> writes:
>>
>> > When checking permissions on an overlayfs inode we do not take into
>> > account either device cgroup restrictions nor security permissions.
>> > This allows a user to mount an overlayfs layer over a restricted device
>> > directory and by pass those permissions to open otherwise restricted
>> > files.
>> >
>> > Use devcgroup_inode_permission() and security_inode_permission() against
>> > the underlying inodes when calculating ovl_permission().
>>
>> Andy,
>>
>> Thanks for the patch.
>>
>> __devcgroup_inode_permission() and security_inode_permission() are not
>> exported to modules, so this will not work if overlayfs is a module.
>>
>> We could export those but I think a better solution is to split out the
>> part of inode_permission() that doesn't check for a read-only fs and
>> export that.
>
> Yeah that makes much more sense.  I got caught out by some exports
> required for aufs3 which was also applied in my test environment.
>
> How about the following pair of patches?
>
> -apw
> --

Hi,

what is the status of this patch?

Ubuntu has it included in precise kernel and it fixes a security issues as well.
[1] says:
CVE-2012-0055
BugLink: http://bugs.launchpad.net/bugs/915941
BugLink: http://bugs.launchpad.net/bugs/918212

What's up with the followup (splitted) patches which should replace
this single one?
My inbox says:
[PATCH 1/2] inode_only_permission: export inode level permissions checks
[PATCH 2/2] overlayfs: switch to use inode_only_permissions

As I build OverlayFS as module those two new patches would be much appreciated.
To quote Miklos:
"__devcgroup_inode_permission() and security_inode_permission() are not
exported to modules, so this will not work if overlayfs is a module."

Thanks for any clarification in advance.

- Sedat -

[1] http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-precise.git;a=commit;h=59aa87f808b13463bc717ad69362b5372f6c5574