linux-kernel - Re: [PATCH 1/9] procfs: use flags to deny or allow access to /proc/<pid>/$entry

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: <20140526191353.GB6887@dztty>
Date:	Mon, 26 May 2014 20:13:53 +0100
From:	Djalal Harouni <tixxdz@...ndz.org>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Kees Cook <keescook@...omium.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Alexey Dobriyan <adobriyan@...il.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux FS Devel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH 1/9] procfs: use flags to deny or allow access to
 /proc/<pid>/$entry

On Mon, May 26, 2014 at 11:06:40AM -0700, Andy Lutomirski wrote:
> On Mon, May 26, 2014 at 10:21 AM, Djalal Harouni <tixxdz@...ndz.org> wrote:
> > On Mon, May 26, 2014 at 09:57:16AM -0700, Andy Lutomirski wrote:
> >> On Mon, May 26, 2014 at 6:27 AM, Djalal Harouni <tixxdz@...ndz.org> wrote:
> >> > Add the deny or allow flags, so we can perform proper permission checks
> >> > and set the result accordingly. These flags are needed in case we have
> >> > to cache the result of permission checks that are done during ->open()
> >> > time. Later during ->read(), we can decide to allow or deny the read().
> >> >
> >> > The pid entries that need these flags are:
> >> > /proc/<pid>/stat
> >> > /proc/<pid>/wchan
> >> > /proc/<pid>/maps  (will be handled in next patches).
> >> >
> >> > These files are world readable, userspace depend on that. To prevent
> >> > ASLR leaks and to avoid breaking userspace, we follow this scheme:
> >> >
> >> > a) Perform permission checks during ->open()
> >> > b) Cache the result of a) and return success
> >> > c) Recheck the cached result during ->read()
> >>
> >> Why is (c) needed?
> > In order to not break these entries, some of them are world readable.
> >
> > So we perform the re-check that *single* cached integer, in order to
> > allow access for the non-sensitive, and block or pad with zeros the
> > sensitive.
> 
> What I mean is: why not just not re-check?  Is it to paper over the
> lack of revoke.
Ahh ok, you mean *re-check* the cached permission during ->read() since
this is necessary, and do *not* re-check ptrace capabilities during
->read()!

Indeed, this is precisely due to the lack of revoke! if we do not
re-check ptrace capabilities during ->read() we may offer this scenario
to attackers:

 open(/proc/$process_I_can_ptrace/*, O_RDONLY)
 and make the $process_I_can_ptrace exec a suid binary, this will pass
 the cached permission of ->open() and let users to read the
 /proc/<suid-exec>/* entries.

 In this case a process like "cat" which we find in all systems can be
 used to disclose sensitive data.

In the other hand if we continue to do the ptrace capability check
during read() then attackers need to find a *suid* binary that reads
from specified input in order to bypass that ptrace check during
->read() instead of using a normal program. This is a big difference!


So in the mean time, Yes we must let the re-check ptrace capability
during ->read() to reduce the attack surface. Later if there is a
revoke(), then you can remove that ptrace check and just check the cached
permission during ->read(), revoke will handle it, in case of an exec!


> >> >  /*
> >> > + * Flags used to deny or allow current to access /proc/<pid>/$entry
> >> > + * after proper permission checks.
> >> > + */
> >> > +enum {
> >> > +       PID_ENTRY_DENY  = 0,    /* Deny access */
> >> > +       PID_ENTRY_ALLOW = 1,    /* Allow access */
> >> > +};
> >>
> >> I think this would be less alarming if this were:
> >>
> >> #define PID_ENTRY_DENY ((void *)1UL)
> >> #define PID_ENTRY_ALLOW ((void *)2UL)
> > Hmm,
> >
> > I would like to keep it enum, enum is type-safe and I want to follow the
> > semantics of /proc/pid/stat and others:
> 
> It's not type-safe the way you're doing it, though.
Can you please shed some light Andy, thank you in advance!


> --Andy
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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