Re: [PATCH v2 2/9] procfs: add proc_allow_access() to check if file's opener may access task

[Djalal Harouni <tixxdz@opendz.org>]

On Fri, Oct 04, 2013 at 05:35:22PM -0700, Eric W. Biederman wrote:
> Andy Lutomirski <luto@amacapital.net> writes:
> 
> > On Fri, Oct 4, 2013 at 3:55 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> >> Andy Lutomirski <luto@amacapital.net> writes:
> >>
> >>> On Fri, Oct 4, 2013 at 12:41 PM, Djalal Harouni <tixxdz@opendz.org> wrote:
> >>>> On Fri, Oct 04, 2013 at 12:32:09PM -0700, Andy Lutomirski wrote:
> >>>>> On Fri, Oct 4, 2013 at 12:27 PM, Djalal Harouni <tixxdz@opendz.org> wrote:
> >>
> >>>>> > So sorry Andy, I don't follow what you are describing.
> >>>>>
> >>>>> And what parameters are you passing to security_ptrace_access_check?
> >>>>> It's supposed to be f_cred, right?  Because you want to make sure
> >>>>> that, if the opener had some low-privilege label, the target has
> >>>>> execed and gotten a more secure label, and the reader has a
> >>>>> high-privilege label, that the opener's label is checked against the
> >>>>> target's new label.
> >>>> The current's cred each time.
> >>>
> >>> Exactly.  Hence the NAK.
> >>>
> >>>>
> >>>> Is there some mechanism to check what you describe?
> >>>>
> >>>
> >>> No.  You could try to add one, but getting it to be compatible with
> >>> YAMA might be really messy.
> >>>
> >>> Or you could see if destroying and recreating all the inodes on exec
> >>> or some other revoke-like approach would work.
> >>
> >> This is a revoke like approach, and yes proc has a fully functional
> >> revoke infrastructure.  Right now that revoke is based on the process
> >> going away.  The problem challenge is that the process is morphing.
> >>
> >> The practical question is which runtime checks do we want to perform.
> >>
> >> If we can say in no uncertain terms that short of a suid exec that
> >> no calls (such as setuid) can change the process permissions beyond
> >> our ability to access the file, we can detect and exec and use that
> >> as a signal.
> >
> > If you could ptrace a process before it calls setuid and you can't
> > after it calls setuid, then this is stupid and doesn't matter -- once
> > you've pwned a process, you retain your pwnership at least until exec.
> 
> That is a reasonable principle to work from.  Still I am seeing cases
> where dumpable can change in principle if not in fact with a cred
> change.
> 
> But yes exec does appear to be the event to focus on.
> 
> > So yes, except that it's not just suid exec.  It's any exec that any
> > LSM would not do if no_new_privs were set.
> 
> >> Alternatively we may to look at a processes credentials and in all
> >> cases where those change use that as a signal that the file must
> >> be reopened.
> >
> > Hmm.  So why don't we just do a revoke whenever an exec that changes
> > cred happens?  (This will have some false positives, like unsharing
> > userns, I think, but we probably don't care.)
> 
> But because it is proc and because people do crazy things we have to
> investigate and test before we merge something like that.  I believe
> there was a patch not long ago that would fail an access if the openers
> and and the accessors creds which blew up rather quickly.
> 
> But it is definitely worth looking at.
We can track current exec or even target exec, use the percpu id solution
that was discussed here, and it seems Ingo like the solution:
https://lkml.org/lkml/2012/3/11/23

Grsecurity already do this!


Now to not break things and allow file descriptor to be passed, we couple
the exec id with the *cred and capability superset* check

The advantage of this solution would be:
* We'll have only a *one* ptrace_may_access(),LSM and permission check
  during ->open() or ->read()/write()...

A one check will do the job.


We do this by:
1) During ->open() store the opener exec id somewhere...

2) Call ptrace_may_access() and LSM check during ->open() or later
during ->read(),write()...

3) Then during ->read(),write(): check if current's exec id matches the
opener exec id, if not then there was an execve and continue with the
cred superset check using file->f_cred, the logic here is to check if
opener is privileged than current:

  * Same user namespace: check same uid, gid and capability superset
  * Different user namespaces: check if opener is an ancestor and its
    file->f_cred->euid is the owner...

   IOW make sure that opener is more privileged than current, or with
   the same privileges!

If it fails we return 0, end of file! otherwise allow the operation.


So if opener execve a more privileged process, the read() will fail.

This will make it easy to have a *one* ptrace_may_access() during all
syscalls!


Thoughts ?

-- 
Djalal Harouni
http://opendz.org