From patchwork Wed Aug 16 21:14:03 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 9904613 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 540B160244 for ; Wed, 16 Aug 2017 21:17:21 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 461E32892D for ; Wed, 16 Aug 2017 21:17:21 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3AB1328A53; Wed, 16 Aug 2017 21:17:21 +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.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, 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 0904E2892D for ; Wed, 16 Aug 2017 21:17:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752154AbdHPVRT (ORCPT ); Wed, 16 Aug 2017 17:17:19 -0400 Received: from mail-pg0-f67.google.com ([74.125.83.67]:38876 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752105AbdHPVRR (ORCPT ); Wed, 16 Aug 2017 17:17:17 -0400 Received: by mail-pg0-f67.google.com with SMTP id 123so6399545pga.5; Wed, 16 Aug 2017 14:17:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=fpZWgFrH3RSzc34mvou5IPzU/zqhMz4W++ZBcNV5Poc=; b=SUSM+3NBa/nrWagOS5OpkHuTqTkfnvljHgARxaGjR+TYw0Vpsh1IqoTe7/4tY82PM2 j2R8kqmS8p1Awz/c8c5nPj+Htu7sICPTXZkcV25gQ84X49Hh7o8D7w2wC74jRdOTrWEh VNs81vwhGweoNJgkkdUA//eo12GqjFTwULxoLePPaP2aewmuOm14ukkERUgAOTsOGLWp ToXsG6pYfcU5so7zlrhe+ldQ4lhBX6ojp5YGJI9JyM/3VoCnS+p6GPN36RdoErC559p2 pt0xFL/6kg4LELPIRjPapk40n1jcy702llqMq9cv8aYAloq8TGtUcgmhqcRkKPdNgd+0 MyXQ== 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=fpZWgFrH3RSzc34mvou5IPzU/zqhMz4W++ZBcNV5Poc=; b=TiZrpooZR4KmdjBuG+bYDScZBNs3HMsOVYCSI0g2Rxb+isFqFhHj+prYRMK1tCc4jK Yu9e5VUrZ43gzS8l3hHGnOvdrEwuVLSdw4EB4HQfWINGH69s0NulPVYeWw1P/Ipxkimv 15wMKgGMpoH8/75vAsOYAfv2oHdjcfBNQPEWoGZI6BaTGzMCdy1kjTInzRhtn8aSICEc 434+Gd8XxJbx4NDSZ2pLP6BoFJPD5Wdbp7wFcm+1Iy+RNag4DartunaU/DfnY4aNTXo7 vXrjJa5WvBYN7NQ0Vk0oeimZG6vDhFoQohzgwVy6qEnJrQZ52RAUxQeEo1ixbLuoVOhL pfOg== X-Gm-Message-State: AHYfb5gC51uI3CK8W7lUzgUNvZPfNNXJhwEO7FvuuqL6WoX8fHtqjp7W SgfAOiql102TJp87N/PIkA== X-Received: by 10.84.210.203 with SMTP id a69mr3202817pli.395.1502918236343; Wed, 16 Aug 2017 14:17:16 -0700 (PDT) Received: from ebiggers-linuxstation.kir.corp.google.com ([100.66.174.81]) by smtp.gmail.com with ESMTPSA id e189sm3955531pfa.44.2017.08.16.14.17.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 16 Aug 2017 14:17:15 -0700 (PDT) From: Eric Biggers To: keyrings@vger.kernel.org Cc: David Howells , linux-api@vger.kernel.org, linux-cifs@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-nfs@vger.kernel.org, Gwendal Grignou , Jaegeuk Kim , Paul Crowley , Richard Weinberger , Ryo Hashimoto , Tyler Hicks , stable@vger.kernel.org Subject: [PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission Date: Wed, 16 Aug 2017 14:14:03 -0700 Message-Id: <20170816211403.121920-1-ebiggers3@gmail.com> X-Mailer: git-send-email 2.14.1.480.gb18f417b89-goog Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Eric Biggers Currently, a process only needs Search permission on a key to invalidate it with keyctl_invalidate(), which has an effect similar to unlinking it from all keyrings. Unfortunately, this significantly compromises the keys permission system because as a result, there is no way to grant read-only access to keys without also granting de-facto "delete" access. Even worse, processes may invalidate entire _keyrings_, given only permission to "search" them. It is not even possible to block this using SELinux, because SELinux is likewise only asked for Search permission, which needs to be allowed for read-only access. Key invalidation seems to have intended for cases where keyrings are used as caches, and keys can be re-requested at any time by an upcall to /sbin/request-key. However, keyrings are not always used in this way. For example, users of filesystem-level encryption (EXT4, F2FS, and/or UBIFS encryption) usually wish to grant many differently-privileged processes access to the same keys, set up in a shared keyring ahead of time. Permitting everyone to invalidate keys in this scenario enables a trivial denial-of-service. And even users of keyrings as "just caches" may wish to restrict invalidation because it may require significant work or user interaction to regenerate keys. This patch fixes the flaw by requiring both Search and Setattr permission to invalidate a key rather than just Search permission. Requiring Setattr permission is appropriate because Setattr permission also allows revoking (via keyctl_revoke()) or expiring (via keyctl_set_timeout()) the key, which also make the key inaccessible regardless of how many keyrings it is in. Continuing to require Search permission ensures we do not decrease the level of permissions required. Alternatively, the problem could be solved by requiring Write permission. However, that would be less appropriate because Write permission is ostensibly meant to deal with the key payload, not the key itself. A new "Invalidate" permission would also work, but that would require keyctl_setperm() users to start including a new permission which never existed before, which would be much more likely to break users of keyctl_invalidate(). Finally, we could only allow invalidating keys which the kernel has explicitly marked as invalidatible, e.g. the "id_resolver" keys created for NFSv4 ID mapping. However, that likewise would be much more likely to break users. This is a user-visible API change. But we don't seem to have much choice, and any breakage will be limited to users who override the default key permissions using keyctl_setperm() to remove Setattr permission, then later call keyctl_invalidate(). There are probably not many such users, possibly even none at all. In fact, the only users of keyctl_invalidate() I could find are nfsidmap and the Chromium OS cryptohome daemon. nfsidmap will be unaffected because it runs as root and actually relies on the "root is permitted to invalidate certain special keys" exception (KEY_FLAG_ROOT_CAN_INVAL) rather than the actual key permissions. cryptohomed will be unaffected because it already owns the keys and sets KEY_USR_SETATTR permission on them. Cc: linux-api@vger.kernel.org Cc: linux-cifs@vger.kernel.org Cc: linux-fscrypt@vger.kernel.org Cc: linux-nfs@vger.kernel.org Cc: Gwendal Grignou Cc: Jaegeuk Kim Cc: Paul Crowley Cc: Richard Weinberger Cc: Ryo Hashimoto Cc: Tyler Hicks Cc: stable@vger.kernel.org # v3.5+ Signed-off-by: Eric Biggers --- Documentation/security/keys/core.rst | 4 ++-- security/keys/keyctl.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst index 1648fa80b3bf..c8030f628810 100644 --- a/Documentation/security/keys/core.rst +++ b/Documentation/security/keys/core.rst @@ -815,8 +815,8 @@ The keyctl syscall functions are: immediately, though they are still visible in /proc/keys until deleted (they're marked with an 'i' flag). - A process must have search permission on the key for this function to be - successful. + A process must have Search and Setattr permission on the key for this + function to be successful. * Compute a Diffie-Hellman shared secret or public key:: diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index ab0b337c84b4..0739e7934e74 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -400,7 +400,7 @@ long keyctl_revoke_key(key_serial_t id) /* * Invalidate a key. * - * The key must be grant the caller Invalidate permission for this to work. + * The key must grant the caller Search and Setattr permission for this to work. * The key and any links to the key will be automatically garbage collected * immediately. * @@ -416,7 +416,7 @@ long keyctl_invalidate_key(key_serial_t id) kenter("%d", id); - key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH); + key_ref = lookup_user_key(id, 0, KEY_NEED_SEARCH | KEY_NEED_SETATTR); if (IS_ERR(key_ref)) { ret = PTR_ERR(key_ref);