Hi Eric,
On 10/11/22 6:07 PM, Eric Biggers wrote:
Hi Frederick,
On Tue, Oct 04, 2022 at 04:29:27PM -0500, Frederick Lawler wrote:
We want to leverage keyring to store sensitive keys, and then use those
keys for symmetric encryption via the crypto API. Among the key types we
wish to support are: user, logon, encrypted, and trusted.
User key types are already able to have their data copied to user space,
but logon does not support this. Further, trusted and encrypted keys will
return their encrypted data back to user space on read, which make them not
ideal for symmetric encryption.
To support symmetric encryption for these key types, add a new
ALG_SET_KEY_BY_KEY_SERIAL setsockopt() option to the crypto API. This
allows users to pass a key_serial_t to the crypto API to perform
symmetric encryption. The behavior is the same as ALG_SET_KEY, but
the crypto key data is copied in kernel space from a keyring key,
which allows for the support of logon, encrypted, and trusted key types.
Keyring keys must have the KEY_(POS|USR|GRP|OTH)_SEARCH permission set
to leverage this feature. This follows the asymmetric_key type where key
lookup calls eventually lead to keyring_search_rcu() without the
KEYRING_SEARCH_NO_CHECK_PERM flag set.
Signed-off-by: Frederick Lawler <fred@xxxxxxxxxxxxxx>
There was a similar patch several years ago by Ondrej Mosnacek:
https://lore.kernel.org/linux-crypto/20190521100034.9651-1-omosnace@xxxxxxxxxx/T/#u
Have you addressed all the feedback that was raised on that one?
Thanks for sharing that.
I believe I've addressed most of the feedback. Starting with we agree
preferring key_serial_t. I changed to to use IS_REACHABLE(), and set
ALG_SET_KEY_BY_KEY_SERIAL to 10 leaving a comment about libkcapi
reserving values 7-9.
I've made other additional changes since the RFC, so we should consider
this code outdated. I'll submit a v1 that is a bit cleaner after the
merge window.
Your comment about broken crypto algorithms exposing sensitive data is
interesting. We've had similar thoughts about adding additional
permission, but ultimately decided to stick to the pattern asymmetric
key types use.
lookup_user_key() ultimately makes a call into a security hook
security_key_permission() given a key_ref_t, so users can further
restrict access based on keys that way if enabled. We've also had
similar discussions regarding X.509 certificates, and I'm not opposed to
Ondrej's suggestion of disabling this feature by default with Kconfig.
I'll look into this a bit more, and we're open to suggestions here.
Two random nits below:
+ *dest_len = key->datalen;
+ *dest = kmalloc(*dest_len, GFP_KERNEL);
+ if (!*dest)
+ return -ENOMEM;
+
+ memcpy(*dest, ukp->data, *dest_len);
This should use kmemdup(). >
+ } else if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS) &&
+ !strcmp(key->type->name, "encrypted")) {
+ read_key = &read_key_type_encrypted;
+ } else if (IS_ENABLED(CONFIG_TRUSTED_KEYS) &&
+ !strcmp(key->type->name, "trusted")) {
+ read_key = &read_key_type_trusted;
These need to use IS_REACHABLE(), not IS_ENABLED().
- Eric
