Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753097AbbG3N5X (ORCPT ); Thu, 30 Jul 2015 09:57:23 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:47515 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751430AbbG3N5V (ORCPT ); Thu, 30 Jul 2015 09:57:21 -0400 Date: Thu, 30 Jul 2015 13:57:10 +0000 From: Serge Hallyn To: Amir Goldstein Cc: Seth Forshee , Casey Schaufler , Stephen Smalley , Andy Lutomirski , "Eric W. Biederman" , Alexander Viro , Linux FS Devel , LSM List , SELinux-NSA , Serge Hallyn , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 0/7] Initial support for user namespace owned mounts Message-ID: <20150730135710.GA4590@ubuntumail> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: 6976 Lines: 155 Quoting Amir Goldstein (amir@cellrox.com): > On Tue, Jul 28, 2015 at 11:40 PM, Seth Forshee > wrote: > > > > On Wed, Jul 22, 2015 at 05:05:17PM -0700, Casey Schaufler wrote: > > > > This is what I currently think you want for user ns mounts: > > > > > > > > 1. smk_root and smk_default are assigned the label of the backing > > > > device. > > Seth, > > There were 2 main concerns discussed in this thread: > 1. trusting LSM labels outside the namespace > 2. trusting the content of the image file/loopdev > > While your approach addresses the first concern, I suspect it may be placing > an obstacle in a way for resolving the second concern. > > A viable security policy to mitigate the second concern could be: > - Allow only trusted programs (e.g. mkfs, fsck) to write to 'Loopback' images > - Allow mount only of 'Loopback' images > > This should allow the system as a whole to trust unprivileged mounts based on > the trust of the entities that had raw access the the fs layout. Just to be sure I understand right, you're looking for a way to let the host admin trust that the kernel's superblock parsers aren't being fed trash or an exploit? > Alas, if you choose to propagate the backing dev label to contained files, > they would all share the designated 'Loopback' label and render the policy above > useless. > > Any thoughts on how to reconcile this conflict? > > Amir. > > > > > > 2. s_root is assigned the transmute property. > > > > 3. For existing files: > > > > a. Files with the same label as the backing device are accessible. > > > > b. Files with any other label are not accessible. > > > > > > That's right. Accept correct data, reject anything that's not right. > > > > > > > If this is right, there are a couple lingering questions in my mind. > > > > > > > > First, what happens with files created in directories with the same > > > > label as the backing device but without the transmute property set? The > > > > inode for the new file will initially be labeled with smk_of_current(), > > > > but then during d_instantiate it will get smk_default and thus end up > > > > with the label we want. So that seems okay. > > > > > > Yes. > > > > > > > The second is whether files with the SMACK64EXEC attribute is still a > > > > problem. It seems it is, for files with the same label as the backing > > > > store at least. I think we can simply skip the code that reads out this > > > > xattr and sets smk_task for user ns mounts, or else skip assigning the > > > > label to the new task in bprm_set_creds. The latter seems more > > > > consistent with the approach you've suggested for dealing with labels > > > > from disk. > > > > > > Yes, I think that skipping the smk_fetch(XATTR_NAME_SMACKEXEC, ...) in > > > smack_d_instantiate for unprivileged mounts would do the trick. > > > > > > > So I guess all of that seems okay, though perhaps a bit restrictive > > > > given that the user who mounted the filesystem already has full access > > > > to the backing store. > > > > > > In truth, there is no reason to expect that the "user" who did the > > > mount will ever have a Smack label that differs from the label of > > > the backing store. If what we've got here seems restrictive, it's > > > because you've got access from someone other than the "user". > > > > > > > Please let me know whether or not this matches up with what you are > > > > thinking, then I can procede with the implementation. > > > > > > My current mindset is that, if you're going to allow unprivileged > > > mounts of user defined backing stores, this is as safe as we can > > > make it. > > > > All right, I've got a patch which I think does this, and I've managed to > > do some testing to confirm that it behaves like I expect. How does this > > look? > > > > What's missing is getting the label from the block device inode; as > > Stephen discovered the inode that I thought we could get the label from > > turned out to be the wrong one. Afaict we would need a new hook in order > > to do that, so for now I'm using the label of the proccess calling > > mount. > > > > --- > > > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > > index a143328f75eb..8e631a66b03c 100644 > > --- a/security/smack/smack_lsm.c > > +++ b/security/smack/smack_lsm.c > > @@ -662,6 +662,8 @@ static int smack_sb_kern_mount(struct super_block *sb, int flags, void *data) > > skp = smk_of_current(); > > sp->smk_root = skp; > > sp->smk_default = skp; > > + if (sb_in_userns(sb)) > > + transmute = 1; > > } > > /* > > * Initialize the root inode. > > @@ -1023,6 +1025,12 @@ static int smack_inode_permission(struct inode *inode, int mask) > > if (mask == 0) > > return 0; > > > > + if (sb_in_userns(inode->i_sb)) { > > + struct superblock_smack *sbsp = inode->i_sb->s_security; > > + if (smk_of_inode(inode) != sbsp->smk_root) > > + return -EACCES; > > + } > > + > > /* May be droppable after audit */ > > if (no_block) > > return -ECHILD; > > @@ -3220,14 +3228,16 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode) > > if (rc >= 0) > > transflag = SMK_INODE_TRANSMUTE; > > } > > - /* > > - * Don't let the exec or mmap label be "*" or "@". > > - */ > > - skp = smk_fetch(XATTR_NAME_SMACKEXEC, inode, dp); > > - if (IS_ERR(skp) || skp == &smack_known_star || > > - skp == &smack_known_web) > > - skp = NULL; > > - isp->smk_task = skp; > > + if (!sb_in_userns(inode->i_sb)) { > > + /* > > + * Don't let the exec or mmap label be "*" or "@". > > + */ > > + skp = smk_fetch(XATTR_NAME_SMACKEXEC, inode, dp); > > + if (IS_ERR(skp) || skp == &smack_known_star || > > + skp == &smack_known_web) > > + skp = NULL; > > + isp->smk_task = skp; > > + } > > > > skp = smk_fetch(XATTR_NAME_SMACKMMAP, inode, dp); > > if (IS_ERR(skp) || skp == &smack_known_star || > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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/