From patchwork Wed Oct 26 05:54:26 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Eric W. Biederman" X-Patchwork-Id: 9395941 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 53446600BA for ; Wed, 26 Oct 2016 05:56:44 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 254CB295FD for ; Wed, 26 Oct 2016 05:56:44 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 16D7F28ACE; Wed, 26 Oct 2016 05:56:44 +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=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI 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 2686A28ACE for ; Wed, 26 Oct 2016 05:56:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751334AbcJZF4m (ORCPT ); Wed, 26 Oct 2016 01:56:42 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:43233 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750949AbcJZF4l (ORCPT ); Wed, 26 Oct 2016 01:56:41 -0400 Received: from in02.mta.xmission.com ([166.70.13.52]) by out02.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1bzHCZ-0000rG-Ux; Tue, 25 Oct 2016 23:56:40 -0600 Received: from 75-170-125-99.omah.qwest.net ([75.170.125.99] helo=x220.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1bzHCZ-0007Fz-0k; Tue, 25 Oct 2016 23:56:39 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: David Howells Cc: Andy Lutomirski , Jann Horn , James Bottomley , Linux Containers , Oleg Nesterov , Eric Paris , LSM List , keyrings@vger.kernel.org, simo@redhat.com References: <17576.1477412418@warthog.procyon.org.uk> <18335.1477414412@warthog.procyon.org.uk> <1477414605.3079.40.camel@HansenPartnership.com> <20161025170602.GB24481@laptop.thejh.net> <1477418708.3079.52.camel@HansenPartnership.com> <20161025181735.GC24481@laptop.thejh.net> <20947.1477428095@warthog.procyon.org.uk> Date: Wed, 26 Oct 2016 00:54:26 -0500 In-Reply-To: <20947.1477428095@warthog.procyon.org.uk> (David Howells's message of "Tue, 25 Oct 2016 21:41:35 +0100") Message-ID: <87shrju031.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 X-XM-SPF: eid=1bzHCZ-0007Fz-0k; ; ; mid=<87shrju031.fsf@xmission.com>; ; ; hst=in02.mta.xmission.com; ; ; ip=75.170.125.99; ; ; frm=ebiederm@xmission.com; ; ; spf=neutral X-XM-AID: U2FsdGVkX1+3CoLfGrOPONhowF8Rks7vNG4H8OewvQI= X-SA-Exim-Connect-IP: 75.170.125.99 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: Keyrings, user namespaces and the user_struct X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP David Howells writes: > Andy Lutomirski wrote: > >> ... Perhaps we could simply *remove* the concept of named keys and keyrings. > > See Linus's dictum about breaking userspace. > > The problem isn't named keys: keys have to be named - the description is how > they're looked up typically. Further, non-keyring keys can't be looked up > directly by name - you have to search for them in a keyring. > > The issue here is named keyrings and keyctl_join_session_keyring(). It might > well have been a bad idea - though I've seen some people arguing for a single > session keyring shared across all a user's logins, in which case, we might > want this after all (or use the user-default session). > > One thing we perhaps do want to do, though, is restrict the names of keyrings > to the user_namespace in which the keyring was created. Grr... The first round of user namespace support had actually restricted the description lookup to a single user namespace. Then I missed a detail and converted the code to it's current form. Ooops! I think the answer for all of the issues raised in this conversation is to just make the keyring names and the keyring name lookup per user namespace. Maybe a few small additional tweaks to install_user_keyrings to notice if we have the user keyring from the wrong user namespace. Something like the untested patch below. 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 --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h index eb209d4523f5..5ea474df16bd 100644 --- a/include/linux/user_namespace.h +++ b/include/linux/user_namespace.h @@ -52,6 +52,12 @@ struct user_namespace { struct key *persistent_keyring_register; struct rw_semaphore persistent_keyring_register_sem; #endif +#ifdef CONFIG_KEYS +# ifndef KEYRING_NAME_HASH_SIZE +# define KEYRING_NAME_HASH_SIZE (1 << 5) +# endif + struct list_head keyring_name_hash[KEYRING_NAME_HASH_SIZE]; +#endif struct work_struct work; #ifdef CONFIG_SYSCTL struct ctl_table_set set; diff --git a/security/keys/internal.h b/security/keys/internal.h index a705a7d92ad7..e83e9e6129fc 100644 --- a/security/keys/internal.h +++ b/security/keys/internal.h @@ -18,6 +18,7 @@ #include struct iovec; +struct user_namespace; #ifdef __KDEBUG #define kenter(FMT, ...) \ @@ -137,7 +138,7 @@ extern key_ref_t keyring_search_aux(key_ref_t keyring_ref, extern key_ref_t search_my_process_keyrings(struct keyring_search_context *ctx); extern key_ref_t search_process_keyrings(struct keyring_search_context *ctx); -extern struct key *find_keyring_by_name(const char *name, bool skip_perm_check); +extern struct key *find_keyring_by_name(struct user_namespace *ns, const char *name, bool skip_perm_check); extern int install_user_keyrings(void); extern int install_thread_keyring_to_cred(struct cred *); diff --git a/security/keys/keyring.c b/security/keys/keyring.c index c91e4e0cea08..be1bb5ca1c3a 100644 --- a/security/keys/keyring.c +++ b/security/keys/keyring.c @@ -20,6 +20,7 @@ #include #include #include +#include #include "internal.h" /* @@ -55,7 +56,6 @@ static inline void *keyring_key_to_ptr(struct key *key) return key; } -static struct list_head keyring_name_hash[KEYRING_NAME_HASH_SIZE]; static DEFINE_RWLOCK(keyring_name_lock); static inline unsigned keyring_hash(const char *desc) @@ -106,7 +106,7 @@ static DECLARE_RWSEM(keyring_serialise_link_sem); * Publish the name of a keyring so that it can be found by name (if it has * one). */ -static void keyring_publish_name(struct key *keyring) +static void keyring_publish_name(struct user_namespace *ns, struct key *keyring) { int bucket; @@ -115,11 +115,11 @@ static void keyring_publish_name(struct key *keyring) write_lock(&keyring_name_lock); - if (!keyring_name_hash[bucket].next) - INIT_LIST_HEAD(&keyring_name_hash[bucket]); + if (!ns->keyring_name_hash[bucket].next) + INIT_LIST_HEAD(&ns->keyring_name_hash[bucket]); list_add_tail(&keyring->name_link, - &keyring_name_hash[bucket]); + &ns->keyring_name_hash[bucket]); write_unlock(&keyring_name_lock); } @@ -148,9 +148,11 @@ static void keyring_free_preparse(struct key_preparsed_payload *prep) static int keyring_instantiate(struct key *keyring, struct key_preparsed_payload *prep) { + struct user_namespace *ns = (void *)prep->data; + assoc_array_init(&keyring->keys); /* make the keyring available by name if it has one */ - keyring_publish_name(keyring); + keyring_publish_name(ns, keyring); return 0; } @@ -497,13 +499,14 @@ struct key *keyring_alloc(const char *description, kuid_t uid, kgid_t gid, const union key_payload *), struct key *dest) { + void *data = cred->user_ns; struct key *keyring; int ret; keyring = key_alloc(&key_type_keyring, description, uid, gid, cred, perm, flags, restrict_link); if (!IS_ERR(keyring)) { - ret = key_instantiate_and_link(keyring, NULL, 0, dest, NULL); + ret = key_instantiate_and_link(keyring, data, 0, dest, NULL); if (ret < 0) { key_put(keyring); keyring = ERR_PTR(ret); @@ -997,7 +1000,8 @@ key_ref_t find_key_to_update(key_ref_t keyring_ref, * Returns a pointer to the keyring with the keyring's refcount having being * incremented on success. -ENOKEY is returned if a key could not be found. */ -struct key *find_keyring_by_name(const char *name, bool skip_perm_check) +struct key *find_keyring_by_name(struct user_namespace *ns, + const char *name, bool skip_perm_check) { struct key *keyring; int bucket; @@ -1009,16 +1013,13 @@ struct key *find_keyring_by_name(const char *name, bool skip_perm_check) read_lock(&keyring_name_lock); - if (keyring_name_hash[bucket].next) { + if (ns->keyring_name_hash[bucket].next) { /* search this hash bucket for a keyring with a matching name * that's readable and that hasn't been revoked */ list_for_each_entry(keyring, - &keyring_name_hash[bucket], + &ns->keyring_name_hash[bucket], name_link ) { - if (!kuid_has_mapping(current_user_ns(), keyring->user->uid)) - continue; - if (test_bit(KEY_FLAG_REVOKED, &keyring->flags)) continue; diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index 40a885239782..5f527dcb4e79 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -72,7 +72,7 @@ int install_user_keyrings(void) * may have been destroyed by setuid */ sprintf(buf, "_uid.%u", uid); - uid_keyring = find_keyring_by_name(buf, true); + uid_keyring = find_keyring_by_name(cred->user_ns, buf, true); if (IS_ERR(uid_keyring)) { uid_keyring = keyring_alloc(buf, user->uid, INVALID_GID, cred, user_keyring_perm, @@ -88,7 +88,7 @@ int install_user_keyrings(void) * already) */ sprintf(buf, "_uid_ses.%u", uid); - session_keyring = find_keyring_by_name(buf, true); + session_keyring = find_keyring_by_name(cred->user_ns, buf, true); if (IS_ERR(session_keyring)) { session_keyring = keyring_alloc(buf, user->uid, INVALID_GID, @@ -783,7 +783,7 @@ long join_session_keyring(const char *name) mutex_lock(&key_session_mutex); /* look for an existing keyring of this name */ - keyring = find_keyring_by_name(name, false); + keyring = find_keyring_by_name(old->user_ns, name, false); if (PTR_ERR(keyring) == -ENOKEY) { /* not found - try and create a new one */ keyring = keyring_alloc(