Message-ID: <20131003122959.GA3619@dztty>
Date:	Thu, 3 Oct 2013 13:29:59 +0100
From:	Djalal Harouni <tixxdz@...ndz.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Kees Cook <keescook@...omium.org>,
	Andy Lutomirski <luto@...capital.net>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"Serge E. Hallyn" <serge.hallyn@...ntu.com>,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	David Rientjes <rientjes@...gle.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux FS Devel <linux-fsdevel@...r.kernel.org>,
	"kernel-hardening@...ts.openwall.com" 
	<kernel-hardening@...ts.openwall.com>,
	Djalal Harouni <tixxdz@...il.com>
Subject: Re: [PATCH v2 0/9] procfs: protect /proc/<pid>/* files with
 file->f_cred

On Thu, Oct 03, 2013 at 08:12:44AM +0200, Ingo Molnar wrote:
> 
> * Djalal Harouni <tixxdz@...ndz.org> wrote:
> 
> > > Regardless, glibc uses /proc/self/maps, which would be fine here, right?
> >
> > I did not touch /proc/self/maps and others, but I'm planning to fix them 
> > if this solution is accepted.
> > 
> > I'll do the same thing as in /proc/*/stat for maps, let it be 0444, and 
> > try to delay the check to ->read(). So during ->read() perform 
> > ptrace_may_access() on currenct's cred and process_allow_access() on 
> > file's opener cred. This should work.
> 
> Aren't read() time checks fundamentally unrobust? We generally don't do 
> locking on read()s, so such checks - in the general case - are pretty 
> racy.
For procfs yes, read() time checks are unrobust, the general agreement on
procfs is checks should be performed during each syscall.

For the locking on read()/write() IMHO there should be locking by design
for /proc/pid/* entries. Here we are speaking about content that varies,
data attached to other processes, so there is already some locking
mechanism, and for sensitive stuff, we must hold the cred mutex. This
is the standard from the old days of procfs.


And yes some of them are racy, but we can improve it, delay the checks.

>From old Linux git history, before the initial git repository build, I
found that some important checks were done right after gathering the info.


> Now procfs might be special, as by its nature of a pseudofilesystem it's 
> far more atomic than other filesystems, but still IMHO it's a lot more 


> robust security design wise to revoke access to objects you should not 
> have a reference to when a security boundary is crossed, than letting 
> stuff leak through and sprinking all the potential cases that may leak 
> information with permission checks ...
I agree, but those access should also be checked at the beginning, IOW
during ->open(). revoke will not help if revoke is not involved at all,
the task /proc entries may still be valide (no execve).

Currently security boundary is crossed for example arbitrary /proc/*/stack
(and others).
1) The target task does not do an execve (no revoke).
2) current task will open these files and *want* and *will* pass the fd to a
more privileged process to pass the ptrace check which is done only during
->read().


> It's also probably faster: security boundary crossings are relatively rare 
> slowpaths, while read()s are much more common...
Hmm, These two are related? can't get rid of permission checks
especially on this pseudofilesystem!


> So please first get consensus on this fundamental design question before 
> spreading your solution to more areas.
Of course, I did clean the patchset to prove that it will work, and I
only implemented full protection for /proc/*/{stack,syscall,stat} other
files will wait.

But Ingo you can't ignore the fact that:
/proc/*/{stack,syscall} are 0444 mode
/proc/*/{stack,syscall} do not have ptrace_may_access() during open()
/proc/*/{stack,syscall} have the ptrace_may_access() during read()

Every unprivileged process will have an fd on arbitrary privileged
files, then pass fd to a more privileged process...
https://lkml.org/lkml/2013/8/28/544

We need 0400 VFS checks, and ptrace capability check during ->open().


So here revoke is not invloved at all, we'll have to close these issues.
Any thoughts please ? patch submitted but... I can split it again to
try to close these issues. Then wait to see for the file->f_cred or revoke
design ?




> Thanks,
> 
> 	Ingo

-- 
Djalal Harouni
http://opendz.org
--
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