[RFC]Introduce generalized hooks for getting and setting inode secctx v3

[RFC]Introduce generalized hooks for getting and setting inode secctx v3

[David P. Quigley]

This patch set does two things. First it factors the section of vfs_setxattr
that does the real work into a helper function. This allows LSMs the ability to
set the xattrs they need without hitting the permission check inside
vfs_setxattr each time. Second it introduces three new hooks
inode_{get,set}secctx, and inode_notifysecctx.

The first hook retreives all security information the
LSM feels is relavent in the form of a security context. The second hook given
this context can sets both the in-core and on-disk store for the particular
inode. The third hook is used to notify the in-core inode of a change to it's
security state.

This is the third revision of this patch which takes into account concerns by
Casey Schaufler, and Christop Hellwig.

fs/xattr.c               |   55 +++++++++++++++++++++++++++++++++++-----------
 include/linux/security.h |   37 +++++++++++++++++++++++++++++++
 include/linux/xattr.h    |    3 +-
 security/dummy.c         |   17 ++++++++++++++
 security/security.c      |   18 +++++++++++++++
 security/selinux/hooks.c |   32 ++++++++++++++++++++++++++-
 6 files changed, 147 insertions(+), 15 deletions(-)

[PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[David P. Quigley]

This factors out the part of the vfs_setxattr function that performs the
setting of the xattr and its notification. This is needed so the SELinux
implementation of inode_setsecctx can handle the setting of it's xattr while
maintaining the proper separation of layers.

Signed-off-by: David P. Quigley <dpquigl-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
---
 fs/xattr.c            |   55 +++++++++++++++++++++++++++++++++++++-----------
 include/linux/xattr.h |    3 +-
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 3acab16..464265e 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -67,22 +67,28 @@ xattr_permission(struct inode *inode, const char *name, int mask)
 	return permission(inode, mask, NULL);
 }
 
-int
-vfs_setxattr(struct dentry *dentry, char *name, void *value,
+/**
+ *  __vfs_setxattr_noperm - perform setxattr operation without performing
+ *  permission checks.
+ *
+ *  @dentry - object to perform setxattr on
+ *  @name - xattr name to set
+ *  @value - value to set @name to
+ *  @size - size of @value
+ *  @flags
+ *
+ *  returns the result of the internel setxattr or setsecurity operations.
+ *
+ *  This function requires the the caller locks the inode's i_mutex before it
+ *  is executed. It also that the caller will make the appropriate permission
+ *  checks.
+ */
+int __vfs_setxattr_noperm(struct dentry *dentry, char *name, void *value,
 		size_t size, int flags)
 {
 	struct inode *inode = dentry->d_inode;
-	int error;
-
-	error = xattr_permission(inode, name, MAY_WRITE);
-	if (error)
-		return error;
-
-	mutex_lock(&inode->i_mutex);
-	error = security_inode_setxattr(dentry, name, value, size, flags);
-	if (error)
-		goto out;
-	error = -EOPNOTSUPP;
+	int error = -EOPNOTSUPP;
+	
 	if (inode->i_op->setxattr) {
 		error = inode->i_op->setxattr(dentry, name, value, size, flags);
 		if (!error) {
@@ -98,6 +104,29 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
 		if (!error)
 			fsnotify_xattr(dentry);
 	}
+
+	return error;
+}
+
+
+int
+vfs_setxattr(struct dentry *dentry, char *name, void *value,
+		size_t size, int flags)
+{
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	error = xattr_permission(inode, name, MAY_WRITE);
+	if (error)
+		return error;
+
+	mutex_lock(&inode->i_mutex);
+	error = security_inode_setxattr(dentry, name, value, size, flags);
+	if (error)
+		goto out;
+
+	error = __vfs_setxattr_noperm(dentry, name, value, size, flags);
+
 out:
 	mutex_unlock(&inode->i_mutex);
 	return error;
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index df6b95d..324c792 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -49,7 +49,8 @@ struct xattr_handler {
 ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
 ssize_t vfs_getxattr(struct dentry *, char *, void *, size_t);
 ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
-int vfs_setxattr(struct dentry *, char *, void *, size_t, int);
+int __vfs_setxattr_noperm(struct dentry *, char *, void *, size_t, int);
+int do_setxattr(struct dentry *, char *, void *, size_t, int);
 int vfs_removexattr(struct dentry *, char *);
 
 ssize_t generic_getxattr(struct dentry *dentry, const char *name, void *buffer, size_t size);
-- 
1.5.4.1

[PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[David P. Quigley]

This patch introduces two new hooks. One to get all relevant information from
an LSM about an inode an the second given that context to set it on the
inode. The setcontext call takes a flag to indicate if it should set the incore
representation, the ondisk representation or both. This hook is for use in the
labeled NFS code and addresses concerns of how to set security on an inode in a
multi-xattr LSM.

Signed-off-by: David P. Quigley <dpquigl-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
---
 include/linux/security.h |   37 +++++++++++++++++++++++++++++++++++++
 security/dummy.c         |   17 +++++++++++++++++
 security/security.c      |   18 ++++++++++++++++++
 security/selinux/hooks.c |   32 +++++++++++++++++++++++++++++++-
 4 files changed, 103 insertions(+), 1 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index b07357c..220ec46 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1224,6 +1224,23 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@secdata contains the security context.
  *	@seclen contains the length of the security context.
  *
+ * @inode_notifysecctx:
+ * 	Set the incore security context of an inode.
+ * 	@dentry contains the inode we wish to set the security context of.
+ * 	@ctx contains the string which we wish to set in the inode.
+ * 	@ctxlen contains the length of @ctx.
+ *
+ * @inode_setsecctx:
+ * 	Sets both the incore and persistant form of the inode's security context.
+ * 	@dentry contains the inode we wish to set the security context of.
+ * 	@ctx contains the string which we wish to set in the inode.
+ * 	@ctxlen contains the length of @ctx.
+ *
+ * @inode_getsecctx:
+ * 	Returns a string containing all relavent security context information 	
+ * 	@dentry contains the inode we wish to set the security context of.
+ *	@ctx is a pointer to place the allocated security context should be placed.
+ *	@ctxlen points to the place to put the length of @ctx.	
  * This is the main security structure.
  */
 struct security_operations {
@@ -1414,6 +1431,10 @@ struct security_operations {
 	int (*secctx_to_secid)(char *secdata, u32 seclen, u32 *secid);
 	void (*release_secctx)(char *secdata, u32 seclen);
 
+	int (*inode_notifysecctx)(struct dentry *dentry, void *ctx, u32 ctxlen);
+	int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen);
+	int (*inode_getsecctx)(struct dentry *dentry, void **ctx, u32 *ctxlen);
+
 #ifdef CONFIG_SECURITY_NETWORK
 	int (*unix_stream_connect) (struct socket * sock,
 				    struct socket * other, struct sock * newsk);
@@ -1653,6 +1674,9 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
 int security_secctx_to_secid(char *secdata, u32 seclen, u32 *secid);
 void security_release_secctx(char *secdata, u32 seclen);
 
+int security_inode_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
+int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
+int security_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen);
 #else /* CONFIG_SECURITY */
 struct security_mnt_opts {
 };
@@ -2365,6 +2389,19 @@ static inline int security_secctx_to_secid(char *secdata,
 static inline void security_release_secctx(char *secdata, u32 seclen)
 {
 }
+
+static inline int security_inode_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return -EOPNOTSUPP;
+}
+static inline int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return -EOPNOTSUPP;
+}
+static inline int security_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen)
+{
+	return -EOPNOTSUPP;
+}
 #endif	/* CONFIG_SECURITY */
 
 #ifdef CONFIG_SECURITY_NETWORK
diff --git a/security/dummy.c b/security/dummy.c
index 78d8f92..872b45c 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -962,6 +962,20 @@ static void dummy_release_secctx(char *secdata, u32 seclen)
 {
 }
 
+static int dummy_inode_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return -EOPNOTSUPP;
+}
+static int dummy_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return -EOPNOTSUPP;
+}
+
+static int dummy_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen)
+{
+	return -EOPNOTSUPP;
+}
+
 #ifdef CONFIG_KEYS
 static inline int dummy_key_alloc(struct key *key, struct task_struct *ctx,
 				  unsigned long flags)
@@ -1121,6 +1135,9 @@ void security_fixup_ops (struct security_operations *ops)
  	set_to_dummy_if_null(ops, secid_to_secctx);
 	set_to_dummy_if_null(ops, secctx_to_secid);
  	set_to_dummy_if_null(ops, release_secctx);
+	set_to_dummy_if_null(ops, inode_notifysecctx);
+	set_to_dummy_if_null(ops, inode_setsecctx);
+	set_to_dummy_if_null(ops, inode_getsecctx);
 #ifdef CONFIG_SECURITY_NETWORK
 	set_to_dummy_if_null(ops, unix_stream_connect);
 	set_to_dummy_if_null(ops, unix_may_send);
diff --git a/security/security.c b/security/security.c
index b1387a6..39ed78f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -852,6 +852,24 @@ void security_release_secctx(char *secdata, u32 seclen)
 }
 EXPORT_SYMBOL(security_release_secctx);
 
+int security_inode_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return security_ops->inode_notifysecctx(dentry, ctx, ctxlen);
+}
+EXPORT_SYMBOL(security_inode_notifysecctx);
+
+int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return security_ops->inode_setsecctx(dentry, ctx, ctxlen);
+}
+EXPORT_SYMBOL(security_inode_setsecctx);
+
+int security_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen)
+{
+	return security_ops->inode_getsecctx(dentry, ctx, ctxlen);
+}
+EXPORT_SYMBOL(security_inode_getsecctx);
+
 #ifdef CONFIG_SECURITY_NETWORK
 
 int security_unix_stream_connect(struct socket *sock, struct socket *other,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4bf4807..5ce6bbe 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -75,6 +75,7 @@
 #include <linux/string.h>
 #include <linux/selinux.h>
 #include <linux/mutex.h>
+#include <linux/fsnotify.h>
 
 #include "avc.h"
 #include "objsec.h"
@@ -5174,6 +5175,33 @@ static void selinux_release_secctx(char *secdata, u32 seclen)
 	kfree(secdata);
 }
 
+/*
+ *	This hook requires that the inode i_mutex be locked
+ */
+static int selinux_inode_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen, bool setdisk)
+{
+	struct inode *inode = dentry->d_inode;
+
+	return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
+	
+}
+
+/*
+ *	This hook requires that the inode i_mutex be locked
+ */
+static int selinux_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen, bool setdisk)
+{
+	return __vfs_setxattr_noperm(dentry, XATTR_NAME_SELINUX, ctx, ctxlen, 0); 
+}
+
+static int selinux_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen)
+{
+	struct inode *inode = dentry->d_inode;
+	
+	*ctxlen = selinux_inode_getsecurity(inode, XATTR_SELINUX_SUFFIX,
+						ctx, true);
+	return *ctxlen;
+}
 #ifdef CONFIG_KEYS
 
 static int selinux_key_alloc(struct key *k, struct task_struct *tsk,
@@ -5364,7 +5392,9 @@ static struct security_operations selinux_ops = {
 	.secid_to_secctx =		selinux_secid_to_secctx,
 	.secctx_to_secid =		selinux_secctx_to_secid,
 	.release_secctx =		selinux_release_secctx,
-
+	.inode_notifysecctx =		selinux_inode_notifysecctx,
+	.inode_setsecctx =		selinux_inode_setsecctx,
+	.inode_getsecctx =		selinux_inode_getsecctx,
         .unix_stream_connect =		selinux_socket_unix_stream_connect,
 	.unix_may_send =		selinux_socket_unix_may_send,
 
-- 
1.5.4.1

Re: [RFC]Introduce generalized hooks for getting and setting inode secctx v3

[Casey Schaufler]

--- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:

> This patch set does two things. First it factors the section of vfs_setxattr
> that does the real work into a helper function. This allows LSMs the ability
> to
> set the xattrs they need without hitting the permission check inside
> vfs_setxattr each time.

Why is this important? I'm perfectly willing to believe that it is,
but I would hesitate to say that it's completely obvious to the
casual observer. After all, we've gotten by with things the way they
are for some time. Perhaps you could describe the use to which you
would be putting this. I expect that if I go through the backlog
discussions on or about this topic I could probably make some
logical assumptions about what you want to do, but it will save
everyone some time if you could spell it out here.

> Second it introduces three new hooks
> inode_{get,set}secctx, and inode_notifysecctx.
>
> The first hook retreives all security information the
> LSM feels is relavent in the form of a security context. The second hook
> given
> this context can sets both the in-core and on-disk store for the particular
> inode. The third hook is used to notify the in-core inode of a change to it's
> security state.

I still dislike having an interface that explicitly disallows
security attribute integrity. That does not help me feel secure.

> This is the third revision of this patch which takes into account concerns by
> Casey Schaufler, and Christop Hellwig.



Casey Schaufler
casey@schaufler-ca.com

Re: [RFC]Introduce generalized hooks for getting and setting inode secctx v3

[James Morris]

On Wed, 19 Mar 2008, Casey Schaufler wrote:

> I still dislike having an interface that explicitly disallows
> security attribute integrity. That does not help me feel secure.

What do you mean?


-- 
James Morris
<jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>

Re: [RFC]Introduce generalized hooks for getting and setting inode secctx v3

[Stephen Smalley]

On Wed, 2008-03-19 at 06:38 -0700, Casey Schaufler wrote:
> --- "David P. Quigley" <dpquigl-+05T5uksL2qpZYMLLGbcSA@public.gmane.org> wrote:
> 
> > This patch set does two things. First it factors the section of vfs_setxattr
> > that does the real work into a helper function. This allows LSMs the ability
> > to
> > set the xattrs they need without hitting the permission check inside
> > vfs_setxattr each time.
> 
> Why is this important? I'm perfectly willing to believe that it is,
> but I would hesitate to say that it's completely obvious to the
> casual observer. After all, we've gotten by with things the way they
> are for some time. Perhaps you could describe the use to which you
> would be putting this. I expect that if I go through the backlog
> discussions on or about this topic I could probably make some
> logical assumptions about what you want to do, but it will save
> everyone some time if you could spell it out here.
> 
> > Second it introduces three new hooks
> > inode_{get,set}secctx, and inode_notifysecctx.
> >
> > The first hook retreives all security information the
> > LSM feels is relavent in the form of a security context. The second hook
> > given
> > this context can sets both the in-core and on-disk store for the particular
> > inode. The third hook is used to notify the in-core inode of a change to it's
> > security state.
> 
> I still dislike having an interface that explicitly disallows
> security attribute integrity. That does not help me feel secure.

Which part of our prior explanations of this functionality didn't you
understand?

http://marc.info/?l=linux-fsdevel&m=120515271614741&w=2

I would agree though that the final patch submission ought to include
some of that prior explanation in its patch description for historical
purposes.

-- 
Stephen Smalley
National Security Agency

Re: [RFC]Introduce generalized hooks for getting and setting inode secctx v3

[Casey Schaufler]

--- Stephen Smalley <sds@tycho.nsa.gov> wrote:

> 
> On Wed, 2008-03-19 at 06:38 -0700, Casey Schaufler wrote:
> > --- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:
> > 
> > > This patch set does two things. First it factors the section of
> vfs_setxattr
> > > that does the real work into a helper function. This allows LSMs the
> ability
> > > to
> > > set the xattrs they need without hitting the permission check inside
> > > vfs_setxattr each time.
> > 
> > Why is this important? I'm perfectly willing to believe that it is,
> > but I would hesitate to say that it's completely obvious to the
> > casual observer. After all, we've gotten by with things the way they
> > are for some time. Perhaps you could describe the use to which you
> > would be putting this. I expect that if I go through the backlog
> > discussions on or about this topic I could probably make some
> > logical assumptions about what you want to do, but it will save
> > everyone some time if you could spell it out here.
> > 
> > > Second it introduces three new hooks
> > > inode_{get,set}secctx, and inode_notifysecctx.
> > >
> > > The first hook retreives all security information the
> > > LSM feels is relavent in the form of a security context. The second hook
> > > given
> > > this context can sets both the in-core and on-disk store for the
> particular
> > > inode. The third hook is used to notify the in-core inode of a change to
> it's
> > > security state.
> > 
> > I still dislike having an interface that explicitly disallows
> > security attribute integrity. That does not help me feel secure.
> 
> Which part of our prior explanations of this functionality didn't you
> understand?

Oh, cut the crap. What part of my explainations don't you understand?

I understand the functionality. That is not my point. My point is
that inode_notifysecctx() explicitly prohibits the LSM from providing
integrity of the security attributes by introducing a differentiation
between the "in-core" and "on-disk" values, and making it explicit
that the one is set, but not the other.

Clearly this is the direction you intend to go. Have fun with it.
I've raised the issue, y'all aren't seeing it. Maybe I'm wrong,
it has happened before.

> http://marc.info/?l=linux-fsdevel&m=120515271614741&w=2
> 
> I would agree though that the final patch submission ought to include
> some of that prior explanation in its patch description for historical
> purposes.

Yes indeed.

Thank you.


Casey Schaufler
casey@schaufler-ca.com

Re: [RFC]Introduce generalized hooks for getting and setting inode secctx v3

[Stephen Smalley]

On Wed, 2008-03-19 at 08:11 -0700, Casey Schaufler wrote:
> --- Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > 
> > On Wed, 2008-03-19 at 06:38 -0700, Casey Schaufler wrote:
> > > --- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:
> > > 
> > > > This patch set does two things. First it factors the section of
> > vfs_setxattr
> > > > that does the real work into a helper function. This allows LSMs the
> > ability
> > > > to
> > > > set the xattrs they need without hitting the permission check inside
> > > > vfs_setxattr each time.
> > > 
> > > Why is this important? I'm perfectly willing to believe that it is,
> > > but I would hesitate to say that it's completely obvious to the
> > > casual observer. After all, we've gotten by with things the way they
> > > are for some time. Perhaps you could describe the use to which you
> > > would be putting this. I expect that if I go through the backlog
> > > discussions on or about this topic I could probably make some
> > > logical assumptions about what you want to do, but it will save
> > > everyone some time if you could spell it out here.
> > > 
> > > > Second it introduces three new hooks
> > > > inode_{get,set}secctx, and inode_notifysecctx.
> > > >
> > > > The first hook retreives all security information the
> > > > LSM feels is relavent in the form of a security context. The second hook
> > > > given
> > > > this context can sets both the in-core and on-disk store for the
> > particular
> > > > inode. The third hook is used to notify the in-core inode of a change to
> > it's
> > > > security state.
> > > 
> > > I still dislike having an interface that explicitly disallows
> > > security attribute integrity. That does not help me feel secure.
> > 
> > Which part of our prior explanations of this functionality didn't you
> > understand?
> 
> Oh, cut the crap. What part of my explainations don't you understand?
> 
> I understand the functionality. That is not my point. My point is
> that inode_notifysecctx() explicitly prohibits the LSM from providing
> integrity of the security attributes by introducing a differentiation
> between the "in-core" and "on-disk" values, and making it explicit
> that the one is set, but not the other.
> 
> Clearly this is the direction you intend to go. Have fun with it.
> I've raised the issue, y'all aren't seeing it. Maybe I'm wrong,
> it has happened before.

It is absolutely no different than the way that uids and other inode
attributes are handled; NFS client has to update the incore inode state
based on the server's information and that is not the same as setting it
in the backing filesystem.  I've only said that two or three times.

> > http://marc.info/?l=linux-fsdevel&m=120515271614741&w=2
> > 
> > I would agree though that the final patch submission ought to include
> > some of that prior explanation in its patch description for historical
> > purposes.
> 
> Yes indeed.
> 
> Thank you.
> 
> 
> Casey Schaufler
> casey@schaufler-ca.com
> 
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.
-- 
Stephen Smalley
National Security Agency

Re: [RFC]Introduce generalized hooks for getting and setting inode secctx v3

[James Morris]

On Wed, 19 Mar 2008, Casey Schaufler wrote:

> Oh, cut the crap. What part of my explainations don't you understand?
> 
> I understand the functionality. That is not my point. My point is
> that inode_notifysecctx() explicitly prohibits the LSM from providing
> integrity of the security attributes by introducing a differentiation
> between the "in-core" and "on-disk" values, and making it explicit
> that the one is set, but not the other.
> 
> Clearly this is the direction you intend to go. Have fun with it.
> I've raised the issue, y'all aren't seeing it. Maybe I'm wrong,
> it has happened before.

Please stop trolling.


- James
-- 
James Morris
<jmorris@namei.org>