From patchwork Tue Nov 28 19:10:37 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 10080911 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 4F91460353 for ; Tue, 28 Nov 2017 19:11:44 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 44D53295F2 for ; Tue, 28 Nov 2017 19:11:44 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 39ACD295F4; Tue, 28 Nov 2017 19:11: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.8 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,FREEMAIL_FROM,RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=unavailable 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 BCA6E295FD for ; Tue, 28 Nov 2017 19:11:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752857AbdK1TL2 (ORCPT ); Tue, 28 Nov 2017 14:11:28 -0500 Received: from mail-io0-f195.google.com ([209.85.223.195]:46512 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752367AbdK1TL1 (ORCPT ); Tue, 28 Nov 2017 14:11:27 -0500 Received: by mail-io0-f195.google.com with SMTP id x129so981462iod.13; Tue, 28 Nov 2017 11:11:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=iMBrSyuZsUlPerJayBYVG19uVjsen+QgQQtg//ZKFUY=; b=Uo5SRFqRwfZhAglZ9cMmN9pFH62lQuzLkDvxe1zjjdec5ZPeuWeAuCRI5tscRdXP1S n4BkH/O7WnqzEgUiuTAjP+JLj8WzSzY5hW7MMu1j0MMHgZn50SfO4Tzu+7P2yOmfiBr+ BBsBredJUDeByvLqIQcX1pTcisrr1CC/BQN+yv4HZazszMoBizFwv1/kcVWG3hUH08+n d3xe7MJD8Zbr7oRz71r6h1PC9iBtCjn08Jv3kqS2rr5pxiytFtbX6k2XQV7gUfHoZu2t WH6+rWb3A1vaj/ZV0CaGL5L5dMPCVkb241aU0c3B5Lgay2pM1/mkixvPAOfW5of/iXy7 FQ4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=iMBrSyuZsUlPerJayBYVG19uVjsen+QgQQtg//ZKFUY=; b=JWhv53LARAdHJWC3e6fmKQhwQTDC3asFroT0VjooOI3bizRe40tEif8S7UqGHE6ySJ fcNXxaBvdxpWB+LNxM6RhswzpFFjVBvA25i6ApjRaafrLTCvu6GuF6Tl0Njh2nnfTqze YexIIANm7HsPj0edGO/qw/PsK7v3nJmHKHCqMf55OzNLOjvIQ7YoUtVg4BXHQsv9rcjz 9mm9rPHXQmLtAn1qWz5pgF5tht0p7li+S0aLOluaLCcikj5do1oQEHtuGdw1Fw0cCR7+ 2lr60GIbgWMdJ8SknowGCYgAk1gzjvn6lE0gTAYSADqz2HFiDwz63dHPsL0PeXiJkgiK ZScw== X-Gm-Message-State: AJaThX6R4YOKAN+GsoBzkIJrfl3/4juNPtBYSiXvmLSG5E20I7IBbUJV 74UnIjl36eOYX+B7pTPh1zbaqooz X-Google-Smtp-Source: AGs4zMa8UsXOZACRNBrbqvk3IY2DRodqnU7EjsGHPYPL1akOvx+EatHkummy9EaCjhMThVhPEgTivQ== X-Received: by 10.107.79.4 with SMTP id d4mr289746iob.12.1511896286277; Tue, 28 Nov 2017 11:11:26 -0800 (PST) Received: from ebiggers-linuxstation.kir.corp.google.com ([100.66.175.88]) by smtp.gmail.com with ESMTPSA id 81sm104598itl.20.2017.11.28.11.11.25 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 28 Nov 2017 11:11:25 -0800 (PST) From: Eric Biggers To: keyrings@vger.kernel.org, David Howells Cc: linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Eric Biggers , stable@vger.kernel.org Subject: [PATCH v2] KEYS: add missing permission check for request_key() destination Date: Tue, 28 Nov 2017 11:10:37 -0800 Message-Id: <20171128191037.86213-1-ebiggers3@gmail.com> X-Mailer: git-send-email 2.15.0.417.g466bffb3ac-goog Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP From: Eric Biggers When the request_key() syscall is not passed a destination keyring, it links the requested key (if constructed) into the "default" request-key keyring. This should require Write permission to the keyring. However, there is actually no permission check. This can be abused to add keys to any keyring to which only Search permission is granted. This is because Search permission allows joining the keyring. keyctl_set_reqkey_keyring(KEY_REQKEY_DEFL_SESSION_KEYRING) then will set the default request-key keyring to the session keyring. Then, request_key() can be used to add keys to the keyring. Both negatively and positively instantiated keys can be added using this method. Adding negative keys is trivial. Adding a positive key is a bit trickier. It requires that either /sbin/request-key positively instantiates the key, or that another thread adds the key to the process keyring at just the right time, such that request_key() misses it initially but then finds it in construct_alloc_key(). Fix this bug by checking for Write permission to the keyring in construct_get_dest_keyring() when the default keyring is being used. We don't do the permission check for non-default keyrings because that was already done by the earlier call to lookup_user_key(). Also, request_key_and_link() is currently passed a 'struct key *' rather than a key_ref_t, so the "possessed" bit is unavailable. We also don't do the permission check for the "requestor keyring", to continue to support the use case described by commit 8bbf4976b59f ("KEYS: Alter use of key instantiation link-to-keyring argument") where /sbin/request-key recursively calls request_key() to add keys to the original requestor's destination keyring. (I don't know of any users who actually do that, though...) Fixes: 3e30148c3d52 ("[PATCH] Keys: Make request-key create an authorisation key") Cc: # v2.6.13+ Signed-off-by: Eric Biggers --- v2: also skip permission check if default dest_keyring is NULL security/keys/request_key.c | 46 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/security/keys/request_key.c b/security/keys/request_key.c index e8036cd0ad54..7dc741382154 100644 --- a/security/keys/request_key.c +++ b/security/keys/request_key.c @@ -251,11 +251,12 @@ static int construct_key(struct key *key, const void *callout_info, * The keyring selected is returned with an extra reference upon it which the * caller must release. */ -static void construct_get_dest_keyring(struct key **_dest_keyring) +static int construct_get_dest_keyring(struct key **_dest_keyring) { struct request_key_auth *rka; const struct cred *cred = current_cred(); struct key *dest_keyring = *_dest_keyring, *authkey; + int ret; kenter("%p", dest_keyring); @@ -264,6 +265,8 @@ static void construct_get_dest_keyring(struct key **_dest_keyring) /* the caller supplied one */ key_get(dest_keyring); } else { + bool do_perm_check = true; + /* use a default keyring; falling through the cases until we * find one that we actually have */ switch (cred->jit_keyring) { @@ -278,8 +281,10 @@ static void construct_get_dest_keyring(struct key **_dest_keyring) dest_keyring = key_get(rka->dest_keyring); up_read(&authkey->sem); - if (dest_keyring) + if (dest_keyring) { + do_perm_check = false; break; + } } case KEY_REQKEY_DEFL_THREAD_KEYRING: @@ -314,11 +319,29 @@ static void construct_get_dest_keyring(struct key **_dest_keyring) default: BUG(); } + + /* + * Require Write permission on the keyring. This is essential + * because the default keyring may be the session keyring, and + * joining a keyring only requires Search permission. + * + * However, this check is skipped for the "requestor keyring" so + * that /sbin/request-key can itself use request_key() to add + * keys to the original requestor's destination keyring. + */ + if (dest_keyring && do_perm_check) { + ret = key_permission(make_key_ref(dest_keyring, 1), + KEY_NEED_WRITE); + if (ret) { + key_put(dest_keyring); + return ret; + } + } } *_dest_keyring = dest_keyring; kleave(" [dk %d]", key_serial(dest_keyring)); - return; + return 0; } /* @@ -444,11 +467,15 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx, if (ctx->index_key.type == &key_type_keyring) return ERR_PTR(-EPERM); - user = key_user_lookup(current_fsuid()); - if (!user) - return ERR_PTR(-ENOMEM); + ret = construct_get_dest_keyring(&dest_keyring); + if (ret) + goto error; - construct_get_dest_keyring(&dest_keyring); + user = key_user_lookup(current_fsuid()); + if (!user) { + ret = -ENOMEM; + goto error_put_dest_keyring; + } ret = construct_alloc_key(ctx, dest_keyring, flags, user, &key); key_user_put(user); @@ -463,7 +490,7 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx, } else if (ret == -EINPROGRESS) { ret = 0; } else { - goto couldnt_alloc_key; + goto error_put_dest_keyring; } key_put(dest_keyring); @@ -473,8 +500,9 @@ static struct key *construct_key_and_link(struct keyring_search_context *ctx, construction_failed: key_negate_and_link(key, key_negative_timeout, NULL, NULL); key_put(key); -couldnt_alloc_key: +error_put_dest_keyring: key_put(dest_keyring); +error: kleave(" = %d", ret); return ERR_PTR(ret); }