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

From: Stephen Smalley
Date: Tue Feb 01 2011 - 14:04:52 EST

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@xxxxxxxxxxxx>
> Signed-off-by: Lucian Adrian Grijincu <lucian.grijincu@xxxxxxxxx>

Acked-by: Stephen Smalley <sds@xxxxxxxxxxxxx>

> ---
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/