LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch
@ 2011-01-31  3:26 Lucian Adrian Grijincu
  2011-01-31 13:59 ` Stephen Smalley
  0 siblings, 1 reply; 9+ messages in thread
From: Lucian Adrian Grijincu @ 2011-01-31  3:26 UTC (permalink / raw)
  To: Stephen Smalley, James Morris, Eric Paris, Al Viro,
	Christoph Hellwig, Lucian Adrian Grijincu, Dave Chinner,
	Arnd Bergmann, linux-kernel, linux-security-module, selinux,
	Eric W. Biederman

[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]

Eric's patch was rejected because it broke selinux labeling:
http://thread.gmane.org/gmane.linux.kernel.lsm/9807/focus=9841

       This seems to break labeling.  Prior to this patch, I see:

       # ls -lZ /proc/1/net/rpc/nfsd.fh
       -rw-------. root root system_u:object_r:sysctl_rpc_t:s0 channel

       with the patch:

       # ls -lZ /proc/1/net/rpc/nfsd.fh
       -rw-------. root root system_u:object_r:proc_t:s0      channel

My patch seems to have fixed this problem (it correctly reports
sysctl_rpc_t in this case), but my selinux experience is ε > 0 and I
have no ideea what else it may have broken.

I ran 'find /proc | xargs ls -Z > f' on a kernel with an without
these patches:
* http://swarm.cs.pub.ro/~lucian/store/ls-Z-with-patch
* http://swarm.cs.pub.ro/~lucian/store/ls-Z-without-patch

My setup is a custom busybox live CD with selinux enabled, with
/etc/selinux and /usr/share/selinux/default copied from Ubuntu 10.10's
selinux-policy-default package. I'm sure there are lots of reasons why
this is not correcly configured, etc., but I have no experience
regarding selinux. I can make all the scripts/sources/configs/etc
available to anyone interested.

NOTE: this patch will be merged with:
  security/selinux: Simplify proc inode to security label mapping

I'm only prividing this patch separately to point out the differences
to Eric W. Biederman's patch.

Both of these patches apply cleanly agains Linux 2.6.37.

Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
---
 fs/proc/proc_sysctl.c    |    1 -
 security/selinux/hooks.c |   20 ++++++++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

[-- Attachment #2: 0002-RFC-selinux-sysctl-fix-selinux-labeling-broken-by-la.patch --]
[-- Type: text/x-patch, Size: 1727 bytes --]

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index b652cb0..b17619d 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -31,7 +31,6 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
 	ei->sysctl_entry = table;
 
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-	inode->i_flags |= S_PRIVATE; /* tell selinux to ignore this inode */
 	inode->i_mode = table->mode;
 	if (!table->child) {
 		inode->i_mode |= S_IFREG;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 51615f6..5f58019 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1132,8 +1132,23 @@ static int selinux_proc_get_sid(struct dentry *dentry,
 	path = dentry_path(dentry, buffer, PAGE_SIZE);
 	if (IS_ERR(path))
 		rc = PTR_ERR(path);
-	else
+	else {
+		/* because dentry is not hashed, dentry_path() will
+		 * append "//deleted" to the end of the string. We'll
+		 * remove this as no /proc/ file is named so. */
+		int pathlen = strlen(path);
+		int dellen = strlen("//deleted");
+		if ((pathlen > dellen) && strcmp(path + pathlen - dellen, "//deleted") == 0)
+			path[pathlen-dellen] = '\0';
+
+		/* each process gets a /proc/PID/ entry. Strip off the
+		 * PID part to get a valid selinux labeling. */
+		while (path[1] >= '0' && path[1] <= '9') {
+			path[1] = '/';
+			path++;
+		}
 		rc = security_genfs_sid("proc", path, tclass, sid);
+	}
 	free_page((unsigned long)buffer);
 	return rc;
 }
@@ -1464,9 +1479,6 @@ static int inode_has_perm(const struct cred *cred,
 
 	validate_creds(cred);
 
-	if (unlikely(IS_PRIVATE(inode)))
-		return 0;
-
 	sid = cred_sid(cred);
 	isec = inode->i_security;
 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch
  2011-01-31  3:26 [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch Lucian Adrian Grijincu
@ 2011-01-31 13:59 ` Stephen Smalley
  2011-01-31 14:14   ` Lucian Adrian Grijincu
  2011-01-31 16:27   ` Lucian Adrian Grijincu
  0 siblings, 2 replies; 9+ messages in thread
From: Stephen Smalley @ 2011-01-31 13:59 UTC (permalink / raw)
  To: Lucian Adrian Grijincu
  Cc: James Morris, Eric Paris, Al Viro, Christoph Hellwig,
	Dave Chinner, Arnd Bergmann, linux-kernel, linux-security-module,
	selinux, Eric W. Biederman

On Mon, 2011-01-31 at 05:26 +0200, Lucian Adrian Grijincu wrote:
> Eric's patch was rejected because it broke selinux labeling:
> http://thread.gmane.org/gmane.linux.kernel.lsm/9807/focus=9841
> 
>        This seems to break labeling.  Prior to this patch, I see:
> 
>        # ls -lZ /proc/1/net/rpc/nfsd.fh
>        -rw-------. root root system_u:object_r:sysctl_rpc_t:s0 channel
> 
>        with the patch:
> 
>        # ls -lZ /proc/1/net/rpc/nfsd.fh
>        -rw-------. root root system_u:object_r:proc_t:s0      channel
> 
> My patch seems to have fixed this problem (it correctly reports
> sysctl_rpc_t in this case), but my selinux experience is ε > 0 and I
> have no ideea what else it may have broken.
> 
> I ran 'find /proc | xargs ls -Z > f' on a kernel with an without
> these patches:
> * http://swarm.cs.pub.ro/~lucian/store/ls-Z-with-patch
> * http://swarm.cs.pub.ro/~lucian/store/ls-Z-without-patch
> 
> My setup is a custom busybox live CD with selinux enabled, with
> /etc/selinux and /usr/share/selinux/default copied from Ubuntu 10.10's
> selinux-policy-default package. I'm sure there are lots of reasons why
> this is not correcly configured, etc., but I have no experience
> regarding selinux. I can make all the scripts/sources/configs/etc
> available to anyone interested.
> 
> NOTE: this patch will be merged with:
>   security/selinux: Simplify proc inode to security label mapping
> 
> I'm only prividing this patch separately to point out the differences
> to Eric W. Biederman's patch.
> 
> Both of these patches apply cleanly agains Linux 2.6.37.
> 
> Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
> ---
>  fs/proc/proc_sysctl.c    |    1 -
>  security/selinux/hooks.c |   20 ++++++++++++++++----
>  2 files changed, 16 insertions(+), 5 deletions(-)

It would be better to include the patch inline for review.  In any
event, a few observations on your patch:
- We don't want to replace "incestuous" knowledge of proc with
"incestous" knowledge of the dcache.  So rather than encoding knowledge
of the magical "//deleted" suffix into selinux, use an interface to the
dcache (or add one if none exists) that does not append that suffix at
all.  I think apparmor did something similar to deal with the (deleted)
suffix for d_path.

- You don't need special handling of /proc/PID entries.  Those are
labeled via the security_task_to_inode -> selinux_task_to_inode hook,
called from proc_pid_make_inode and the _revalidate functions.

- Don't remove the IS_PRIVATE() test from inode_has_perm(), as other
inodes beyond just the /proc/sys ones are marked with that flag
(original usage was for reiserfs xattr inodes).

-- 
Stephen Smalley
National Security Agency


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch
  2011-01-31 13:59 ` Stephen Smalley
@ 2011-01-31 14:14   ` Lucian Adrian Grijincu
  2011-01-31 14:21     ` Stephen Smalley
  2011-01-31 16:27   ` Lucian Adrian Grijincu
  1 sibling, 1 reply; 9+ messages in thread
From: Lucian Adrian Grijincu @ 2011-01-31 14:14 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: James Morris, Eric Paris, Al Viro, Christoph Hellwig,
	Dave Chinner, Arnd Bergmann, linux-kernel, linux-security-module,
	selinux, Eric W. Biederman

On Mon, Jan 31, 2011 at 3:59 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> - Don't remove the IS_PRIVATE() test from inode_has_perm(), as other
> inodes beyond just the /proc/sys ones are marked with that flag
> (original usage was for reiserfs xattr inodes).


Are you sure? I believe it was added here:

    [PATCH] selinux: enhance selinux to always ignore private inodes

    Hmmm...turns out to not be quite enough, as the /proc/sys inodes
aren't truly
    private to the fs, so we can run into them in a variety of security hooks
    beyond just the inode hooks, such as security_file_permission (when reading
    and writing them via the vfs helpers), security_sb_mount (when
mounting other
    filesystems on directories in proc like binfmt_misc), and deeper within the
    security module itself (as in flush_unauthorized_files upon
inheritance across
    execve).  So I think we have to add an IS_PRIVATE() guard within SELinux, as
    below.  Note however that the use of the private flag here could
be confusing,
    as these inodes are _not_ private to the fs, are exposed to userspace, and
    security modules must implement the sysctl hook to get any access
control over
    them.


http://thread.gmane.org/gmane.comp.security.selinux/341/focus=519


In my patch I don't care about IS_PRIVATE, because I don't mark proc
inodes as PRIVATE any more.


This patch added S_ISPRIVATE to proc inodes:
    [PATCH] sysctl: hide the sysctl proc inodes from selinux
    86a71dbd3e81e8870d0f0e56b87875f57e58222b

This one added the IS_PRIVATE check:
    [PATCH] selinux: enhance selinux to always ignore private inodes
    bbaca6c2e7ef0f663bc31be4dad7cf530f6c4962


I'll remove the check from my patch if you say it's used in other
places too, but the original usage does not seem to be "for reiserfs
xattr inodes".

-- 
 .
..: Lucian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch
  2011-01-31 14:14   ` Lucian Adrian Grijincu
@ 2011-01-31 14:21     ` Stephen Smalley
  0 siblings, 0 replies; 9+ messages in thread
From: Stephen Smalley @ 2011-01-31 14:21 UTC (permalink / raw)
  To: Lucian Adrian Grijincu
  Cc: James Morris, Eric Paris, Al Viro, Christoph Hellwig,
	Dave Chinner, Arnd Bergmann, linux-kernel, linux-security-module,
	selinux, Eric W. Biederman

On Mon, 2011-01-31 at 16:14 +0200, Lucian Adrian Grijincu wrote:
> On Mon, Jan 31, 2011 at 3:59 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > - Don't remove the IS_PRIVATE() test from inode_has_perm(), as other
> > inodes beyond just the /proc/sys ones are marked with that flag
> > (original usage was for reiserfs xattr inodes).
> 
> 
> Are you sure? I believe it was added here:
> 
>     [PATCH] selinux: enhance selinux to always ignore private inodes
> 
>     Hmmm...turns out to not be quite enough, as the /proc/sys inodes
> aren't truly
>     private to the fs, so we can run into them in a variety of security hooks
>     beyond just the inode hooks, such as security_file_permission (when reading
>     and writing them via the vfs helpers), security_sb_mount (when
> mounting other
>     filesystems on directories in proc like binfmt_misc), and deeper within the
>     security module itself (as in flush_unauthorized_files upon
> inheritance across
>     execve).  So I think we have to add an IS_PRIVATE() guard within SELinux, as
>     below.  Note however that the use of the private flag here could
> be confusing,
>     as these inodes are _not_ private to the fs, are exposed to userspace, and
>     security modules must implement the sysctl hook to get any access
> control over
>     them.
> 
> 
> http://thread.gmane.org/gmane.comp.security.selinux/341/focus=519
> 
> 
> In my patch I don't care about IS_PRIVATE, because I don't mark proc
> inodes as PRIVATE any more.
> 
> 
> This patch added S_ISPRIVATE to proc inodes:
>     [PATCH] sysctl: hide the sysctl proc inodes from selinux
>     86a71dbd3e81e8870d0f0e56b87875f57e58222b
> 
> This one added the IS_PRIVATE check:
>     [PATCH] selinux: enhance selinux to always ignore private inodes
>     bbaca6c2e7ef0f663bc31be4dad7cf530f6c4962
> 
> 
> I'll remove the check from my patch if you say it's used in other
> places too, but the original usage does not seem to be "for reiserfs
> xattr inodes".

Ok, my mistake - you can leave that part alone.  The original usage of
the S_PRIVATE flag was for reiserfs xattr inodes, but it appears that
you are correct that we don't need the IS_PRIVATE() guard within the
selinux code for that use case.

-- 
Stephen Smalley
National Security Agency


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch
  2011-01-31 13:59 ` Stephen Smalley
  2011-01-31 14:14   ` Lucian Adrian Grijincu
@ 2011-01-31 16:27   ` Lucian Adrian Grijincu
  2011-01-31 16:59     ` Stephen Smalley
  1 sibling, 1 reply; 9+ messages in thread
From: Lucian Adrian Grijincu @ 2011-01-31 16:27 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: James Morris, Eric Paris, Al Viro, Christoph Hellwig,
	Dave Chinner, Arnd Bergmann, linux-kernel, linux-security-module,
	selinux, Eric W. Biederman

On Mon, Jan 31, 2011 at 3:59 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> - You don't need special handling of /proc/PID entries.  Those are
> labeled via the security_task_to_inode -> selinux_task_to_inode hook,
> called from proc_pid_make_inode and the _revalidate functions.


On a clean 2.6.37 I ran:
   ls -Z /proc/1/net/rpc/nfs    >> find.txt
   ls -Z /proc/1/limits         >> find.txt
   ls -Z /proc/1/net/netfilter  >> find.txt

with a printk() for the path computed in selinux_proc_get_sid. These
paths printed were:
  /net/rpc/nfs
  /net/netfilter
  /net/netfilter/nf_log [ and others files in netfilter dir ]


So, while there may be some ->sid initializing in the hooks you
mention, these ones get their sid from selinux_proc_get_sid called
from inode_doinit_with_dentry.

There was nothing for /proc/1/limits because it got filtered by this
check in inode_doinit_with_dentry():
         struct proc_inode *proci = PROC_I(inode);
         if (proci->pde) {
                selinux_proc_get_sid(...)


I do need to get the PID part out to compute a valid path for
selinux_proc_get_sid().

Before Eric's patch the path was computed by walking the struct
proc_dir_entry->parent links. These links stopped before the PID
entry: /proc/PID/net/stuff -> /net/stuff.

Because now we walk the path without /proc/ -> /PID/net/stuff, we will
send a path to selinux_proc_get_sid that will not be mappable to a
valid label so it will default to proc_t.


I know understand why I saw some differences between running 'find
/proc | xargs ls -Z' with and without these patches: Eric's patches
called selinux_proc_get_sid() without this check:
         struct proc_inode *proci = PROC_I(inode);
         if (proci->pde) {


I've updated my patch to take this into consideration. I'll post an
update later with the patches merged and without the "//deleted"
manipulation.


Again, I have next to nothing selinux experience, and probably my test
system is very bad configured to run a relevant selinux test. I've
uploaded here the output and dmesg from running 'find /proc | xargs ls
-Z' on 2.6.37 with and without these two patches:
http://swarm.cs.pub.ro/~lucian/store/v4/

There are now fewer differences than before, but I'd like to point
something out: *without* the patches files in /proc/sys/* get labeled
like this.
-r--r--r-- unknown                          /proc/sys/fs/file-nr
-rw-r--r-- unknown                          /proc/sys/debug/exception-trace
-rw-r--r-- unknown                          /proc/sys/dev/cdrom/autoclose
-rw-r--r-- unknown                          /proc/sys/kernel/sem
-rw-r--r-- unknown
/proc/sys/net/ipv4/conf/all/accept_local

but with the patches:
-r--r--r-- system_u:object_r:sysctl_fs_t:s0 /proc/sys/fs/file-nr
-rw-r--r-- system_u:object_r:sysctl_t:s0    /proc/sys/debug/exception-trace
-rw-r--r-- system_u:object_r:sysctl_dev_t:s0 /proc/sys/dev/cdrom/autoclose
-rw-r--r-- system_u:object_r:sysctl_kernel_t:s0 /proc/sys/kernel/sem
-rw-r--r-- system_u:object_r:sysctl_net_t:s0
/proc/sys/net/ipv4/conf/all/accept_local


There seem to be no labeling mismatches elsewhere. So either sysctl
labeling is broken in 2.6.37 or my test setup is broken.

I'll try to find out if there's an earlier kernel that works on my
setul for sysctl too.

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index b652cb0..b17619d 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -31,7 +31,6 @@ static struct inode *proc_sys_make_inode(struct
super_block *sb,
 	ei->sysctl_entry = table;

 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-	inode->i_flags |= S_PRIVATE; /* tell selinux to ignore this inode */
 	inode->i_mode = table->mode;
 	if (!table->child) {
 		inode->i_mode |= S_IFREG;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 51615f6..af8be17 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1132,8 +1132,23 @@ static int selinux_proc_get_sid(struct dentry *dentry,
 	path = dentry_path(dentry, buffer, PAGE_SIZE);
 	if (IS_ERR(path))
 		rc = PTR_ERR(path);
-	else
+	else {
+		/* because dentry is not hashed, dentry_path() will
+		 * append "//deleted" to the end of the string. We'll
+		 * remove this as no /proc/ file is named so. */
+		int pathlen = strlen(path);
+		int dellen = strlen("//deleted");
+		if ((pathlen > dellen) && strcmp(path + pathlen - dellen, "//deleted") == 0)
+			path[pathlen-dellen] = '\0';
+
+		/* each process gets a /proc/PID/ entry. Strip off the
+		 * PID part to get a valid selinux labeling. */
+		while (path[1] >= '0' && path[1] <= '9') {
+			path[1] = '/';
+			path++;
+		}
 		rc = security_genfs_sid("proc", path, tclass, sid);
+	}
 	free_page((unsigned long)buffer);
 	return rc;
 }
@@ -1302,7 +1317,8 @@ static int inode_doinit_with_dentry(struct inode
*inode, struct dentry *opt_dent
 		isec->sid = sbsec->sid;

 		if ((sbsec->flags & SE_SBPROC) && !S_ISLNK(inode->i_mode)) {
-			if (opt_dentry) {
+			struct proc_inode *proci = PROC_I(inode);
+			if (opt_dentry && (proci->pde || proci->sysctl)) {
 				isec->sclass = inode_mode_to_security_class(inode->i_mode);
 				rc = selinux_proc_get_sid(opt_dentry,
 							  isec->sclass,
@@ -1464,9 +1480,6 @@ static int inode_has_perm(const struct cred *cred,

 	validate_creds(cred);

-	if (unlikely(IS_PRIVATE(inode)))
-		return 0;
-
 	sid = cred_sid(cred);
 	isec = inode->i_security;

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch
  2011-01-31 16:27   ` Lucian Adrian Grijincu
@ 2011-01-31 16:59     ` Stephen Smalley
  2011-01-31 17:03       ` Lucian Adrian Grijincu
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2011-01-31 16:59 UTC (permalink / raw)
  To: Lucian Adrian Grijincu
  Cc: James Morris, Eric Paris, Al Viro, Christoph Hellwig,
	Dave Chinner, Arnd Bergmann, linux-kernel, linux-security-module,
	selinux, Eric W. Biederman

On Mon, 2011-01-31 at 18:27 +0200, Lucian Adrian Grijincu wrote:
> There are now fewer differences than before, but I'd like to point
> something out: *without* the patches files in /proc/sys/* get labeled
> like this.
> -r--r--r-- unknown                          /proc/sys/fs/file-nr
> -rw-r--r-- unknown                          /proc/sys/debug/exception-trace
> -rw-r--r-- unknown                          /proc/sys/dev/cdrom/autoclose
> -rw-r--r-- unknown                          /proc/sys/kernel/sem
> -rw-r--r-- unknown
> /proc/sys/net/ipv4/conf/all/accept_local
> 
> but with the patches:
> -r--r--r-- system_u:object_r:sysctl_fs_t:s0 /proc/sys/fs/file-nr
> -rw-r--r-- system_u:object_r:sysctl_t:s0    /proc/sys/debug/exception-trace
> -rw-r--r-- system_u:object_r:sysctl_dev_t:s0 /proc/sys/dev/cdrom/autoclose
> -rw-r--r-- system_u:object_r:sysctl_kernel_t:s0 /proc/sys/kernel/sem
> -rw-r--r-- system_u:object_r:sysctl_net_t:s0
> /proc/sys/net/ipv4/conf/all/accept_local
> 
> 
> There seem to be no labeling mismatches elsewhere. So either sysctl
> labeling is broken in 2.6.37 or my test setup is broken.

/proc/sys inode labeling was disabled earlier (hence marked S_PRIVATE)
when /proc/sys was reimplemented by Eric, so all access control
on /proc/sys was switched to using the sysctl hook rather than the
inode-based checking.  That's why you don't get a result from ls -Z
on /proc/sys on current kernels.  Getting actual labeling working again
for those inodes would be a win, so your patch is an improvement in that
regard for selinux.

-- 
Stephen Smalley
National Security Agency


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch
  2011-01-31 16:59     ` Stephen Smalley
@ 2011-01-31 17:03       ` Lucian Adrian Grijincu
  2011-01-31 18:35         ` Stephen Smalley
  0 siblings, 1 reply; 9+ messages in thread
From: Lucian Adrian Grijincu @ 2011-01-31 17:03 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: James Morris, Eric Paris, Al Viro, Christoph Hellwig,
	Dave Chinner, Arnd Bergmann, linux-kernel, linux-security-module,
	selinux, Eric W. Biederman

On Mon, Jan 31, 2011 at 6:59 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> /proc/sys inode labeling was disabled earlier (hence marked S_PRIVATE)
> when /proc/sys was reimplemented by Eric, so all access control
> on /proc/sys was switched to using the sysctl hook rather than the
> inode-based checking.  That's why you don't get a result from ls -Z
> on /proc/sys on current kernels.  Getting actual labeling working again
> for those inodes would be a win, so your patch is an improvement in that
> regard for selinux.


Oh, OK. Thanks for letting me know.

Do you see anything else that is wrong with these patches (apart from
"//deleted")?

-- 
 .
..: Lucian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch
  2011-01-31 17:03       ` Lucian Adrian Grijincu
@ 2011-01-31 18:35         ` Stephen Smalley
  2011-01-31 19:55           ` Stephen Smalley
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2011-01-31 18:35 UTC (permalink / raw)
  To: Lucian Adrian Grijincu
  Cc: James Morris, Eric Paris, Al Viro, Christoph Hellwig,
	Dave Chinner, Arnd Bergmann, linux-kernel, linux-security-module,
	selinux, Eric W. Biederman

On Mon, 2011-01-31 at 19:03 +0200, Lucian Adrian Grijincu wrote:
> On Mon, Jan 31, 2011 at 6:59 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > /proc/sys inode labeling was disabled earlier (hence marked S_PRIVATE)
> > when /proc/sys was reimplemented by Eric, so all access control
> > on /proc/sys was switched to using the sysctl hook rather than the
> > inode-based checking.  That's why you don't get a result from ls -Z
> > on /proc/sys on current kernels.  Getting actual labeling working again
> > for those inodes would be a win, so your patch is an improvement in that
> > regard for selinux.
> 
> 
> Oh, OK. Thanks for letting me know.
> 
> Do you see anything else that is wrong with these patches (apart from
> "//deleted")?

No, although I think someone should take them for a spin on a modern
Fedora in enforcing mode for a bit, and likely run the selinux testsuite
too.

-- 
Stephen Smalley
National Security Agency


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch
  2011-01-31 18:35         ` Stephen Smalley
@ 2011-01-31 19:55           ` Stephen Smalley
  0 siblings, 0 replies; 9+ messages in thread
From: Stephen Smalley @ 2011-01-31 19:55 UTC (permalink / raw)
  To: Lucian Adrian Grijincu
  Cc: James Morris, Eric Paris, Al Viro, Christoph Hellwig,
	Dave Chinner, Arnd Bergmann, linux-kernel, linux-security-module,
	selinux, Eric W. Biederman

On Mon, 2011-01-31 at 13:35 -0500, Stephen Smalley wrote:
> On Mon, 2011-01-31 at 19:03 +0200, Lucian Adrian Grijincu wrote:
> > On Mon, Jan 31, 2011 at 6:59 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > /proc/sys inode labeling was disabled earlier (hence marked S_PRIVATE)
> > > when /proc/sys was reimplemented by Eric, so all access control
> > > on /proc/sys was switched to using the sysctl hook rather than the
> > > inode-based checking.  That's why you don't get a result from ls -Z
> > > on /proc/sys on current kernels.  Getting actual labeling working again
> > > for those inodes would be a win, so your patch is an improvement in that
> > > regard for selinux.
> > 
> > 
> > Oh, OK. Thanks for letting me know.
> > 
> > Do you see anything else that is wrong with these patches (apart from
> > "//deleted")?
> 
> No, although I think someone should take them for a spin on a modern
> Fedora in enforcing mode for a bit, and likely run the selinux testsuite
> too.

Booting F14 with your patch applied yields a large number of AVC denials
of the form:
type=AVC msg=audit(1296503592.932:1220139): avc:  denied  { read } for
pid=1896 comm="gnome-settings-" path="anon_inode:inotify"
dev=anon_inodefs ino=5312
scontext=system_u:system_r:xdm_t:s0-s0:c0.c1023
tcontext=system_u:object_r:unlabeled_t:s0 tclass=file

So I assume that the anon_inodefs inodes are being marked private too,
and relying on that test within inode_has_perm to avoid permission
checks.  Which would mean that you need to leave that test alone after
all.

The /proc labeling looks good though.

-- 
Stephen Smalley
National Security Agency


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-01-31 19:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-31  3:26 [PATCH 2/2] RFC: selinux: sysctl: fix selinux labeling broken by last patch Lucian Adrian Grijincu
2011-01-31 13:59 ` Stephen Smalley
2011-01-31 14:14   ` Lucian Adrian Grijincu
2011-01-31 14:21     ` Stephen Smalley
2011-01-31 16:27   ` Lucian Adrian Grijincu
2011-01-31 16:59     ` Stephen Smalley
2011-01-31 17:03       ` Lucian Adrian Grijincu
2011-01-31 18:35         ` Stephen Smalley
2011-01-31 19:55           ` Stephen Smalley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).