From patchwork Thu Sep 1 13:22:50 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Seth Forshee X-Patchwork-Id: 9309037 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A0D52607D6 for ; Thu, 1 Sep 2016 13:22:59 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 91395293F2 for ; Thu, 1 Sep 2016 13:22:59 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8592B2940C; Thu, 1 Sep 2016 13:22:59 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E6EB8293F2 for ; Thu, 1 Sep 2016 13:22:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754879AbcIANWz (ORCPT ); Thu, 1 Sep 2016 09:22:55 -0400 Received: from mail-oi0-f53.google.com ([209.85.218.53]:34380 "EHLO mail-oi0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754325AbcIANWx (ORCPT ); Thu, 1 Sep 2016 09:22:53 -0400 Received: by mail-oi0-f53.google.com with SMTP id l203so115696162oib.1 for ; Thu, 01 Sep 2016 06:22:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=+eNb+V2caTXT7DHPYRLHbzKnfTRezM6oMaVgwQG8goQ=; b=Uzf0jHociLzbfAWaghF0i/2nycuCvkgGfifo+yLXu4jEjSQEJeTFJ8uxtS7tE7f/W1 KQC9T+MUwzF1Sq7RKdawEgegWToMoRmrYsykjm78xQhw1cFHXlB84/xH73ceWj3JQU0l z2/+dEku80Sd+Glzwr9+vtb0dtjtxFx8/yNpXP3zLfJGvArFI9DiXnZtwvu2oHgAGEBW +e0qkmSXVB8xX5p5dwrlqk1QDtqKY5eOZyXpMRuv+YOtjKvqP5S+S/amTXwtfLLvrHE8 XHTaqtGihQBCireEmct0MHQu+LvWDEP0ysczb3PWGou4+MYtxKX4zk4Cgx0bfUzTJAm8 ePdg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=+eNb+V2caTXT7DHPYRLHbzKnfTRezM6oMaVgwQG8goQ=; b=FmNvCuEovjgaVY+y5tGyuAPUXDZf07hN+hbLhQQvQCjeSJHEqXuHdswHQa8vSbpFpM V/lZ5g5NQo5KWHEK6fVuOuoZp4V1MMtZ+dUHm6FR7MLUtIwQBmcp64msfV+gQNBQ/YIE YWuTSxduRVTnGstXDWTcsaVDA8k4s9bG49EvmhWd5pjvXOfyNOR6Lz3VDoAhRpDJveUp 1t3wz43neNXGtRiHgL37zjlt7t2g/ZlR6vNk4j6PS0mx3YJZhZinWPDNaa7cNCCqBJv5 8GAgfQy2fhfA8nE1tmRYEg3WUPpKfjLWR98GzcIkgDQdXPLzhMBeNiL5it6Jao42j7h1 PM+g== X-Gm-Message-State: AE9vXwOIZov5vDCqEBw58S0g2aYnT8UY8+R1qgbZdIAGTTZfS43gKoMf/HEmgAwpMhLre6pw X-Received: by 10.157.43.8 with SMTP id o8mr15315913otb.166.1472736172507; Thu, 01 Sep 2016 06:22:52 -0700 (PDT) Received: from localhost ([2605:a601:aa9:6620:dc1d:dab1:2cc:568a]) by smtp.gmail.com with ESMTPSA id n12sm1860958otb.36.2016.09.01.06.22.51 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 01 Sep 2016 06:22:51 -0700 (PDT) Date: Thu, 1 Sep 2016 08:22:50 -0500 From: Seth Forshee To: "Eric W. Biederman" Cc: Mimi Zohar , Dmitry Kasatkin , linux-ima-devel@lists.sourceforge.net, linux-ima-user@lists.sourceforge.net, linux-security-module@vger.kernel.org Subject: Re: [PATCH 0/1] EVM/IMA xattr hardening and unprivileged user ns mounts Message-ID: <20160901132250.GB145647@ubuntu-hedt> References: <1470057550-58098-1-git-send-email-seth.forshee@canonical.com> <1470080186.23563.427.camel@linux.vnet.ibm.com> <871t28w7h1.fsf@x220.int.ebiederm.org> <1470106743.23563.531.camel@linux.vnet.ibm.com> <87shuntg88.fsf@x220.int.ebiederm.org> <1470163748.23563.570.camel@linux.vnet.ibm.com> <87a8gvotui.fsf@x220.int.ebiederm.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87a8gvotui.fsf@x220.int.ebiederm.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Aug 02, 2016 at 02:26:29PM -0500, Eric W. Biederman wrote: > Mimi Zohar writes: > > > On Di, 2016-08-02 at 09:09 -0500, Eric W. Biederman wrote: > >> Mimi Zohar writes: > >> > >> > On Mo, 2016-08-01 at 15:38 -0500, Eric W. Biederman wrote: > >> >> Mimi Zohar writes: > >> >> > >> >> > On Mo, 2016-08-01 at 08:19 -0500, Seth Forshee wrote: > >> >> >> I was taking a look at IMA and EVM in the context of allowing mounts in > >> >> >> user namespaces from users with only capabilities in that namespace > >> >> >> (i.e. no global capabilities). As a result of that I found just a > >> >> >> handful of places where the size of an xattr or the values it contains > >> >> >> lack sufficient validation to protect against malicious xattrs. The > >> >> >> fixes for these are worthwhile on their own, so I'm sending them in the > >> >> >> following patch. > >> >> >> > >> >> >> More generally though I'd like to request some guidance on how IMA and > >> >> >> EVM should handle xattrs in mounts from globally-unprivileged users. For > >> >> >> context, the intended use case for this is allowing fuse mounts in user > >> >> >> namespace containers, and possibly mounts with a limited set of other > >> >> >> filesystems in the future. All that would actually be needed to mount > >> >> >> though are private user and mount namespaces. > >> >> >> > >> >> >> Based on what I see, I suspect that we can treat the user ns mounts like > >> >> >> any other mount. I don't see that this would cause harm as long as the > >> >> >> filesystem can't trigger bugs in the xattr parsing, and if the system is > >> >> >> configured for IMA enforcment this policy should also be applied to > >> >> >> these mounts. > >> >> > > >> >> > "security.evm" is written as a result of one of the protected security > >> >> > xattrs being written. From your question, I assume that in this use > >> >> > case scenario there are two concerns: > >> >> > - parsing improper security.evm xattrs > >> >> > - updating the protected security xattrs, causing the EVM xattr to > >> >> > "bless" the change. > >> >> > > >> >> > The first issue you're addressing with the patch. As for the second > >> >> > issue, as long as root in the namespace can not write security.xattrs, > >> >> > there shouldn't be a problem. > >> >> > >> >> Assume that it is allowed that root in a namespace will be allowed to > >> >> write security xattrs for a filesystem it has mounted. Certainly for > >> >> the capability xattr this makes sense. > >> > > >> > Yeah, I remember. Having one capability xattr sounded interesting. > >> > Have the capability patches been upstreamed? To label files in a > >> > container, root in the namespace will need to be able to write an IMA > >> > xattr. Whether it is possible to use a single xattr, is yet to be > >> > determined. > > > >> For the most part yes. As part of this merge window Linus > >> merged the changes to store s_user_ns on a super block and the code to > >> use that. > > > > Great! I'll take a look. > > > >> I have kept back the code to relax the permission checks in the obvious > >> cases. There is little immediate gain from that code and I noticed a > >> few places where s_user_ns is not being handled properly. Primarily > >> IMA and EVM (so this disucssion). > > > > Ok > > > >> >> If understand you correctly is that the key used to sign the HMAC in > >> >> security.evm is machine local. As such I expect the proper semantics > >> >> here will be to not update the security.evm xattrs on write of a > >> >> filesystem with s_user_ns != &init_user_ns. > >> > > >> > The EVM xattr is verified before it can be changed, to prevent making an > >> > existing invalid hmac, valid. If the namespace has the ability to > >> > change any of the metadata included in the hmac, the EVM hmac will need > >> > to be updated with the file metadata, otherwise nobody will be able to > >> > access the file. > >> > >> To be clear with evm enabled and security.evm attributes present if > >> the hmac is wrong access to the file is denied? > > > > Yes, if the file is included in the IMA policy and the EVM > > hmac/signature verification fails, then access to the file is denied. > > Makes sense. > > >> What happens in the case of new files and in the case of file without a > >> security.evm xattr? > > > > New files are a special case. The first EVM protected xattr* written > > will cause the security.evm xattr to be written, without first > > validating the existing xattr. > > > > *evm_config_xattrnames[] contains the list of protected xattrs. > > Thank you. > > >> >> On read I am not certain what the right thing to do is, ignore the > >> >> attribute or do what happens on validation failure. > >> > > >> >> > Any changes made offline/elsewhere, would > >> >> > result in the security.evm HMAC not verifying, causing a denial of > >> >> > service. Unless of course the EVM HMAC key has somehow been > >> >> > leaked/exfiltrated from the kernel. > >> >> > >> >> One scenario where this is likely to come up, and the kind of scenario > >> >> we are planning for is a filesystem on a usb stick is transported from > >> >> one machine to another. One the second system it is mounted directly > >> >> (ideally) or through fuse running a copy of it's filesystem driver in > >> >> userspace to guard against filesystem implementation bugs. > >> >> > >> >> So if that filesystem has security.evm xattrs if it appears things will > >> >> work very poorly. > >> > > >> > It could work for files with EVM signatures, obviously not with an HMAC. > >> > >> Fair. > >> > >> What policy would you recommend for (evm and ima) dealing with > >> filesystems that are mounted by a user namespace root and thus have > >> sb->s_user_ns != &init_user_ns? > > > > I think as long as it can not escape the mount namespace, allowing it to > > write both the EVM and IMA xattrs should be fine. In terms of reading > > the xattrs, real root would need to be able to differentiate the xattrs' > > origin. > > It is a little more subtle than that, but essentially it is true that > a filesystem will not propogate into a mount namespace owned by root in > more privileged user namespace. > > The piece of orgin that is retained in the kernel for a filesystem is > sb->s_user_ns. So the information is there. It is not currently > exported explicitly to userspace, but who owns the mount namespace a > filesystem is in, is a big clue. > > But yes any reader knows which filesystem an xattr is coming from so > this should not be a point of confusion. > > >> Like Seth I am not much of an expert in the semantics of IMA and EVM. > >> I just know that ima calcualtes a hash of a file and does something with > >> it. EVM caculcates a hash of other xattrs and the classic file > >> attributes. > > > > EVM protects file metadata. For immutable files, such as ELF > > executables, the security.evm should contain a file signature. For > > mutable files, security.evm should contain an hmac. > > You know I am not certain what the distinction between a file signature > and a hmac is in this context. I've been reading back through all of this, and I'm not sure any conclusion was reached. For my part, I'm uneasy about writing out hmacs to mounts from a non-root user. So I'll make a proposal - let's not read or write EVM or IMA xattrs for filesystems from non-init user namespace, essentially behaving as though they don't support xattrs. Something like the (untested) patch below. Thanks, Seth --- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 11c1d30bd705..5a1738524fbb 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -182,7 +182,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, int error; int size; - if (!inode->i_op->getxattr) + if (inode->i_sb->s_user_ns != &init_user_ns || !inode->i_op->getxattr) return -EOPNOTSUPP; desc = init_desc(type); if (IS_ERR(desc)) diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 35ab453ce861..3d16e4c523c4 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -78,7 +78,7 @@ static int evm_find_protected_xattrs(struct dentry *dentry) int error; int count = 0; - if (!inode->i_op->getxattr) + if (inode->i_sb->s_user_ns != &init_user_ns || !inode->i_op->getxattr) return -EOPNOTSUPP; for (xattr = evm_config_xattrnames; *xattr != NULL; xattr++) { @@ -118,6 +118,9 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, enum integrity_status evm_status = INTEGRITY_PASS; int rc, xattr_len; + if (d_backing_inode(dentry)->i_sb->s_user_ns != &init_user_ns) + return INTEGRITY_UNKNOWN; + if (iint && iint->evm_status == INTEGRITY_PASS) return iint->evm_status; diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index a13fc6809554..7f6915a4f660 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -50,6 +50,9 @@ static int ima_fix_xattr(struct dentry *dentry, int rc, offset; u8 algo = iint->ima_hash->algo; + if (d_backing_inode(dentry)->i_sb->s_user_ns != &init_user_ns) + return -EOPNOTSUPP; + if (algo <= HASH_ALGO_SHA1) { offset = 1; iint->ima_hash->xattr.sha1.type = IMA_XATTR_DIGEST; @@ -170,7 +173,7 @@ int ima_read_xattr(struct dentry *dentry, { struct inode *inode = d_backing_inode(dentry); - if (!inode->i_op->getxattr) + if (inode->i_sb->s_user_ns != &init_user_ns || !inode->i_op->getxattr) return 0; return vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)xattr_value, @@ -198,7 +201,7 @@ int ima_appraise_measurement(enum ima_hooks func, enum integrity_status status = INTEGRITY_UNKNOWN; int rc = xattr_len, hash_start = 0; - if (!inode->i_op->getxattr) + if (inode->i_sb->s_user_ns != &init_user_ns || !inode->i_op->getxattr) return INTEGRITY_UNKNOWN; if (rc <= 0) {