linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 1/1] crypto: af_alg - Support symmetric encryption via keyring keys
@ 2022-10-04 21:29 Frederick Lawler
  2022-10-11 23:07 ` Eric Biggers
  0 siblings, 1 reply; 4+ messages in thread
From: Frederick Lawler @ 2022-10-04 21:29 UTC (permalink / raw)
  To: herbert, davem, ebiggers, hch, smueller
  Cc: linux-kernel, linux-crypto, kernel-team, Frederick Lawler

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@cloudflare.com>
---
 Documentation/crypto/userspace-if.rst |  15 ++-
 crypto/af_alg.c                       | 142 +++++++++++++++++++++++++-
 include/uapi/linux/if_alg.h           |   1 +
 3 files changed, 154 insertions(+), 4 deletions(-)

diff --git a/Documentation/crypto/userspace-if.rst b/Documentation/crypto/userspace-if.rst
index b45dabbf69d6..f80f243e227e 100644
--- a/Documentation/crypto/userspace-if.rst
+++ b/Documentation/crypto/userspace-if.rst
@@ -131,9 +131,9 @@ from the kernel crypto API. If the buffer is too small for the message
 digest, the flag MSG_TRUNC is set by the kernel.
 
 In order to set a message digest key, the calling application must use
-the setsockopt() option of ALG_SET_KEY. If the key is not set the HMAC
-operation is performed without the initial HMAC state change caused by
-the key.
+the setsockopt() option of ALG_SET_KEY or ALG_SET_KEY_BY_KEY_SERIAL. If the
+key is not set the HMAC operation is performed without the initial HMAC state
+change caused by the key.
 
 Symmetric Cipher API
 --------------------
@@ -382,6 +382,15 @@ mentioned optname:
 
    -  the RNG cipher type to provide the seed
 
+- ALG_SET_KEY_BY_KEY_SERIAL -- Setting the key via keyring key_serial_t.
+   This operation behaves the same as ALG_SET_KEY. The decrypted
+   data is copied from a keyring key, and uses that data as the
+   key for symmetric encryption.
+
+   The passed in key_serial_t must have the KEY_(POS|USR|GRP|OTH)_SEARCH
+   permission set, otherwise -EPERM is returned. Supports key types: user,
+   logon, encrypted, and trusted.
+
 -  ALG_SET_AEAD_AUTHSIZE -- Setting the authentication tag size for
    AEAD ciphers. For a encryption operation, the authentication tag of
    the given size will be generated. For a decryption operation, the
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index e893c0f6c879..da949089def2 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -12,6 +12,8 @@
 #include <linux/crypto.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/key.h>
+#include <linux/key-type.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/net.h>
@@ -19,6 +21,10 @@
 #include <linux/sched.h>
 #include <linux/sched/signal.h>
 #include <linux/security.h>
+#include <linux/string.h>
+#include <keys/user-type.h>
+#include <keys/trusted-type.h>
+#include <keys/encrypted-type.h>
 
 struct alg_type_list {
 	const struct af_alg_type *type;
@@ -222,6 +228,136 @@ static int alg_setkey(struct sock *sk, sockptr_t ukey, unsigned int keylen)
 	return err;
 }
 
+#ifdef CONFIG_KEYS
+
+static int read_key_type_user(const struct key *key, u8 **dest, u16 *dest_len)
+{
+	const struct user_key_payload *ukp;
+
+	ukp = user_key_payload_locked(key);
+	if (IS_ERR_OR_NULL(ukp))
+		return -EKEYREVOKED;
+
+	*dest_len = key->datalen;
+	*dest = kmalloc(*dest_len, GFP_KERNEL);
+	if (!*dest)
+		return -ENOMEM;
+
+	memcpy(*dest, ukp->data, *dest_len);
+	return 0;
+}
+
+static int read_key_type_encrypted(const struct key *key, u8 **dest, u16 *dest_len)
+{
+	const struct encrypted_key_payload *ekp;
+
+	ekp = dereference_key_locked(key);
+	if (IS_ERR_OR_NULL(ekp))
+		return -EKEYREVOKED;
+
+	*dest_len = ekp->decrypted_datalen;
+	*dest = kmalloc(*dest_len, GFP_KERNEL);
+	if (!*dest)
+		return -ENOMEM;
+
+	memcpy(*dest, ekp->decrypted_data, *dest_len);
+
+	return 0;
+}
+
+static int read_key_type_trusted(const struct key *key, u8 **dest, u16 *dest_len)
+{
+	const struct trusted_key_payload *tkp;
+
+	tkp = dereference_key_locked(key);
+	if (IS_ERR_OR_NULL(tkp))
+		return -EKEYREVOKED;
+
+	*dest_len = tkp->key_len;
+	*dest = kmalloc(*dest_len, GFP_KERNEL);
+	if (!*dest)
+		return -ENOMEM;
+
+	memcpy(*dest, tkp->key, *dest_len);
+	return 0;
+}
+
+static int alg_setkey_by_key_serial(struct sock *sk, sockptr_t ukey, unsigned int keylen)
+{
+	u8 *ukey_serial;
+	int err;
+	u8 *key_data;
+	u16 key_data_len;
+	struct key *key;
+	key_ref_t key_ref;
+	key_serial_t *key_serial;
+	int (*read_key)(const struct key *key, u8 **dest, u16 *dest_len);
+
+	struct alg_sock *ask = alg_sk(sk);
+	const struct af_alg_type *type = ask->type;
+
+	ukey_serial = sock_kmalloc(sk, keylen, GFP_KERNEL);
+	if (!ukey_serial)
+		return -ENOMEM;
+
+	err = -EFAULT;
+	if (copy_from_sockptr(ukey_serial, ukey, keylen))
+		goto out;
+
+	key_serial = (key_serial_t *)ukey_serial;
+	key_ref = lookup_user_key(*key_serial, 0, KEY_NEED_SEARCH);
+	if (IS_ERR(key_ref)) {
+		err = PTR_ERR(key_ref);
+		goto out;
+	}
+
+	key = key_ref_to_ptr(key_ref);
+
+	down_read(&key->sem);
+
+	err = -ENOPROTOOPT;
+	if (!strcmp(key->type->name, "user") ||
+	    !strcmp(key->type->name, "logon")) {
+		read_key = &read_key_type_user;
+	} 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;
+	} else {
+		up_read(&key->sem);
+		goto out;
+	}
+
+	err = read_key(key, &key_data, &key_data_len);
+	if (err) {
+		up_read(&key->sem);
+		kfree_sensitive(key_data);
+		goto out;
+	}
+
+	up_read(&key->sem);
+
+	err = type->setkey(ask->private, key_data, key_data_len);
+
+	kfree_sensitive(key_data);
+
+out:
+	sock_kzfree_s(sk, ukey_serial, keylen);
+
+	return err;
+}
+
+#else
+
+static inline int alg_setkey_by_key_serial(struct sock *sk, sockptr_t ukey, unsigned int keylen)
+{
+	return -ENOPROTOOPT;
+}
+
+#endif
+
 static int alg_setsockopt(struct socket *sock, int level, int optname,
 			  sockptr_t optval, unsigned int optlen)
 {
@@ -242,12 +378,16 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
 
 	switch (optname) {
 	case ALG_SET_KEY:
+	case ALG_SET_KEY_BY_KEY_SERIAL:
 		if (sock->state == SS_CONNECTED)
 			goto unlock;
 		if (!type->setkey)
 			goto unlock;
 
-		err = alg_setkey(sk, optval, optlen);
+		if (optname == ALG_SET_KEY_BY_KEY_SERIAL)
+			err = alg_setkey_by_key_serial(sk, optval, optlen);
+		else
+			err = alg_setkey(sk, optval, optlen);
 		break;
 	case ALG_SET_AEAD_AUTHSIZE:
 		if (sock->state == SS_CONNECTED)
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index 578b18aab821..0824fbc026a1 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -52,6 +52,7 @@ struct af_alg_iv {
 #define ALG_SET_AEAD_ASSOCLEN		4
 #define ALG_SET_AEAD_AUTHSIZE		5
 #define ALG_SET_DRBG_ENTROPY		6
+#define ALG_SET_KEY_BY_KEY_SERIAL	7
 
 /* Operations */
 #define ALG_OP_DECRYPT			0
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH 1/1] crypto: af_alg - Support symmetric encryption via keyring keys
  2022-10-04 21:29 [RFC PATCH 1/1] crypto: af_alg - Support symmetric encryption via keyring keys Frederick Lawler
@ 2022-10-11 23:07 ` Eric Biggers
  2022-10-12 14:49   ` Frederick Lawler
  0 siblings, 1 reply; 4+ messages in thread
From: Eric Biggers @ 2022-10-11 23:07 UTC (permalink / raw)
  To: Frederick Lawler
  Cc: herbert, davem, hch, smueller, linux-kernel, linux-crypto,
	kernel-team, Ondrej Mosnacek

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@cloudflare.com>

There was a similar patch several years ago by Ondrej Mosnacek:
https://lore.kernel.org/linux-crypto/20190521100034.9651-1-omosnace@redhat.com/T/#u

Have you addressed all the feedback that was raised on that one?

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH 1/1] crypto: af_alg - Support symmetric encryption via keyring keys
  2022-10-11 23:07 ` Eric Biggers
@ 2022-10-12 14:49   ` Frederick Lawler
  2022-10-12 15:21     ` Stephan Mueller
  0 siblings, 1 reply; 4+ messages in thread
From: Frederick Lawler @ 2022-10-12 14:49 UTC (permalink / raw)
  To: Eric Biggers
  Cc: herbert, davem, hch, smueller, linux-kernel, linux-crypto,
	kernel-team, Ondrej Mosnacek

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@cloudflare.com>
> 
> There was a similar patch several years ago by Ondrej Mosnacek:
> https://lore.kernel.org/linux-crypto/20190521100034.9651-1-omosnace@redhat.com/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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH 1/1] crypto: af_alg - Support symmetric encryption via keyring keys
  2022-10-12 14:49   ` Frederick Lawler
@ 2022-10-12 15:21     ` Stephan Mueller
  0 siblings, 0 replies; 4+ messages in thread
From: Stephan Mueller @ 2022-10-12 15:21 UTC (permalink / raw)
  To: Eric Biggers, Frederick Lawler
  Cc: herbert, davem, hch, linux-kernel, linux-crypto, kernel-team,
	Ondrej Mosnacek

Am Mittwoch, 12. Oktober 2022, 16:49:56 CEST schrieb Frederick Lawler:

Hi Frederick,

> 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.

This reservation should not be observed. I provided patches for adding AF_ALG 
interfaces for KPP and AKCIPHER some time ago which were rejected. Libkcapi 
still contains the interface implementations but are not compiled by default. 

As the patches are rejected, their values should not be considered as 
relevant. Thus, I think your patch should not keep holes in the numbers.

Ciao
Stephan



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-10-12 15:21 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-04 21:29 [RFC PATCH 1/1] crypto: af_alg - Support symmetric encryption via keyring keys Frederick Lawler
2022-10-11 23:07 ` Eric Biggers
2022-10-12 14:49   ` Frederick Lawler
2022-10-12 15:21     ` Stephan Mueller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).