From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932473AbcETLTb (ORCPT ); Fri, 20 May 2016 07:19:31 -0400 Received: from e28smtp06.in.ibm.com ([125.16.236.6]:57344 "EHLO e28smtp06.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753222AbcETLT2 (ORCPT ); Fri, 20 May 2016 07:19:28 -0400 X-IBM-Helo: d28dlp03.in.ibm.com X-IBM-MailFrom: zohar@linux.vnet.ibm.com X-IBM-RcptTo: linux-api@vger.kernel.org;linux-kernel@vger.kernel.org;linux-security-module@vger.kernel.org Message-ID: <1463743150.2465.100.camel@linux.vnet.ibm.com> Subject: Re: [PATCH RFC] user-namespaced file capabilities - now with more magic From: Mimi Zohar To: "Serge E. Hallyn" Cc: LKML , Jann Horn , "Eric W. Biederman" , Seth Forshee , LSM , "Andrew G. Morgan" , Kees Cook , Michael Kerrisk-manpages , "Serge E. Hallyn" , Linux API , Andy Lutomirski , Linux Containers Date: Fri, 20 May 2016 07:19:10 -0400 In-Reply-To: <20160520034048.GA31216@mail.hallyn.com> References: <87h9egp2oq.fsf@x220.int.ebiederm.org> <20160503051921.GA31551@mail.hallyn.com> <87bn4nhejj.fsf@x220.int.ebiederm.org> <20160507231012.GA11076@pc.thejh.net> <20160511210221.GA24015@mail.hallyn.com> <20160516211523.GA5282@mail.hallyn.com> <20160516214804.GA5926@mail.hallyn.com> <20160518215752.GA9187@mail.hallyn.com> <1463691236.2465.74.camel@linux.vnet.ibm.com> <20160520034048.GA31216@mail.hallyn.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.11 (3.12.11-1.fc21) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16052011-0021-0000-0000-00000CAC71A2 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2016-05-19 at 22:40 -0500, Serge E. Hallyn wrote: > Quoting Mimi Zohar (zohar@linux.vnet.ibm.com): > > On Wed, 2016-05-18 at 16:57 -0500, Serge E. Hallyn wrote: > > > diff --git a/fs/xattr.c b/fs/xattr.c > > > index 4861322..5c0e7ae 100644 > > > --- a/fs/xattr.c > > > +++ b/fs/xattr.c > > > @@ -94,11 +94,26 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, > > > { > > > struct inode *inode = dentry->d_inode; > > > int error = -EOPNOTSUPP; > > > + void *wvalue = NULL; > > > + size_t wsize = 0; > > > int issec = !strncmp(name, XATTR_SECURITY_PREFIX, > > > XATTR_SECURITY_PREFIX_LEN); > > > > > > - if (issec) > > > + if (issec) { > > > inode->i_flags &= ~S_NOSEC; > > > + /* if root in a non-init user_ns tries to set > > > + * security.capability, write a security.nscapability > > > + * in its place */ > > > + if (!strcmp(name, "security.capability") && > > > + current_user_ns() != &init_user_ns) { > > > + cap_setxattr_make_nscap(dentry, value, size, &wvalue, &wsize); > > > + if (!wvalue) > > > + return -EPERM; > > > + value = wvalue; > > > + size = wsize; > > > + name = "security.nscapability"; > > > + } > > > > The call to capable_wrt_inode_uidgid() is hidden behind > > cap_setxattr_make_nscap(). Does it make sense to call it here instead, > > before the security.capability test? This would lay the foundation for > > doing something similar for IMA. > > Might make sense to move that. Though looking at it with fresh eyes I wonder > whether adding less code here at __vfs_setxattr_noperm(), i.e. > > if (!cap_setxattr_makenscap(dentry, &value, &size, &name)) > return -EPERM; > > would be cleaner. Yes, it would be cleaner, but I'm suggesting you do all the hard work making it generic. Then the rest of us can follow your lead. Its more likely that you'll get it right. At a high level, it might look like: /* Permit root in a non-init user_ns to modify the security * namespace xattr equivalents (eg. nscapability, ns_ima, etc). */ if ((current_user_ns() != &init_user_ns) && capable_wrt_inode_uidgid(inode, CAP_SETFCAP)) { if security..capability call capability /* set nscapability? */ else if security.ima call ima /* set ns_ima? */ } Mimi