|
Message-ID: <20120312224136.GH10787@dztty> Date: Mon, 12 Mar 2012 23:41:36 +0100 From: Djalal Harouni <tixxdz@...ndz.org> To: "Eric W. Biederman" <ebiederm@...ssion.com> 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 On Mon, Mar 12, 2012 at 02:47:43PM -0700, Eric W. Biederman wrote: > 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. I'm not sure I follow you here, these proc files suffer from the same problems, can you please point what are these non-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. Yes. > Adding an exec_id to additional files simply breaks existing > applications for no good reason. Can you please point these applications that this patch will break ? So we do not do it. I've tested 'ps' but perhaps I've missed something ? We just return 0 at read() which is a correct thing to do. > 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 before and during. I say before since this is what we are tying to emulate. > to something we shouldn't have permissions to. > > In general that means reference counting or locks. All exec_id can Locks ? counting yes this perhaps can work as Alan suggested, but a simple check will catch all the things without any node list nor count inc/dec. Linus's /proc/pid/mem patch do not even do that, he just keep the old mm at open time. > 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. Alan's case refers to how to handle all the /proc/<pid>/* files and any other object that should be attached to its process image. > > 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. Yes if you are speaking about {maps,smaps...} then it should also be at open time and at each syscall (it depends on files) and for the /proc/pid/{maps,smaps,numa_maps} yes it is not safe to give non-privileged processes an fd to an objects attached to a privileged process. > Well I am sorry. The permission checks have happened at access time for > ages and that is deliberate. Yes (perhaps even before I use Linux :) ), but IMHO they are not perfect, if they were, then we'll not see all the /proc/<pid>/ problems. The current patches protect /proc/pid/{maps,smaps,numa_maps} without breaking things, there are no checks at open which is not safe, but I did not add it since I was not sure and I don't want to break things (glibc FORITFY_SOURCE ...). My patches add the ptrace checks at open only for /proc/<pid>/{environ,pagemap,mem} which is safe, I did not include /proc/pid/{auxv,maps,smaps,...} so please if you have _real_ cases of problems just post them. > 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. No it's not confused, we just use the /proc/self as it is easy to explain. Sorry if you feel that. > If you expect /proc/ not to let you find out things about yourself > if you are a suid executable that just seems silly. Ah yes this one about suid/privileged, if you are still privileged then there is no harm, but if you drop your privileges IMHO that means that you do not want to do privileged operations, but that current == task in ptrace which is before the creds check will just allow you to do so. Well, this is another subject. > 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. I really don't understant what you are trying to say/prove here, this is the same issue of the last /proc/<pid>/mem one that got fixed by Linus and Olge patches. These are dynamic files. For arguments I'll re-try. > Eric Thanks 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/ -- tixxdz http://opendz.org
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.