linux-doc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] KEYS: Add ECDH support
@ 2024-03-30  6:55 Zhang Yiqun
  2024-03-30  7:04 ` Eric Biggers
  2024-03-30 11:00 ` Jarkko Sakkinen
  0 siblings, 2 replies; 9+ messages in thread
From: Zhang Yiqun @ 2024-03-30  6:55 UTC (permalink / raw)
  To: dhowells, jarkko, corbet; +Cc: keyrings, linux-doc, linux-kernel, Zhang Yiqun

This patch is to introduce ECDH into keyctl syscall for
userspace usage, containing public key generation and
shared secret computation.

It is mainly based on dh code, so it has the same condition
to the input which only user keys is supported. The output
result is storing into the buffer with the provided length.

Signed-off-by: Zhang Yiqun <zhangyiqun@phytium.com.cn>
---
 Documentation/security/keys/core.rst |  62 ++++++
 include/linux/compat.h               |   4 +
 include/uapi/linux/keyctl.h          |  11 +
 security/keys/Kconfig                |  12 +
 security/keys/Makefile               |   2 +
 security/keys/compat_ecdh.c          |  50 +++++
 security/keys/ecdh.c                 | 318 +++++++++++++++++++++++++++
 security/keys/internal.h             |  44 ++++
 security/keys/keyctl.c               |  10 +
 9 files changed, 513 insertions(+)
 create mode 100644 security/keys/compat_ecdh.c
 create mode 100644 security/keys/ecdh.c

diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
index 326b8a973828..9749466a3c95 100644
--- a/Documentation/security/keys/core.rst
+++ b/Documentation/security/keys/core.rst
@@ -884,6 +884,68 @@ The keyctl syscall functions are:
      and either the buffer length or the OtherInfo length exceeds the
      allowed length.
 
+  *  Compute a elliptic curve Diffie-Hellman shared secret::
+
+	long keyctl(KEYCTL_ECDH_COMPUTE, struct keyctl_ecdh_params *params,
+		    char *buffer, size_t buflen,
+		    struct keyctl_curve_params *curve);
+
+     The params struct contains serial numbers for two keys::
+
+	 - The local private key
+	 - The shared public key
+
+     The value computed is::
+
+	result = private EC-MUTIPLY public
+
+     The buffer length must be at least the length of the prime, or zero.
+
+     If the buffer length is nonzero, the length of the result is
+     returned when it is successfully calculated and copied in to the
+     buffer. When the buffer length is zero, the minimum required
+     buffer length is returned.
+
+     The curve parameter struct keyctl_curve_params is as follows:
+
+	 - ``char *curvename`` specifies the curve parameter used in
+	   the following computation.
+
+     This function will return error EOPNOTSUPP if the key type is not
+     supported, error ENOKEY if the key could not be found, or error
+     EACCES if the key is not readable by the caller.
+
+  *  Generate a elliptic curve Diffie-Hellman shared public key::
+
+	long keyctl(KEYCTL_ECDH_GENPUBKEY,
+		    struct keyctl_ecdh_params *params,
+		    char *buffer, size_t buflen,
+		    struct keyctl_curve_params *curve);
+
+     The params struct contains serial numbers for one keys::
+
+	 - The local private key
+
+     The value computed is::
+
+	result = private EC-MUTIPLY G
+
+     The buffer length must be at least the length of the prime, or zero.
+
+     If the buffer length is nonzero, the length of the result is
+     returned when it is successfully calculated and copied in to the
+     buffer. When the buffer length is zero, the minimum required
+     buffer length is returned.
+
+     The curve parameter struct keyctl_curve_params is as follows:
+
+	 - ``char *curvename`` specifies the curve parameter used in
+	   the following computation.
+
+     This function will return error EOPNOTSUPP if the key type is not
+     supported, error ENOKEY if the key could not be found, or error
+     EACCES if the key is not readable by the caller.
+
 
   *  Restrict keyring linkage::
 
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 233f61ec8afc..1f989ef5c9e1 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -411,6 +411,10 @@ struct compat_keyctl_kdf_params {
 	__u32 __spare[8];
 };
 
+struct compat_keyctl_curve_params {
+	compat_uptr_t curvename;
+};
+
 struct compat_stat;
 struct compat_statfs;
 struct compat_statfs64;
diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 4c8884eea808..77b5d9d837a2 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -70,6 +70,8 @@
 #define KEYCTL_MOVE			30	/* Move keys between keyrings */
 #define KEYCTL_CAPABILITIES		31	/* Find capabilities of keyrings subsystem */
 #define KEYCTL_WATCH_KEY		32	/* Watch a key or ring of keys for changes */
+#define KEYCTL_ECDH_COMPUTE		33	/* Compute EC Diffie-Hellman values */
+#define KEYCTL_ECDH_GENPUBKEY		34	/* Generate EC Diffie-Hellman public keys */
 
 /* keyctl structures */
 struct keyctl_dh_params {
@@ -90,6 +92,15 @@ struct keyctl_kdf_params {
 	__u32 __spare[8];
 };
 
+struct keyctl_ecdh_params {
+	__s32 priv;
+	__s32 pub;
+};
+
+struct keyctl_curve_params {
+	char __user *curvename;
+};
+
 #define KEYCTL_SUPPORTS_ENCRYPT		0x01
 #define KEYCTL_SUPPORTS_DECRYPT		0x02
 #define KEYCTL_SUPPORTS_SIGN		0x04
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index abb03a1b2a5c..b36be8d8d501 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -125,6 +125,18 @@ config KEY_DH_OPERATIONS
 
 	 If you are unsure as to whether this is required, answer N.
 
+config KEY_ECDH_OPERATIONS
+       bool "Elliptic Curve Diffie-Hellman operations on retained keys"
+       depends on KEYS
+       select CRYPTO
+       select CRYPTO_ECDH
+       help
+	 This option provides support for calculating Elliptic Curve
+	 Diffie-Hellman public keys and shared secrets using values
+	 stored as keys in the kernel.
+
+	 If you are unsure as to whether this is required, answer N.
+
 config KEY_NOTIFICATIONS
 	bool "Provide key/keyring change notifications"
 	depends on KEYS && WATCH_QUEUE
diff --git a/security/keys/Makefile b/security/keys/Makefile
index 5f40807f05b3..590fc4724f37 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -17,11 +17,13 @@ obj-y := \
 	request_key_auth.o \
 	user_defined.o
 compat-obj-$(CONFIG_KEY_DH_OPERATIONS) += compat_dh.o
+compat-obj-$(CONFIG_KEY_ECDH_OPERATIONS) += compat_ecdh.o
 obj-$(CONFIG_COMPAT) += compat.o $(compat-obj-y)
 obj-$(CONFIG_PROC_FS) += proc.o
 obj-$(CONFIG_SYSCTL) += sysctl.o
 obj-$(CONFIG_PERSISTENT_KEYRINGS) += persistent.o
 obj-$(CONFIG_KEY_DH_OPERATIONS) += dh.o
+obj-$(CONFIG_KEY_ECDH_OPERATIONS) += ecdh.o
 obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += keyctl_pkey.o
 
 #
diff --git a/security/keys/compat_ecdh.c b/security/keys/compat_ecdh.c
new file mode 100644
index 000000000000..040d2a1c5548
--- /dev/null
+++ b/security/keys/compat_ecdh.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* 32-bit compatibility syscall for 64-bit systems for ECDH operations
+ *
+ * Copyright (C) 2024 Zhang Yiqun <zhangyiqun@phytium.com.cn>
+ */
+
+#include <linux/uaccess.h>
+
+#include "internal.h"
+
+/*
+ * Perform the ECDH computation or ECDH based key derivation.
+ *
+ * If successful, 0 will be returned.
+ */
+long compat_keyctl_ecdh_compute(struct keyctl_ecdh_params __user *params,
+				char __user *buffer, size_t buflen,
+				struct compat_keyctl_curve_params __user *curve)
+{
+	struct keyctl_curve_params curvecopy;
+	struct compat_keyctl_curve_params compat_curvecopy;
+
+	if (!curve)
+		return -EINVAL;
+
+	if (copy_from_user(&compat_curvecopy, curve, sizeof(compat_curvecopy)) != 0)
+		return -EFAULT;
+
+	curvecopy.curvename = compat_ptr(compat_curvecopy.curvename);
+
+	return keyctl_ecdh_compute(params, buffer, buflen, &curvecopy);
+}
+
+long compat_keyctl_ecdh_genpubkey(struct keyctl_ecdh_params __user *params,
+				  char __user *buffer, size_t buflen,
+				  struct compat_keyctl_curve_params __user *curve)
+{
+	struct keyctl_curve_params curvecopy;
+	struct compat_keyctl_curve_params compat_curvecopy;
+
+	if (!curve)
+		return -EINVAL;
+
+	if (copy_from_user(&compat_curvecopy, curve, sizeof(compat_curvecopy)) != 0)
+		return -EFAULT;
+
+	curvecopy.curvename = compat_ptr(compat_curvecopy.curvename);
+
+	return keyctl_ecdh_genpubkey(params, buffer, buflen, &curvecopy);
+}
diff --git a/security/keys/ecdh.c b/security/keys/ecdh.c
new file mode 100644
index 000000000000..5e5be22b920c
--- /dev/null
+++ b/security/keys/ecdh.c
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Crypto operations using stored keys
+ *
+ * Copyright (c) 2024 Zhang Yiqun <zhangyiqun@phytium.com.cn>
+ */
+
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/scatterlist.h>
+#include <linux/crypto.h>
+#include <crypto/hash.h>
+#include <crypto/kpp.h>
+#include <crypto/ecdh.h>
+#include <keys/user-type.h>
+#include "internal.h"
+
+static ssize_t ecdh_data_from_key(key_serial_t keyid, char **data)
+{
+	struct key *key;
+	key_ref_t key_ref;
+	long status;
+	ssize_t ret;
+
+	key_ref = lookup_user_key(keyid, 0, KEY_NEED_READ);
+	if (IS_ERR(key_ref)) {
+		ret = -ENOKEY;
+		goto error;
+	}
+
+	key = key_ref_to_ptr(key_ref);
+
+	ret = -EOPNOTSUPP;
+	if (key->type == &key_type_user) {
+		down_read(&key->sem);
+		status = key_validate(key);
+		if (status == 0) {
+			const struct user_key_payload *payload;
+			uint8_t *duplicate;
+
+			payload = user_key_payload_locked(key);
+
+			duplicate = kmemdup(payload->data, payload->datalen,
+					    GFP_KERNEL);
+			if (duplicate) {
+				*data = duplicate;
+				ret = payload->datalen;
+			} else {
+				ret = -ENOMEM;
+			}
+		}
+		up_read(&key->sem);
+	}
+
+	key_put(key);
+error:
+	return ret;
+}
+
+static void ecdh_free_data(struct ecdh *ecdh)
+{
+	kfree_sensitive(ecdh->key);
+}
+
+long keyctl_ecdh_compute(struct keyctl_ecdh_params __user *params,
+		       char __user *buffer, size_t buflen,
+		       struct keyctl_curve_params __user *curve)
+{
+	long ret;
+	ssize_t dlen;
+	int secretlen;
+	int outlen;
+	struct keyctl_ecdh_params pcopy;
+	struct ecdh ecdh_inputs;
+	struct scatterlist insg;
+	struct scatterlist outsg;
+	DECLARE_CRYPTO_WAIT(compl);
+	struct crypto_kpp *tfm;
+	struct kpp_request *req;
+	uint8_t *secret;
+	uint8_t *outbuf;
+	char *dhname;
+
+	if (!params || (!buffer && buflen) || !curve) {
+		ret = -EINVAL;
+		goto out1;
+	}
+
+	if (copy_from_user(&pcopy, params, sizeof(pcopy)) != 0) {
+		ret = -EFAULT;
+		goto out1;
+	}
+
+	memset(&ecdh_inputs, 0, sizeof(ecdh_inputs));
+
+	dlen = ecdh_data_from_key(pcopy.priv, &ecdh_inputs.key);
+	if (dlen < 0) {
+		ret = dlen;
+		goto out1;
+	}
+	ecdh_inputs.key_size = dlen;
+
+	secretlen = crypto_ecdh_key_len(&ecdh_inputs);
+	secret = kmalloc(secretlen, GFP_KERNEL);
+	if (!secret) {
+		ret = -ENOMEM;
+		goto out2;
+	}
+
+	ret = crypto_ecdh_encode_key(secret, secretlen, &ecdh_inputs);
+	if (ret)
+		goto out3;
+
+	dhname = strndup_user(curve->curvename, CRYPTO_MAX_ALG_NAME);
+
+	tfm = crypto_alloc_kpp(dhname, 0, 0);
+	if (IS_ERR(tfm)) {
+		ret = PTR_ERR(tfm);
+		goto out3;
+	}
+
+	kfree(dhname);
+
+	ret = crypto_kpp_set_secret(tfm, secret, secretlen);
+	if (ret)
+		goto out4;
+
+	outlen = crypto_kpp_maxsize(tfm);
+
+	if (buflen == 0) {
+		ret = outlen;
+		goto out4;
+	} else if (outlen > buflen) {
+		ret = -EOVERFLOW;
+		goto out4;
+	}
+
+	outbuf = kzalloc(outlen, GFP_KERNEL);
+	if (!outbuf) {
+		ret = -ENOMEM;
+		goto out4;
+	}
+
+	dlen = ecdh_data_from_key(pcopy.pub, (char **)&outbuf);
+	if (dlen != outlen) {
+		ret = dlen;
+		goto out5;
+	}
+
+	sg_init_one(&insg, outbuf, outlen);
+	sg_init_one(&outsg, outbuf, outlen/2);
+
+	req = kpp_request_alloc(tfm, GFP_KERNEL);
+	if (!req) {
+		ret = -ENOMEM;
+		goto out5;
+	}
+
+	kpp_request_set_input(req, &insg, outlen);
+	kpp_request_set_output(req, &outsg, outlen/2);
+	init_completion(&compl.completion);
+	kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
+				 CRYPTO_TFM_REQ_MAY_SLEEP,
+				 crypto_req_done, &compl);
+
+	/*
+	 * For DH, generate_public_key and generate_shared_secret are
+	 * the same calculation
+	 */
+	ret = crypto_kpp_compute_shared_secret(req);
+	if (ret == -EINPROGRESS) {
+		wait_for_completion(&compl.completion);
+		ret = compl.err;
+		if (ret)
+			goto out6;
+	}
+
+	if (copy_to_user(buffer, outbuf, req->dst_len) == 0)
+		ret = req->dst_len;
+	else
+		ret = -EFAULT;
+
+out6:
+	kpp_request_free(req);
+out5:
+	kfree_sensitive(outbuf);
+out4:
+	crypto_free_kpp(tfm);
+out3:
+	kfree_sensitive(secret);
+out2:
+	ecdh_free_data(&ecdh_inputs);
+out1:
+	return ret;
+}
+
+long keyctl_ecdh_genpubkey(struct keyctl_ecdh_params __user *params,
+		       char __user *buffer, size_t buflen,
+		       struct keyctl_curve_params __user *curve)
+{
+	long ret;
+	ssize_t dlen;
+	int secretlen;
+	int outlen;
+	struct keyctl_ecdh_params pcopy;
+	struct ecdh ecdh_inputs;
+	struct scatterlist outsg;
+	DECLARE_CRYPTO_WAIT(compl);
+	struct crypto_kpp *tfm;
+	struct kpp_request *req;
+	uint8_t *secret;
+	uint8_t *outbuf;
+	char *dhname;
+
+	if (!params || (!buffer && buflen)) {
+		ret = -EINVAL;
+		goto out1;
+	}
+
+	if (copy_from_user(&pcopy, params, sizeof(pcopy)) != 0) {
+		ret = -EFAULT;
+		goto out1;
+	}
+
+	memset(&ecdh_inputs, 0, sizeof(ecdh_inputs));
+
+	dlen = ecdh_data_from_key(pcopy.priv, &ecdh_inputs.key);
+	if (dlen < 0) {
+		ret = dlen;
+		goto out1;
+	}
+	ecdh_inputs.key_size = dlen;
+
+	secretlen = crypto_ecdh_key_len(&ecdh_inputs);
+	secret = kmalloc(secretlen, GFP_KERNEL);
+	if (!secret) {
+		ret = -ENOMEM;
+		goto out2;
+	}
+
+	ret = crypto_ecdh_encode_key(secret, secretlen, &ecdh_inputs);
+	if (ret)
+		goto out3;
+
+	dhname = strndup_user(curve->curvename, CRYPTO_MAX_ALG_NAME);
+
+	tfm = crypto_alloc_kpp(dhname, 0, 0);
+	if (IS_ERR(tfm)) {
+		ret = PTR_ERR(tfm);
+		goto out3;
+	}
+
+	kfree(dhname);
+
+	ret = crypto_kpp_set_secret(tfm, secret, secretlen);
+	if (ret)
+		goto out4;
+
+	outlen = crypto_kpp_maxsize(tfm);
+
+	if (buflen == 0) {
+		ret = outlen;
+		goto out4;
+	} else if (outlen > buflen) {
+		ret = -EOVERFLOW;
+		goto out4;
+	}
+
+	outbuf = kzalloc(outlen, GFP_KERNEL);
+	if (!outbuf) {
+		ret = -ENOMEM;
+		goto out4;
+	}
+
+	sg_init_one(&outsg, outbuf, outlen);
+
+	req = kpp_request_alloc(tfm, GFP_KERNEL);
+	if (!req) {
+		ret = -ENOMEM;
+		goto out5;
+	}
+
+	kpp_request_set_input(req, NULL, 0);
+	kpp_request_set_output(req, &outsg, outlen);
+	init_completion(&compl.completion);
+	kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG |
+				 CRYPTO_TFM_REQ_MAY_SLEEP,
+				 crypto_req_done, &compl);
+
+	/*
+	 * For DH, generate_public_key and generate_shared_secret are
+	 * the same calculation
+	 */
+	ret = crypto_kpp_generate_public_key(req);
+	if (ret == -EINPROGRESS) {
+		wait_for_completion(&compl.completion);
+		ret = compl.err;
+		if (ret)
+			goto out6;
+	}
+
+	if (copy_to_user(buffer, outbuf, req->dst_len) == 0)
+		ret = req->dst_len;
+	else
+		ret = -EFAULT;
+
+out6:
+	kpp_request_free(req);
+out5:
+	kfree_sensitive(outbuf);
+out4:
+	crypto_free_kpp(tfm);
+out3:
+	kfree_sensitive(secret);
+out2:
+	ecdh_free_data(&ecdh_inputs);
+out1:
+	return ret;
+}
diff --git a/security/keys/internal.h b/security/keys/internal.h
index 2cffa6dc8255..165523e29b52 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -306,6 +306,50 @@ static inline long compat_keyctl_dh_compute(
 #endif
 #endif
 
+#ifdef CONFIG_KEY_ECDH_OPERATIONS
+extern long keyctl_ecdh_compute(struct keyctl_ecdh_params __user *param,
+			      char __user *buffer, size_t buflen,
+			      struct keyctl_curve_params __user *curve);
+extern long keyctl_ecdh_genpubkey(struct keyctl_ecdh_params __user *param,
+			      char __user *buffer, size_t buflen,
+			      struct keyctl_curve_params __user *curve);
+#ifdef CONFIG_COMPAT
+extern long compat_keyctl_ecdh_compute(struct keyctl_ecdh_params __user *params,
+				char __user *buffer, size_t buflen,
+				struct compat_keyctl_curve_params __user *curve);
+extern long compat_keyctl_ecdh_genpubkey(struct keyctl_ecdh_params __user *params,
+				char __user *buffer, size_t buflen,
+				struct compat_keyctl_curve_params __user *curve);
+#endif
+#else
+static inline long keyctl_ecdh_compute(struct keyctl_ecdh_params __user *params,
+				     char __user *buffer, size_t buflen,
+				     struct keyctl_curve_params __user *kdf)
+{
+	return -EOPNOTSUPP;
+}
+static inline long keyctl_ecdh_genpubkey(struct keyctl_ecdh_params __user *params,
+				     char __user *buffer, size_t buflen,
+				     struct keyctl_curve_params __user *kdf)
+{
+	return -EOPNOTSUPP;
+}
+#ifdef CONFIG_COMPAT
+long compat_keyctl_ecdh_compute(struct keyctl_ecdh_params __user *params,
+				char __user *buffer, size_t buflen,
+				struct compat_keyctl_curve_params __user *curve)
+{
+	return -EOPNOTSUPP;
+}
+long compat_keyctl_ecdh_genpubkey(struct keyctl_ecdh_params __user *params,
+				char __user *buffer, size_t buflen,
+				struct compat_keyctl_curve_params __user *curve)
+{
+	return -EOPNOTSUPP;
+}
+#endif
+#endif
+
 #ifdef CONFIG_ASYMMETRIC_KEY_TYPE
 extern long keyctl_pkey_query(key_serial_t,
 			      const char __user *,
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 10ba439968f7..e690c130386a 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -2019,6 +2019,16 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case KEYCTL_WATCH_KEY:
 		return keyctl_watch_key((key_serial_t)arg2, (int)arg3, (int)arg4);
 
+	case KEYCTL_ECDH_COMPUTE:
+		return keyctl_ecdh_compute((struct keyctl_ecdh_params __user *) arg2,
+					 (char __user *) arg3, (size_t) arg4,
+					 (struct keyctl_curve_params __user *) arg5);
+
+	case KEYCTL_ECDH_GENPUBKEY:
+		return keyctl_ecdh_genpubkey((struct keyctl_ecdh_params __user *) arg2,
+					 (char __user *) arg3, (size_t) arg4,
+					 (struct keyctl_curve_params __user *) arg5);
+
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
2.17.1


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

* Re: [PATCH] KEYS: Add ECDH support
  2024-03-30  6:55 [PATCH] KEYS: Add ECDH support Zhang Yiqun
@ 2024-03-30  7:04 ` Eric Biggers
  2024-03-30 13:09   ` James Bottomley
  2024-03-30 11:00 ` Jarkko Sakkinen
  1 sibling, 1 reply; 9+ messages in thread
From: Eric Biggers @ 2024-03-30  7:04 UTC (permalink / raw)
  To: Zhang Yiqun
  Cc: dhowells, jarkko, corbet, keyrings, linux-doc, linux-kernel,
	linux-crypto

[+Cc linux-crypto]

On Sat, Mar 30, 2024 at 02:55:06PM +0800, Zhang Yiqun wrote:
> This patch is to introduce ECDH into keyctl syscall for
> userspace usage, containing public key generation and
> shared secret computation.
> 
> It is mainly based on dh code, so it has the same condition
> to the input which only user keys is supported. The output
> result is storing into the buffer with the provided length.
> 
> Signed-off-by: Zhang Yiqun <zhangyiqun@phytium.com.cn>
> ---
>  Documentation/security/keys/core.rst |  62 ++++++
>  include/linux/compat.h               |   4 +
>  include/uapi/linux/keyctl.h          |  11 +
>  security/keys/Kconfig                |  12 +
>  security/keys/Makefile               |   2 +
>  security/keys/compat_ecdh.c          |  50 +++++
>  security/keys/ecdh.c                 | 318 +++++++++++++++++++++++++++
>  security/keys/internal.h             |  44 ++++
>  security/keys/keyctl.c               |  10 +
>  9 files changed, 513 insertions(+)
>  create mode 100644 security/keys/compat_ecdh.c
>  create mode 100644 security/keys/ecdh.c

Nacked-by: Eric Biggers <ebiggers@google.com>

The existing KEYCTL_PKEY_*, KEYCTL_DH_COMPUTE, and AF_ALG are causing enough
problems.  We do not need any more UAPIs like this.  They are hard to maintain,
break often, not properly documented, increase the kernel's attack surface, and
what they do is better done in userspace.

Please refer to the recent thread
https://lore.kernel.org/linux-crypto/CZSHRUIJ4RKL.34T4EASV5DNJM@matfyz.cz/T/#u
where these issues were discussed in detail.

- Eric

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

* Re: [PATCH] KEYS: Add ECDH support
  2024-03-30  6:55 [PATCH] KEYS: Add ECDH support Zhang Yiqun
  2024-03-30  7:04 ` Eric Biggers
@ 2024-03-30 11:00 ` Jarkko Sakkinen
  1 sibling, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-03-30 11:00 UTC (permalink / raw)
  To: Zhang Yiqun, dhowells, corbet; +Cc: keyrings, linux-doc, linux-kernel

On Sat Mar 30, 2024 at 8:55 AM EET, Zhang Yiqun wrote:
> This patch is to introduce ECDH into keyctl syscall for

"Introduce Elliptic Curve Diffie-Hellman (ECDH)"

What does it mean to "introduce ECDH into keyctl syscall"?

> userspace usage, containing public key generation and
> shared secret computation.

Who else uses syscalls other than user space? You are implying
that there some other client.

>
> It is mainly based on dh code, so it has the same condition
> to the input which only user keys is supported. The output
> result is storing into the buffer with the provided length.

What is "dh code"?

>
> Signed-off-by: Zhang Yiqun <zhangyiqun@phytium.com.cn>
> ---
>  Documentation/security/keys/core.rst |  62 ++++++
>  include/linux/compat.h               |   4 +
>  include/uapi/linux/keyctl.h          |  11 +
>  security/keys/Kconfig                |  12 +
>  security/keys/Makefile               |   2 +
>  security/keys/compat_ecdh.c          |  50 +++++
>  security/keys/ecdh.c                 | 318 +++++++++++++++++++++++++++
>  security/keys/internal.h             |  44 ++++
>  security/keys/keyctl.c               |  10 +
>  9 files changed, 513 insertions(+)
>  create mode 100644 security/keys/compat_ecdh.c
>  create mode 100644 security/keys/ecdh.c
>
> diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst
> index 326b8a973828..9749466a3c95 100644
> --- a/Documentation/security/keys/core.rst
> +++ b/Documentation/security/keys/core.rst
> @@ -884,6 +884,68 @@ The keyctl syscall functions are:
>       and either the buffer length or the OtherInfo length exceeds the
>       allowed length.
>  
> +  *  Compute a elliptic curve Diffie-Hellman shared secret::
        Compute an Elliptic Curve Diffie-Hellman (ECDH) shared secret:

> +
> +	long keyctl(KEYCTL_ECDH_COMPUTE, struct keyctl_ecdh_params *params,
> +		    char *buffer, size_t buflen,
> +		    struct keyctl_curve_params *curve);
> +
> +     The params struct contains serial numbers for two keys::
> +
> +	 - The local private key
> +	 - The shared public key
> +
> +     The value computed is::
> +
> +	result = private EC-MUTIPLY public
> +
> +     The buffer length must be at least the length of the prime, or zero.
> +
> +     If the buffer length is nonzero, the length of the result is
> +     returned when it is successfully calculated and copied in to the
> +     buffer. When the buffer length is zero, the minimum required
> +     buffer length is returned.
> +
> +     The curve parameter struct keyctl_curve_params is as follows:
> +
> +	 - ``char *curvename`` specifies the curve parameter used in
> +	   the following computation.
> +
> +     This function will return error EOPNOTSUPP if the key type is not
> +     supported, error ENOKEY if the key could not be found, or error
> +     EACCES if the key is not readable by the caller.
> +
> +  *  Generate a elliptic curve Diffie-Hellman shared public key::

s/::/:/ 

various locations

> +
> +	long keyctl(KEYCTL_ECDH_GENPUBKEY,
> +		    struct keyctl_ecdh_params *params,
> +		    char *buffer, size_t buflen,
> +		    struct keyctl_curve_params *curve);
> +
> +     The params struct contains serial numbers for one keys::
> +
> +	 - The local private key
> +
> +     The value computed is::
> +
> +	result = private EC-MUTIPLY G
> +
> +     The buffer length must be at least the length of the prime, or zero.
> +
> +     If the buffer length is nonzero, the length of the result is
> +     returned when it is successfully calculated and copied in to the
> +     buffer. When the buffer length is zero, the minimum required
> +     buffer length is returned.
> +
> +     The curve parameter struct keyctl_curve_params is as follows:
> +
> +	 - ``char *curvename`` specifies the curve parameter used in

s/``char *curvename``/char *curvename/

> +	   the following computation.
> +
> +     This function will return error EOPNOTSUPP if the key type is not
> +     supported, error ENOKEY if the key could not be found, or error
> +     EACCES if the key is not readable by the caller.
> +
>  
>    *  Restrict keyring linkage::
>  
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 233f61ec8afc..1f989ef5c9e1 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -411,6 +411,10 @@ struct compat_keyctl_kdf_params {
>  	__u32 __spare[8];
>  };
>  
> +struct compat_keyctl_curve_params {
> +	compat_uptr_t curvename;
> +};

Please, do not create this at all.

> +
>  struct compat_stat;
>  struct compat_statfs;
>  struct compat_statfs64;
> diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
> index 4c8884eea808..77b5d9d837a2 100644
> --- a/include/uapi/linux/keyctl.h
> +++ b/include/uapi/linux/keyctl.h
> @@ -70,6 +70,8 @@
>  #define KEYCTL_MOVE			30	/* Move keys between keyrings */
>  #define KEYCTL_CAPABILITIES		31	/* Find capabilities of keyrings subsystem */
>  #define KEYCTL_WATCH_KEY		32	/* Watch a key or ring of keys for changes */
> +#define KEYCTL_ECDH_COMPUTE		33	/* Compute EC Diffie-Hellman values */
> +#define KEYCTL_ECDH_GENPUBKEY		34	/* Generate EC Diffie-Hellman public keys */
>  
>  /* keyctl structures */
>  struct keyctl_dh_params {
> @@ -90,6 +92,15 @@ struct keyctl_kdf_params {
>  	__u32 __spare[8];
>  };
>  
> +struct keyctl_ecdh_params {
> +	__s32 priv;
> +	__s32 pub;
> +};
> +
> +struct keyctl_curve_params {
> +	char __user *curvename;

This will cause ABI to be a total trainwreck because the size changes
depending on arch bitsize. Please never do anything like this in an
ioctl API.

I.e. please change to __u64 curve_name

> +};
>
> +
>  #define KEYCTL_SUPPORTS_ENCRYPT		0x01
>  #define KEYCTL_SUPPORTS_DECRYPT		0x02
>  #define KEYCTL_SUPPORTS_SIGN		0x04
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index abb03a1b2a5c..b36be8d8d501 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -125,6 +125,18 @@ config KEY_DH_OPERATIONS
>  
>  	 If you are unsure as to whether this is required, answer N.
>  
> +config KEY_ECDH_OPERATIONS
> +       bool "Elliptic Curve Diffie-Hellman operations on retained keys"
> +       depends on KEYS
> +       select CRYPTO
> +       select CRYPTO_ECDH
> +       help
> +	 This option provides support for calculating Elliptic Curve
> +	 Diffie-Hellman public keys and shared secrets using values
> +	 stored as keys in the kernel.
> +
> +	 If you are unsure as to whether this is required, answer N.
> +
>  config KEY_NOTIFICATIONS
>  	bool "Provide key/keyring change notifications"
>  	depends on KEYS && WATCH_QUEUE
> diff --git a/security/keys/Makefile b/security/keys/Makefile
> index 5f40807f05b3..590fc4724f37 100644
> --- a/security/keys/Makefile
> +++ b/security/keys/Makefile
> @@ -17,11 +17,13 @@ obj-y := \
>  	request_key_auth.o \
>  	user_defined.o
>  compat-obj-$(CONFIG_KEY_DH_OPERATIONS) += compat_dh.o
> +compat-obj-$(CONFIG_KEY_ECDH_OPERATIONS) += compat_ecdh.o
>  obj-$(CONFIG_COMPAT) += compat.o $(compat-obj-y)
>  obj-$(CONFIG_PROC_FS) += proc.o
>  obj-$(CONFIG_SYSCTL) += sysctl.o
>  obj-$(CONFIG_PERSISTENT_KEYRINGS) += persistent.o
>  obj-$(CONFIG_KEY_DH_OPERATIONS) += dh.o
> +obj-$(CONFIG_KEY_ECDH_OPERATIONS) += ecdh.o
>  obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += keyctl_pkey.o
>  
>  #
> diff --git a/security/keys/compat_ecdh.c b/security/keys/compat_ecdh.c
> new file mode 100644
> index 000000000000..040d2a1c5548
> --- /dev/null
> +++ b/security/keys/compat_ecdh.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* 32-bit compatibility syscall for 64-bit systems for ECDH operations
> + *
> + * Copyright (C) 2024 Zhang Yiqun <zhangyiqun@phytium.com.cn>
> + */
> +
> +#include <linux/uaccess.h>
> +

Not sure what is the purpose of this empty line?

> +#include "internal.h"
> +
> +/*
> + * Perform the ECDH computation or ECDH based key derivation.
> + *
> + * If successful, 0 will be returned.
> + */
> +long compat_keyctl_ecdh_compute(struct keyctl_ecdh_params __user *params,
> +				char __user *buffer, size_t buflen,
> +				struct compat_keyctl_curve_params __user *curve)
> +{
> +	struct keyctl_curve_params curvecopy;
> +	struct compat_keyctl_curve_params compat_curvecopy;

reverse xmas tree declaration order (various locations)

> +
> +	if (!curve)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&compat_curvecopy, curve, sizeof(compat_curvecopy)) != 0)
> +		return -EFAULT;
> +
> +	curvecopy.curvename = compat_ptr(compat_curvecopy.curvename);
> +
> +	return keyctl_ecdh_compute(params, buffer, buflen, &curvecopy);
> +}
> +
> +long compat_keyctl_ecdh_genpubkey(struct keyctl_ecdh_params __user *params,
> +				  char __user *buffer, size_t buflen,
> +				  struct compat_keyctl_curve_params __user *curve)
> +{
> +	struct keyctl_curve_params curvecopy;
> +	struct compat_keyctl_curve_params compat_curvecopy;
> +
> +	if (!curve)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&compat_curvecopy, curve, sizeof(compat_curvecopy)) != 0)
> +		return -EFAULT;
> +
> +	curvecopy.curvename = compat_ptr(compat_curvecopy.curvename);
> +
> +	return keyctl_ecdh_genpubkey(params, buffer, buflen, &curvecopy);
> +}
> diff --git a/security/keys/ecdh.c b/security/keys/ecdh.c
> new file mode 100644
> index 000000000000..5e5be22b920c
> --- /dev/null
> +++ b/security/keys/ecdh.c
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Crypto operations using stored keys
> + *
> + * Copyright (c) 2024 Zhang Yiqun <zhangyiqun@phytium.com.cn>
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +#include <linux/scatterlist.h>
> +#include <linux/crypto.h>
> +#include <crypto/hash.h>
> +#include <crypto/kpp.h>
> +#include <crypto/ecdh.h>
> +#include <keys/user-type.h>
> +#include "internal.h"
> +
> +static ssize_t ecdh_data_from_key(key_serial_t keyid, char **data)
> +{
> +	struct key *key;
> +	key_ref_t key_ref;
> +	long status;
> +	ssize_t ret;
> +
> +	key_ref = lookup_user_key(keyid, 0, KEY_NEED_READ);
> +	if (IS_ERR(key_ref)) {
> +		ret = -ENOKEY;
> +		goto error;
> +	}
> +
> +	key = key_ref_to_ptr(key_ref);
> +
> +	ret = -EOPNOTSUPP;
> +	if (key->type == &key_type_user) {
> +		down_read(&key->sem);
> +		status = key_validate(key);
> +		if (status == 0) {
> +			const struct user_key_payload *payload;
> +			uint8_t *duplicate;
> +
> +			payload = user_key_payload_locked(key);
> +
> +			duplicate = kmemdup(payload->data, payload->datalen,
> +					    GFP_KERNEL);
> +			if (duplicate) {
> +				*data = duplicate;
> +				ret = payload->datalen;
> +			} else {
> +				ret = -ENOMEM;
> +			}
> +		}
> +		up_read(&key->sem);
> +	}
> +
> +	key_put(key);
> +error:
> +	return ret;
> +}
> +
> +static void ecdh_free_data(struct ecdh *ecdh)
> +{
> +	kfree_sensitive(ecdh->key);
> +}
> +
> +long keyctl_ecdh_compute(struct keyctl_ecdh_params __user *params,
> +		       char __user *buffer, size_t buflen,
> +		       struct keyctl_curve_params __user *curve)
> +{
> +	long ret;
> +	ssize_t dlen;
> +	int secretlen;
> +	int outlen;
> +	struct keyctl_ecdh_params pcopy;
> +	struct ecdh ecdh_inputs;
> +	struct scatterlist insg;

in_sg

> +	struct scatterlist outsg;

out_sg

> +	DECLARE_CRYPTO_WAIT(compl);
> +	struct crypto_kpp *tfm;
> +	struct kpp_request *req;
> +	uint8_t *secret;
> +	uint8_t *outbuf;

out_buf

> +	char *dhname;
> +
> +	if (!params || (!buffer && buflen) || !curve) {
> +		ret = -EINVAL;
> +		goto out1;
> +	}

return -EINVAL; (remove out1 label)

> +
> +	if (copy_from_user(&pcopy, params, sizeof(pcopy)) != 0) {
> +		ret = -EFAULT;
> +		goto out1;
> +	}
> +
> +	memset(&ecdh_inputs, 0, sizeof(ecdh_inputs));
> +
> +	dlen = ecdh_data_from_key(pcopy.priv, &ecdh_inputs.key);
> +	if (dlen < 0) {
> +		ret = dlen;
> +		goto out1;
> +	}
> +	ecdh_inputs.key_size = dlen;
> +
> +	secretlen = crypto_ecdh_key_len(&ecdh_inputs);
> +	secret = kmalloc(secretlen, GFP_KERNEL);
> +	if (!secret) {
> +		ret = -ENOMEM;
> +		goto out2;

goto err_free_data;

> +	}
> +
> +	ret = crypto_ecdh_encode_key(secret, secretlen, &ecdh_inputs);
> +	if (ret)
> +		goto out3;


goto err_free_secret;

... you probably get the idea, please do this for all labels.

> +
> +	dhname = strndup_user(curve->curvename, CRYPTO_MAX_ALG_NAME);
> +
> +	tfm = crypto_alloc_kpp(dhname, 0, 0);
> +	if (IS_ERR(tfm)) {
> +		ret = PTR_ERR(tfm);
> +		goto out3;


So who free's dhname in this case?

BR, Jarkko


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

* Re: [PATCH] KEYS: Add ECDH support
  2024-03-30  7:04 ` Eric Biggers
@ 2024-03-30 13:09   ` James Bottomley
  2024-03-31  0:48     ` Eric Biggers
  2024-03-31 15:44     ` Jarkko Sakkinen
  0 siblings, 2 replies; 9+ messages in thread
From: James Bottomley @ 2024-03-30 13:09 UTC (permalink / raw)
  To: Eric Biggers, Zhang Yiqun
  Cc: dhowells, jarkko, corbet, keyrings, linux-doc, linux-kernel,
	linux-crypto

On Sat, 2024-03-30 at 00:04 -0700, Eric Biggers wrote:
> [+Cc linux-crypto]
> 
> On Sat, Mar 30, 2024 at 02:55:06PM +0800, Zhang Yiqun wrote:
> > This patch is to introduce ECDH into keyctl syscall for
> > userspace usage, containing public key generation and
> > shared secret computation.
> > 
> > It is mainly based on dh code, so it has the same condition
> > to the input which only user keys is supported. The output
> > result is storing into the buffer with the provided length.
> > 
> > Signed-off-by: Zhang Yiqun <zhangyiqun@phytium.com.cn>
> > ---
> >  Documentation/security/keys/core.rst |  62 ++++++
> >  include/linux/compat.h               |   4 +
> >  include/uapi/linux/keyctl.h          |  11 +
> >  security/keys/Kconfig                |  12 +
> >  security/keys/Makefile               |   2 +
> >  security/keys/compat_ecdh.c          |  50 +++++
> >  security/keys/ecdh.c                 | 318
> > +++++++++++++++++++++++++++
> >  security/keys/internal.h             |  44 ++++
> >  security/keys/keyctl.c               |  10 +
> >  9 files changed, 513 insertions(+)
> >  create mode 100644 security/keys/compat_ecdh.c
> >  create mode 100644 security/keys/ecdh.c
> 
> Nacked-by: Eric Biggers <ebiggers@google.com>
> 
> The existing KEYCTL_PKEY_*, KEYCTL_DH_COMPUTE, and AF_ALG are causing
> enough problems.  We do not need any more UAPIs like this.  They are
> hard to maintain, break often, not properly documented, increase the
> kernel's attack surface, and what they do is better done in
> userspace.

Actually that's not entirely true.  There is a use case for keys which
is where you'd like to harden unwrapped key handling and don't have the
ability to use a device.  The kernel provides a harder exfiltration
environment than user space, so there is a use case for getting the
kernel to handle operations on unwrapped keys for the protection it
affords the crytpographic key material.

For instance there are people who use the kernel keyring to replace
ssh-agent and thus *reduce* the attack surface they have for storing
ssh keys:

https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/

The same thing could be done with gpg keys or the gnome keyring.

> Please refer to the recent thread
> https://lore.kernel.org/linux-crypto/CZSHRUIJ4RKL.34T4EASV5DNJM@matfyz.cz/T/#u
> where these issues were discussed in detail.

This thread was talking about using the kernel for handling the
algorithms themselves (which is probably best done in userspace) and
didn't address using the kernel to harden the key protection
environment.

James


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

* Re: [PATCH] KEYS: Add ECDH support
  2024-03-30 13:09   ` James Bottomley
@ 2024-03-31  0:48     ` Eric Biggers
  2024-03-31  2:38       ` Denis Kenzior
  2024-03-31 13:01       ` James Bottomley
  2024-03-31 15:44     ` Jarkko Sakkinen
  1 sibling, 2 replies; 9+ messages in thread
From: Eric Biggers @ 2024-03-31  0:48 UTC (permalink / raw)
  To: James Bottomley
  Cc: Zhang Yiqun, dhowells, jarkko, corbet, keyrings, linux-doc,
	linux-kernel, linux-crypto

On Sat, Mar 30, 2024 at 09:09:51AM -0400, James Bottomley wrote:
> On Sat, 2024-03-30 at 00:04 -0700, Eric Biggers wrote:
> > [+Cc linux-crypto]
> > 
> > On Sat, Mar 30, 2024 at 02:55:06PM +0800, Zhang Yiqun wrote:
> > > This patch is to introduce ECDH into keyctl syscall for
> > > userspace usage, containing public key generation and
> > > shared secret computation.
> > > 
> > > It is mainly based on dh code, so it has the same condition
> > > to the input which only user keys is supported. The output
> > > result is storing into the buffer with the provided length.
> > > 
> > > Signed-off-by: Zhang Yiqun <zhangyiqun@phytium.com.cn>
> > > ---
> > >  Documentation/security/keys/core.rst |  62 ++++++
> > >  include/linux/compat.h               |   4 +
> > >  include/uapi/linux/keyctl.h          |  11 +
> > >  security/keys/Kconfig                |  12 +
> > >  security/keys/Makefile               |   2 +
> > >  security/keys/compat_ecdh.c          |  50 +++++
> > >  security/keys/ecdh.c                 | 318
> > > +++++++++++++++++++++++++++
> > >  security/keys/internal.h             |  44 ++++
> > >  security/keys/keyctl.c               |  10 +
> > >  9 files changed, 513 insertions(+)
> > >  create mode 100644 security/keys/compat_ecdh.c
> > >  create mode 100644 security/keys/ecdh.c
> > 
> > Nacked-by: Eric Biggers <ebiggers@google.com>
> > 
> > The existing KEYCTL_PKEY_*, KEYCTL_DH_COMPUTE, and AF_ALG are causing
> > enough problems.  We do not need any more UAPIs like this.  They are
> > hard to maintain, break often, not properly documented, increase the
> > kernel's attack surface, and what they do is better done in
> > userspace.
> 
> Actually that's not entirely true.  There is a use case for keys which
> is where you'd like to harden unwrapped key handling and don't have the
> ability to use a device.  The kernel provides a harder exfiltration
> environment than user space, so there is a use case for getting the
> kernel to handle operations on unwrapped keys for the protection it
> affords the crytpographic key material.
> 
> For instance there are people who use the kernel keyring to replace
> ssh-agent and thus *reduce* the attack surface they have for storing
> ssh keys:
> 
> https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/
> 
> The same thing could be done with gpg keys or the gnome keyring.

First, that blog post never actually said that the "replace ssh-agent with
kernel keyrings" idea was deployed.  It sounds like a proof of concept idea that
someone thought was interesting and decided to blog about.  Upstream OpenSSH has
no support for Linux keyrings.  It seems unlikely it would get added, especially
given the OpenSSH developers' healthy skepticism of using broken Linux-isms.
You're welcome to bring it up on openssh-unix-dev and get their buy-in first.

Second, as mentioned by the blog post, the kernel also does not support private
keys in the default OpenSSH format.  That sort of thing is an example of the
fundamental problem with trying to make the kernel support every cryptographic
protocol and format in existence.  Userspace simply has much more flexibility to
implement whatever it happens to need.

Third, ssh-agent is already a separate process, and like any other process the
kernel enforces isolation of its address space.  The potential loopholes are
ptrace and coredumps, which ssh-agent already disables, except for ptrace by
root which it can't do alone, but the system administrator can do that by
setting the ptrace_scope sysctl to 3 or by using SELinux.

Amusingly, the existing KEYCTL_DH_* APIs, and the KEYCTL_ECDH_* APIs proposed by
this patch, only operate on user keys that the process has READ access to.  This
means that the keys can be trivially extracted by a shell script running in your
user session.  That's *less* secure than using an isolated process...

That's not to mention that merging this will likely add vulnerabilities
reachable by unprivileged users, just as the original KEYCTL_DH_* did.  I had to
fix some of them last time around (e.g. commits 12d41a023efb, bbe240454d86,
3619dec5103d, 1d9ddde12e3c, ccd9888f14a8, 199512b1234f).  I don't really feel
like doing it again. (Wait, was this supposed to *improve* security?)

> > Please refer to the recent thread
> > https://lore.kernel.org/linux-crypto/CZSHRUIJ4RKL.34T4EASV5DNJM@matfyz.cz/T/#u
> > where these issues were discussed in detail.
> 
> This thread was talking about using the kernel for handling the
> algorithms themselves (which is probably best done in userspace) and
> didn't address using the kernel to harden the key protection
> environment.

This patch is about using the kernel to handle a class of algorithm,
Elliptic-Curve Diffie-Hellman.  Which specific algorithm in that class (i.e.
which elliptic curve), who knows.  Just like the existing APIs like this, it's
undocumented which algorithm(s) are actually supported.

- Eric

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

* Re: [PATCH] KEYS: Add ECDH support
  2024-03-31  0:48     ` Eric Biggers
@ 2024-03-31  2:38       ` Denis Kenzior
  2024-03-31  2:38         ` Denis Kenzior
  2024-03-31 13:01       ` James Bottomley
  1 sibling, 1 reply; 9+ messages in thread
From: Denis Kenzior @ 2024-03-31  2:38 UTC (permalink / raw)
  To: Eric Biggers, James Bottomley
  Cc: Zhang Yiqun, dhowells, jarkko, corbet, keyrings, linux-doc,
	linux-kernel, linux-crypto

Hi Eric,

> 
> Amusingly, the existing KEYCTL_DH_* APIs, and the KEYCTL_ECDH_* APIs proposed by
> this patch, only operate on user keys that the process has READ access to.  This
> means that the keys can be trivially extracted by a shell script running in your
> user session.  That's *less* secure than using an isolated process...
> 

I can see this being true for user or session keys, but I don't think this is 
true of process or thread specific keys.  At least I couldn't read any keys out 
of a test app when I tried.

Regards,
-Denis

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

* Re: [PATCH] KEYS: Add ECDH support
  2024-03-31  2:38       ` Denis Kenzior
@ 2024-03-31  2:38         ` Denis Kenzior
  0 siblings, 0 replies; 9+ messages in thread
From: Denis Kenzior @ 2024-03-31  2:38 UTC (permalink / raw)
  To: Eric Biggers, James Bottomley
  Cc: Zhang Yiqun, dhowells, jarkko, corbet, keyrings, linux-doc,
	linux-kernel, linux-crypto

Hi Eric,

> 
> Amusingly, the existing KEYCTL_DH_* APIs, and the KEYCTL_ECDH_* APIs proposed by
> this patch, only operate on user keys that the process has READ access to.  This
> means that the keys can be trivially extracted by a shell script running in your
> user session.  That's *less* secure than using an isolated process...
> 

I can see this being true for user or session keys, but I don't think this is 
true of process or thread specific keys.  At least I couldn't read any keys out 
of a test app when I tried.

Regards,
-Denis

X-sender: <linux-kernel+bounces-125931-steffen.klassert=secunet.com@vger.kernel.org>
X-Receiver: <steffen.klassert@secunet.com> ORCPT=rfc822;steffen.klassert@secunet.com NOTIFY=NEVER; X-ExtendedProps=BQAVABYAAgAAAAUAFAARAPDFCS25BAlDktII2g02frgPADUAAABNaWNyb3NvZnQuRXhjaGFuZ2UuVHJhbnNwb3J0LkRpcmVjdG9yeURhdGEuSXNSZXNvdXJjZQIAAAUAagAJAAEAAAAAAAAABQAWAAIAAAUAQwACAAAFAEYABwADAAAABQBHAAIAAAUAEgAPAGIAAAAvbz1zZWN1bmV0L291PUV4Y2hhbmdlIEFkbWluaXN0cmF0aXZlIEdyb3VwIChGWURJQk9IRjIzU1BETFQpL2NuPVJlY2lwaWVudHMvY249U3RlZmZlbiBLbGFzc2VydDY4YwUACwAXAL4AAACheZxkHSGBRqAcAp3ukbifQ049REI2LENOPURhdGFiYXNlcyxDTj1FeGNoYW5nZSBBZG1pbmlzdHJhdGl2ZSBHcm91cCAoRllESUJPSEYyM1NQRExUKSxDTj1BZG1pbmlzdHJhdGl2ZSBHcm91cHMsQ049c2VjdW5ldCxDTj1NaWNyb3NvZnQgRXhjaGFuZ2UsQ049U2VydmljZXMsQ049Q29uZmlndXJhdGlvbixEQz1zZWN1bmV0LERDPWRlBQAOABEABiAS9uuMOkqzwmEZDvWNNQUAHQAPAAwAAABtYngtZXNzZW4tMDIFADwAAgAADwA2AAAATWljcm9zb2Z0LkV4Y2hhbmdlLlRyYW5zcG9ydC5NYWlsUmVjaXBpZW50LkRpc3BsYXlOYW1lDwARAAAAS2xhc3NlcnQsIFN0ZWZmZW4FAAwAAgAABQBsAAIAAAUAWAAXAEoAAADwxQktuQQJQ5LSCNoNNn64Q049S2xhc3NlcnQgU3RlZmZlbixPVT1Vc2VycyxPVT1NaWdyYXRpb24sREM9c2VjdW5ldCxEQz1kZQUAJgACAAEFACIADwAxAAAAQXV0b1Jlc3BvbnNlU3VwcHJlc3M6IDANClRyYW5zbWl0SGlzdG9yeTogRmFsc2UNCg8ALwAAAE1pY3Jvc29mdC5FeGNoYW5nZS5UcmFuc3BvcnQuRXhwYW5zaW9uR3JvdXBUeXBlDwAVAAAATWVtYmVyc0dyb3VwRXhwYW5zaW9uBQAjAAIAAQ==
X-CreatedBy: MSExchange15
X-HeloDomain: b.mx.secunet.com
X-ExtendedProps: BQBjAAoAwapAQuxQ3AgFAGEACAABAAAABQA3AAIAAA8APAAAAE1pY3Jvc29mdC5FeGNoYW5nZS5UcmFuc3BvcnQuTWFpbFJlY2lwaWVudC5Pcmdhbml6YXRpb25TY29wZREAAAAAAAAAAAAAAAAAAAAAAAUASQACAAEFAGIACgB2AAAAqIoAAAUABAAUIAEAAAAcAAAAc3RlZmZlbi5rbGFzc2VydEBzZWN1bmV0LmNvbQUABgACAAEFACkAAgABDwAJAAAAQ0lBdWRpdGVkAgABBQACAAcAAQAAAAUAAwAHAAAAAAAFAAUAAgABBQBkAA8AAwAAAEh1Yg==
X-Source: SMTP:Default MBX-DRESDEN-01
X-SourceIPAddress: 62.96.220.37
X-EndOfInjectedXHeaders: 12053
Received: from cas-essen-02.secunet.de (10.53.40.202) by
 mbx-dresden-01.secunet.de (10.53.40.199) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
 15.1.2507.37; Sun, 31 Mar 2024 04:38:26 +0200
Received: from b.mx.secunet.com (62.96.220.37) by cas-essen-02.secunet.de
 (10.53.40.202) with Microsoft SMTP Server (version=TLS1_2,
 cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.37 via Frontend
 Transport; Sun, 31 Mar 2024 04:38:26 +0200
Received: from localhost (localhost [127.0.0.1])
	by b.mx.secunet.com (Postfix) with ESMTP id 2AB2D20322
	for <steffen.klassert@secunet.com>; Sun, 31 Mar 2024 04:38:26 +0200 (CEST)
X-Virus-Scanned: by secunet
X-Spam-Flag: NO
X-Spam-Score: -2.749
X-Spam-Level:
X-Spam-Status: No, score=-2.749 tagged_above=-999 required=2.1
	tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1,
	DKIM_VALID_AU=-0.1, FREEMAIL_FORGED_FROMDOMAIN=0.001,
	FREEMAIL_FROM=0.001, HEADER_FROM_DIFFERENT_DOMAINS=0.249,
	MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_NONE=-0.0001,
	SPF_HELO_NONE=0.001, SPF_PASS=-0.001]
	autolearn=unavailable autolearn_force=no
Authentication-Results: a.mx.secunet.com (amavisd-new);
	dkim=pass (2048-bit key) header.d=gmail.com
Received: from b.mx.secunet.com ([127.0.0.1])
	by localhost (a.mx.secunet.com [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id Y6JZ0jC6kB3o for <steffen.klassert@secunet.com>;
	Sun, 31 Mar 2024 04:38:25 +0200 (CEST)
Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=147.75.80.249; helo=am.mirrors.kernel.org; envelope-from=linux-kernel+bounces-125931-steffen.klassert=secunet.com@vger.kernel.org; receiver=steffen.klassert@secunet.com 
DKIM-Filter: OpenDKIM Filter v2.11.0 b.mx.secunet.com A615920199
Authentication-Results: b.mx.secunet.com;
	dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="XleIRPSp"
Received: from am.mirrors.kernel.org (am.mirrors.kernel.org [147.75.80.249])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by b.mx.secunet.com (Postfix) with ESMTPS id A615920199
	for <steffen.klassert@secunet.com>; Sun, 31 Mar 2024 04:38:25 +0200 (CEST)
Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by am.mirrors.kernel.org (Postfix) with ESMTPS id 584181F218D9
	for <steffen.klassert@secunet.com>; Sun, 31 Mar 2024 02:38:25 +0000 (UTC)
Received: from localhost.localdomain (localhost.localdomain [127.0.0.1])
	by smtp.subspace.kernel.org (Postfix) with ESMTP id EB1C03D6D;
	Sun, 31 Mar 2024 02:38:11 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org;
	dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="XleIRPSp"
Received: from mail-oo1-f45.google.com (mail-oo1-f45.google.com [209.85.161.45])
	(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))
	(No client certificate requested)
	by smtp.subspace.kernel.org (Postfix) with ESMTPS id 54A6C181;
	Sun, 31 Mar 2024 02:38:08 +0000 (UTC)
Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.161.45
ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;
	t=1711852689; cv=none; b=gXNEolHo55cI9s9E0fe7uOOSm88Jz7dwj3ls8ge3nw8RDM4vYnsK3QkV/TYCu8HKWXSxelrGFg26OaTa0ta2xAeaumLm+bNicuklMkDBxzgMakTmXxNf8xfV/uZLU1lr3i868qhdgUvfJgx0ptM9DjM8hr8IuQzNZ6hDb2tE66w=
ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org;
	s=arc-20240116; t=1711852689; c=relaxed/simple;
	bh=SNeN2CYj+6bo6yYIrP8F5A4iIl0/Q9yGn+qoKYRUWH4=;
	h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:
	 In-Reply-To:Content-Type; b=WX0BCLiJLkXYSF23cXoAUUoCaN3U++73B96a084d5eByR6abt19vx+RRgPeFHn/FNkK/J6TmDIJzyF4IYk3FZTEjQ9I/pyxKjnmYqJBhqHBxbDk+/e+NGJ90rlOfa4MF1hGhvlAequCF3PKJT9TuvWJc3UIpKmFlHj11ZC0GCMk=
ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=XleIRPSp; arc=none smtp.client-ip=209.85.161.45
Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com
Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com
Received: by mail-oo1-f45.google.com with SMTP id 006d021491bc7-5a47cecb98bso2073304eaf.0;
        Sat, 30 Mar 2024 19:38:08 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=gmail.com; s=20230601; t=1711852687; x=1712457487; darn=vger.kernel.org;
        h=content-transfer-encoding:in-reply-to:from:references:cc:to
         :content-language:subject:user-agent:mime-version:date:message-id
         :from:to:cc:subject:date:message-id:reply-to;
        bh=lUs6NaIvupBIrN3kNgIHykr6WEWtZD3EhPX18G9uddY=;
        b=XleIRPSp55P9VHB7a2r/titnJwBaAjVmwFFWncW/trJnpln7+XtSjSvi9uqMgHENno
         mXoHhat/Z/Iu/etVc504MD8mbcqjpCdo92CyUAjqoOvDmqxWOTlUEoKSZpMXMU1tjGDE
         XbpXwWhrrBDGTCSBhMimQlOAAiFIgIn6MMASG45+bZdtNZH3XVRJ5bVJUjjXsZsqVSuD
         EAr0yLfv7Xw4ek1Nrgh1EsDej1shKOAN+fHmpmCt2k67uc0kQqgZTsLvlgNdkJABYumM
         NqMAc3CT/Ikjfg8Q4m9fVK1ahVo7HmKBBnIuDdfrFTz3L4Mf85eUXTvDWIq6NjILU5Zk
         EE5A==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20230601; t=1711852687; x=1712457487;
        h=content-transfer-encoding:in-reply-to:from:references:cc:to
         :content-language:subject:user-agent:mime-version:date:message-id
         :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;
        bh=lUs6NaIvupBIrN3kNgIHykr6WEWtZD3EhPX18G9uddY=;
        b=dR8iI2wu/bgmWoKgd3fQy1Qs3x8Gg8zaFnPHbg0FZVEZ0RXNeEaOyPNYJ1aIfjy24D
         15h1F4+67W51lrSYAej/3JXzlWZZfwN+sEoBc+2m+UdBjvHf18AI150uR/j7E1kwLdbX
         sGYqzl76u5sQvTr0S681UIXwuJI8SbyuckQSRFHBqVNDfJVH0TIeYJflzO0R6FpSMC8R
         9zz21UCZ3xMyhIWChGyQmc7Wt48iBBORmO2pxhHMcl7c4qiPRydQGd654U39D+gB3weL
         ELldBLvIApWZmFYYutf7AN/jmQk6rSAo74Gm8P8UkZUIxFx+1qy0xVLs34o8roNuo1WX
         +QIg==
X-Forwarded-Encrypted: i=1; AJvYcCVwL3cU3nCsvXYDTwS66oYsIOZe9xNTNPxXhx+b5LI0hF4hupv8P5wIUYO2JPUDl0WepDijhDJPYBg1N560PbnJ5RAa4R88i26Vu9VypwbBZecB5aqbsTeOFXgu4wuUSU6yA7yyNW3bsx+drdJwvoi1WZf5gLyATZ+18fbURnSBI4TAocIuILlIqVomkqoToJcDzLA9S5fTrbiTCkqMZeE=
X-Gm-Message-State: AOJu0YyW7TJsviPHgdYwWIVD+v3Bv1LiX1phxqUeZ9O5THjJ2TKywZ9M
	IdQhsJIEEtX7xf5p5m/dh/a51J+VTrkHVZa0tY90NWObJeeGoGmG
X-Google-Smtp-Source: AGHT+IG8y/mw6Sg+NJ68AiBnOVhIGZhncP4yQFjwCnn6QvLTTTIKr8wBQM2lppgPQLGZ1h9+K7oVJA==
X-Received: by 2002:a05:6820:260e:b0:5a5:639a:2fb8 with SMTP id cy14-20020a056820260e00b005a5639a2fb8mr6074835oob.4.1711852687375;
        Sat, 30 Mar 2024 19:38:07 -0700 (PDT)
Received: from [192.168.1.22] (070-114-247-242.res.spectrum.com. [70.114.247.242])
        by smtp.googlemail.com with ESMTPSA id bf14-20020a056820174e00b005a4bcb155basm1611035oob.23.2024.03.30.19.38.06
        (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);
        Sat, 30 Mar 2024 19:38:07 -0700 (PDT)
Message-ID: <aae9bc89-ca34-400f-9c5e-44be1df2befa@gmail.com>
Date: Sat, 30 Mar 2024 21:38:05 -0500
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] KEYS: Add ECDH support
Content-Language: en-US
To: Eric Biggers <ebiggers@kernel.org>,
 James Bottomley <James.Bottomley@hansenpartnership.com>
Cc: Zhang Yiqun <zhangyiqun@phytium.com.cn>, dhowells@redhat.com,
 jarkko@kernel.org, corbet@lwn.net, keyrings@vger.kernel.org,
 linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-crypto@vger.kernel.org
References: <20240330065506.3146-1-zhangyiqun@phytium.com.cn>
 <20240330070436.GA2116@sol.localdomain>
 <087bbfcf95c9014ee8f87d482773244f0833b892.camel@HansenPartnership.com>
 <20240331004844.GA104623@sol.localdomain>
From: Denis Kenzior <denkenz@gmail.com>
In-Reply-To: <20240331004844.GA104623@sol.localdomain>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Return-Path: linux-kernel+bounces-125931-steffen.klassert=secunet.com@vger.kernel.org
X-MS-Exchange-Organization-OriginalArrivalTime: 31 Mar 2024 02:38:26.2603
 (UTC)
X-MS-Exchange-Organization-Network-Message-Id: 25f48256-d726-4dcd-c568-08dc512ba45a
X-MS-Exchange-Organization-OriginalClientIPAddress: 62.96.220.37
X-MS-Exchange-Organization-OriginalServerIPAddress: 10.53.40.202
X-MS-Exchange-Organization-Cross-Premises-Headers-Processed: cas-essen-02.secunet.de
X-MS-Exchange-Organization-OrderedPrecisionLatencyInProgress: LSRV=mbx-dresden-01.secunet.de:TOTAL-HUB=0.407|SMR=0.357(SMRDE=0.050|SMRC=0.306(SMRCL=0.100|X-SMRCR=0.306))|CAT=0.049(CATOS=0.011
 (CATSM=0.011(CATSM-Malware
 Agent=0.011))|CATRESL=0.023(CATRESLP2R=0.017)|CATORES=0.012
 (CATRS=0.012(CATRS-Index Routing Agent=0.011)));2024-03-31T02:38:26.699Z
X-MS-Exchange-Forest-ArrivalHubServer: mbx-dresden-01.secunet.de
X-MS-Exchange-Organization-AuthSource: cas-essen-02.secunet.de
X-MS-Exchange-Organization-AuthAs: Anonymous
X-MS-Exchange-Organization-FromEntityHeader: Internet
X-MS-Exchange-Organization-OriginalSize: 9469
X-MS-Exchange-Organization-HygienePolicy: Standard
X-MS-Exchange-Organization-MessageLatency: SRV=cas-essen-02.secunet.de:TOTAL-FE=0.032|SMR=0.022(SMRPI=0.020(SMRPI-FrontendProxyAgent=0.019))|SMS=0.009
X-MS-Exchange-Organization-AVStamp-Enterprise: 1.0
X-MS-Exchange-Organization-Recipient-Limit-Verified: True
X-MS-Exchange-Organization-TotalRecipientCount: 1
X-MS-Exchange-Organization-Rules-Execution-History: 0b0cf904-14ac-4724-8bdf-482ee6223cf2%%%fd34672d-751c-45ae-a963-ed177fcabe23%%%d8080257-b0c3-47b4-b0db-23bc0c8ddb3c%%%95e591a2-5d7d-4afa-b1d0-7573d6c0a5d9%%%f7d0f6bc-4dcc-4876-8c5d-b3d6ddbb3d55%%%16355082-c50b-4214-9c7d-d39575f9f79b
X-MS-Exchange-Forest-RulesExecuted: mbx-dresden-01
X-MS-Exchange-Organization-RulesExecuted: mbx-dresden-01
X-MS-Exchange-Forest-IndexAgent-0: AQ0CZW4AAfgBAAAPAAADH4sIAAAAAAAEAE1SXWvbMBRVE38k7jLYP7
 hvheDlJwxCE2jYHkbpy56KYt/Uoq5kJHmr/9N+5I6kzBQuRvfcc849
 Ev6bPSg6WtXUm2pTfUPR/m10Sr/0U02+Y+J35Tx6+n78df/04/nw8L
 yl/c+Tq0nqNlKuk+P9PKPBmsE4buk8BU/fKWDSN11NRvcTmYGt9IyG
 RseWXnlyYEkfDaFu2DnqpKPH4/5Asom9NzuiJ3gFzzeW+oMmOjRS05
 nJW/VbyR57+N1b2fgYhCS5jvueXGPV4MmOWoeLKU2TGW3wjFkcVimj
 4yrp7xxteyBb4M1oOWwMoYMSB+VML4P/NfNut4vvuKlOMY1jTrc/c1
 B4OzJdjE2bzLwsxq/pPHo6UWv0XbiU0q9JGy4cleYyvw20vrMsW3ID
 N+qimuiB1HtPPUsXnBoz9m0wi0Spp/RMZvSbCl6SPIMnh4H+dKwhwM
 txuwvxH/lF2tbhv/h6YB0SbCohFmKZic9rcbsU2UqsixtRiXIh8kwU
 QFBAPokyC5WDsBAFDnlsS7GK8jwVyEDwxbm4mqzyyE8gvuCvRYXC4U
 YsMAXzv88qelZFOAQ8Liowwt5SfAE/RUr4bAtCGSVok2eaLj5M8YUh
 fLIQbAkCmLGds+Up0oxEq6oUt/k/l0NX+VQDAAABDs4BUmV0cmlldm
 VyT3BlcmF0b3IsMTAsMTtSZXRyaWV2ZXJPcGVyYXRvciwxMSwwO1Bv
 c3REb2NQYXJzZXJPcGVyYXRvciwxMCwwO1Bvc3REb2NQYXJzZXJPcG
 VyYXRvciwxMSwwO1Bvc3RXb3JkQnJlYWtlckRpYWdub3N0aWNPcGVy
 YXRvciwxMCwwO1Bvc3RXb3JkQnJlYWtlckRpYWdub3N0aWNPcGVyYX
 RvciwxMSwwO1RyYW5zcG9ydFdyaXRlclByb2R1Y2VyLDIwLDY=
X-MS-Exchange-Forest-IndexAgent: 1 725
X-MS-Exchange-Forest-EmailMessageHash: 759BD60A
X-MS-Exchange-Forest-Language: en
X-MS-Exchange-Organization-Processed-By-Journaling: Journal Agent

Hi Eric,

> 
> Amusingly, the existing KEYCTL_DH_* APIs, and the KEYCTL_ECDH_* APIs proposed by
> this patch, only operate on user keys that the process has READ access to.  This
> means that the keys can be trivially extracted by a shell script running in your
> user session.  That's *less* secure than using an isolated process...
> 

I can see this being true for user or session keys, but I don't think this is 
true of process or thread specific keys.  At least I couldn't read any keys out 
of a test app when I tried.

Regards,
-Denis


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

* Re: [PATCH] KEYS: Add ECDH support
  2024-03-31  0:48     ` Eric Biggers
  2024-03-31  2:38       ` Denis Kenzior
@ 2024-03-31 13:01       ` James Bottomley
  1 sibling, 0 replies; 9+ messages in thread
From: James Bottomley @ 2024-03-31 13:01 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Zhang Yiqun, dhowells, jarkko, corbet, keyrings, linux-doc,
	linux-kernel, linux-crypto

On Sat, 2024-03-30 at 17:48 -0700, Eric Biggers wrote:
> On Sat, Mar 30, 2024 at 09:09:51AM -0400, James Bottomley wrote:
[...]
> > For instance there are people who use the kernel keyring to replace
> > ssh-agent and thus *reduce* the attack surface they have for
> > storing
> > ssh keys:
> > 
> > https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/
> > 
> > The same thing could be done with gpg keys or the gnome keyring.
> 
> First, that blog post never actually said that the "replace ssh-agent
> with kernel keyrings" idea was deployed.  It sounds like a proof of
> concept idea that someone thought was interesting and decided to blog
> about.  Upstream OpenSSH has no support for Linux keyrings.

The openssh community is incredibly resistant to out of house
innovation.  It has no support for engine or provider keys, for TPM
keys, or for that systemd start patch xz just exploited ...

>   It seems unlikely it would get added, especially given the OpenSSH
> developers' healthy skepticism of using broken Linux-isms.
> You're welcome to bring it up on openssh-unix-dev and get their buy-
> in first.

I also didn't say just openssh.  You picked the one you apparently know
hardly ever accepts anyone else's ideas.  I don't disagree that finding
implementors is reasonable ... I just wouldn't pick openssh as the
first upstream target.

> Second, as mentioned by the blog post, the kernel also does not
> support private keys in the default OpenSSH format.  That sort of
> thing is an example of the fundamental problem with trying to make
> the kernel support every cryptographic protocol and format in
> existence.  Userspace simply has much more flexibility to implement
> whatever it happens to need.

That's a complete red herring.  You don't need the kernel keyrings to
support every format, you just need a user space converter to import to
the kernel keyring format.  Every device or token that can replace key
handling has their own internal format and they all come with importers
that do conversion.

> Third, ssh-agent is already a separate process, and like any other
> process the kernel enforces isolation of its address space.  The
> potential loopholes are ptrace and coredumps, which ssh-agent already
> disables, except for ptrace by root which it can't do alone, but the
> system administrator can do that by setting the ptrace_scope sysctl
> to 3 or by using SELinux.

Well, a) this doesn't survive privilege escalation and b) I don't think
many people would buy into the notion that we should remove security
functions from the kernel and give them to userspace daemons because
it's safer.

James


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

* Re: [PATCH] KEYS: Add ECDH support
  2024-03-30 13:09   ` James Bottomley
  2024-03-31  0:48     ` Eric Biggers
@ 2024-03-31 15:44     ` Jarkko Sakkinen
  1 sibling, 0 replies; 9+ messages in thread
From: Jarkko Sakkinen @ 2024-03-31 15:44 UTC (permalink / raw)
  To: James Bottomley, Eric Biggers, Zhang Yiqun
  Cc: dhowells, corbet, keyrings, linux-doc, linux-kernel, linux-crypto

On Sat Mar 30, 2024 at 3:09 PM EET, James Bottomley wrote:
> On Sat, 2024-03-30 at 00:04 -0700, Eric Biggers wrote:
> > [+Cc linux-crypto]
> > 
> > On Sat, Mar 30, 2024 at 02:55:06PM +0800, Zhang Yiqun wrote:
> > > This patch is to introduce ECDH into keyctl syscall for
> > > userspace usage, containing public key generation and
> > > shared secret computation.
> > > 
> > > It is mainly based on dh code, so it has the same condition
> > > to the input which only user keys is supported. The output
> > > result is storing into the buffer with the provided length.
> > > 
> > > Signed-off-by: Zhang Yiqun <zhangyiqun@phytium.com.cn>
> > > ---
> > >  Documentation/security/keys/core.rst |  62 ++++++
> > >  include/linux/compat.h               |   4 +
> > >  include/uapi/linux/keyctl.h          |  11 +
> > >  security/keys/Kconfig                |  12 +
> > >  security/keys/Makefile               |   2 +
> > >  security/keys/compat_ecdh.c          |  50 +++++
> > >  security/keys/ecdh.c                 | 318
> > > +++++++++++++++++++++++++++
> > >  security/keys/internal.h             |  44 ++++
> > >  security/keys/keyctl.c               |  10 +
> > >  9 files changed, 513 insertions(+)
> > >  create mode 100644 security/keys/compat_ecdh.c
> > >  create mode 100644 security/keys/ecdh.c
> > 
> > Nacked-by: Eric Biggers <ebiggers@google.com>
> > 
> > The existing KEYCTL_PKEY_*, KEYCTL_DH_COMPUTE, and AF_ALG are causing
> > enough problems.  We do not need any more UAPIs like this.  They are
> > hard to maintain, break often, not properly documented, increase the
> > kernel's attack surface, and what they do is better done in
> > userspace.
>
> Actually that's not entirely true.  There is a use case for keys which
> is where you'd like to harden unwrapped key handling and don't have the
> ability to use a device.  The kernel provides a harder exfiltration
> environment than user space, so there is a use case for getting the
> kernel to handle operations on unwrapped keys for the protection it
> affords the crytpographic key material.
>
> For instance there are people who use the kernel keyring to replace
> ssh-agent and thus *reduce* the attack surface they have for storing
> ssh keys:
>
> https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/
>
> The same thing could be done with gpg keys or the gnome keyring.

Eric has a correct standing given that the commit message does not have
motivation part at all. 

With a description of the problem that this patch is supposed to solve
this would be more meaningful to review.

BR, Jarkko

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

end of thread, other threads:[~2024-03-31 16:28 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-30  6:55 [PATCH] KEYS: Add ECDH support Zhang Yiqun
2024-03-30  7:04 ` Eric Biggers
2024-03-30 13:09   ` James Bottomley
2024-03-31  0:48     ` Eric Biggers
2024-03-31  2:38       ` Denis Kenzior
2024-03-31  2:38         ` Denis Kenzior
2024-03-31 13:01       ` James Bottomley
2024-03-31 15:44     ` Jarkko Sakkinen
2024-03-30 11:00 ` Jarkko Sakkinen

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