From patchwork Wed Sep 27 19:50:41 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 9974657 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 5899E60365 for ; Wed, 27 Sep 2017 19:53:13 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 49D2D28ECA for ; Wed, 27 Sep 2017 19:53:13 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3E6DE2930D; Wed, 27 Sep 2017 19:53:13 +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 E79F22931B for ; Wed, 27 Sep 2017 19:53:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751998AbdI0TvP (ORCPT ); Wed, 27 Sep 2017 15:51:15 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:34138 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751136AbdI0TvL (ORCPT ); Wed, 27 Sep 2017 15:51:11 -0400 Received: by mail-pf0-f196.google.com with SMTP id g65so7597921pfe.1; Wed, 27 Sep 2017 12:51:10 -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:in-reply-to:references; bh=9ExQugm3BkEKimGzeyp+UmyhxfiRZ2dtYnbYh0yfrjI=; b=S/q5OwaI5qT8yEuT54yang/rJKvl7Au1QkhNrRYcEikZlnp2r0CChmLCMM6Q4tFDDe gBZjGZxSG7GbBdIzdSTA71gneEolDtivniKI2ZX8FXgNrd5VF1abZU3P7jlaBN+am3a+ Fdof17Zj7v+E7vZtUL/kRz/5fgo6QLIPsqlaowc4nIHTCWN0aoLY+zQAvRCFv3gPTCqq XC6Ik7un5xd+RJdIlIupvrwk8MIfEjmcLxKTRuE3CQjszFueX63bLs8rNh5pl2v7Cy5e XHt6xYUtI2qbNct69+IdyFdFTj46RHts9Rrw/lfjmeSw1prUy/Z84ftRZdoLwTI0IRFZ q9dQ== 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:in-reply-to :references; bh=9ExQugm3BkEKimGzeyp+UmyhxfiRZ2dtYnbYh0yfrjI=; b=iAgHFesTwsDX9oUb0Ucr4Gw5NNLtJRApTmcAASHBIwk15edOsftP5Ru52QWVCuVarD nB00lXnWCKwT/7zddAE52icX51PBWMe03MS5Tz3VSbfaFjgmMqXuw4u3ykGP8m1v02On bUlJ7bCfgmSMDss27JMZZTwb4/8V/P02z0vtYxBscxVFJfxEMM28ZsOSvKoVFou0JjAR mwaMmCo3NPtCWRY4QjnGFyM6I6uQ/yGDsEg50pcXiPFn8Aaih6sS3rUU16ZbXBMPm24W hyVpHss8jgpbKjSG5xNgXX3tQW2fiWmFRO9ilhEfkp61D6DFMQ2XcxYR80/OBZNm7E5W UscA== X-Gm-Message-State: AHPjjUi879E9wNaEXpA4935KdPdI8avEYx9urRSaHUyBD/yreBlFICUY 5fn0qQCEz/rDqvpgpPqFGhdf3yCA X-Google-Smtp-Source: AOwi7QBDFbchTMfgWEdzjGPX4EomrzR01vdhVd69gZgLLT2mKq/pFuP7LJ9u7sYDYnX9Z+TNfsiTBw== X-Received: by 10.98.67.134 with SMTP id l6mr2248519pfi.165.1506541870004; Wed, 27 Sep 2017 12:51:10 -0700 (PDT) Received: from ebiggers-linuxstation.kir.corp.google.com ([100.66.174.81]) by smtp.gmail.com with ESMTPSA id d5sm24954499pfg.26.2017.09.27.12.51.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 27 Sep 2017 12:51:09 -0700 (PDT) From: Eric Biggers To: keyrings@vger.kernel.org Cc: David Howells , Michael Halcrow , linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, Eric Biggers , stable@vger.kernel.org Subject: [PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key Date: Wed, 27 Sep 2017 12:50:41 -0700 Message-Id: <20170927195047.122358-2-ebiggers3@gmail.com> X-Mailer: git-send-email 2.14.2.822.g60be5d43e6-goog In-Reply-To: <20170927195047.122358-1-ebiggers3@gmail.com> References: <20170927195047.122358-1-ebiggers3@gmail.com> Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP From: Eric Biggers Currently, add_key() will, when passed a key that already exists, call the key's ->update() method. But this is heavily broken in the case where the key is uninstantiated because it doesn't call __key_instantiate_and_link(). Consequently, it doesn't do most of the things that are supposed to happen when the key is instantiated, such as setting KEY_FLAG_INSTANTIATED, clearing KEY_FLAG_USER_CONSTRUCT and awakening tasks waiting on it, and incrementing key->user->nikeys. It also never takes key_construction_mutex, which means that ->instantiate() can run concurrently with ->update() on the same key. In the case of the "user" and "logon" key types this causes a memory leak, at best. Maybe even worse, the ->update() methods of the "encrypted" and "trusted" key types actually just dereference a NULL pointer when passed an uninstantiated key. Therefore, change find_key_to_update() to return NULL if the found key is uninstantiated, so that add_key() replaces the key rather than instantiating it. This seems to be better than fixing __key_update() to call __key_instantiate_and_link(), since given all the bugs noted above as well as that the existing behavior was undocumented and keyctl_instantiate() is supposed to be used instead, I doubt anyone was relying on the existing behavior. This patch only affects *uninstantiated* keys. For now we still allow a negatively instantiated key to be updated (thereby positively instantiating it), although that's broken too (the next patch fixes it) and I'm not sure that anyone actually uses that functionality either. Here is a simple reproducer for the bug using the "encrypted" key type (requires CONFIG_ENCRYPTED_KEYS=y), though as noted above the bug pertained to more than just the "encrypted" key type: #include #include #include int main(void) { int ringid = keyctl_join_session_keyring(NULL); if (fork()) { for (;;) { const char payload[] = "update user:foo 32"; usleep(rand() % 10000); add_key("encrypted", "desc", payload, sizeof(payload), ringid); keyctl_clear(ringid); } } else { for (;;) request_key("encrypted", "desc", "callout_info", ringid); } } It causes: BUG: unable to handle kernel NULL pointer dereference at 0000000000000018 IP: encrypted_update+0xb0/0x170 PGD 7a178067 P4D 7a178067 PUD 77269067 PMD 0 PREEMPT SMP CPU: 0 PID: 340 Comm: reproduce Tainted: G D 4.14.0-rc1-00025-g428490e38b2e #796 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 task: ffff8a467a39a340 task.stack: ffffb15c40770000 RIP: 0010:encrypted_update+0xb0/0x170 RSP: 0018:ffffb15c40773de8 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff8a467a275b00 RCX: 0000000000000000 RDX: 0000000000000005 RSI: ffff8a467a275b14 RDI: ffffffffb742f303 RBP: ffffb15c40773e20 R08: 0000000000000000 R09: ffff8a467a275b17 R10: 0000000000000020 R11: 0000000000000000 R12: 0000000000000000 R13: 0000000000000000 R14: ffff8a4677057180 R15: ffff8a467a275b0f FS: 00007f5d7fb08700(0000) GS:ffff8a467f200000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000018 CR3: 0000000077262005 CR4: 00000000001606f0 Call Trace: key_create_or_update+0x2bc/0x460 SyS_add_key+0x10c/0x1d0 entry_SYSCALL_64_fastpath+0x1f/0xbe RIP: 0033:0x7f5d7f211259 RSP: 002b:00007ffed03904c8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f8 RAX: ffffffffffffffda RBX: 000000003b2a7955 RCX: 00007f5d7f211259 RDX: 00000000004009e4 RSI: 00000000004009ff RDI: 0000000000400a04 RBP: 0000000068db8bad R08: 000000003b2a7955 R09: 0000000000000004 R10: 000000000000001a R11: 0000000000000246 R12: 0000000000400868 R13: 00007ffed03905d0 R14: 0000000000000000 R15: 0000000000000000 Code: 77 28 e8 64 34 1f 00 45 31 c0 31 c9 48 8d 55 c8 48 89 df 48 8d 75 d0 e8 ff f9 ff ff 85 c0 41 89 c4 0f 88 84 00 00 00 4c 8b 7d c8 <49> 8b 75 18 4c 89 ff e8 24 f8 ff ff 85 c0 41 89 c4 78 6d 49 8b RIP: encrypted_update+0xb0/0x170 RSP: ffffb15c40773de8 CR2: 0000000000000018 Cc: [v2.6.12+] Signed-off-by: Eric Biggers --- security/keys/keyring.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/security/keys/keyring.c b/security/keys/keyring.c index 4fa82a8a9c0e..129a4175760b 100644 --- a/security/keys/keyring.c +++ b/security/keys/keyring.c @@ -1056,8 +1056,8 @@ EXPORT_SYMBOL(keyring_restrict); * caller must also hold a lock on the keyring semaphore. * * Returns a pointer to the found key with usage count incremented if - * successful and returns NULL if not found. Revoked and invalidated keys are - * skipped over. + * successful and returns NULL if not found. Revoked, invalidated, and + * uninstantiated keys are skipped over. (But negative keys are not!) * * If successful, the possession indicator is propagated from the keyring ref * to the returned key reference. @@ -1084,8 +1084,10 @@ key_ref_t find_key_to_update(key_ref_t keyring_ref, found: key = keyring_ptr_to_key(object); - if (key->flags & ((1 << KEY_FLAG_INVALIDATED) | - (1 << KEY_FLAG_REVOKED))) { + if ((key->flags & ((1 << KEY_FLAG_INVALIDATED) | + (1 << KEY_FLAG_REVOKED) | + (1 << KEY_FLAG_INSTANTIATED))) != + (1 << KEY_FLAG_INSTANTIATED)) { kleave(" = NULL [x]"); return NULL; }