Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753496Ab3JBOzO (ORCPT ); Wed, 2 Oct 2013 10:55:14 -0400 Received: from numidia.opendz.org ([98.142.220.152]:36583 "EHLO numidia.opendz.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753434Ab3JBOzK (ORCPT ); Wed, 2 Oct 2013 10:55:10 -0400 Date: Wed, 2 Oct 2013 15:55:06 +0100 From: Djalal Harouni To: Andy Lutomirski Cc: "Eric W. Biederman" , Kees Cook , Al Viro , Andrew Morton , Linus Torvalds , Ingo Molnar , "Serge E. Hallyn" , Cyrill Gorcunov , David Rientjes , LKML , linux-fsdevel@vger.kernel.org, kernel-hardening@lists.openwall.com, tixxdz@gmail.com Subject: Re: [PATCH v2 2/9] procfs: add proc_allow_access() to check if file's opener may access task Message-ID: <20131002145506.GA2669@dztty> References: <1380659178-28605-1-git-send-email-tixxdz@opendz.org> <1380659178-28605-3-git-send-email-tixxdz@opendz.org> <524B78A2.40007@amacapital.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <524B78A2.40007@amacapital.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3957 Lines: 102 On Tue, Oct 01, 2013 at 06:36:34PM -0700, Andy Lutomirski wrote: > On 10/01/2013 01:26 PM, Djalal Harouni wrote: > > Since /proc entries varies at runtime, permission checks need to happen > > during each system call. > > > > However even with that /proc file descriptors can be passed to a more > > privileged process (e.g. a suid-exec) which will pass the classic > > ptrace_may_access() permission check. The open() call will be issued in > > general by an unprivileged process while the disclosure of sensitive > > /proc information will happen using a more privileged process at > > read(),write()... > > > > Therfore we need a more sophisticated check to detect if the cred of the > > process have changed, and if the cred of the original opener that are > > stored in the file->f_cred have enough permission to access the task's > > /proc entries during read(), write()... > > > > Add the proc_allow_access() function that will receive the file->f_cred > > as an argument, and tries to check if the opener had enough permission > > to access the task's /proc entries. > > > > This function should be used with the ptrace_may_access() check. > > > > Cc: Kees Cook > > Suggested-by: Eric W. Biederman > > Signed-off-by: Djalal Harouni > > --- > > fs/proc/base.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > fs/proc/internal.h | 2 ++ > > 2 files changed, 58 insertions(+) > > > > diff --git a/fs/proc/base.c b/fs/proc/base.c > > index e834946..c29eeae 100644 > > --- a/fs/proc/base.c > > +++ b/fs/proc/base.c > > @@ -168,6 +168,62 @@ int proc_same_open_cred(const struct cred *fcred) > > cap_issubset(cred->cap_permitted, fcred->cap_permitted)); > > } > > > > +/* Returns 0 on success, -errno on denial. */ > > +static int __proc_allow_access(const struct cred *cred, > > + struct task_struct *task, unsigned int mode) > > +{ > > + int ret = 0; > > + const struct cred *tcred; > > + const struct cred *fcred = cred; > > + > > + rcu_read_lock(); > > + tcred = __task_cred(task); > > + if (uid_eq(fcred->uid, tcred->euid) && > > + uid_eq(fcred->uid, tcred->suid) && > > + uid_eq(fcred->uid, tcred->uid) && > > + gid_eq(fcred->gid, tcred->egid) && > > + gid_eq(fcred->gid, tcred->sgid) && > > + gid_eq(fcred->gid, tcred->gid)) > > + goto out; > > + > > What's this for? Is it supposed to be an optimization? If so, it looks > potentially exploitable, although I don't really understand what you're > trying to do. This function should be used in addition to the ptrace_may_access() one. This will help to check if the original process that opened a /proc/pid/* file is able to read(),write() to it. As we have said, the file descriptor can be passed to a more privileged process to pass the ptrace_may_access(). If we detect that current's cred have changed between ->open() (unprivileged) and ->read() (privileged, exec a suid-exec), then we call this helper function to check the file's opener cred against the target task during this ->read(),->write()... This should block vulnerabilities like the /proc/pid/mem http://lwn.net/Articles/476947/ Since current will not have the same privileges of the process that opened the file (cred have changed). So: This is not an optimization! Can you elaborate more on the potentially exploitable please ? It has the same logic of ptrace_may_access(). Didn't want to modifiy ptrace_may_access() since it relies on current and its context and if we do, we'll have to touch the capability checks also. So just add this one here and call it only if current's cred change. > --Andy -- Djalal Harouni http://opendz.org -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/