Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc

["José Bollo" <jobol@nonadev.net>]

Le mardi 20 décembre 2016 à 12:03 -0800, Casey Schaufler a écrit :
> On 12/20/2016 11:35 AM, Stephen Smalley wrote:
> > On Tue, 2016-12-20 at 11:07 -0800, Casey Schaufler wrote:
> > > On 12/20/2016 10:28 AM, Stephen Smalley wrote:
> > > > On Tue, 2016-12-20 at 10:17 -0800, Casey Schaufler wrote:
> > > > > On 12/20/2016 8:50 AM, Stephen Smalley wrote:
> > > > > > On Tue, 2016-12-20 at 17:39 +0100, José Bollo wrote:
> > > > > > > Le mardi 20 décembre 2016 à 11:14 -0500, Stephen Smalley
> > > > > > > a
> > > > > > > écrit
> > > > > > > :
> > > > > > > > 
> > > > > > > > Looking at your PTAGS implementation, I feel it is only
> > > > > > > > fair to
> > > > > > > > warn
> > > > > > > > you that your usage of /proc/pid/attr is insecure,
> > > > > > > > regardless
> > > > > > > > of
> > > > > > > > whether you use task security blobs or cred security
> > > > > > > > blobs.
> > > > > > > 
> > > > > > > Fair?!
> > > > > > > 
> > > > > > > > Getting the attributes of another process via /proc/pid
> > > > > > > > files
> > > > > > > > is
> > > > > > > > inherently racy, as the process may exit and another
> > > > > > > > process
> > > > > > > > with
> > > > > > > > different attributes may be created with the same pid
> > > > > > > > (and
> > > > > > > > no,
> > > > > > > > this
> > > > > > > > is not theoretical; it has been demonstrated).
> > > > > > > 
> > > > > > > I know. And I'm surprized that you dont do anything to
> > > > > > > change
> > > > > > > that.
> > > > > > 
> > > > > > There is a reason why SO_PEERSEC and SCM_SECURITY
> > > > > > exist.  Again,
> > > > > > learn
> > > > > > from the upstream security modules rather than re-inventing
> > > > > > them,
> > > > > > badly.
> > > > > 
> > > > > SO_PEERSEC and SCM_SECURITY are spiffy for processes that are
> > > > > sending each other messages, but they identify the attributes
> > > > > associated with the message, not the process. Neither SELinux
> > > > > nor Smack get the information to send off of the process, it
> > > > > comes from the socket structure.
> > > > 
> > > > Yes, but in the SELinux case at least, the socket is labeled
> > > > with
> > > > the
> > > > label of the creating process (except in the rare case of using
> > > > setsockcreatecon(3), which can only be used by suitably
> > > > authorized
> > > > processes).
> > > 
> > > Yes, it's the same with Smack. When it's not the label
> > > of the process it's the label the system wants the peer
> > > to think it is.
> > > 
> > > >   So in general it serves quite well as a means of obtaining
> > > > the peer label, which can then be used in access control (and
> > > > this
> > > > is
> > > > in fact being used in a variety of applications in Linux and
> > > > Android).
> > > 
> > > But only between processes that are in direct, explicit
> > > communication.
> > > There is no denying that these mechanisms work. They just aren't
> > > applicable to Jose's use.
> > 
> > If you say so (although it is unclear to me why or what exactly his
> > use
> > case is), but regardless, there is also no denying that getting and
> > setting another process' attributes via /proc/pid files is
> > inherently
> > racy.  
> 
> You're right. How can we fix that? I have seen a gazillion cases
> where system security would be much simpler and easier to enforce
> and develop if we could (safely) ensure that the process under
> /proc/pid wouldn't change on you without you knowing.

Yes, fully agreed. It summarizes well my mind. There is here a good job
to be done. I can imagine solution to this problem. And what Stephen
thinks as a law is just a bug.

> > He even acknowledged as much.  So we are left with a "security"
> > module whose only purpose is to support getting and setting process
> > tags for security enforcement purposes, and yet does so in a known-
> > insecure manner.  Again, this is why I keep suggesting that he
> > needs to
> > reconsider his approach, not merely figure out how to implement
> > per-
> > task security blobs.
> 
> Whether or not Jose moves forward with PTAGS we have identified
> an issue with the current cred based hooks for AppArmor, TOMOYO
> and at least one other proposed module. Regardless of the issues
> of /proc/pid there is work to be done.