[PATCH] security/selinux: fix /proc/sys/ labeling

[PATCH] security/selinux: fix /proc/sys/ labeling

[Lucian Adrian Grijincu]

From: Eric W. Biederman <ebiederm@xmission.com>

This fixes an old (2007) selinux regression: filesystem labeling for
/proc/sys returned
     -r--r--r-- unknown                          /proc/sys/fs/file-nr
instead of
     -r--r--r-- system_u:object_r:sysctl_fs_t:s0 /proc/sys/fs/file-nr

Events that lead to breaking of /proc/sys selinux labeling:

1) sysctl was reimplemented to route all calls through /proc/sysctl

    commit 77b14db502cb85a031fe8fde6c85d52f3e0acb63
    [PATCH] sysctl: reimplement the sysctl proc support

2) proc_dir_entry was removed from ctl_table:

    commit 3fbfa98112fc3962c416452a0baf2214381030e6
    [PATCH] sysctl: remove the proc_dir_entry member for the sysctl tables

3) selinux still walked the proc_dir_entry tree to apply
   labeling. Because ctl_tables don't have a proc_dir_entry, we did
   not label /proc/sys/ inodes any more. To achieve this the /proc/sys
   inodes were marked private and private inodes were ignored by
   selinux.

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

    commit 86a71dbd3e81e8870d0f0e56b87875f57e58222b
    [PATCH] sysctl: hide the sysctl proc inodes from selinux

Access control checks have been done by means of a special sysctl hook
that was called for read/write accesses to any /proc/sys/ entry.

We don't have to do this because, instead of walking the
proc_dir_entry tree we can walk the dentry tree (as done in this
patch). With this patch:
* we don't mark /proc/sys inodes as private
* we don't need the sysclt security hook
* we walk the dentry tree to find the path to the inode.

We have to strip the PID in /proc/PID/ entries that have a
proc_dir_entry because selinux does not know how to label paths like
'/1/net/rpc/nfsd.fh' (and defaults to 'proc_t' labeling). Selinux does
know of '/net/rpc/nfsd.fh' (and applies the 'sysctl_rpc_t' label).

PID stripping from the path was done implicitly in the previous code
because the proc_dir_entry tree had the root in '/net' in the example
from above. The dentry tree has the root in '/1'.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
---
 fs/proc/proc_sysctl.c    |    1 -
 security/selinux/hooks.c |  119 +++++++---------------------------------------
 2 files changed, 18 insertions(+), 102 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 09a1f92..fb707e0 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -32,7 +32,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 e276eb4..7c5dfb1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -43,7 +43,6 @@
 #include <linux/fdtable.h>
 #include <linux/namei.h>
 #include <linux/mount.h>
-#include <linux/proc_fs.h>
 #include <linux/netfilter_ipv4.h>
 #include <linux/netfilter_ipv6.h>
 #include <linux/tty.h>
@@ -70,7 +69,6 @@
 #include <net/ipv6.h>
 #include <linux/hugetlb.h>
 #include <linux/personality.h>
-#include <linux/sysctl.h>
 #include <linux/audit.h>
 #include <linux/string.h>
 #include <linux/selinux.h>
@@ -1120,39 +1118,35 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
 }
 
 #ifdef CONFIG_PROC_FS
-static int selinux_proc_get_sid(struct proc_dir_entry *de,
+static int selinux_proc_get_sid(struct dentry *dentry,
 				u16 tclass,
 				u32 *sid)
 {
-	int buflen, rc;
-	char *buffer, *path, *end;
+	int rc;
+	char *buffer, *path;
 
 	buffer = (char *)__get_free_page(GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
-	buflen = PAGE_SIZE;
-	end = buffer+buflen;
-	*--end = '\0';
-	buflen--;
-	path = end-1;
-	*path = '/';
-	while (de && de != de->parent) {
-		buflen -= de->namelen + 1;
-		if (buflen < 0)
-			break;
-		end -= de->namelen;
-		memcpy(end, de->name, de->namelen);
-		*--end = '/';
-		path = end;
-		de = de->parent;
+	path = dentry_path_raw(dentry, buffer, PAGE_SIZE);
+	if (IS_ERR(path))
+		rc = PTR_ERR(path);
+	else {
+		/* each process gets a /proc/PID/ entry. Strip off the
+		 * PID part to get a valid selinux labeling.
+		 * e.g. /proc/1/net/rpc/nfs -> /net/rpc/nfs */
+		while (path[1] >= '0' && path[1] <= '9') {
+			path[1] = '/';
+			path++;
+		}
+		rc = security_genfs_sid("proc", path, tclass, sid);
 	}
-	rc = security_genfs_sid("proc", path, tclass, sid);
 	free_page((unsigned long)buffer);
 	return rc;
 }
 #else
-static int selinux_proc_get_sid(struct proc_dir_entry *de,
+static int selinux_proc_get_sid(struct dentry *dentry,
 				u16 tclass,
 				u32 *sid)
 {
@@ -1317,9 +1311,9 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 
 		if ((sbsec->flags & SE_SBPROC) && !S_ISLNK(inode->i_mode)) {
 			struct proc_inode *proci = PROC_I(inode);
-			if (proci->pde) {
+			if (opt_dentry && (proci->pde || proci->sysctl)) {
 				isec->sclass = inode_mode_to_security_class(inode->i_mode);
-				rc = selinux_proc_get_sid(proci->pde,
+				rc = selinux_proc_get_sid(opt_dentry,
 							  isec->sclass,
 							  &sid);
 				if (rc)
@@ -1862,82 +1856,6 @@ static int selinux_capable(struct task_struct *tsk, const struct cred *cred,
 	return task_has_capability(tsk, cred, cap, audit);
 }
 
-static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
-{
-	int buflen, rc;
-	char *buffer, *path, *end;
-
-	rc = -ENOMEM;
-	buffer = (char *)__get_free_page(GFP_KERNEL);
-	if (!buffer)
-		goto out;
-
-	buflen = PAGE_SIZE;
-	end = buffer+buflen;
-	*--end = '\0';
-	buflen--;
-	path = end-1;
-	*path = '/';
-	while (table) {
-		const char *name = table->procname;
-		size_t namelen = strlen(name);
-		buflen -= namelen + 1;
-		if (buflen < 0)
-			goto out_free;
-		end -= namelen;
-		memcpy(end, name, namelen);
-		*--end = '/';
-		path = end;
-		table = table->parent;
-	}
-	buflen -= 4;
-	if (buflen < 0)
-		goto out_free;
-	end -= 4;
-	memcpy(end, "/sys", 4);
-	path = end;
-	rc = security_genfs_sid("proc", path, tclass, sid);
-out_free:
-	free_page((unsigned long)buffer);
-out:
-	return rc;
-}
-
-static int selinux_sysctl(ctl_table *table, int op)
-{
-	int error = 0;
-	u32 av;
-	u32 tsid, sid;
-	int rc;
-
-	sid = current_sid();
-
-	rc = selinux_sysctl_get_sid(table, (op == 0001) ?
-				    SECCLASS_DIR : SECCLASS_FILE, &tsid);
-	if (rc) {
-		/* Default to the well-defined sysctl SID. */
-		tsid = SECINITSID_SYSCTL;
-	}
-
-	/* The op values are "defined" in sysctl.c, thereby creating
-	 * a bad coupling between this module and sysctl.c */
-	if (op == 001) {
-		error = avc_has_perm(sid, tsid,
-				     SECCLASS_DIR, DIR__SEARCH, NULL);
-	} else {
-		av = 0;
-		if (op & 004)
-			av |= FILE__READ;
-		if (op & 002)
-			av |= FILE__WRITE;
-		if (av)
-			error = avc_has_perm(sid, tsid,
-					     SECCLASS_FILE, av, NULL);
-	}
-
-	return error;
-}
-
 static int selinux_quotactl(int cmds, int type, int id, struct super_block *sb)
 {
 	const struct cred *cred = current_cred();
@@ -5398,7 +5316,6 @@ static struct security_operations selinux_ops = {
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
 	.capset =			selinux_capset,
-	.sysctl =			selinux_sysctl,
 	.capable =			selinux_capable,
 	.quotactl =			selinux_quotactl,
 	.quota_on =			selinux_quota_on,
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH] security: remove unused security_sysctl hook

[Lucian Adrian Grijincu]

The only user for this hook was selinux. sysctl routes every call
through /proc/sys/. Selinux and other security modules use the file
system checks for sysctl too, so no need for this hook any more.

Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
---
 include/linux/security.h |   13 -------------
 kernel/sysctl.c          |    5 -----
 security/capability.c    |    6 ------
 security/security.c      |    5 -----
 4 files changed, 0 insertions(+), 29 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index c642bb8..e7b48dc 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1257,12 +1257,6 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@cap contains the capability <include/linux/capability.h>.
  *	@audit: Whether to write an audit message or not
  *	Return 0 if the capability is granted for @tsk.
- * @sysctl:
- *	Check permission before accessing the @table sysctl variable in the
- *	manner specified by @op.
- *	@table contains the ctl_table structure for the sysctl variable.
- *	@op contains the operation (001 = search, 002 = write, 004 = read).
- *	Return 0 if permission is granted.
  * @syslog:
  *	Check permission before accessing the kernel message ring or changing
  *	logging to the console.
@@ -1383,7 +1377,6 @@ struct security_operations {
 		       const kernel_cap_t *permitted);
 	int (*capable) (struct task_struct *tsk, const struct cred *cred,
 			int cap, int audit);
-	int (*sysctl) (struct ctl_table *table, int op);
 	int (*quotactl) (int cmds, int type, int id, struct super_block *sb);
 	int (*quota_on) (struct dentry *dentry);
 	int (*syslog) (int type);
@@ -1665,7 +1658,6 @@ int security_capset(struct cred *new, const struct cred *old,
 int security_capable(int cap);
 int security_real_capable(struct task_struct *tsk, int cap);
 int security_real_capable_noaudit(struct task_struct *tsk, int cap);
-int security_sysctl(struct ctl_table *table, int op);
 int security_quotactl(int cmds, int type, int id, struct super_block *sb);
 int security_quota_on(struct dentry *dentry);
 int security_syslog(int type);
@@ -1883,11 +1875,6 @@ int security_real_capable_noaudit(struct task_struct *tsk, int cap)
 	return ret;
 }
 
-static inline int security_sysctl(struct ctl_table *table, int op)
-{
-	return 0;
-}
-
 static inline int security_quotactl(int cmds, int type, int id,
 				     struct super_block *sb)
 {
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 0f1bd83..56f6fc1 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1685,13 +1685,8 @@ static int test_perm(int mode, int op)
 
 int sysctl_perm(struct ctl_table_root *root, struct ctl_table *table, int op)
 {
-	int error;
 	int mode;
 
-	error = security_sysctl(table, op & (MAY_READ | MAY_WRITE | MAY_EXEC));
-	if (error)
-		return error;
-
 	if (root->permissions)
 		mode = root->permissions(root, current->nsproxy, table);
 	else
diff --git a/security/capability.c b/security/capability.c
index 2a5df2b..ebe3b5d 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -12,11 +12,6 @@
 
 #include <linux/security.h>
 
-static int cap_sysctl(ctl_table *table, int op)
-{
-	return 0;
-}
-
 static int cap_syslog(int type)
 {
 	return 0;
@@ -880,7 +875,6 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, capable);
 	set_to_cap_if_null(ops, quotactl);
 	set_to_cap_if_null(ops, quota_on);
-	set_to_cap_if_null(ops, sysctl);
 	set_to_cap_if_null(ops, syslog);
 	set_to_cap_if_null(ops, settime);
 	set_to_cap_if_null(ops, vm_enough_memory);
diff --git a/security/security.c b/security/security.c
index 739e403..53d793a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -182,11 +182,6 @@ int security_real_capable_noaudit(struct task_struct *tsk, int cap)
 	return ret;
 }
 
-int security_sysctl(struct ctl_table *table, int op)
-{
-	return security_ops->sysctl(table, op);
-}
-
 int security_quotactl(int cmds, int type, int id, struct super_block *sb)
 {
 	return security_ops->quotactl(cmds, type, id, sb);
-- 
1.7.4.rc1.7.g2cf08.dirty

Re: [PATCH] security/selinux: fix /proc/sys/ labeling

[Stephen Smalley]

On Tue, 2011-02-01 at 02:17 +0200, Lucian Adrian Grijincu wrote:
> From: Eric W. Biederman <ebiederm@xmission.com>

Is this patch really from Eric or just derived from an earlier patch by
him?

> 
> This fixes an old (2007) selinux regression: filesystem labeling for
> /proc/sys returned
>      -r--r--r-- unknown                          /proc/sys/fs/file-nr
> instead of
>      -r--r--r-- system_u:object_r:sysctl_fs_t:s0 /proc/sys/fs/file-nr
> 
> Events that lead to breaking of /proc/sys selinux labeling:
> 
> 1) sysctl was reimplemented to route all calls through /proc/sysctl
> 
>     commit 77b14db502cb85a031fe8fde6c85d52f3e0acb63
>     [PATCH] sysctl: reimplement the sysctl proc support
> 
> 2) proc_dir_entry was removed from ctl_table:
> 
>     commit 3fbfa98112fc3962c416452a0baf2214381030e6
>     [PATCH] sysctl: remove the proc_dir_entry member for the sysctl tables
> 
> 3) selinux still walked the proc_dir_entry tree to apply
>    labeling. Because ctl_tables don't have a proc_dir_entry, we did
>    not label /proc/sys/ inodes any more. To achieve this the /proc/sys
>    inodes were marked private and private inodes were ignored by
>    selinux.
> 
>     commit bbaca6c2e7ef0f663bc31be4dad7cf530f6c4962
>     [PATCH] selinux: enhance selinux to always ignore private inodes
> 
>     commit 86a71dbd3e81e8870d0f0e56b87875f57e58222b
>     [PATCH] sysctl: hide the sysctl proc inodes from selinux
> 
> Access control checks have been done by means of a special sysctl hook
> that was called for read/write accesses to any /proc/sys/ entry.
> 
> We don't have to do this because, instead of walking the
> proc_dir_entry tree we can walk the dentry tree (as done in this
> patch). With this patch:
> * we don't mark /proc/sys inodes as private
> * we don't need the sysclt security hook
> * we walk the dentry tree to find the path to the inode.
> 
> We have to strip the PID in /proc/PID/ entries that have a
> proc_dir_entry because selinux does not know how to label paths like
> '/1/net/rpc/nfsd.fh' (and defaults to 'proc_t' labeling). Selinux does
> know of '/net/rpc/nfsd.fh' (and applies the 'sysctl_rpc_t' label).
> 
> PID stripping from the path was done implicitly in the previous code
> because the proc_dir_entry tree had the root in '/net' in the example
> from above. The dentry tree has the root in '/1'.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

And did Eric truly sign off on this patch or just on an earlier one?

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e276eb4..7c5dfb1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1317,9 +1311,9 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>  
>  		if ((sbsec->flags & SE_SBPROC) && !S_ISLNK(inode->i_mode)) {
>  			struct proc_inode *proci = PROC_I(inode);
> -			if (proci->pde) {
> +			if (opt_dentry && (proci->pde || proci->sysctl)) {
>  				isec->sclass = inode_mode_to_security_class(inode->i_mode);
> -				rc = selinux_proc_get_sid(proci->pde,
> +				rc = selinux_proc_get_sid(opt_dentry,
>  							  isec->sclass,
>  							  &sid);
>  				if (rc)

It would be nice if we could eliminate the last remaining piece of proc
internal knowledge from this code - why do we need the proci->pde ||
proci->sysctl test here?  What changes without it?

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] security/selinux: fix /proc/sys/ labeling

[Lucian Adrian Grijincu]

On Tue, Feb 1, 2011 at 5:02 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> Is this patch really from Eric or just derived from an earlier patch by him?


No, sorry for the confusion.
I seem to have triggered a git send-email bug.

>> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>
> And did Eric truly sign off on this patch or just on an earlier one?


Just the earlier one. I added his sign-off because of this paragraph
in SubmittingPatches:
| The Signed-off-by: tag indicates that the signer was involved in the
| development of the patch, or that he/she was in the patch's delivery path.

>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index e276eb4..7c5dfb1 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -1317,9 +1311,9 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>>
>>               if ((sbsec->flags & SE_SBPROC) && !S_ISLNK(inode->i_mode)) {
>>                       struct proc_inode *proci = PROC_I(inode);
>> -                     if (proci->pde) {
>> +                     if (opt_dentry && (proci->pde || proci->sysctl)) {
>>                               isec->sclass = inode_mode_to_security_class(inode->i_mode);
>> -                             rc = selinux_proc_get_sid(proci->pde,
>> +                             rc = selinux_proc_get_sid(opt_dentry,
>>                                                         isec->sclass,
>>                                                         &sid);
>>                               if (rc)
>
> It would be nice if we could eliminate the last remaining piece of proc
> internal knowledge from this code - why do we need the proci->pde ||
> proci->sysctl test here?  What changes without it?


Without we label all nodes in /proc/ through selinux_proc_get_sid.

/proc/1/limits should not get it's sid from here, but from
security_task_to_inode -> selinux_task_to_inode.

Without the check we send "/1/limits" to selinux_proc_get_sid, which
strips off "/1" leaving "/limits". This will be labeled with "proc_t"
IIRC.


-- 
 .
..: Lucian

Re: [PATCH] security/selinux: fix /proc/sys/ labeling

[Stephen Smalley]

On Tue, 2011-02-01 at 17:53 +0200, Lucian Adrian Grijincu wrote:
> On Tue, Feb 1, 2011 at 5:02 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > Is this patch really from Eric or just derived from an earlier patch by him?
> 
> 
> No, sorry for the confusion.
> I seem to have triggered a git send-email bug.
> 
> >> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> >
> > And did Eric truly sign off on this patch or just on an earlier one?
> 
> 
> Just the earlier one. I added his sign-off because of this paragraph
> in SubmittingPatches:
> | The Signed-off-by: tag indicates that the signer was involved in the
> | development of the patch, or that he/she was in the patch's delivery path.
> 
> >
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index e276eb4..7c5dfb1 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -1317,9 +1311,9 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
> >>
> >>               if ((sbsec->flags & SE_SBPROC) && !S_ISLNK(inode->i_mode)) {
> >>                       struct proc_inode *proci = PROC_I(inode);
> >> -                     if (proci->pde) {
> >> +                     if (opt_dentry && (proci->pde || proci->sysctl)) {
> >>                               isec->sclass = inode_mode_to_security_class(inode->i_mode);
> >> -                             rc = selinux_proc_get_sid(proci->pde,
> >> +                             rc = selinux_proc_get_sid(opt_dentry,
> >>                                                         isec->sclass,
> >>                                                         &sid);
> >>                               if (rc)
> >
> > It would be nice if we could eliminate the last remaining piece of proc
> > internal knowledge from this code - why do we need the proci->pde ||
> > proci->sysctl test here?  What changes without it?
> 
> 
> Without we label all nodes in /proc/ through selinux_proc_get_sid.
> 
> /proc/1/limits should not get it's sid from here, but from
> security_task_to_inode -> selinux_task_to_inode.
> 
> Without the check we send "/1/limits" to selinux_proc_get_sid, which
> strips off "/1" leaving "/limits". This will be labeled with "proc_t"
> IIRC.

Are you sure?  Those inodes should be labeled by proc_pid_make_inode()
-> security_task_to_inode() -> selinux_task_to_inode(), which will set
the inode SID to match the associated task SID, and set the
isec->initialized flag.  Then when inode_doinit_with_dentry gets called
later, it should bail immediately due to isec->initialized already being
set.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH] security/selinux: fix /proc/sys/ labeling

[Lucian Adrian Grijincu]

On Tue, Feb 1, 2011 at 5:59 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> Just the earlier one. I added his sign-off because of this paragraph
>> in SubmittingPatches:
>> | The Signed-off-by: tag indicates that the signer was involved in the
>> | development of the patch, or that he/she was in the patch's delivery path.


So should I leave Eric's sign-off here?


>> Without we label all nodes in /proc/ through selinux_proc_get_sid.
>>
>> /proc/1/limits should not get it's sid from here, but from
>> security_task_to_inode -> selinux_task_to_inode.
>>
>> Without the check we send "/1/limits" to selinux_proc_get_sid, which
>> strips off "/1" leaving "/limits". This will be labeled with "proc_t"
>> IIRC.
>
> Are you sure?  Those inodes should be labeled by proc_pid_make_inode()
> -> security_task_to_inode() -> selinux_task_to_inode(), which will set
> the inode SID to match the associated task SID, and set the
> isec->initialized flag.  Then when inode_doinit_with_dentry gets called
> later, it should bail immediately due to isec->initialized already being
> set.



I'll post an updated patch without those checks. I tested and 'find
/proc | xargs ls -Z' said the same thing with and without those
checks.

I remember doing the same test yesterday and saw some differences, but
I must have compared the wrong files.

-- 
 .
..: Lucian

Re: [PATCH] security/selinux: fix /proc/sys/ labeling

[Stephen Smalley]

On Tue, 2011-02-01 at 18:32 +0200, Lucian Adrian Grijincu wrote:
> On Tue, Feb 1, 2011 at 5:59 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> Just the earlier one. I added his sign-off because of this paragraph
> >> in SubmittingPatches:
> >> | The Signed-off-by: tag indicates that the signer was involved in the
> >> | development of the patch, or that he/she was in the patch's delivery path.
> 
> 
> So should I leave Eric's sign-off here?

I guess so, given that paragraph.

> >> Without we label all nodes in /proc/ through selinux_proc_get_sid.
> >>
> >> /proc/1/limits should not get it's sid from here, but from
> >> security_task_to_inode -> selinux_task_to_inode.
> >>
> >> Without the check we send "/1/limits" to selinux_proc_get_sid, which
> >> strips off "/1" leaving "/limits". This will be labeled with "proc_t"
> >> IIRC.
> >
> > Are you sure?  Those inodes should be labeled by proc_pid_make_inode()
> > -> security_task_to_inode() -> selinux_task_to_inode(), which will set
> > the inode SID to match the associated task SID, and set the
> > isec->initialized flag.  Then when inode_doinit_with_dentry gets called
> > later, it should bail immediately due to isec->initialized already being
> > set.
> 
> 
> 
> I'll post an updated patch without those checks. I tested and 'find
> /proc | xargs ls -Z' said the same thing with and without those
> checks.
> 
> I remember doing the same test yesterday and saw some differences, but
> I must have compared the wrong files.

Ok, good.  That gets rid of the last vestige of proc implementation
details in selinux.

-- 
Stephen Smalley
National Security Agency

[PATCH 1/2] security/selinux: fix /proc/sys/ labeling

[Lucian Adrian Grijincu]

This fixes an old (2007) selinux regression: filesystem labeling for
/proc/sys returned
     -r--r--r-- unknown                          /proc/sys/fs/file-nr
instead of
     -r--r--r-- system_u:object_r:sysctl_fs_t:s0 /proc/sys/fs/file-nr

Events that lead to breaking of /proc/sys/ selinux labeling:

1) sysctl was reimplemented to route all calls through /proc/sys/

    commit 77b14db502cb85a031fe8fde6c85d52f3e0acb63
    [PATCH] sysctl: reimplement the sysctl proc support

2) proc_dir_entry was removed from ctl_table:

    commit 3fbfa98112fc3962c416452a0baf2214381030e6
    [PATCH] sysctl: remove the proc_dir_entry member for the sysctl tables

3) selinux still walked the proc_dir_entry tree to apply
   labeling. Because ctl_tables don't have a proc_dir_entry, we did
   not label /proc/sys/ inodes any more. To achieve this the /proc/sys/
   inodes were marked private and private inodes were ignored by
   selinux.

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

    commit 86a71dbd3e81e8870d0f0e56b87875f57e58222b
    [PATCH] sysctl: hide the sysctl proc inodes from selinux

Access control checks have been done by means of a special sysctl hook
that was called for read/write accesses to any /proc/sys/ entry.

We don't have to do this because, instead of walking the
proc_dir_entry tree we can walk the dentry tree (as done in this
patch). With this patch:
* we don't mark /proc/sys/ inodes as private
* we don't need the sysclt security hook
* we walk the dentry tree to find the path to the inode.

We have to strip the PID in /proc/PID/ entries that have a
proc_dir_entry because selinux does not know how to label paths like
'/1/net/rpc/nfsd.fh' (and defaults to 'proc_t' labeling). Selinux does
know of '/net/rpc/nfsd.fh' (and applies the 'sysctl_rpc_t' label).

PID stripping from the path was done implicitly in the previous code
because the proc_dir_entry tree had the root in '/net' in the example
from above. The dentry tree has the root in '/1'.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
---
 fs/proc/proc_sysctl.c    |    1 -
 security/selinux/hooks.c |  120 +++++++---------------------------------------
 2 files changed, 18 insertions(+), 103 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 09a1f92..fb707e0 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -32,7 +32,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 e276eb4..5231b95 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -43,7 +43,6 @@
 #include <linux/fdtable.h>
 #include <linux/namei.h>
 #include <linux/mount.h>
-#include <linux/proc_fs.h>
 #include <linux/netfilter_ipv4.h>
 #include <linux/netfilter_ipv6.h>
 #include <linux/tty.h>
@@ -70,7 +69,6 @@
 #include <net/ipv6.h>
 #include <linux/hugetlb.h>
 #include <linux/personality.h>
-#include <linux/sysctl.h>
 #include <linux/audit.h>
 #include <linux/string.h>
 #include <linux/selinux.h>
@@ -1120,39 +1118,35 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
 }
 
 #ifdef CONFIG_PROC_FS
-static int selinux_proc_get_sid(struct proc_dir_entry *de,
+static int selinux_proc_get_sid(struct dentry *dentry,
 				u16 tclass,
 				u32 *sid)
 {
-	int buflen, rc;
-	char *buffer, *path, *end;
+	int rc;
+	char *buffer, *path;
 
 	buffer = (char *)__get_free_page(GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
-	buflen = PAGE_SIZE;
-	end = buffer+buflen;
-	*--end = '\0';
-	buflen--;
-	path = end-1;
-	*path = '/';
-	while (de && de != de->parent) {
-		buflen -= de->namelen + 1;
-		if (buflen < 0)
-			break;
-		end -= de->namelen;
-		memcpy(end, de->name, de->namelen);
-		*--end = '/';
-		path = end;
-		de = de->parent;
+	path = dentry_path_raw(dentry, buffer, PAGE_SIZE);
+	if (IS_ERR(path))
+		rc = PTR_ERR(path);
+	else {
+		/* each process gets a /proc/PID/ entry. Strip off the
+		 * PID part to get a valid selinux labeling.
+		 * e.g. /proc/1/net/rpc/nfs -> /net/rpc/nfs */
+		while (path[1] >= '0' && path[1] <= '9') {
+			path[1] = '/';
+			path++;
+		}
+		rc = security_genfs_sid("proc", path, tclass, sid);
 	}
-	rc = security_genfs_sid("proc", path, tclass, sid);
 	free_page((unsigned long)buffer);
 	return rc;
 }
 #else
-static int selinux_proc_get_sid(struct proc_dir_entry *de,
+static int selinux_proc_get_sid(struct dentry *dentry,
 				u16 tclass,
 				u32 *sid)
 {
@@ -1316,10 +1310,9 @@ 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)) {
-			struct proc_inode *proci = PROC_I(inode);
-			if (proci->pde) {
+			if (opt_dentry) {
 				isec->sclass = inode_mode_to_security_class(inode->i_mode);
-				rc = selinux_proc_get_sid(proci->pde,
+				rc = selinux_proc_get_sid(opt_dentry,
 							  isec->sclass,
 							  &sid);
 				if (rc)
@@ -1862,82 +1855,6 @@ static int selinux_capable(struct task_struct *tsk, const struct cred *cred,
 	return task_has_capability(tsk, cred, cap, audit);
 }
 
-static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
-{
-	int buflen, rc;
-	char *buffer, *path, *end;
-
-	rc = -ENOMEM;
-	buffer = (char *)__get_free_page(GFP_KERNEL);
-	if (!buffer)
-		goto out;
-
-	buflen = PAGE_SIZE;
-	end = buffer+buflen;
-	*--end = '\0';
-	buflen--;
-	path = end-1;
-	*path = '/';
-	while (table) {
-		const char *name = table->procname;
-		size_t namelen = strlen(name);
-		buflen -= namelen + 1;
-		if (buflen < 0)
-			goto out_free;
-		end -= namelen;
-		memcpy(end, name, namelen);
-		*--end = '/';
-		path = end;
-		table = table->parent;
-	}
-	buflen -= 4;
-	if (buflen < 0)
-		goto out_free;
-	end -= 4;
-	memcpy(end, "/sys", 4);
-	path = end;
-	rc = security_genfs_sid("proc", path, tclass, sid);
-out_free:
-	free_page((unsigned long)buffer);
-out:
-	return rc;
-}
-
-static int selinux_sysctl(ctl_table *table, int op)
-{
-	int error = 0;
-	u32 av;
-	u32 tsid, sid;
-	int rc;
-
-	sid = current_sid();
-
-	rc = selinux_sysctl_get_sid(table, (op == 0001) ?
-				    SECCLASS_DIR : SECCLASS_FILE, &tsid);
-	if (rc) {
-		/* Default to the well-defined sysctl SID. */
-		tsid = SECINITSID_SYSCTL;
-	}
-
-	/* The op values are "defined" in sysctl.c, thereby creating
-	 * a bad coupling between this module and sysctl.c */
-	if (op == 001) {
-		error = avc_has_perm(sid, tsid,
-				     SECCLASS_DIR, DIR__SEARCH, NULL);
-	} else {
-		av = 0;
-		if (op & 004)
-			av |= FILE__READ;
-		if (op & 002)
-			av |= FILE__WRITE;
-		if (av)
-			error = avc_has_perm(sid, tsid,
-					     SECCLASS_FILE, av, NULL);
-	}
-
-	return error;
-}
-
 static int selinux_quotactl(int cmds, int type, int id, struct super_block *sb)
 {
 	const struct cred *cred = current_cred();
@@ -5398,7 +5315,6 @@ static struct security_operations selinux_ops = {
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
 	.capset =			selinux_capset,
-	.sysctl =			selinux_sysctl,
 	.capable =			selinux_capable,
 	.quotactl =			selinux_quotactl,
 	.quota_on =			selinux_quota_on,
-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 2/2] security: remove unused security_sysctl hook

[Lucian Adrian Grijincu]

The only user for this hook was selinux. sysctl routes every call
through /proc/sys/. Selinux and other security modules use the file
system checks for sysctl too, so no need for this hook any more.

Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
---
 include/linux/security.h |   13 -------------
 kernel/sysctl.c          |    5 -----
 security/capability.c    |    6 ------
 security/security.c      |    5 -----
 4 files changed, 0 insertions(+), 29 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index c642bb8..e7b48dc 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1257,12 +1257,6 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@cap contains the capability <include/linux/capability.h>.
  *	@audit: Whether to write an audit message or not
  *	Return 0 if the capability is granted for @tsk.
- * @sysctl:
- *	Check permission before accessing the @table sysctl variable in the
- *	manner specified by @op.
- *	@table contains the ctl_table structure for the sysctl variable.
- *	@op contains the operation (001 = search, 002 = write, 004 = read).
- *	Return 0 if permission is granted.
  * @syslog:
  *	Check permission before accessing the kernel message ring or changing
  *	logging to the console.
@@ -1383,7 +1377,6 @@ struct security_operations {
 		       const kernel_cap_t *permitted);
 	int (*capable) (struct task_struct *tsk, const struct cred *cred,
 			int cap, int audit);
-	int (*sysctl) (struct ctl_table *table, int op);
 	int (*quotactl) (int cmds, int type, int id, struct super_block *sb);
 	int (*quota_on) (struct dentry *dentry);
 	int (*syslog) (int type);
@@ -1665,7 +1658,6 @@ int security_capset(struct cred *new, const struct cred *old,
 int security_capable(int cap);
 int security_real_capable(struct task_struct *tsk, int cap);
 int security_real_capable_noaudit(struct task_struct *tsk, int cap);
-int security_sysctl(struct ctl_table *table, int op);
 int security_quotactl(int cmds, int type, int id, struct super_block *sb);
 int security_quota_on(struct dentry *dentry);
 int security_syslog(int type);
@@ -1883,11 +1875,6 @@ int security_real_capable_noaudit(struct task_struct *tsk, int cap)
 	return ret;
 }
 
-static inline int security_sysctl(struct ctl_table *table, int op)
-{
-	return 0;
-}
-
 static inline int security_quotactl(int cmds, int type, int id,
 				     struct super_block *sb)
 {
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 0f1bd83..56f6fc1 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1685,13 +1685,8 @@ static int test_perm(int mode, int op)
 
 int sysctl_perm(struct ctl_table_root *root, struct ctl_table *table, int op)
 {
-	int error;
 	int mode;
 
-	error = security_sysctl(table, op & (MAY_READ | MAY_WRITE | MAY_EXEC));
-	if (error)
-		return error;
-
 	if (root->permissions)
 		mode = root->permissions(root, current->nsproxy, table);
 	else
diff --git a/security/capability.c b/security/capability.c
index 2a5df2b..ebe3b5d 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -12,11 +12,6 @@
 
 #include <linux/security.h>
 
-static int cap_sysctl(ctl_table *table, int op)
-{
-	return 0;
-}
-
 static int cap_syslog(int type)
 {
 	return 0;
@@ -880,7 +875,6 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, capable);
 	set_to_cap_if_null(ops, quotactl);
 	set_to_cap_if_null(ops, quota_on);
-	set_to_cap_if_null(ops, sysctl);
 	set_to_cap_if_null(ops, syslog);
 	set_to_cap_if_null(ops, settime);
 	set_to_cap_if_null(ops, vm_enough_memory);
diff --git a/security/security.c b/security/security.c
index 739e403..53d793a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -182,11 +182,6 @@ int security_real_capable_noaudit(struct task_struct *tsk, int cap)
 	return ret;
 }
 
-int security_sysctl(struct ctl_table *table, int op)
-{
-	return security_ops->sysctl(table, op);
-}
-
 int security_quotactl(int cmds, int type, int id, struct super_block *sb)
 {
 	return security_ops->quotactl(cmds, type, id, sb);
-- 
1.7.4.rc1.7.g2cf08.dirty

Re: [PATCH 2/2] security: remove unused security_sysctl hook

[Stephen Smalley]

On Tue, 2011-02-01 at 18:44 +0200, Lucian Adrian Grijincu wrote:
> The only user for this hook was selinux. sysctl routes every call
> through /proc/sys/. Selinux and other security modules use the file
> system checks for sysctl too, so no need for this hook any more.
> 
> Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>

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

> ---
>  include/linux/security.h |   13 -------------
>  kernel/sysctl.c          |    5 -----
>  security/capability.c    |    6 ------
>  security/security.c      |    5 -----
>  4 files changed, 0 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c642bb8..e7b48dc 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1257,12 +1257,6 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	@cap contains the capability <include/linux/capability.h>.
>   *	@audit: Whether to write an audit message or not
>   *	Return 0 if the capability is granted for @tsk.
> - * @sysctl:
> - *	Check permission before accessing the @table sysctl variable in the
> - *	manner specified by @op.
> - *	@table contains the ctl_table structure for the sysctl variable.
> - *	@op contains the operation (001 = search, 002 = write, 004 = read).
> - *	Return 0 if permission is granted.
>   * @syslog:
>   *	Check permission before accessing the kernel message ring or changing
>   *	logging to the console.
> @@ -1383,7 +1377,6 @@ struct security_operations {
>  		       const kernel_cap_t *permitted);
>  	int (*capable) (struct task_struct *tsk, const struct cred *cred,
>  			int cap, int audit);
> -	int (*sysctl) (struct ctl_table *table, int op);
>  	int (*quotactl) (int cmds, int type, int id, struct super_block *sb);
>  	int (*quota_on) (struct dentry *dentry);
>  	int (*syslog) (int type);
> @@ -1665,7 +1658,6 @@ int security_capset(struct cred *new, const struct cred *old,
>  int security_capable(int cap);
>  int security_real_capable(struct task_struct *tsk, int cap);
>  int security_real_capable_noaudit(struct task_struct *tsk, int cap);
> -int security_sysctl(struct ctl_table *table, int op);
>  int security_quotactl(int cmds, int type, int id, struct super_block *sb);
>  int security_quota_on(struct dentry *dentry);
>  int security_syslog(int type);
> @@ -1883,11 +1875,6 @@ int security_real_capable_noaudit(struct task_struct *tsk, int cap)
>  	return ret;
>  }
>  
> -static inline int security_sysctl(struct ctl_table *table, int op)
> -{
> -	return 0;
> -}
> -
>  static inline int security_quotactl(int cmds, int type, int id,
>  				     struct super_block *sb)
>  {
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 0f1bd83..56f6fc1 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1685,13 +1685,8 @@ static int test_perm(int mode, int op)
>  
>  int sysctl_perm(struct ctl_table_root *root, struct ctl_table *table, int op)
>  {
> -	int error;
>  	int mode;
>  
> -	error = security_sysctl(table, op & (MAY_READ | MAY_WRITE | MAY_EXEC));
> -	if (error)
> -		return error;
> -
>  	if (root->permissions)
>  		mode = root->permissions(root, current->nsproxy, table);
>  	else
> diff --git a/security/capability.c b/security/capability.c
> index 2a5df2b..ebe3b5d 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -12,11 +12,6 @@
>  
>  #include <linux/security.h>
>  
> -static int cap_sysctl(ctl_table *table, int op)
> -{
> -	return 0;
> -}
> -
>  static int cap_syslog(int type)
>  {
>  	return 0;
> @@ -880,7 +875,6 @@ void __init security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, capable);
>  	set_to_cap_if_null(ops, quotactl);
>  	set_to_cap_if_null(ops, quota_on);
> -	set_to_cap_if_null(ops, sysctl);
>  	set_to_cap_if_null(ops, syslog);
>  	set_to_cap_if_null(ops, settime);
>  	set_to_cap_if_null(ops, vm_enough_memory);
> diff --git a/security/security.c b/security/security.c
> index 739e403..53d793a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -182,11 +182,6 @@ int security_real_capable_noaudit(struct task_struct *tsk, int cap)
>  	return ret;
>  }
>  
> -int security_sysctl(struct ctl_table *table, int op)
> -{
> -	return security_ops->sysctl(table, op);
> -}
> -
>  int security_quotactl(int cmds, int type, int id, struct super_block *sb)
>  {
>  	return security_ops->quotactl(cmds, type, id, sb);
> -- 
> 1.7.4.rc1.7.g2cf08.dirty

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] security: remove unused security_sysctl hook

[Eric Paris]

On Feb 1, 2011, at 2:05 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:

> On Tue, 2011-02-01 at 18:44 +0200, Lucian Adrian Grijincu wrote:
>> The only user for this hook was selinux. sysctl routes every call
>> through /proc/sys/. Selinux and other security modules use the file
>> system checks for sysctl too, so no need for this hook any more.
>> 
>> Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
> 
> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

I've applied both to the selinux tree. Thanks

-Eric 
> 
>> ---
>> include/linux/security.h |   13 -------------
>> kernel/sysctl.c          |    5 -----
>> security/capability.c    |    6 ------
>> security/security.c      |    5 -----
>> 4 files changed, 0 insertions(+), 29 deletions(-)
>> 
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index c642bb8..e7b48dc 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -1257,12 +1257,6 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>>  *    @cap contains the capability <include/linux/capability.h>.
>>  *    @audit: Whether to write an audit message or not
>>  *    Return 0 if the capability is granted for @tsk.
>> - * @sysctl:
>> - *    Check permission before accessing the @table sysctl variable in the
>> - *    manner specified by @op.
>> - *    @table contains the ctl_table structure for the sysctl variable.
>> - *    @op contains the operation (001 = search, 002 = write, 004 = read).
>> - *    Return 0 if permission is granted.
>>  * @syslog:
>>  *    Check permission before accessing the kernel message ring or changing
>>  *    logging to the console.
>> @@ -1383,7 +1377,6 @@ struct security_operations {
>>               const kernel_cap_t *permitted);
>>    int (*capable) (struct task_struct *tsk, const struct cred *cred,
>>            int cap, int audit);
>> -    int (*sysctl) (struct ctl_table *table, int op);
>>    int (*quotactl) (int cmds, int type, int id, struct super_block *sb);
>>    int (*quota_on) (struct dentry *dentry);
>>    int (*syslog) (int type);
>> @@ -1665,7 +1658,6 @@ int security_capset(struct cred *new, const struct cred *old,
>> int security_capable(int cap);
>> int security_real_capable(struct task_struct *tsk, int cap);
>> int security_real_capable_noaudit(struct task_struct *tsk, int cap);
>> -int security_sysctl(struct ctl_table *table, int op);
>> int security_quotactl(int cmds, int type, int id, struct super_block *sb);
>> int security_quota_on(struct dentry *dentry);
>> int security_syslog(int type);
>> @@ -1883,11 +1875,6 @@ int security_real_capable_noaudit(struct task_struct *tsk, int cap)
>>    return ret;
>> }
>> 
>> -static inline int security_sysctl(struct ctl_table *table, int op)
>> -{
>> -    return 0;
>> -}
>> -
>> static inline int security_quotactl(int cmds, int type, int id,
>>                     struct super_block *sb)
>> {
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 0f1bd83..56f6fc1 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -1685,13 +1685,8 @@ static int test_perm(int mode, int op)
>> 
>> int sysctl_perm(struct ctl_table_root *root, struct ctl_table *table, int op)
>> {
>> -    int error;
>>    int mode;
>> 
>> -    error = security_sysctl(table, op & (MAY_READ | MAY_WRITE | MAY_EXEC));
>> -    if (error)
>> -        return error;
>> -
>>    if (root->permissions)
>>        mode = root->permissions(root, current->nsproxy, table);
>>    else
>> diff --git a/security/capability.c b/security/capability.c
>> index 2a5df2b..ebe3b5d 100644
>> --- a/security/capability.c
>> +++ b/security/capability.c
>> @@ -12,11 +12,6 @@
>> 
>> #include <linux/security.h>
>> 
>> -static int cap_sysctl(ctl_table *table, int op)
>> -{
>> -    return 0;
>> -}
>> -
>> static int cap_syslog(int type)
>> {
>>    return 0;
>> @@ -880,7 +875,6 @@ void __init security_fixup_ops(struct security_operations *ops)
>>    set_to_cap_if_null(ops, capable);
>>    set_to_cap_if_null(ops, quotactl);
>>    set_to_cap_if_null(ops, quota_on);
>> -    set_to_cap_if_null(ops, sysctl);
>>    set_to_cap_if_null(ops, syslog);
>>    set_to_cap_if_null(ops, settime);
>>    set_to_cap_if_null(ops, vm_enough_memory);
>> diff --git a/security/security.c b/security/security.c
>> index 739e403..53d793a 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -182,11 +182,6 @@ int security_real_capable_noaudit(struct task_struct *tsk, int cap)
>>    return ret;
>> }
>> 
>> -int security_sysctl(struct ctl_table *table, int op)
>> -{
>> -    return security_ops->sysctl(table, op);
>> -}
>> -
>> int security_quotactl(int cmds, int type, int id, struct super_block *sb)
>> {
>>    return security_ops->quotactl(cmds, type, id, sb);
>> -- 
>> 1.7.4.rc1.7.g2cf08.dirty
> 
> -- 
> Stephen Smalley
> National Security Agency
> 

Re: [PATCH 2/2] security: remove unused security_sysctl hook

[Lucian Adrian Grijincu]

On Tue, Feb 1, 2011 at 10:06 PM, Eric Paris <eparis@redhat.com> wrote:
>
>
>
>
> On Feb 1, 2011, at 2:05 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
>> On Tue, 2011-02-01 at 18:44 +0200, Lucian Adrian Grijincu wrote:
>>> The only user for this hook was selinux. sysctl routes every call
>>> through /proc/sys/. Selinux and other security modules use the file
>>> system checks for sysctl too, so no need for this hook any more.
>>>
>>> Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
>>
>> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>
>
> I've applied both to the selinux tree. Thanks


I've checked both these trees (on all published branches):
  git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6.git
  git://git.infradead.org/users/eparis/selinux.git

and there's no trace of these patches.


Am I not looking in the right places for these patches?

-- 
 .
..: Lucian

Re: [PATCH 2/2] security: remove unused security_sysctl hook

[Eric Paris]

On Mon, 2011-02-14 at 21:33 +0200, Lucian Adrian Grijincu wrote:
> On Tue, Feb 1, 2011 at 10:06 PM, Eric Paris <eparis@redhat.com> wrote:
> >
> >
> >
> >
> > On Feb 1, 2011, at 2:05 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >
> >> On Tue, 2011-02-01 at 18:44 +0200, Lucian Adrian Grijincu wrote:
> >>> The only user for this hook was selinux. sysctl routes every call
> >>> through /proc/sys/. Selinux and other security modules use the file
> >>> system checks for sysctl too, so no need for this hook any more.
> >>>
> >>> Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
> >>
> >> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>
> >
> > I've applied both to the selinux tree. Thanks
> 
> 
> I've checked both these trees (on all published branches):
>   git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6.git
>   git://git.infradead.org/users/eparis/selinux.git
> 
> and there's no trace of these patches.
> 
> 
> Am I not looking in the right places for these patches?

The second one is the right tree, I just forgot to push!  Pushed now!
Sorry.

-Eric

Re: [PATCH 2/2] security: remove unused security_sysctl hook

[Lucian Adrian Grijincu]

On Mon, Feb 14, 2011 at 9:53 PM, Eric Paris <eparis@redhat.com> wrote:
>> >> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>


FWIW you forgot Stephen's Acked-by.


>> I've checked both these trees (on all published branches):
>>   git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6.git
>>   git://git.infradead.org/users/eparis/selinux.git
>>
>> and there's no trace of these patches.
>>
>>
>> Am I not looking in the right places for these patches?
>
> The second one is the right tree, I just forgot to push!  Pushed now!


MAINTAINERS says:
SELINUX SECURITY MODULE
T:      git git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6.git

Should It be changed to point to your tree? Or should a new entry be
added to point to your tree?

-- 
 .
..: Lucian

Re: [PATCH 2/2] security: remove unused security_sysctl hook

[James Morris]

On Mon, 14 Feb 2011, Lucian Adrian Grijincu wrote:

> 
> MAINTAINERS says:
> SELINUX SECURITY MODULE
> T:      git git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6.git
> 
> Should It be changed to point to your tree? Or should a new entry be
> added to point to your tree?

That should probably be changed to Eric's tree.

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/2] security/selinux: fix /proc/sys/ labeling

[Stephen Smalley]

On Tue, 2011-02-01 at 18:42 +0200, Lucian Adrian Grijincu wrote:
> This fixes an old (2007) selinux regression: filesystem labeling for
> /proc/sys returned
>      -r--r--r-- unknown                          /proc/sys/fs/file-nr
> instead of
>      -r--r--r-- system_u:object_r:sysctl_fs_t:s0 /proc/sys/fs/file-nr
> 
> Events that lead to breaking of /proc/sys/ selinux labeling:
> 
> 1) sysctl was reimplemented to route all calls through /proc/sys/
> 
>     commit 77b14db502cb85a031fe8fde6c85d52f3e0acb63
>     [PATCH] sysctl: reimplement the sysctl proc support
> 
> 2) proc_dir_entry was removed from ctl_table:
> 
>     commit 3fbfa98112fc3962c416452a0baf2214381030e6
>     [PATCH] sysctl: remove the proc_dir_entry member for the sysctl tables
> 
> 3) selinux still walked the proc_dir_entry tree to apply
>    labeling. Because ctl_tables don't have a proc_dir_entry, we did
>    not label /proc/sys/ inodes any more. To achieve this the /proc/sys/
>    inodes were marked private and private inodes were ignored by
>    selinux.
> 
>     commit bbaca6c2e7ef0f663bc31be4dad7cf530f6c4962
>     [PATCH] selinux: enhance selinux to always ignore private inodes
> 
>     commit 86a71dbd3e81e8870d0f0e56b87875f57e58222b
>     [PATCH] sysctl: hide the sysctl proc inodes from selinux
> 
> Access control checks have been done by means of a special sysctl hook
> that was called for read/write accesses to any /proc/sys/ entry.
> 
> We don't have to do this because, instead of walking the
> proc_dir_entry tree we can walk the dentry tree (as done in this
> patch). With this patch:
> * we don't mark /proc/sys/ inodes as private
> * we don't need the sysclt security hook
> * we walk the dentry tree to find the path to the inode.
> 
> We have to strip the PID in /proc/PID/ entries that have a
> proc_dir_entry because selinux does not know how to label paths like
> '/1/net/rpc/nfsd.fh' (and defaults to 'proc_t' labeling). Selinux does
> know of '/net/rpc/nfsd.fh' (and applies the 'sysctl_rpc_t' label).
> 
> PID stripping from the path was done implicitly in the previous code
> because the proc_dir_entry tree had the root in '/net' in the example
> from above. The dentry tree has the root in '/1'.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>

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

> ---
>  fs/proc/proc_sysctl.c    |    1 -
>  security/selinux/hooks.c |  120 +++++++---------------------------------------
>  2 files changed, 18 insertions(+), 103 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 09a1f92..fb707e0 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -32,7 +32,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 e276eb4..5231b95 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -43,7 +43,6 @@
>  #include <linux/fdtable.h>
>  #include <linux/namei.h>
>  #include <linux/mount.h>
> -#include <linux/proc_fs.h>
>  #include <linux/netfilter_ipv4.h>
>  #include <linux/netfilter_ipv6.h>
>  #include <linux/tty.h>
> @@ -70,7 +69,6 @@
>  #include <net/ipv6.h>
>  #include <linux/hugetlb.h>
>  #include <linux/personality.h>
> -#include <linux/sysctl.h>
>  #include <linux/audit.h>
>  #include <linux/string.h>
>  #include <linux/selinux.h>
> @@ -1120,39 +1118,35 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
>  }
>  
>  #ifdef CONFIG_PROC_FS
> -static int selinux_proc_get_sid(struct proc_dir_entry *de,
> +static int selinux_proc_get_sid(struct dentry *dentry,
>  				u16 tclass,
>  				u32 *sid)
>  {
> -	int buflen, rc;
> -	char *buffer, *path, *end;
> +	int rc;
> +	char *buffer, *path;
>  
>  	buffer = (char *)__get_free_page(GFP_KERNEL);
>  	if (!buffer)
>  		return -ENOMEM;
>  
> -	buflen = PAGE_SIZE;
> -	end = buffer+buflen;
> -	*--end = '\0';
> -	buflen--;
> -	path = end-1;
> -	*path = '/';
> -	while (de && de != de->parent) {
> -		buflen -= de->namelen + 1;
> -		if (buflen < 0)
> -			break;
> -		end -= de->namelen;
> -		memcpy(end, de->name, de->namelen);
> -		*--end = '/';
> -		path = end;
> -		de = de->parent;
> +	path = dentry_path_raw(dentry, buffer, PAGE_SIZE);
> +	if (IS_ERR(path))
> +		rc = PTR_ERR(path);
> +	else {
> +		/* each process gets a /proc/PID/ entry. Strip off the
> +		 * PID part to get a valid selinux labeling.
> +		 * e.g. /proc/1/net/rpc/nfs -> /net/rpc/nfs */
> +		while (path[1] >= '0' && path[1] <= '9') {
> +			path[1] = '/';
> +			path++;
> +		}
> +		rc = security_genfs_sid("proc", path, tclass, sid);
>  	}
> -	rc = security_genfs_sid("proc", path, tclass, sid);
>  	free_page((unsigned long)buffer);
>  	return rc;
>  }
>  #else
> -static int selinux_proc_get_sid(struct proc_dir_entry *de,
> +static int selinux_proc_get_sid(struct dentry *dentry,
>  				u16 tclass,
>  				u32 *sid)
>  {
> @@ -1316,10 +1310,9 @@ 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)) {
> -			struct proc_inode *proci = PROC_I(inode);
> -			if (proci->pde) {
> +			if (opt_dentry) {
>  				isec->sclass = inode_mode_to_security_class(inode->i_mode);
> -				rc = selinux_proc_get_sid(proci->pde,
> +				rc = selinux_proc_get_sid(opt_dentry,
>  							  isec->sclass,
>  							  &sid);
>  				if (rc)
> @@ -1862,82 +1855,6 @@ static int selinux_capable(struct task_struct *tsk, const struct cred *cred,
>  	return task_has_capability(tsk, cred, cap, audit);
>  }
>  
> -static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
> -{
> -	int buflen, rc;
> -	char *buffer, *path, *end;
> -
> -	rc = -ENOMEM;
> -	buffer = (char *)__get_free_page(GFP_KERNEL);
> -	if (!buffer)
> -		goto out;
> -
> -	buflen = PAGE_SIZE;
> -	end = buffer+buflen;
> -	*--end = '\0';
> -	buflen--;
> -	path = end-1;
> -	*path = '/';
> -	while (table) {
> -		const char *name = table->procname;
> -		size_t namelen = strlen(name);
> -		buflen -= namelen + 1;
> -		if (buflen < 0)
> -			goto out_free;
> -		end -= namelen;
> -		memcpy(end, name, namelen);
> -		*--end = '/';
> -		path = end;
> -		table = table->parent;
> -	}
> -	buflen -= 4;
> -	if (buflen < 0)
> -		goto out_free;
> -	end -= 4;
> -	memcpy(end, "/sys", 4);
> -	path = end;
> -	rc = security_genfs_sid("proc", path, tclass, sid);
> -out_free:
> -	free_page((unsigned long)buffer);
> -out:
> -	return rc;
> -}
> -
> -static int selinux_sysctl(ctl_table *table, int op)
> -{
> -	int error = 0;
> -	u32 av;
> -	u32 tsid, sid;
> -	int rc;
> -
> -	sid = current_sid();
> -
> -	rc = selinux_sysctl_get_sid(table, (op == 0001) ?
> -				    SECCLASS_DIR : SECCLASS_FILE, &tsid);
> -	if (rc) {
> -		/* Default to the well-defined sysctl SID. */
> -		tsid = SECINITSID_SYSCTL;
> -	}
> -
> -	/* The op values are "defined" in sysctl.c, thereby creating
> -	 * a bad coupling between this module and sysctl.c */
> -	if (op == 001) {
> -		error = avc_has_perm(sid, tsid,
> -				     SECCLASS_DIR, DIR__SEARCH, NULL);
> -	} else {
> -		av = 0;
> -		if (op & 004)
> -			av |= FILE__READ;
> -		if (op & 002)
> -			av |= FILE__WRITE;
> -		if (av)
> -			error = avc_has_perm(sid, tsid,
> -					     SECCLASS_FILE, av, NULL);
> -	}
> -
> -	return error;
> -}
> -
>  static int selinux_quotactl(int cmds, int type, int id, struct super_block *sb)
>  {
>  	const struct cred *cred = current_cred();
> @@ -5398,7 +5315,6 @@ static struct security_operations selinux_ops = {
>  	.ptrace_traceme =		selinux_ptrace_traceme,
>  	.capget =			selinux_capget,
>  	.capset =			selinux_capset,
> -	.sysctl =			selinux_sysctl,
>  	.capable =			selinux_capable,
>  	.quotactl =			selinux_quotactl,
>  	.quota_on =			selinux_quota_on,

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 1/2] security/selinux: fix /proc/sys/ labeling

[Eric W. Biederman]

Lucian Adrian Grijincu <lucian.grijincu@gmail.com> writes:

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e276eb4..5231b95 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -43,7 +43,6 @@
>  #include <linux/fdtable.h>
>  #include <linux/namei.h>
>  #include <linux/mount.h>
> -#include <linux/proc_fs.h>
>  #include <linux/netfilter_ipv4.h>
>  #include <linux/netfilter_ipv6.h>
>  #include <linux/tty.h>
> @@ -70,7 +69,6 @@
>  #include <net/ipv6.h>
>  #include <linux/hugetlb.h>
>  #include <linux/personality.h>
> -#include <linux/sysctl.h>
>  #include <linux/audit.h>
>  #include <linux/string.h>
>  #include <linux/selinux.h>
> @@ -1120,39 +1118,35 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
>  }
>  
>  #ifdef CONFIG_PROC_FS
> -static int selinux_proc_get_sid(struct proc_dir_entry *de,
> +static int selinux_proc_get_sid(struct dentry *dentry,
>  				u16 tclass,
>  				u32 *sid)
>  {
> -	int buflen, rc;
> -	char *buffer, *path, *end;
> +	int rc;
> +	char *buffer, *path;
>  
>  	buffer = (char *)__get_free_page(GFP_KERNEL);
>  	if (!buffer)
>  		return -ENOMEM;
>  
> -	buflen = PAGE_SIZE;
> -	end = buffer+buflen;
> -	*--end = '\0';
> -	buflen--;
> -	path = end-1;
> -	*path = '/';
> -	while (de && de != de->parent) {
> -		buflen -= de->namelen + 1;
> -		if (buflen < 0)
> -			break;
> -		end -= de->namelen;
> -		memcpy(end, de->name, de->namelen);
> -		*--end = '/';
> -		path = end;
> -		de = de->parent;
> +	path = dentry_path_raw(dentry, buffer, PAGE_SIZE);

What kernel has a dentry_path_raw?  Perhaps you mean __dentry_path?

> +	if (IS_ERR(path))
> +		rc = PTR_ERR(path);
> +	else {
> +		/* each process gets a /proc/PID/ entry. Strip off the
> +		 * PID part to get a valid selinux labeling.
> +		 * e.g. /proc/1/net/rpc/nfs -> /net/rpc/nfs */
> +		while (path[1] >= '0' && path[1] <= '9') {
> +			path[1] = '/';
> +			path++;
> +		}
> +		rc = security_genfs_sid("proc", path, tclass, sid);
>  	}
> -	rc = security_genfs_sid("proc", path, tclass, sid);
>  	free_page((unsigned long)buffer);
>  	return rc;
>  }
>  #else

Eric

Re: [PATCH 1/2] security/selinux: fix /proc/sys/ labeling

[Eric W. Biederman]

Lucian Adrian Grijincu <lucian.grijincu@gmail.com> writes:

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e276eb4..5231b95 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -43,7 +43,6 @@
>  #include <linux/fdtable.h>
>  #include <linux/namei.h>
>  #include <linux/mount.h>
> -#include <linux/proc_fs.h>
>  #include <linux/netfilter_ipv4.h>
>  #include <linux/netfilter_ipv6.h>
>  #include <linux/tty.h>
> @@ -70,7 +69,6 @@
>  #include <net/ipv6.h>
>  #include <linux/hugetlb.h>
>  #include <linux/personality.h>
> -#include <linux/sysctl.h>
>  #include <linux/audit.h>
>  #include <linux/string.h>
>  #include <linux/selinux.h>
> @@ -1120,39 +1118,35 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
>  }
>  
>  #ifdef CONFIG_PROC_FS
> -static int selinux_proc_get_sid(struct proc_dir_entry *de,
> +static int selinux_proc_get_sid(struct dentry *dentry,
>  				u16 tclass,
>  				u32 *sid)
>  {
> -	int buflen, rc;
> -	char *buffer, *path, *end;
> +	int rc;
> +	char *buffer, *path;
>  
>  	buffer = (char *)__get_free_page(GFP_KERNEL);
>  	if (!buffer)
>  		return -ENOMEM;
>  
> -	buflen = PAGE_SIZE;
> -	end = buffer+buflen;
> -	*--end = '\0';
> -	buflen--;
> -	path = end-1;
> -	*path = '/';
> -	while (de && de != de->parent) {
> -		buflen -= de->namelen + 1;
> -		if (buflen < 0)
> -			break;
> -		end -= de->namelen;
> -		memcpy(end, de->name, de->namelen);
> -		*--end = '/';
> -		path = end;
> -		de = de->parent;
> +	path = dentry_path_raw(dentry, buffer, PAGE_SIZE);

What kernel has a dentry_path_raw?  Perhaps you mean __dentry_path?

> +	if (IS_ERR(path))
> +		rc = PTR_ERR(path);
> +	else {
> +		/* each process gets a /proc/PID/ entry. Strip off the
> +		 * PID part to get a valid selinux labeling.
> +		 * e.g. /proc/1/net/rpc/nfs -> /net/rpc/nfs */
> +		while (path[1] >= '0' && path[1] <= '9') {
> +			path[1] = '/';
> +			path++;
> +		}
> +		rc = security_genfs_sid("proc", path, tclass, sid);
>  	}
> -	rc = security_genfs_sid("proc", path, tclass, sid);
>  	free_page((unsigned long)buffer);
>  	return rc;
>  }
>  #else

Eric

Re: [PATCH 1/2] security/selinux: fix /proc/sys/ labeling

[Lucian Adrian Grijincu]

On Tue, Feb 1, 2011 at 9:33 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> What kernel has a dentry_path_raw?  Perhaps you mean __dentry_path?


See the function here:
https://github.com/mirrors/linux-2.6/blob/70d1f365568e0cdbc9f4ab92428e1830fdb09ab0/fs/dcache.c

The last patches were against 2.6.38-rc2 because the dcache layer got
rewritten in 2.6.38 http://lwn.net/Articles/421784/

__dentry_path is now static (in fs/dcache.c) and does not take the
necessary locks.

dentry_path_raw is __dentry_path with locks

    ec2447c278ee973d35f38e53ca16ba7f965ae33d
    hostfs: simplify locking

    Remove dcache_lock locking from hostfs filesystem, and move it into dcache
    helpers. All that is required is a coherent path name. Protection from
    concurrent modification of the namespace after path name generation is not
    provided in current code, because dcache_lock is dropped before the path is
    used.

    Signed-off-by: Nick Piggin <npiggin@kernel.dk>



-- 
 .
..: Lucian

Re: [PATCH 1/2] security/selinux: fix /proc/sys/ labeling

[Eric W. Biederman]

Lucian Adrian Grijincu <lucian.grijincu@gmail.com> writes:

> On Tue, Feb 1, 2011 at 9:33 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> What kernel has a dentry_path_raw?  Perhaps you mean __dentry_path?
>
>
> See the function here:
> https://github.com/mirrors/linux-2.6/blob/70d1f365568e0cdbc9f4ab92428e1830fdb09ab0/fs/dcache.c
>
> The last patches were against 2.6.38-rc2 because the dcache layer got
> rewritten in 2.6.38 http://lwn.net/Articles/421784/
>
> __dentry_path is now static (in fs/dcache.c) and does not take the
> necessary locks.

Thanks.  I thought I was looking at the latest source but it turns out
I fat fingered something and my tree was still 2.6.36-rc3.  Sigh.

dentry_path_raw does seem reasonable.

Eric