From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [PATCH 2/2] proc,security: move restriction on writing /proc/pid/attr nodes to proc To: Casey Schaufler , Stephen Smalley , =?UTF-8?Q?Jos=c3=a9_Bollo?= , selinux@tycho.nsa.gov References: <1481910072-11392-1-git-send-email-sds@tycho.nsa.gov> <1481910072-11392-2-git-send-email-sds@tycho.nsa.gov> <1482140693.2178.1.camel@nonadev.net> <1482158025.28570.10.camel@tycho.nsa.gov> <1482162073.2178.6.camel@nonadev.net> <1482162765.28570.33.camel@tycho.nsa.gov> Cc: paul@paul-moore.com, james.l.morris@oracle.com, linux-security-module@vger.kernel.org From: John Johansen Message-ID: <8e8baad3-f39e-8d3a-661d-3302a96d43f1@canonical.com> Date: Mon, 19 Dec 2016 12:36:09 -0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 List-Id: "Security-Enhanced Linux \(SELinux\) mailing list" List-Post: List-Help: On 12/19/2016 08:32 AM, Casey Schaufler wrote: > On 12/19/2016 7:52 AM, Stephen Smalley wrote: >> On Mon, 2016-12-19 at 16:41 +0100, José Bollo wrote: >>> Le lundi 19 décembre 2016 à 09:33 -0500, Stephen Smalley a écrit : >>>> On Mon, 2016-12-19 at 10:44 +0100, José Bollo wrote: >>>>> Le vendredi 16 décembre 2016 à 12:41 -0500, Stephen Smalley a >>>>> écrit : >>>>>> >>>>>> Processes can only alter their own security attributes via >>>>>> /proc/pid/attr nodes. This is presently enforced by each >>>>>> individual >>>>>> security module and is also imposed by the Linux credentials >>>>>> implementation, which only allows a task to alter its own >>>>>> credentials. >>>>>> Move the check enforcing this restriction from the individual >>>>>> security modules to proc_pid_attr_write() before calling the >>>>>> security >>>>>> hook, >>>>>> and drop the unnecessary task argument to the security hook >>>>>> since >>>>>> it >>>>>> can >>>>>> only ever be the current task. >>>>> Hi Stephen, >>>>> >>>>> The module PTAGS that I made relies on the ability to write files >>>>> in >>>>> /proc/pid/attr/... >>>>> >>>>> Why did I used this files? Thaere are serious reasons: >>>>> - follows the life cycle of the process >>>>> - implements pid namespace >>>>> >>>>> There is absolutely no way to get these features easyly outside >>>>> that >>>>> implementation context of /proc/pid/attr/xxx. >>>>> >>>>> For these reasons I vote against that change. >>>> It is already prohibited by the credentials framework; >>> If it were REALLY prohibited, your patch shouldn't exist ;) >> You can't do it safely (and your current implementation is unsafe). >> Please, read Documentation/security/credentials.txt in full, and look >> at the setprocattr implementations in the upstream security modules for >> reference. > > I just reviewed the referenced documentation (thanks David) > and it does clearly identify why, from the viewpoint of the > credential implementation, modifying another task's credentials > is unsafe. Both Smack and SELinux view modification of task > credentials as unsafe from a security viewpoint, so neither of > these modules would force the credential implementation to > allow the behavior. > > This, unfortunately, leaves Jose in a bit of a bind. It looks > to me like there are options, so let's not just tell him to > go away. The options I see include: > > - Fix the credential code so modifying another task is safe > I don't advocate this approach because the existing > implementation is only reasonable because of the > restriction. > - Revert shared credentials > I confess to not being a fan of shared credentials, > for reason of complexity. I would not expect a > proposal to go back to embedding credentials in > their containing structures to be considered seriously. > Nonetheless, it would solve the problem. > - Add a security blob for the task > It is unfortunate that when shared credentials were > introduced the task blob was not renamed a cred blob. > It would be simple and consistent to add a security > blob to the task in addition to the blob in the cred. > I would add a t_security field to the task structure > that associates security data with the task. This > blob would be used for data that does not meet intent > of a credential, which I believe may be the case for > Jose's PTAGS. > > I know which I would prefer, but as I expect someone to shout > that none can possibly be acceptable I think I'll stop here and > let the discussion go a while. > > ("Light fuse. Stand back. :)) > Tetsuo has been asking for the return of the task security blob for years. It does makes sense as not everything fits the cred model, eg. seccomp rules aren't part of the cred, and I don't think it makes sense for PTAGS either. AppArmor could make use of it as well, as currently we abuse the cred a little bit to store the change_hat, and setexeccon info.