linux-kernel - Re: [PATCH] /proc Security Hooks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20071016125403.40ec5c18@laptopd505.fenrus.org>
Date:	Tue, 16 Oct 2007 12:54:03 -0700
From:	Arjan van de Ven <arjan@...radead.org>
To:	Max Kellermann <mk@...all.com>
Cc:	chrisw@...s-sol.org, linux-security-module@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] /proc Security Hooks

On Tue, 16 Oct 2007 21:38:50 +0200
Max Kellermann <mk@...all.com> wrote:

> Add two LSM hooks for limiting access to the proc file system.
> 
> security_proc_task() defines the visibility of tasks in /proc.
> 
> security_proc_generic() lets the LSM define who will see "generic"
> proc entries (see fs/proc/generic.c).
> 
> This patch attempts to unify duplicated code found in modules like
> Linux VServer.

can you please merge this patch only when you also merge the first user
of it? That's the only way we can keep the LSM hooks sane... is to see
them in thew conect of a user.

> +#ifdef CONFIG_SECURITY_PROC
> +		if (security_proc_task(task) != 0)
> +			continue;
> +#endif

please don't use an ifdef like this; just make security_proc_task() be
a define to 0 in the header for that CONFIG_ .. 
In addition, why is this a separate config option? LSM should really
only be one big switch... microswitches like this don't make any sense.

>  		if (proc_pid_fill_cache(filp, dirent, filldir, task,
> tgid) < 0) { put_task_struct(task);
>  			goto out;
> @@ -2453,6 +2457,11 @@ static struct dentry *proc_task_lookup(struct
> inode *dir, struct dentry * dentry goto out;
>  	if (leader->tgid != task->tgid)
>  		goto out_drop_task;
> +#ifdef CONFIG_SECURITY_PROC
> +	result = ERR_PTR(security_proc_task(task));
> +	if (IS_ERR(result))
> +		goto out_drop_task;
> +#endif

same here

> +#ifdef CONFIG_SECURITY_PROC
> +#include <linux/security.h>
> +#endif

don't put ifdefs around includes please.... if anything the ifdef
should be inside the header

> +#ifdef CONFIG_SECURITY_PROC
> +				error = security_proc_generic(de);
> +				if (error != 0)
> +					break;
> +#endif

and this ifdef should die too
>  				spin_unlock(&proc_subdir_lock);
>  				error = -EINVAL;
>  				inode = proc_get_inode(dir->i_sb,
> ino, de); @@ -483,6 +491,10 @@ int proc_readdir(struct file * filp,
>  
>  				/* filldir passes info to user space
> */ de_get(de);
> +#ifdef CONFIG_SECURITY_PROC
> +				if (security_proc_generic(de) != 0)
> +					goto skip;
> +#endif

as does this one... but the goto looks horrid to me
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/