Message-ID: <m11uoxij9c.fsf@fess.ebiederm.org>
Date:	Mon, 12 Mar 2012 14:47:43 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Djalal Harouni <tixxdz@...ndz.org>
Cc:	linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Vasiliy Kulikov <segoon@...nwall.com>,
	Kees Cook <keescook@...omium.org>,
	Solar Designer <solar@...nwall.com>,
	WANG Cong <xiyou.wangcong@...il.com>,
	James Morris <james.l.morris@...cle.com>,
	Oleg Nesterov <oleg@...hat.com>,
	linux-security-module@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, Alan Cox <alan@...rguk.ukuu.org.uk>,
	Greg KH <gregkh@...uxfoundation.org>,
	Ingo Molnar <mingo@...e.hu>, Stephen Wilson <wilsons@...rt.ca>,
	"Jason A. Donenfeld" <Jason@...c4.com>
Subject: Re: [PATCH 0/9] proc: protect /proc/<pid>/* files across execve

Djalal Harouni <tixxdz@...ndz.org> writes:

> On Mon, Mar 12, 2012 at 12:13:15PM -0700, Eric W. Biederman wrote:
>> Djalal Harouni <tixxdz@...ndz.org> writes:
>> 
>> > Procfs files and other important objects may contain sensitive information
>> > which must not be seen, inherited or processed across execve.
>> 
>> So I am dense.  /proc/<pid>/mem was special in that it uses a different
>> set of checks than other files, and to do those access checks
>> /proc/<pid>/mem needed to look at exec_id.
> If you are referring to the old protection, yes it was against an ID, but
> not uniq IDs, so you can execve a suid do some tricks to have a match on IDs
> and bypass the protection, how: by opening your /proc/self/mem and pass
> the fd to the exec suid who at read/write time will process its own
> /proc/self/mem

Yes that case is silly and I don't care.  I care that you seem to be
stomping all over the non-silly cases using the excuse that there
was one silly case.

>> For all of the access checks that are not written in that silly way.
>> What is wrong with ptrace_may_access run at every read/write of a file?
> As it was noted, these files change during runtime, so even if you do the
> ptrace check at each syscall (which is of course a good thing), you must be
> sure that you are doing the check on the right target and the processed
> file belongs really to the appropriate process image of the target.

The right target is by definition the current value of the process in
question.

/proc/<pid>/<attr> files are supposed to work after an exec.

Adding an exec_id to additional files simply breaks existing
applications for no good reason.

What is needed is for safety is to guard against the race of exec
happening during a read or a write, so that we don't get access
to something we shouldn't have permissions to.

In general that means reference counting or locks.  All exec_id can
meaningfully be used for in the general case is a trigger to try again.

> This is not news. Alan's historical links:
> http://lkml.org/lkml/2012/1/29/35

Alan's case refers to how to handle /proc/<pid>/maps the right way.

> The previous discussion on kernel-hardening:
> (includes some variants described by Vasiliy and other problems which I'll
> try to discuss here)
> http://www.openwall.com/lists/kernel-hardening/2012/02/10/1

In your post on openwall I just see you arguing that the current
and deliberate semantics of the permission checks on the proc files are
wrong.  Because the permission checks happen at access time rather than
at open time.

Well I am sorry.  The permission checks have happened at access time for
ages and that is deliberate.

Furthermore one of your confused arguments seems to imply that there is
a such a thing as a /proc/self file distinct from a /proc/<pid> file.
There not.  /proc/self is just a symlink into /proc/<pid> and as such
does not open new security holes.

If you expect /proc/ not to let you find out things about yourself
if you are a suid executable that just seems silly.

So in short you seem to be arguing for changing the semantics of access
of every file under /proc/<pid> because there is cognitive dissonance
between your understanding of those files and what is implemented.  I
see no acknowledgement that you are arguing for a semantic change or
any arguments in favor of that change.  At best I see is an argument
that says you the files don't work the way you expect which is most
definitely not sufficient for a change.

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

Powered by blists - more mailing lists