From patchwork Thu Oct 27 16:18:13 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: 9399865 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 1558560231 for ; Thu, 27 Oct 2016 16:21:00 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0468A2A358 for ; Thu, 27 Oct 2016 16:21:00 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id ED7412A35B; Thu, 27 Oct 2016 16:20: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=-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 654F62A358 for ; Thu, 27 Oct 2016 16:20:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755429AbcJ0QU6 (ORCPT ); Thu, 27 Oct 2016 12:20:58 -0400 Received: from out01.mta.xmission.com ([166.70.13.231]:60618 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755186AbcJ0QU5 (ORCPT ); Thu, 27 Oct 2016 12:20:57 -0400 Received: from in02.mta.xmission.com ([166.70.13.52]) by out01.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1bznQ5-00019N-A4; Thu, 27 Oct 2016 10:20:45 -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 1bznQ3-0001Ge-Qh; Thu, 27 Oct 2016 10:20:45 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: David Howells Cc: Jann Horn , Andy Lutomirski , James Bottomley , Linux Containers , Oleg Nesterov , Eric Paris , LSM List , keyrings@vger.kernel.org, simo@redhat.com References: <87mvhrrng3.fsf@xmission.com> <20161026143856.GL3334@pc.thejh.net> <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> <9243.1477492490@warthog.procyon.org.uk> <9610.1477493338@warthog.procyon.org.uk> <3677.1477506925@warthog.procyon.org.uk> Date: Thu, 27 Oct 2016 11:18:13 -0500 In-Reply-To: <3677.1477506925@warthog.procyon.org.uk> (David Howells's message of "Wed, 26 Oct 2016 19:35:25 +0100") Message-ID: <87twbxojei.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 X-XM-SPF: eid=1bznQ3-0001Ge-Qh; ; ; mid=<87twbxojei.fsf@xmission.com>; ; ; hst=in02.mta.xmission.com; ; ; ip=75.170.125.99; ; ; frm=ebiederm@xmission.com; ; ; spf=neutral X-XM-AID: U2FsdGVkX1+Xj/aotl7l72MRGZbMoV7LvaYI6ycLhkw= 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: > Eric W. Biederman wrote: > >> > Jann Horn wrote: >> > >> >> find_keyring_by_name() checks that the UID of the keyring's owner is >> >> mapped into the current user namespace. But that doesn't catch the >> >> scenario I described: The keyring is created in an attacker-created >> >> namespace and looked up from the init namespace, into which all kuids are >> >> mapped. >> > >> > Ah - gotcha. >> >> Unless I am misreading something it actually gets worse. You don't even >> need a user namespace. You can just call keyctl_join_session_keyring >> and the named keyring of your choice will be created. > > It's meant to do that - assuming a keyring of that name doesn't yet exist or > isn't accessible by you. > > I should perhaps prohibit the creation of a _uid.N keyring. However, I think > that conceptually, it should be possible to join the user-session keyring as > your session - or I should make it so that the user-session keyring doesn't > exist (but I'm pretty certain that will break userspace). >> Plus there are various really weird things in their where the keyring >> names of _tid, _pid, _ses, get reused over and over again. > > True, however per-thread (_tid) and per-process(_pid) keyrings are always > allocated by key_alloc() and never looked up by name when being created. > > Anonymous session (_ses) keyrings are also created by key_alloc() and not > looked up when created. It's only when a named session keyring is requested > that a look up by name is done. > > I could make the per-thread, per-process and anon-session keyrings nameless by > default, or prefix them with '.' and not permit joining of a keyring whose > name begins with a '.' (you aren't allowed to use add_key() to create a such > keyrings, so that really ought to be extended to here too). > >> So it looks like there are some significant things to fix. > > So: > > (1) Make the keyring name list per user_namespace. > > (2) Don't let _uid.N, _tid, _pid and _ses (anonymous session) keyrings be > joined. > > (3) Don't let keyrings whose name begins '.' be joined. > > (4) Don't keep the user and user-session keyrings in the user_struct, but > rather make them subordinates of the user namespace. > > for starters. I would start with 2 and 3. Then as a band-aid I would tweak join keyctl_join_session_keyring to do something like: if (memcmp(name, "_uid_ses.", 9) == 0) { unsigned long uid; kuid_t kuid; uid = strtoul(name, name, 10); kuid = make_kuid(current_user_ns(), uid); if (!valid_uid(kuid)) return -EINVAL; uid = from_kuid(&init_user_ns, kuid); snprintf(name + 9, sizeof(*name), "%u", uid); } } That allows the uid and uid_session keyring silliness to be fixed with just: I am quite certain that I only coded it with the name using cred->user_ns and not &init_user_ns is so that in a user namespace the lookup up from keyctl_join_sesson_keyring would continue to work. And changing the name we are looking up works just as well. Then we can discuss changing the semantics to the uid and uid session keyrings that live in the uid_struct. I don't think I have yet seen a clear justification for the change in semantics. I have just seen it stated that it is desired to change the semantics that way. I think the change in semantics needs to be it's own conversation. With the added bonus that all of the fixes I have outlined are simple enough they should not be problematic to backport as fixes. 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/security/keys/process_keys.c b/security/keys/process_keys.c index 40a885239782..37f787716575 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -53,7 +53,7 @@ int install_user_keyrings(void) user_keyring_perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_ALL; cred = current_cred(); user = cred->user; - uid = from_kuid(cred->user_ns, user->uid); + uid = from_kuid(&init_user_ns, user->uid); kenter("%p{%u}", user, uid);