Re: [PATCH] crypto: af_alg - implement keyring support

[Ondrej Mosnacek <omosnace@redhat.com>]

On Sat, May 25, 2019 at 5:10 AM Eric Biggers <ebiggers@kernel.org> wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> > This patch adds new socket options to AF_ALG that allow setting key from
> > kernel keyring. For simplicity, each keyring key type (logon, user,
> > trusted, encrypted) has its own socket option name and the value is just
> > the key description string that identifies the key to be used. The key
> > description doesn't need to be NULL-terminated, but bytes after the
> > first zero byte are ignored.
> >
> > Note that this patch also adds three socket option names that are
> > already defined and used in libkcapi [1], but have never been added to
> > the kernel...
> >
> > Tested via libkcapi with keyring patches [2] applied (user and logon key
> > types only).
> >
> > [1] https://github.com/smuellerDD/libkcapi
> > [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> The "interesting" thing about this is that given a key to which you have only
> Search permission, you can request plaintext-ciphertext pairs with it using any
> algorithm from the kernel's crypto API.  So if there are any really broken
> algorithms and they happen to take the correct length key, you can effectively
> read the key.  That's true even if you don't have Read permission on the key
> and/or the key is of the "logon" type which doesn't have a ->read() method.

Well, initially I was looking for a "KEY_NEED_USE" permission that
would allow using the key for encryption/decryption, but not to
actually read it. But then I was told by some people that the
KEY_NEED_SEARCH permission already serves exactly this purpose (i.e.
that when you can find the key, it means you can use it). I would
imagine that any practical use case for trusted keys would involve
encrypting/decrypting some data with the key (maybe not flowing
directly from/to userspace, but what use is a key with which you can
encrypt only some "internal" data...?), so I'm not sure where we want
to draw the boundary of what is safe to do with (userspace-unreadable
but findable) keyring keys... Maybe the keyring API needs some way to
control the intended usage of each key (something a bit like the "key
usage" in X.509 certificates [1]) - so you can e.g. mark some key to
be used for XYZ, but not for AF_ALG or dm-crypt...

Either way, I agree that this functionality opens up a potential
security hole (in that it makes it much more likely that a
vulnerability in the crypto drivers or crypto algorithms themselves
can reveal the value of a key that is not supposed to be readable by
userspace). However, I'm not sure how to mitigate this without some
new "KEY_NEED_PROCESS_ARBITRARY_DATA" permission or something... For
now I can at least add a Kconfig option to enable/disable keyring
support in AF_ALG so that people/distros who want both keyring and
AF_ALG enabled, but do not want to expose keyring keys via AF_ALG, can
just disable it.

BTW, I'm still undecided if I should convert this patch to use key IDs
rather than descriptions, but I tend to prefer to stay with the
current approach (mainly because it would be a lot of effort to
rewrite everything :)

[1] https://tools.ietf.org/html/rfc5280#section-4.2.1.3




--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.