From patchwork Mon Sep 25 19:11:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dimitri John Ledkov X-Patchwork-Id: 818621 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=surgut.co.uk header.i=@surgut.co.uk header.b="AbBq2SmH"; dkim-atps=neutral Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id 3y1g7X6dFNz9tXK; Tue, 26 Sep 2017 22:22:16 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1dwosK-0004d9-7G; Tue, 26 Sep 2017 12:22:08 +0000 Received: from mail-wm0-f51.google.com ([74.125.82.51]) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1dwYma-00069G-T1 for kernel-team@lists.ubuntu.com; Mon, 25 Sep 2017 19:11:09 +0000 Received: by mail-wm0-f51.google.com with SMTP id r74so621048wme.4 for ; Mon, 25 Sep 2017 12:11:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=surgut.co.uk; s=google; h=sender:from:to:cc:subject:date:message-id; bh=2EWON3AhtQDpKYUADbuwu/+dHEoBLkOB3rbCqdtWYMo=; b=AbBq2SmH6K9XkyiOsRtMnXA3196I7ACRsx/050r3WTYC3caysq6EZPGXRfJvEiMRkt mNccxIEgbDpGQHA80datFOhBAluu6oMTRZzzIo22R5SdIim6GwHXAsKuB82xQ5j52V5R 6eMv1nGRNE1LphcnnlJxyINeJL5Zt2S3+bgKA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id; bh=2EWON3AhtQDpKYUADbuwu/+dHEoBLkOB3rbCqdtWYMo=; b=JoqBVJYAS/01BxP3xh0WPvh+WzW5auTpGyg3tt1vh+Xijk7WVJamw36U2mMU3vWdrK sDxqyfPUayh+wKegcl4h32uxUciYDr6ECmXgx/vG0udWTRi5igQKC2yju1LNS1ekreV6 aHqIoF20lbcrHWTmuoqOiPqcXR1DzlHYYQpb8AGsWFJY6EOsgiiNZVEbFmHKR9NTmnsz NHW3+u3dSPWbJy55BuJucnrQRfHUEuRP77fEyKh5eRRhn89o4I3jM6fgTqzXfrYWZiXW rGszJg4Zshg/bPJskoOQH6/gwOA4se4ToFHSWIImV3FMhxuMi2CYvX1fCAtpdHQwxb7Y ZOeQ== X-Gm-Message-State: AHPjjUh+WB1E6B/LX7QZg+X05QY84yteYjxgGibeMCvpH+gWHmbjODtS R6+qmIyagehJMLDux/fCNQGjmQEznFA= X-Google-Smtp-Source: AOwi7QAbt/P6WzzA4clWuyoL7zUsUMWyMfFVjE1ogf9vNvByIUuBjyGZbJx27OrFp1C7n+/Ksk4MgA== X-Received: by 10.28.214.206 with SMTP id n197mr1100652wmg.21.1506366667961; Mon, 25 Sep 2017 12:11:07 -0700 (PDT) Received: from localhost ([82.129.99.182]) by smtp.gmail.com with ESMTPSA id x5sm3876021wre.18.2017.09.25.12.11.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 25 Sep 2017 12:11:07 -0700 (PDT) From: Dimitri John Ledkov To: kernel-team@lists.ubuntu.com Subject: [PATCH] KEYS: allow changing key ownership with CAP_SYS_ADMIN in a NS Date: Mon, 25 Sep 2017 20:11:04 +0100 Message-Id: <1506366664-4557-1-git-send-email-xnox@ubuntu.com> X-Mailer: git-send-email 2.7.4 X-Mailman-Approved-At: Tue, 26 Sep 2017 12:22:06 +0000 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Dimitri John Ledkov MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" Currently, changing key ownership from one namespaced uid/gid to another namespaced uid/gid is only allowed by processes that have CAP_SYS_ADMIN in the intial namespace. Fix the capability check to also check the capability in the current capability. Fixes: https://github.com/systemd/systemd/pull/6876 Signed-off-by: Dimitri John Ledkov --- Dear Ubuntu Kernel Team, I am consider to submit this patch upstream. Could you please review this patch, before I do so? I've generated the patch with the full function context, to point out that it does make_kuid/make_kgid using current_user_ns() but not the capability check. Which imho is silly. Regards, Dimitri. security/keys/keyctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index ab0b337c84b4..dc554bb80325 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -822,65 +822,65 @@ long keyctl_chown_key(key_serial_t id, uid_t user, gid_t group) struct key_user *newowner, *zapowner = NULL; struct key *key; key_ref_t key_ref; long ret; kuid_t uid; kgid_t gid; uid = make_kuid(current_user_ns(), user); gid = make_kgid(current_user_ns(), group); ret = -EINVAL; if ((user != (uid_t) -1) && !uid_valid(uid)) goto error; if ((group != (gid_t) -1) && !gid_valid(gid)) goto error; ret = 0; if (user == (uid_t) -1 && group == (gid_t) -1) goto error; key_ref = lookup_user_key(id, KEY_LOOKUP_CREATE | KEY_LOOKUP_PARTIAL, KEY_NEED_SETATTR); if (IS_ERR(key_ref)) { ret = PTR_ERR(key_ref); goto error; } key = key_ref_to_ptr(key_ref); /* make the changes with the locks held to prevent chown/chown races */ ret = -EACCES; down_write(&key->sem); - if (!capable(CAP_SYS_ADMIN)) { + if (!ns_capable(current_user_ns(), CAP_SYS_ADMIN)) { /* only the sysadmin can chown a key to some other UID */ if (user != (uid_t) -1 && !uid_eq(key->uid, uid)) goto error_put; /* only the sysadmin can set the key's GID to a group other * than one of those that the current process subscribes to */ if (group != (gid_t) -1 && !gid_eq(gid, key->gid) && !in_group_p(gid)) goto error_put; } /* change the UID */ if (user != (uid_t) -1 && !uid_eq(uid, key->uid)) { ret = -ENOMEM; newowner = key_user_lookup(uid); if (!newowner) goto error_put; /* transfer the quota burden to the new user */ if (test_bit(KEY_FLAG_IN_QUOTA, &key->flags)) { unsigned maxkeys = uid_eq(uid, GLOBAL_ROOT_UID) ? key_quota_root_maxkeys : key_quota_maxkeys; unsigned maxbytes = uid_eq(uid, GLOBAL_ROOT_UID) ? key_quota_root_maxbytes : key_quota_maxbytes; spin_lock(&newowner->lock); if (newowner->qnkeys + 1 >= maxkeys || newowner->qnbytes + key->quotalen >= maxbytes || newowner->qnbytes + key->quotalen < newowner->qnbytes) goto quota_overrun; newowner->qnkeys++;