[PATCH] CacheFiles: Add calls to path-based security hooks

[PATCH] CacheFiles: Add calls to path-based security hooks

[David Howells]

Add calls to path-based security hooks into CacheFiles as, unlike inode-based
security, these aren't implicit in the vfs_mkdir() and similar calls.

Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> 
Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/cachefiles/namei.c |   52 +++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index 42c7faf..a0358c2 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -275,6 +275,7 @@ static int cachefiles_bury_object(struct cachefiles_cache *cache,
 				  bool preemptive)
 {
 	struct dentry *grave, *trap;
+	struct path path, path_to_graveyard;
 	char nbuffer[8 + 8 + 1];
 	int ret;
 
@@ -287,10 +288,18 @@ static int cachefiles_bury_object(struct cachefiles_cache *cache,
 	/* non-directories can just be unlinked */
 	if (!S_ISDIR(rep->d_inode->i_mode)) {
 		_debug("unlink stale object");
-		ret = vfs_unlink(dir->d_inode, rep);
 
-		if (preemptive)
-			cachefiles_mark_object_buried(cache, rep);
+		path.mnt = cache->mnt;
+		path.dentry = dir;
+		ret = security_path_unlink(&path, rep);
+		if (ret < 0) {
+			cachefiles_io_error(cache, "Unlink security error");
+		} else {
+			ret = vfs_unlink(dir->d_inode, rep);
+
+			if (preemptive)
+				cachefiles_mark_object_buried(cache, rep);
+		}
 
 		mutex_unlock(&dir->d_inode->i_mutex);
 
@@ -379,12 +388,23 @@ try_again:
 	}
 
 	/* attempt the rename */
-	ret = vfs_rename(dir->d_inode, rep, cache->graveyard->d_inode, grave);
-	if (ret != 0 && ret != -ENOMEM)
-		cachefiles_io_error(cache, "Rename failed with error %d", ret);
+	path.mnt = cache->mnt;
+	path.dentry = dir;
+	path_to_graveyard.mnt = cache->mnt;
+	path_to_graveyard.dentry = cache->graveyard;
+	ret = security_path_rename(&path, rep, &path_to_graveyard, grave);
+	if (ret < 0) {
+		cachefiles_io_error(cache, "Rename security error %d", ret);
+	} else {
+		ret = vfs_rename(dir->d_inode, rep,
+				 cache->graveyard->d_inode, grave);
+		if (ret != 0 && ret != -ENOMEM)
+			cachefiles_io_error(cache,
+					    "Rename failed with error %d", ret);
 
-	if (preemptive)
-		cachefiles_mark_object_buried(cache, rep);
+		if (preemptive)
+			cachefiles_mark_object_buried(cache, rep);
+	}
 
 	unlock_rename(cache->graveyard, dir);
 	dput(grave);
@@ -448,6 +468,7 @@ int cachefiles_walk_to_object(struct cachefiles_object *parent,
 {
 	struct cachefiles_cache *cache;
 	struct dentry *dir, *next = NULL;
+	struct path path;
 	unsigned long start;
 	const char *name;
 	int ret, nlen;
@@ -458,6 +479,7 @@ int cachefiles_walk_to_object(struct cachefiles_object *parent,
 
 	cache = container_of(parent->fscache.cache,
 			     struct cachefiles_cache, cache);
+	path.mnt = cache->mnt;
 
 	ASSERT(parent->dentry);
 	ASSERT(parent->dentry->d_inode);
@@ -511,6 +533,10 @@ lookup_again:
 			if (ret < 0)
 				goto create_error;
 
+			path.dentry = dir;
+			ret = security_path_mkdir(&path, next, 0);
+			if (ret < 0)
+				goto create_error;
 			start = jiffies;
 			ret = vfs_mkdir(dir->d_inode, next, 0);
 			cachefiles_hist(cachefiles_mkdir_histogram, start);
@@ -536,6 +562,10 @@ lookup_again:
 			if (ret < 0)
 				goto create_error;
 
+			path.dentry = dir;
+			ret = security_path_mknod(&path, next, S_IFREG, 0);
+			if (ret < 0)
+				goto create_error;
 			start = jiffies;
 			ret = vfs_create(dir->d_inode, next, S_IFREG, NULL);
 			cachefiles_hist(cachefiles_create_histogram, start);
@@ -692,6 +722,7 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
 {
 	struct dentry *subdir;
 	unsigned long start;
+	struct path path;
 	int ret;
 
 	_enter(",,%s", dirname);
@@ -719,6 +750,11 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
 
 		_debug("attempt mkdir");
 
+		path.mnt = cache->mnt;
+		path.dentry = dir;
+		ret = security_path_mkdir(&path, subdir, 0700);
+		if (ret < 0)
+			goto mkdir_error;
 		ret = vfs_mkdir(dir->d_inode, subdir, 0700);
 		if (ret < 0)
 			goto mkdir_error;

Re: [PATCH] CacheFiles: Add calls to path-based security hooks

[Tetsuo Handa]

David Howells wrote ( http://marc.info/?l=linux-security-module&m=129303225204910&w=2 ):
> > Both kernels did not check other permissions (e.g. create, unlink, rename)
> > at all (probably because security_path_*() are not called when cache files
> > are manipulated).
> 
> I think the security_path_*() functions postdate the inclusion of CacheFiles
> upstream.  This needs to be addressed.  It's not affected me directly thus far,
> so I haven't come across it yet.

I proposed inserting security_path_*() functions into only pre VFS helper
functions. This was my intention that only pathnames supplied from userspace
are checked so that users can enumerate pathnames without being bothered by
kernel internal business (e.g. filesystem needs to access pathname which was
not supplied from userspace).

> Furthermore, as mentioned above, CacheFiles needs updating to make use of
> path-based security, and TOMOYO should offer the administrator the option to
> nominate a context from which the kernel will manipulate the filesystem.

Well, I prefer enabling both inode based MAC (e.g. SELinux/Smack) and name
based MAC (e.g. TOMOYO/AppArmor) at the same time.

The reason why name based MAC checks filename is that filename has meaning.
Filename determines

(1) whether application can find the file or not

(2) for what purpose the file is used if access to that file is granted

. Therefore, name based MAC strictly restricts pathname changes.

However, checking filenames like

  /var/fscache/cache/@4a/I03nfs/@30/Ji000000000000000--fHg8hi8400/@75/Es0g000w...DB1ry

makes little sense because they are merely sequential (or random) names.

Our concerns regarding cachefilesd are mostly

(3) whether application can access the cached files or not

(4) what security label should be assigned to cached files

. Definitely inode based MAC can do better.



Anyway, we want below patch if we adopt your patch.

  ERROR: "security_path_rename" [fs/cachefiles/cachefiles.ko] undefined!
  ERROR: "security_path_unlink" [fs/cachefiles/cachefiles.ko] undefined!
  ERROR: "security_path_mkdir" [fs/cachefiles/cachefiles.ko] undefined!

Regards.
--------------------
 From 861f928bff6de99e0cfad54fa0cf803d35080a29 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 23 Dec 2010 11:33:16 +0900
Subject: [PATCH] Export security_path hooks for cachefiles.

cachefiles module calls security_path hooks. Export them.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 security/security.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/security/security.c b/security/security.c
index 1b798d3..568b98d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -360,6 +360,7 @@ int security_path_mkdir(struct path *dir, struct dentry *dentry, int mode)
 		return 0;
 	return security_ops->path_mkdir(dir, dentry, mode);
 }
+EXPORT_SYMBOL(security_path_mkdir);
 
 int security_path_rmdir(struct path *dir, struct dentry *dentry)
 {
@@ -374,6 +375,7 @@ int security_path_unlink(struct path *dir, struct dentry *dentry)
 		return 0;
 	return security_ops->path_unlink(dir, dentry);
 }
+EXPORT_SYMBOL(security_path_unlink);
 
 int security_path_symlink(struct path *dir, struct dentry *dentry,
 			  const char *old_name)
@@ -400,6 +402,7 @@ int security_path_rename(struct path *old_dir, struct dentry *old_dentry,
 	return security_ops->path_rename(old_dir, old_dentry, new_dir,
 					 new_dentry);
 }
+EXPORT_SYMBOL(security_path_rename);
 
 int security_path_truncate(struct path *path)
 {
-- 
1.6.1