[PATCH 2/5] selinux: adds a private inode operation

[PATCH 2/5] selinux: adds a private inode operation

[Jeffrey Mahoney]

ReiserFS implements extended attributes by backing them with regular files.
This means that selinux would create an infinite series of
xattrs-on-xattrs-on-xattrs etc. However, due to the locking of xattrs, this
series never happens. Instead, a locking loop occurs.

This patch allows SElinux to mark inodes as "private," such that SElinux
attributes aren't associated with them.

Original implementation by Stephen Smalley <sds@epoch.ncsc.mil>

Signed-off-by: Jeff Mahoney <jeffm@novell.com>

diff -ruNpX dontdiff linux-2.6.9/include/linux/security.h linux-2.6.9.selinux/include/linux/security.h
--- linux-2.6.9/include/linux/security.h	2004-08-14 01:37:30.000000000 -0400
+++ linux-2.6.9.selinux/include/linux/security.h	2004-11-20 17:06:29.000000000 -0500
@@ -412,6 +412,11 @@ struct swap_info_struct;
  *	associated with @dentry into @buffer.  @buffer may be NULL to 
  *	request the size of the buffer required.  
  *	Returns number of bytes used/required on success.
+ * @inode_mark_private:
+ *      Set up the security state of @inode to reflect the fact that the inode
+ *	is private, i.e. used internally by the filesystem for purposes such
+ *	as xattr storage and not accessible by userspace.  This property should
+ *	then be inherited by all nodes under this node.
  *
  * Security hooks for file operations
  *
@@ -1111,6 +1116,7 @@ struct security_operations {
   	int (*inode_getsecurity)(struct dentry *dentry, const char *name, void *buffer, size_t size);
   	int (*inode_setsecurity)(struct dentry *dentry, const char *name, const void *value, size_t size, int flags);
   	int (*inode_listsecurity)(struct dentry *dentry, char *buffer);
+  	void (*inode_mark_private)(struct inode *inode);
 
 	int (*file_permission) (struct file * file, int mask);
 	int (*file_alloc_security) (struct file * file);
@@ -1590,6 +1596,11 @@ static inline int security_inode_listsec
 	return security_ops->inode_listsecurity(dentry, buffer);
 }
 
+static inline void security_inode_mark_private(struct inode *inode)
+{
+	security_ops->inode_mark_private(inode);
+}
+
 static inline int security_file_permission (struct file *file, int mask)
 {
 	return security_ops->file_permission (file, mask);
@@ -2229,6 +2240,9 @@ static inline int security_inode_listsec
 	return 0;
 }
 
+static inline void security_inode_mark_private(struct inode *inode)
+{ }
+
 static inline int security_file_permission (struct file *file, int mask)
 {
 	return 0;
diff -ruNpX dontdiff linux-2.6.9/security/dummy.c linux-2.6.9.selinux/security/dummy.c
--- linux-2.6.9/security/dummy.c	2004-11-19 14:40:58.000000000 -0500
+++ linux-2.6.9.selinux/security/dummy.c	2004-11-19 18:28:03.000000000 -0500
@@ -462,6 +462,11 @@ static int dummy_inode_listsecurity(stru
 	return 0;
 }
 
+static void dummy_inode_mark_private(struct inode *inode)
+{
+	return;
+}
+
 static int dummy_file_permission (struct file *file, int mask)
 {
 	return 0;
@@ -949,6 +954,7 @@ void security_fixup_ops (struct security
 	set_to_dummy_if_null(ops, inode_getsecurity);
 	set_to_dummy_if_null(ops, inode_setsecurity);
 	set_to_dummy_if_null(ops, inode_listsecurity);
+	set_to_dummy_if_null(ops, inode_mark_private);
 	set_to_dummy_if_null(ops, file_permission);
 	set_to_dummy_if_null(ops, file_alloc_security);
 	set_to_dummy_if_null(ops, file_free_security);
diff -ruNpX dontdiff linux-2.6.9/security/selinux/hooks.c linux-2.6.9.selinux/security/selinux/hooks.c
--- linux-2.6.9/security/selinux/hooks.c	2004-11-19 14:40:58.000000000 -0500
+++ linux-2.6.9.selinux/security/selinux/hooks.c	2004-11-20 17:11:22.000000000 -0500
@@ -740,6 +740,15 @@ static int inode_doinit_with_dentry(stru
 	if (isec->initialized)
 		goto out;
 
+	if (opt_dentry && opt_dentry->d_parent && opt_dentry->d_parent->d_inode) {
+		struct inode_security_struct *pisec = opt_dentry->d_parent->d_inode->i_security;
+		if (pisec->inherit) {
+			isec->sid = pisec->sid;
+			isec->initialized = 1;
+			goto out;
+		}
+	}
+
 	down(&isec->sem);
 	hold_sem = 1;
 	if (isec->initialized)
@@ -2391,6 +2400,15 @@ static int selinux_inode_listsecurity(st
 	return len;
 }
 
+static void selinux_inode_mark_private(struct inode *inode)
+{
+	struct inode_security_struct *isec = inode->i_security;
+
+	isec->sid = SECINITSID_KERNEL;
+	isec->initialized = 1;
+	isec->inherit = 1;
+}
+
 /* file security operations */
 
 static int selinux_file_permission(struct file *file, int mask)
@@ -4253,6 +4271,7 @@ struct security_operations selinux_ops =
 	.inode_getsecurity =            selinux_inode_getsecurity,
 	.inode_setsecurity =            selinux_inode_setsecurity,
 	.inode_listsecurity =           selinux_inode_listsecurity,
+	.inode_mark_private =           selinux_inode_mark_private,
 
 	.file_permission =		selinux_file_permission,
 	.file_alloc_security =		selinux_file_alloc_security,
-- 
Jeff Mahoney
SuSE Labs

Re: [PATCH 2/5] selinux: adds a private inode operation

[Stephen Smalley]

On Sat, 2004-11-20 at 19:13, Jeffrey Mahoney wrote:
<snip>
> diff -ruNpX dontdiff linux-2.6.9/security/selinux/hooks.c linux-2.6.9.selinux/security/selinux/hooks.c
> --- linux-2.6.9/security/selinux/hooks.c	2004-11-19 14:40:58.000000000 -0500
> +++ linux-2.6.9.selinux/security/selinux/hooks.c	2004-11-20 17:11:22.000000000 -0500
> @@ -740,6 +740,15 @@ static int inode_doinit_with_dentry(stru
>  	if (isec->initialized)
>  		goto out;
>  
> +	if (opt_dentry && opt_dentry->d_parent && opt_dentry->d_parent->d_inode) {
> +		struct inode_security_struct *pisec = opt_dentry->d_parent->d_inode->i_security;
> +		if (pisec->inherit) {
> +			isec->sid = pisec->sid;
> +			isec->initialized = 1;
> +			goto out;
> +		}
> +	}
> +

Shouldn't this be using dget_parent() for safety?

> @@ -2391,6 +2400,15 @@ static int selinux_inode_listsecurity(st
>  	return len;
>  }
>  
> +static void selinux_inode_mark_private(struct inode *inode)
> +{
> +	struct inode_security_struct *isec = inode->i_security;
> +
> +	isec->sid = SECINITSID_KERNEL;
> +	isec->initialized = 1;
> +	isec->inherit = 1;
> +}
> +

Don't we also need to modify inode_has_perm() to skip checking if the
inode has the kernel SID (as is already done by socket_has_perm) to
avoid the search checks when the reiserfs code looks up xattrs? 
Otherwise, we'll see access attempts by the process context on
directories with the kernel SID upon such lookups.

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH 2/5] selinux: adds a private inode operation

[Stephen Smalley]

On Mon, 2004-11-22 at 08:35, Stephen Smalley wrote:
> Don't we also need to modify inode_has_perm() to skip checking if the
> inode has the kernel SID (as is already done by socket_has_perm) to
> avoid the search checks when the reiserfs code looks up xattrs? 
> Otherwise, we'll see access attempts by the process context on
> directories with the kernel SID upon such lookups.

Actually, I think we need a new flag field in the inode_security_struct
to explicitly mark these "private" inodes for SELinux, so that
inode_has_perm() can skip permission checking on them while still
applying checks to any other inodes that may have the kernel SID (e.g.
/proc/pid inodes for kernel threads).

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH 2/5] selinux: adds a private inode operation

[James Morris]

On Mon, 22 Nov 2004, Stephen Smalley wrote:

> Actually, I think we need a new flag field in the inode_security_struct
> to explicitly mark these "private" inodes for SELinux, so that
> inode_has_perm() can skip permission checking on them while still
> applying checks to any other inodes that may have the kernel SID (e.g.
> /proc/pid inodes for kernel threads).

Agreed.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH 2/5] selinux: adds a private inode operation

[Stephen Smalley]

On Sat, 2004-11-20 at 19:13, Jeffrey Mahoney wrote:
> diff -ruNpX dontdiff linux-2.6.9/security/selinux/hooks.c linux-2.6.9.selinux/security/selinux/hooks.c
> --- linux-2.6.9/security/selinux/hooks.c	2004-11-19 14:40:58.000000000 -0500
> +++ linux-2.6.9.selinux/security/selinux/hooks.c	2004-11-20 17:11:22.000000000 -0500
> @@ -740,6 +740,15 @@ static int inode_doinit_with_dentry(stru
>  	if (isec->initialized)
>  		goto out;
>  
> +	if (opt_dentry && opt_dentry->d_parent && opt_dentry->d_parent->d_inode) {
> +		struct inode_security_struct *pisec = opt_dentry->d_parent->d_inode->i_security;
> +		if (pisec->inherit) {
> +			isec->sid = pisec->sid;
> +			isec->initialized = 1;
> +			goto out;
> +		}
> +	}
> +
>  	down(&isec->sem);
>  	hold_sem = 1;
>  	if (isec->initialized)

Actually, isn't this code unnecessary given that patch 3/5 ensures that
the selinux_inode_mark_private() hook is called from
reiserfs_new_inode() on the new inode if the directory is private?  I
think that eliminates the need to perform this test and inheritance in
inode_doinit, which is called by the d_instantiate.

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH 2/5] selinux: adds a private inode operation

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Stephen Smalley wrote:
| On Sat, 2004-11-20 at 19:13, Jeffrey Mahoney wrote:
|
|>diff -ruNpX dontdiff linux-2.6.9/security/selinux/hooks.c
linux-2.6.9.selinux/security/selinux/hooks.c
|>--- linux-2.6.9/security/selinux/hooks.c	2004-11-19 14:40:58.000000000
- -0500
|>+++ linux-2.6.9.selinux/security/selinux/hooks.c	2004-11-20
17:11:22.000000000 -0500
|>@@ -740,6 +740,15 @@ static int inode_doinit_with_dentry(stru
|> 	if (isec->initialized)
|> 		goto out;
|>
|>+	if (opt_dentry && opt_dentry->d_parent &&
opt_dentry->d_parent->d_inode) {
|>+		struct inode_security_struct *pisec =
opt_dentry->d_parent->d_inode->i_security;
|>+		if (pisec->inherit) {
|>+			isec->sid = pisec->sid;
|>+			isec->initialized = 1;
|>+			goto out;
|>+		}
|>+	}
|>+
|> 	down(&isec->sem);
|> 	hold_sem = 1;
|> 	if (isec->initialized)
|
|
| Actually, isn't this code unnecessary given that patch 3/5 ensures that
| the selinux_inode_mark_private() hook is called from
| reiserfs_new_inode() on the new inode if the directory is private?  I
| think that eliminates the need to perform this test and inheritance in
| inode_doinit, which is called by the d_instantiate.
|

Yes, you're right. The isec->initialized check means that code never
gets executed.

- -Jeff

- --
Jeff Mahoney
SuSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFBoiomLPWxlyuTD7IRAu3TAKCJK4LycKusauFJ/QPUIJSC3hqzaACgmsPD
Gte20LrcLzyB6Yjc83JJmr0=
=5sgF
-----END PGP SIGNATURE-----

-- 
Jeff Mahoney
SuSE Labs

Re: [PATCH 2/5] selinux: adds a private inode operation

[Stephen Smalley]

On Mon, 2004-11-22 at 13:04, Jeff Mahoney wrote:
> Yes, you're right. The isec->initialized check means that code never
> gets executed.

Ok.  How about the untested diff below relative to your patch?  It
removes the unnecessary code, replaces the unused inherits flag field
(legacy from earlier code) with a private flag field, does not set the
SID in selinux_inode_mark_private (leaving it with the unlabeled SID,
which will ensure that we notice it if it ever reaches a SELinux
permission check), and modifies SELinux permission checking functions
and post_create() to test for the private flag and skip SELinux
processing in that case.  Note that this means that reiserfs should be
able to use the VFS helpers if desired without interference by SELinux
when accessing these private inodes.

diff -X /home/sds/dontdiff -rup linux-2.6.10-rc2-mm3/security/selinux/hooks.c linux-2.6/security/selinux/hooks.c
--- linux-2.6.10-rc2-mm3/security/selinux/hooks.c	2004-11-22 08:30:49.000000000 -0500
+++ linux-2.6/security/selinux/hooks.c	2004-11-22 13:20:08.510714592 -0500
@@ -737,15 +736,6 @@ static int inode_doinit_with_dentry(stru
 	if (isec->initialized)
 		goto out;
 
-	if (opt_dentry && opt_dentry->d_parent && opt_dentry->d_parent->d_inode) {
-		struct inode_security_struct *pisec = opt_dentry->d_parent->d_inode->i_security;
-		if (pisec->inherit) {
-			isec->sid = pisec->sid;
-			isec->initialized = 1;
-			goto out;
-		}
-	}
-
 	down(&isec->sem);
 	hold_sem = 1;
 	if (isec->initialized)
@@ -983,6 +973,9 @@ int inode_has_perm(struct task_struct *t
 	tsec = tsk->security;
 	isec = inode->i_security;
 
+	if (isec->private)
+		return 0;
+
 	if (!adp) {
 		adp = &ad;
 		AVC_AUDIT_DATA_INIT(&ad, FS);
@@ -1064,6 +1057,9 @@ static int may_create(struct inode *dir,
 	dsec = dir->i_security;
 	sbsec = dir->i_sb->s_security;
 
+	if (dsec->private)
+		return 0;
+
 	AVC_AUDIT_DATA_INIT(&ad, FS);
 	ad.u.fs.dentry = dentry;
 
@@ -1111,6 +1107,9 @@ static int may_link(struct inode *dir,
 	dsec = dir->i_security;
 	isec = dentry->d_inode->i_security;
 
+	if (dsec->private)
+		return 0;
+
 	AVC_AUDIT_DATA_INIT(&ad, FS);
 	ad.u.fs.dentry = dentry;
 
@@ -1157,6 +1156,9 @@ static inline int may_rename(struct inod
 	old_is_dir = S_ISDIR(old_dentry->d_inode->i_mode);
 	new_dsec = new_dir->i_security;
 
+	if (old_dsec->private)
+		return 0;
+
 	AVC_AUDIT_DATA_INIT(&ad, FS);
 
 	ad.u.fs.dentry = old_dentry;
@@ -1292,7 +1294,10 @@ static int post_create(struct inode *dir
 	dsec = dir->i_security;
 	sbsec = dir->i_sb->s_security;
 
+	if (dsec->private)
+		return 0;
+
 	inode = dentry->d_inode;
 	if (!inode) {
 		/* Some file system types (e.g. NFS) may not instantiate

@@ -2379,9 +2384,8 @@ static void selinux_inode_mark_private(s
 {
 	struct inode_security_struct *isec = inode->i_security;
 
-	isec->sid = SECINITSID_KERNEL;
+	isec->private = 1;
 	isec->initialized = 1;
-	isec->inherit = 1;
 }
 
 /* file security operations */

diff -X /home/sds/dontdiff -rup linux-2.6.10-rc2-mm3/security/selinux/include/objsec.h linux-2.6/security/selinux/include/objsec.h
--- linux-2.6.10-rc2-mm3/security/selinux/include/objsec.h	2004-11-22 08:30:49.000000000 -0500
+++ linux-2.6/security/selinux/include/objsec.h	2004-11-22 13:00:22.418028088 -0500
@@ -45,7 +45,7 @@ struct inode_security_struct {
 	u16 sclass;       /* security class of this object */
 	unsigned char initialized;     /* initialization flag */
 	struct semaphore sem;
-	unsigned char inherit;         /* inherit SID from parent entry */
+       unsigned char private;         /* private file, skip checks */
 };
 
 struct file_security_struct {


-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH 2/5] selinux: adds a private inode operation

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Stephen Smalley wrote:
| On Mon, 2004-11-22 at 13:04, Jeff Mahoney wrote:
|
|>Yes, you're right. The isec->initialized check means that code never
|>gets executed.
|
|
| Ok.  How about the untested diff below relative to your patch?  It
| removes the unnecessary code, replaces the unused inherits flag field
| (legacy from earlier code) with a private flag field, does not set the
| SID in selinux_inode_mark_private (leaving it with the unlabeled SID,
| which will ensure that we notice it if it ever reaches a SELinux
| permission check), and modifies SELinux permission checking functions
| and post_create() to test for the private flag and skip SELinux
| processing in that case.  Note that this means that reiserfs should be
| able to use the VFS helpers if desired without interference by SELinux
| when accessing these private inodes.
|

Excellent. Thanks. Preliminary testing works as expected (ie: deadlocks
don't occur, xattrs/<dir> is removed when owning file is deleted)

I've integrated the changes into my patch set. With those issues
addressed, would you feel these would be appropriate for inclusion? I
suspect you may have gotten questions as many interested parties in this
feature working as I have.

- -Jeff

- --
Jeff Mahoney
SuSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFBojkiLPWxlyuTD7IRAoM/AJ9XQ/twgcSTiRGQU1kOp01A2NlhywCfWrvF
/LtguPq+tMjFe1uThpR4fws=
=izmO
-----END PGP SIGNATURE-----

Re: [PATCH 2/5] selinux: adds a private inode operation

[Chris Wright]

* Jeff Mahoney (jeffm@suse.com) wrote:
> Excellent. Thanks. Preliminary testing works as expected (ie: deadlocks
> don't occur, xattrs/<dir> is removed when owning file is deleted)
> 
> I've integrated the changes into my patch set. With those issues
> addressed, would you feel these would be appropriate for inclusion? I
> suspect you may have gotten questions as many interested parties in this
> feature working as I have.

Why add extra hook, when this could be done in VFS with i_flags?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH 2/5] selinux: adds a private inode operation

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Chris Wright wrote:
| * Jeff Mahoney (jeffm@suse.com) wrote:
|
|>Excellent. Thanks. Preliminary testing works as expected (ie: deadlocks
|>don't occur, xattrs/<dir> is removed when owning file is deleted)
|>
|>I've integrated the changes into my patch set. With those issues
|>addressed, would you feel these would be appropriate for inclusion? I
|>suspect you may have gotten questions as many interested parties in this
|>feature working as I have.
|
|
| Why add extra hook, when this could be done in VFS with i_flags?

Sure, it could be done w/ an i_flags bit. However, since it's explicitly
related to the security infrastructure, I think it's more appropriate
there. There's no change in the size of inode_security_struct, and the
addition of the deref is trivial given how many other places in the
file-io path use the same call table. That said, I'll change it to use
whatever ends up being agreed upon. I'm just looking to get selinux to
not call xattr routines on reiserfs-internal files/directories.

- -Jeff

- --
Jeff Mahoney
SuSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFBo48QLPWxlyuTD7IRAiIlAJ9wN7JPywCVz/UVJ1dVTM1kQphPRQCgoK8J
TMDL9KrBABk49cbfN7AcoOA=
=6RPR
-----END PGP SIGNATURE-----

Re: [PATCH 2/5] selinux: adds a private inode operation

[Stephen Smalley]

On Tue, 2004-11-23 at 14:27, Jeff Mahoney wrote:
> Chris Wright wrote:
> | Why add extra hook, when this could be done in VFS with i_flags?
> 
> Sure, it could be done w/ an i_flags bit. However, since it's explicitly
> related to the security infrastructure, I think it's more appropriate
> there. There's no change in the size of inode_security_struct, and the
> addition of the deref is trivial given how many other places in the
> file-io path use the same call table. That said, I'll change it to use
> whatever ends up being agreed upon. I'm just looking to get selinux to
> not call xattr routines on reiserfs-internal files/directories.

I suppose the question is whether the VFS maintainer thinks that this
reiserfs private inode flag should be made visible in the i_flags, or
whether it should remain private to reiserfs and only explicitly
exported to the security modules via the new hook.  We can implement the
corresponding SELinux support for handling such private inodes either
way.  Would the VFS ever make use of the flag itself, e.g. to skip DAC
permission checking on such inodes as well?

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency