diff mbox

[0/1] EVM/IMA xattr hardening and unprivileged user ns mounts

Message ID 87a8gvotui.fsf@x220.int.ebiederm.org (mailing list archive)
State New, archived
Headers show

Commit Message

Eric W. Biederman Aug. 2, 2016, 7:26 p.m. UTC
Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Di, 2016-08-02 at 09:09 -0500, Eric W. Biederman wrote:
>> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> 
>> > On Mo, 2016-08-01 at 15:38 -0500, Eric W. Biederman wrote:
>> >> Mimi Zohar <zohar@linux.vnet.ibm.com> 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.

>> That last hash as part of the work just merged I have updated to use
>> the uid and gid in s_user_ns instead of &init_user_ns, as on disk that
>> is the namespace the uid and gid are expected to be encoded in.
>
> Ok.  I need to take a look.

It is just the very obvious patch below.

commit 0b3c9761d1e405514a551ed24d3ea89aea26ce14
Author: Seth Forshee <seth.forshee@canonical.com>
Date:   Thu Feb 5 10:44:50 2015 -0600

    evm: Translate user/group ids relative to s_user_ns when computing HMAC
    
    The EVM HMAC should be calculated using the on disk user and
    group ids, so the k[ug]ids in the inode must be translated
    relative to the s_user_ns of the inode's super block.
    
    Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
    Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


Eric
--
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 mbox

Patch

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 30b6b7d0429f..11c1d30bd705 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -151,8 +151,8 @@  static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
        memset(&hmac_misc, 0, sizeof(hmac_misc));
        hmac_misc.ino = inode->i_ino;
        hmac_misc.generation = inode->i_generation;
-       hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
-       hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
+       hmac_misc.uid = from_kuid(inode->i_sb->s_user_ns, inode->i_uid);
+       hmac_misc.gid = from_kgid(inode->i_sb->s_user_ns, inode->i_gid);
        hmac_misc.mode = inode->i_mode;
        crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
        if (evm_hmac_attrs & EVM_ATTR_FSUUID)