Date: Mon, 18 Dec 2017 13:06:55 +0100
From: Greg KH <gregkh@linuxfoundation.org>
To: Eric Biggers <ebiggers3@gmail.com>
Cc: stable@vger.kernel.org, Eric Biggers <ebiggers@google.com>,
        David Howells <dhowells@redhat.com>
Subject: Re: [PATCH 2/2 for 3.18/4.1] KEYS: add missing permission check for
 request_key() destination
Message-ID: <20171218120655.GC31295@kroah.com>
References: <20171215204418.130486-1-ebiggers3@gmail.com>
 <20171215204418.130486-2-ebiggers3@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171215204418.130486-2-ebiggers3@gmail.com>
Sender: stable-owner@vger.kernel.org

On Fri, Dec 15, 2017 at 12:44:18PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> commit 4dca6ea1d9432052afb06baf2e3ae78188a4410b upstream.
> [Please apply to 3.18-stable and 4.1-stable.]
> 
> 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")
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>  security/keys/request_key.c | 48 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 10 deletions(-)
> 

Thanks for the patch, but this was already in the 3.18.88 release.

greg k-h