Re: [v2 PATCH 5/5] Bluetooth: let the crypto subsystem generate the ecc privkey

[Marcel Holtmann]

Hi Tudor,

>>> That Bluetooth SMP knows about the private key is pointless, since the
>>> detection of debug key usage is actually via the public key portion.
>>> With this patch, the Bluetooth SMP will stop keeping a copy of the
>>> ecdh private key and will let the crypto subsystem to generate and
>>> handle the ecdh private key, potentially benefiting of hardware
>>> ecc private key generation and retention.
>>> 
>>> The loop that tries to generate a correct private key is now removed and
>>> we trust the crypto subsystem to generate a correct private key. This
>>> backup logic should be done in crypto, if really needed.
>>> 
>>> Signed-off-by: Tudor Ambarus 
>>> ---
>>> net/bluetooth/ecdh_helper.c | 186 
>>> 
>>> net/bluetooth/ecdh_helper.h |   9 ++-
>>> net/bluetooth/selftest.c|  14 +++-
>>> net/bluetooth/smp.c |  66 +++-
>>> 4 files changed, 147 insertions(+), 128 deletions(-)
>>> 
>>> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
>>> index 16e022f..2155ce8 100644
>>> --- a/net/bluetooth/ecdh_helper.c
>>> +++ b/net/bluetooth/ecdh_helper.c
>>> @@ -49,15 +49,21 @@ static inline void swap_digits(u64 *in, u64 *out, 
>>> unsigned int ndigits)
>>> out[i] = __swab64(in[ndigits - 1 - i]);
>>> }
>>> 
>>> +/* compute_ecdh_secret() - function assumes that the private key was
>>> + * already set.
>>> + * @tfm:  KPP tfm handle allocated with crypto_alloc_kpp().
>>> + * @public_key:   pair's ecc public key.
>>> + * secret:memory where the ecdh computed shared secret will be 
>>> saved.
>>> + *
>>> + * Return: zero on success; error code in case of error.
>>> + */
>>> int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
>>> -   const u8 private_key[32], u8 secret[32])
>>> +   u8 secret[32])
>>> {
>>> struct kpp_request *req;
>>> -   struct ecdh p;
>>> +   u8 *tmp;
>>> struct ecdh_completion result;
>>> struct scatterlist src, dst;
>>> -   u8 *tmp, *buf;
>>> -   unsigned int buf_len;
>>> int err;
>>> 
>>> tmp = kmalloc(64, GFP_KERNEL);
>>> @@ -72,28 +78,6 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
>>> public_key[64],
>>> 
>>> init_completion();
>>> 
>>> -   /* Security Manager Protocol holds digits in litte-endian order
>>> -* while ECC API expect big-endian data
>>> -*/
>>> -   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
>>> -   p.key = (char *)tmp;
>>> -   p.key_size = 32;
>>> -   /* Set curve_id */
>>> -   p.curve_id = ECC_CURVE_NIST_P256;
>>> -   buf_len = crypto_ecdh_key_len();
>>> -   buf = kmalloc(buf_len, GFP_KERNEL);
>>> -   if (!buf) {
>>> -   err = -ENOMEM;
>>> -   goto free_req;
>>> -   }
>>> -
>>> -   crypto_ecdh_encode_key(buf, buf_len, );
>>> -
>>> -   /* Set A private Key */
>>> -   err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
>>> -   if (err)
>>> -   goto free_all;
>>> -
>>> swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
>>> swap_digits((u64 *)_key[32], (u64 *)[32], 4); /* y */
>>> 
>>> @@ -118,26 +102,76 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const 
>>> u8 public_key[64],
>>> memcpy(secret, tmp, 32);
>>> 
>>> free_all:
>>> -   kzfree(buf);
>>> -free_req:
>>> kpp_request_free(req);
>>> free_tmp:
>>> kzfree(tmp);
>>> return err;
>>> }
>>> 
>>> -int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
>>> -  u8 private_key[32])
>>> +/* set_ecdh_privkey() - set or generate ecc private key.
>>> + *
>>> + * Function generates an ecc private key in the crypto subsystem when 
>>> receiving
>>> + * a NULL private key or sets the received key when not NULL.
>>> + *
>>> + * @tfm:   KPP tfm handle allocated with crypto_alloc_kpp().
>>> + * @private_key:   user's ecc private key. When not NULL, the key is 
>>> expected
>>> + * in little endian format.
>>> + *
>>> + * Return: zero on success; error code in case of error.
>>> + */
>>> +int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 private_key[32])
>>> +{
>>> +   u8 *buf, *tmp = NULL;
>>> +   unsigned int buf_len;
>>> +   int err;
>>> +   struct ecdh p = {0};
>>> +
>>> +   p.curve_id = ECC_CURVE_NIST_P256;
>>> +
>>> +   if (private_key) {
>>> +   tmp = kmalloc(32, GFP_KERNEL);
>>> +   if (!tmp)
>>> +   return -ENOMEM;
>>> +   swap_digits((u64 *)private_key, (u64 *)tmp, 4);
>>> +   p.key = tmp;
>>> +   p.key_size = 32;
>>> +   }
>>> +
>>> +   buf_len = crypto_ecdh_key_len();
>>> +   buf = kmalloc(buf_len, GFP_KERNEL);
>>> +   if (!buf) {
>>> +   err = -ENOMEM;
>>> +   goto free_tmp;
>>> +   }
>>> +
>>> +   err = crypto_ecdh_encode_key(buf, buf_len, );
>>> +   if (err)
>>> +   goto free_all;
>>> +
>>> +   err = crypto_kpp_set_secret(tfm, buf, buf_len);
>>> +   /* fall through */

Re: [v2 PATCH 0/5] Bluetooth: let the crypto subsystem generate the ecc privkey

[Marcel Holtmann]

Hi Tudor,

> That Bluetooth SMP knows about the private key is pointless, since the
> detection of debug key usage is actually via the public key portion.
> With this patch set, the Bluetooth SMP will stop keeping a copy of the
> ecdh private key. We let the crypto subsystem to generate and handle
> the ecdh private key, potentially benefiting of hardware ecc private key
> generation and retention.
> 
> Tested with selftest and with btmon and smp-tester on top of hci_vhci,
> with ecdh done in both software and hardware (through atmel-ecc driver).
> All tests passed.
> 
> RFC version can be found at:
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg28036.html
> 
> Changes in v2:
> - add patches 2, 3, 4.
> - adress Marcel's suggestions:
>  - revive the check for accidentally generated debug keys
>  - bypass the handling of private key to the crypto subsytem,
>even when using debug keys.
> 
> 
> Tudor Ambarus (5):
>  Bluetooth: move ecdh allocation outside of ecdh_helper
>  Bluetooth: ecdh_helper - reveal error codes
>  Bluetooth: selftest - check for errors when computing ZZ
>  Bluetooth: ecdh_helper - fix leak of private key
>  Bluetooth: let the crypto subsystem generate the ecc privkey
> 
> net/bluetooth/ecdh_helper.c | 228 ++--
> net/bluetooth/ecdh_helper.h |   9 +-
> net/bluetooth/selftest.c|  46 +++--
> net/bluetooth/smp.c | 127 +++-
> 4 files changed, 240 insertions(+), 170 deletions(-)

all 5 patches have been applied to bluetooth-next tree.

Regards

Marcel

Re: [v2 PATCH 5/5] Bluetooth: let the crypto subsystem generate the ecc privkey

[Marcel Holtmann]

Hi Tudor,

> That Bluetooth SMP knows about the private key is pointless, since the
> detection of debug key usage is actually via the public key portion.
> With this patch, the Bluetooth SMP will stop keeping a copy of the
> ecdh private key and will let the crypto subsystem to generate and
> handle the ecdh private key, potentially benefiting of hardware
> ecc private key generation and retention.
> 
> The loop that tries to generate a correct private key is now removed and
> we trust the crypto subsystem to generate a correct private key. This
> backup logic should be done in crypto, if really needed.
> 
> Signed-off-by: Tudor Ambarus 
> ---
> net/bluetooth/ecdh_helper.c | 186 
> net/bluetooth/ecdh_helper.h |   9 ++-
> net/bluetooth/selftest.c|  14 +++-
> net/bluetooth/smp.c |  66 +++-
> 4 files changed, 147 insertions(+), 128 deletions(-)
> 
> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
> index 16e022f..2155ce8 100644
> --- a/net/bluetooth/ecdh_helper.c
> +++ b/net/bluetooth/ecdh_helper.c
> @@ -49,15 +49,21 @@ static inline void swap_digits(u64 *in, u64 *out, 
> unsigned int ndigits)
>   out[i] = __swab64(in[ndigits - 1 - i]);
> }
> 
> +/* compute_ecdh_secret() - function assumes that the private key was
> + * already set.
> + * @tfm:  KPP tfm handle allocated with crypto_alloc_kpp().
> + * @public_key:   pair's ecc public key.
> + * secret:memory where the ecdh computed shared secret will be saved.
> + *
> + * Return: zero on success; error code in case of error.
> + */
> int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
> - const u8 private_key[32], u8 secret[32])
> + u8 secret[32])
> {
>   struct kpp_request *req;
> - struct ecdh p;
> + u8 *tmp;
>   struct ecdh_completion result;
>   struct scatterlist src, dst;
> - u8 *tmp, *buf;
> - unsigned int buf_len;
>   int err;
> 
>   tmp = kmalloc(64, GFP_KERNEL);
> @@ -72,28 +78,6 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
> public_key[64],
> 
>   init_completion();
> 
> - /* Security Manager Protocol holds digits in litte-endian order
> -  * while ECC API expect big-endian data
> -  */
> - swap_digits((u64 *)private_key, (u64 *)tmp, 4);
> - p.key = (char *)tmp;
> - p.key_size = 32;
> - /* Set curve_id */
> - p.curve_id = ECC_CURVE_NIST_P256;
> - buf_len = crypto_ecdh_key_len();
> - buf = kmalloc(buf_len, GFP_KERNEL);
> - if (!buf) {
> - err = -ENOMEM;
> - goto free_req;
> - }
> -
> - crypto_ecdh_encode_key(buf, buf_len, );
> -
> - /* Set A private Key */
> - err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
> - if (err)
> - goto free_all;
> -
>   swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
>   swap_digits((u64 *)_key[32], (u64 *)[32], 4); /* y */
> 
> @@ -118,26 +102,76 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const 
> u8 public_key[64],
>   memcpy(secret, tmp, 32);
> 
> free_all:
> - kzfree(buf);
> -free_req:
>   kpp_request_free(req);
> free_tmp:
>   kzfree(tmp);
>   return err;
> }
> 
> -int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
> -u8 private_key[32])
> +/* set_ecdh_privkey() - set or generate ecc private key.
> + *
> + * Function generates an ecc private key in the crypto subsystem when 
> receiving
> + * a NULL private key or sets the received key when not NULL.
> + *
> + * @tfm:   KPP tfm handle allocated with crypto_alloc_kpp().
> + * @private_key:   user's ecc private key. When not NULL, the key is expected
> + * in little endian format.
> + *
> + * Return: zero on success; error code in case of error.
> + */
> +int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 private_key[32])
> +{
> + u8 *buf, *tmp = NULL;
> + unsigned int buf_len;
> + int err;
> + struct ecdh p = {0};
> +
> + p.curve_id = ECC_CURVE_NIST_P256;
> +
> + if (private_key) {
> + tmp = kmalloc(32, GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> + swap_digits((u64 *)private_key, (u64 *)tmp, 4);
> + p.key = tmp;
> + p.key_size = 32;
> + }
> +
> + buf_len = crypto_ecdh_key_len();
> + buf = kmalloc(buf_len, GFP_KERNEL);
> + if (!buf) {
> + err = -ENOMEM;
> + goto free_tmp;
> + }
> +
> + err = crypto_ecdh_encode_key(buf, buf_len, );
> + if (err)
> + goto free_all;
> +
> + err = crypto_kpp_set_secret(tfm, buf, buf_len);
> + /* fall through */
> +free_all:
> + kzfree(buf);
> +free_tmp:
> + kzfree(tmp);
> + return err;
> +}
> +
> +/* generate_ecdh_public_key() - function assumes that the 

Re: [RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey

[Marcel Holtmann]

Hi Tudor,

> That Bluetooth SMP knows about the private key is pointless, since the
> detection of debug key usage is actually via the public key portion.
> With this patch, the Bluetooth SMP will stop keeping a copy of the
> ecdh private key, except when using debug keys. This way we let the
> crypto subsystem to generate and handle the ecdh private key,
> potentially benefiting of hardware ecc private key generation and
> retention.
> 
> The loop that tries to generate a correct private key is now removed and
> we trust the crypto subsystem to generate a correct private key. This
> backup logic should be done in crypto, if really needed.
> 
> Keeping the private key in the crypto subsystem implies that we can't
> check if we accidentally generated a debug key. As this event is
> unlikely we can live with it when comparing with the benefit of the
> overall change: hardware private key generation and retention.
> 
> Signed-off-by: Tudor Ambarus 
> ---
> net/bluetooth/ecdh_helper.c | 102 +---
> net/bluetooth/smp.c |  55 +---
> 2 files changed, 67 insertions(+), 90 deletions(-)
> 
> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
> index ac2c708..56e9374 100644
> --- a/net/bluetooth/ecdh_helper.c
> +++ b/net/bluetooth/ecdh_helper.c
> @@ -53,10 +53,10 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
> public_key[64],
>const u8 private_key[32], u8 secret[32])
> {
>   struct kpp_request *req;
> - struct ecdh p;
> + struct ecdh p = {0};
>   struct ecdh_completion result;
>   struct scatterlist src, dst;
> - u8 *tmp, *buf;
> + u8 *tmp, *buf = NULL;
>   unsigned int buf_len;
>   int err = -ENOMEM;
> 
> @@ -70,25 +70,29 @@ bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 
> public_key[64],
> 
>   init_completion();
> 
> - /* Security Manager Protocol holds digits in litte-endian order
> -  * while ECC API expect big-endian data
> -  */
> - swap_digits((u64 *)private_key, (u64 *)tmp, 4);
> - p.key = (char *)tmp;
> - p.key_size = 32;
>   /* Set curve_id */
>   p.curve_id = ECC_CURVE_NIST_P256;
> - buf_len = crypto_ecdh_key_len();
> - buf = kmalloc(buf_len, GFP_KERNEL);
> - if (!buf)
> - goto free_req;
> 
> - crypto_ecdh_encode_key(buf, buf_len, );
> + if (private_key) {
> + /* Security Manager Protocol holds digits in little-endian order
> +  * while ECC API expect big-endian data.
> +  */
> + swap_digits((u64 *)private_key, (u64 *)tmp, 4);
> + p.key = (char *)tmp;
> + p.key_size = 32;
> 
> - /* Set A private Key */
> - err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
> - if (err)
> - goto free_all;
> + buf_len = crypto_ecdh_key_len();
> + buf = kmalloc(buf_len, GFP_KERNEL);
> + if (!buf)
> + goto free_req;
> +
> + crypto_ecdh_encode_key(buf, buf_len, );
> +
> + /* Set A private Key */
> + err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
> + if (err)
> + goto free_all;
> + }
> 
>   swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
>   swap_digits((u64 *)_key[32], (u64 *)[32], 4); /* y */
> @@ -126,14 +130,12 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
> public_key[64],
>   u8 private_key[32])
> {
>   struct kpp_request *req;
> - struct ecdh p;
> + struct ecdh p = {0};
>   struct ecdh_completion result;
>   struct scatterlist dst;
>   u8 *tmp, *buf;
>   unsigned int buf_len;
>   int err = -ENOMEM;
> - const unsigned short max_tries = 16;
> - unsigned short tries = 0;
> 
>   tmp = kmalloc(64, GFP_KERNEL);
>   if (!tmp)
> @@ -145,56 +147,48 @@ bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 
> public_key[64],
> 
>   init_completion();
> 
> + /* Set private Key */
> + if (private_key) {
> + p.key = (char *)private_key;
> + p.key_size = 32;
> + }
> +
>   /* Set curve_id */
>   p.curve_id = ECC_CURVE_NIST_P256;
> - p.key_size = 32;
>   buf_len = crypto_ecdh_key_len();
>   buf = kmalloc(buf_len, GFP_KERNEL);
>   if (!buf)
>   goto free_req;
> 
> - do {
> - if (tries++ >= max_tries)
> - goto free_all;
> -
> - /* Set private Key */
> - p.key = (char *)private_key;
> - crypto_ecdh_encode_key(buf, buf_len, );
> - err = crypto_kpp_set_secret(tfm, buf, buf_len);
> - if (err)
> - goto free_all;
> -
> - sg_init_one(, tmp, 64);
> - kpp_request_set_input(req, NULL, 0);
> - kpp_request_set_output(req, , 64);
> -

Re: [RESEND RFC PATCH 1/2] Bluetooth: move ecdh allocation outside of ecdh_helper

[Marcel Holtmann]

Hi Tudor,

> This change is a prerequisite for letting the crypto subsystem generate
> the ecc private key for ecdh. Before this change, a new crypto tfm was
> allocated, each time, for both key generation and shared secret
> computation. With this change, we allocate a single tfm for both cases.
> We need to bind the key pair generation with the shared secret
> computation via the same crypto tfm. Once the key is set, we can
> compute the shared secret without referring to the private key.
> 
> Signed-off-by: Tudor Ambarus 
> ---
> net/bluetooth/ecdh_helper.c | 32 ---
> net/bluetooth/ecdh_helper.h |  8 +++--
> net/bluetooth/selftest.c| 29 -
> net/bluetooth/smp.c | 77 +
> 4 files changed, 96 insertions(+), 50 deletions(-)
> 
> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
> index c7b1a9a..ac2c708 100644
> --- a/net/bluetooth/ecdh_helper.c
> +++ b/net/bluetooth/ecdh_helper.c
> @@ -23,7 +23,6 @@
> #include "ecdh_helper.h"
> 
> #include 
> -#include 
> #include 
> 
> struct ecdh_completion {
> @@ -50,10 +49,9 @@ static inline void swap_digits(u64 *in, u64 *out, unsigned 
> int ndigits)
>   out[i] = __swab64(in[ndigits - 1 - i]);
> }
> 
> -bool compute_ecdh_secret(const u8 public_key[64], const u8 private_key[32],
> -  u8 secret[32])
> +bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
> +  const u8 private_key[32], u8 secret[32])
> {
> - struct crypto_kpp *tfm;
>   struct kpp_request *req;
>   struct ecdh p;
>   struct ecdh_completion result;
> @@ -66,16 +64,9 @@ bool compute_ecdh_secret(const u8 public_key[64], const u8 
> private_key[32],
>   if (!tmp)
>   return false;
> 
> - tfm = crypto_alloc_kpp("ecdh", CRYPTO_ALG_INTERNAL, 0);
> - if (IS_ERR(tfm)) {
> - pr_err("alg: kpp: Failed to load tfm for kpp: %ld\n",
> -PTR_ERR(tfm));
> - goto free_tmp;
> - }
> -
>   req = kpp_request_alloc(tfm, GFP_KERNEL);
>   if (!req)
> - goto free_kpp;
> + goto free_tmp;
> 
>   init_completion();
> 
> @@ -126,16 +117,14 @@ bool compute_ecdh_secret(const u8 public_key[64], const 
> u8 private_key[32],
>   kzfree(buf);
> free_req:
>   kpp_request_free(req);
> -free_kpp:
> - crypto_free_kpp(tfm);
> free_tmp:
>   kfree(tmp);
>   return (err == 0);
> }
> 
> -bool generate_ecdh_keys(u8 public_key[64], u8 private_key[32])
> +bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
> + u8 private_key[32])
> {
> - struct crypto_kpp *tfm;
>   struct kpp_request *req;
>   struct ecdh p;
>   struct ecdh_completion result;
> @@ -150,16 +139,9 @@ bool generate_ecdh_keys(u8 public_key[64], u8 
> private_key[32])
>   if (!tmp)
>   return false;
> 
> - tfm = crypto_alloc_kpp("ecdh", CRYPTO_ALG_INTERNAL, 0);
> - if (IS_ERR(tfm)) {
> - pr_err("alg: kpp: Failed to load tfm for kpp: %ld\n",
> -PTR_ERR(tfm));
> - goto free_tmp;
> - }
> -
>   req = kpp_request_alloc(tfm, GFP_KERNEL);
>   if (!req)
> - goto free_kpp;
> + goto free_tmp;
> 
>   init_completion();
> 
> @@ -218,8 +200,6 @@ bool generate_ecdh_keys(u8 public_key[64], u8 
> private_key[32])
>   kzfree(buf);
> free_req:
>   kpp_request_free(req);
> -free_kpp:
> - crypto_free_kpp(tfm);
> free_tmp:
>   kfree(tmp);
>   return (err == 0);
> diff --git a/net/bluetooth/ecdh_helper.h b/net/bluetooth/ecdh_helper.h
> index 7a423fa..5cde37d 100644
> --- a/net/bluetooth/ecdh_helper.h
> +++ b/net/bluetooth/ecdh_helper.h
> @@ -20,8 +20,10 @@
>  * COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
>  * SOFTWARE IS DISCLAIMED.
>  */
> +#include 
> #include 
> 
> -bool compute_ecdh_secret(const u8 pub_a[64], const u8 priv_b[32],
> -  u8 secret[32]);
> -bool generate_ecdh_keys(u8 public_key[64], u8 private_key[32]);
> +bool compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pub_a[64],
> +  const u8 priv_b[32], u8 secret[32]);
> +bool generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
> + u8 private_key[32]);
> diff --git a/net/bluetooth/selftest.c b/net/bluetooth/selftest.c
> index 34a1227..126bdc5 100644
> --- a/net/bluetooth/selftest.c
> +++ b/net/bluetooth/selftest.c
> @@ -138,9 +138,9 @@ static const u8 dhkey_3[32] __initconst = {
>   0x7c, 0x1c, 0xf9, 0x49, 0xe6, 0xd7, 0xaa, 0x70,
> };
> 
> -static int __init test_ecdh_sample(const u8 priv_a[32], const u8 priv_b[32],
> -const u8 pub_a[64], const u8 pub_b[64],
> -const u8 dhkey[32])
> +static int __init test_ecdh_sample(struct crypto_kpp *tfm, const u8 
> priv_a[32],
> + 

Re: [RFC PATCH 0/2] let the crypto subsystem generate the ecc privkey

[Marcel Holtmann]

Hi Tudor,

> That Bluetooth SMP knows about the private key is pointless, since the
> detection of debug key usage is actually via the public key portion.
> With this patch set, the Bluetooth SMP will stop keeping a copy of the
> ecdh private key, except when using debug keys. This way we let the
> crypto subsystem to generate and handle the ecdh private key,
> potentially benefiting of hardware ecc private key generation and
> retention.
> 
> Tested with selftest and with btmon and smp-tester on top of hci_vhci,
> with ecdh done in both software and hardware (through atmel-ecc driver).
> All tests passed.
> 
> Tudor Ambarus (2):
>  Bluetooth: move ecdh allocation outside of ecdh_helper
>  Bluetooth: let the crypto subsystem generate the ecc privkey
> 
> net/bluetooth/ecdh_helper.c | 138 ++--
> net/bluetooth/ecdh_helper.h |   8 ++-
> net/bluetooth/selftest.c|  29 +++---
> net/bluetooth/smp.c | 120 --
> 4 files changed, 159 insertions(+), 136 deletions(-)

I only saw the cover letter and the patches never made it to the mailing list.

Regards

Marcel

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Tudor,

>> you still need to get the public key out of the kernel if you want to use it 
>> from user space. Or feed the remote public key if you plan to use some sort 
>> of key derivation function.
> 
> The crypto hardware that I'm working on, generates the private key
> internally within the device and never reveals it to software and
> immediately returns the public key pair. The user can retrieve the
> public key from hardware.

and don’t we want some sort of access control here. Only the user / process 
that requested the private key and has access to the public key is allowed to 
keep using the private key?

>> I am saying this again, if you only have a hammer, everything looks like a 
>> nail. What about actually looking at how this would be used from user space 
>> in real crypto cases.
>> My point is that the usages here are key generation, some sort of 
>> key-exchange-agreement (aka DH) and key derivation into a symmetric key. 
>> Frankly the focus with asymmetric ciphers are the keys and the key 
>> derivation. They are not encryption and decryption of massive amounts of 
>> data.
> 
> The hardware uses it's own private key and the public key received from
> the other end and computes the ecdh shared secret. The hardware computed
> shared secret can then be used for key derivation.

And that is normally the case. Get your local public key, send it to the other 
side, get the other sides public key, give it to the hardware and get shared 
secret.

So how is AF_ALG a good fit here?

Regards

Marcel

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Tudor,

> akcipher can work with its own internal keys, now that we have crypto
> accelerators that can generate keys that never leave the hardware. Going
> through the kernel's key subsystem seems superfluous in this case.
> 
> I also understand the need of going through the kernel's key subsystem
> when the user wants to refer to a key which exists elsewhere, such as in
> TPM or within an SGX software enclave, but this seems orthogonal with
> crypto accelerators with key generation and retention support.
> 
> How should we interface akcipher/kpp with user-space?

you still need to get the public key out of the kernel if you want to use it 
from user space. Or feed the remote public key if you plan to use some sort of 
key derivation function.

I am saying this again, if you only have a hammer, everything looks like a 
nail. What about actually looking at how this would be used from user space in 
real crypto cases.

My point is that the usages here are key generation, some sort of 
key-exchange-agreement (aka DH) and key derivation into a symmetric key. 
Frankly the focus with asymmetric ciphers are the keys and the key derivation. 
They are not encryption and decryption of massive amounts of data.

Regards

Marcel

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Stephan,

>>> The first part is clearly where AF_ALG fits and keyctl does not. This is
>>> provided with the current patch set. As the keyctl API only handles, well,
>>> keys, access to the raw ciphers may not be possible through this API. And
>>> let us face it, a lot of user space code shall support many different
>>> OSes. Thus, if you have a crypto lib in user space who has its own key
>>> management (which is a core element of such libraries and thus cannot be
>>> put into an architecture-dependent code part), having only the keyctl API
>>> on Linux for accelerated asym support may not be helpful.
>> 
>> That argument is just non-sense.
> 
> How interesting. For example, what about NSS with its own key database?

a lot of applications create their own key or certificate database. It also 
means they need to reload and reload them over and over again for each process. 
A lot of things are possible, but why keep doing things more complicated than 
they need to be. As I said before, if you only have a hammer ..

Regards

Marcel

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Stephan,

>> I also would like to have more of those algorithms exposed to userspace,
>> and I'd like to make sure the API is a good fit. There was extensive
>> discussion of AF_ALG akcipher last year. v8 of the patch set updates the
>> implementation but doesn't address the API concerns that kept the previous
>> versions from being merged, so we seem to be at just as much of an impasse
>> as before.
> 
> During last year's discussion, I think we have concluded (and please correct 
> me if I miss something), that the export of the asymmetric HW implementations 
> to user space should
> 
> - allow a streaming mode where the user space uses the kernel as an 
> accelerator (maybe user space has another way of storing keys)

explain to me what a streaming mode with an asymmetric cipher is? They are no 
good for these kind of operations. If you want streaming mode, then you want a 
symmetric cipher.

The keyring subsystem is actually a good storage for keys. If the keyring 
subsystem can use hardware storage for the keys, even better. However right now 
all the certificates and its associated keys are stored perfectly fine in a 
keyring.

> - allow the private key to be retained in kernel space (or even in hardware 
> only) -- i.e. user space only has a handle to the key for a full asym 
> operation

And that is exactly my point. Even if userspace has the key, let it load into 
the kernel as a key inside a keyring. We do not need two ways for loading 
asymmetric keys. They are by nature more complex then any symmetric key.

> The first part is clearly where AF_ALG fits and keyctl does not. This is 
> provided with the current patch set. As the keyctl API only handles, well, 
> keys, access to the raw ciphers may not be possible through this API. And let 
> us face it, a lot of user space code shall support many different OSes. Thus, 
> if you have a crypto lib in user space who has its own key management (which 
> is a core element of such libraries and thus cannot be put into an 
> architecture-dependent code part), having only the keyctl API on Linux for 
> accelerated asym support may not be helpful.

That argument is just non-sense. The crypto libraries that run on multiple OSes 
have already enough abstraction layers to deal with this. And frankly there it 
would be preferred if hardware backed keys are handled properly. Since that is 
what is really needed.

Also I have to repeat my comment from above. The asymmetric ciphers are really 
bad for any kind of streaming operation. Using them in that way is almost 
always going to end badly.

David Howells has patches for sign/verify/encrypt/decrypt operation. Keep in 
mind that all the parameters of the asymmetric keys are really bound to its 
cipher. So it is pretty obvious that the key itself defines what cipher is 
used. I do not get the wish for raw access to RSA or ECDH for example. You need 
the right set of keys and parameters first. Otherwise it is all garbage and not 
even guaranteed to be cryptographically secure.

> The second part is a keyctl domain. I see in-kernel support for this 
> scenario, 
> but have not yet seen the kernel/user interface nor the user space support.

Actually have you looked at Mat’s kernel tree and the support for it in ELL.

> Both options are orthogonal, IMHO.

I don’t agree with that. As explained above, asymmetric ciphers are bound to 
their keys. For me this all feels like a hammer approach. You want to treat 
things as nails since that is the only thing you have. However that is not the 
case. We have the keyring subsystem.

> Tadeusz Struck provided a patch to link the kernel crypto API / 
> algif_akcipher 
> with the keys subsystem to allow the second requirement to be implemented in 
> algif_akcipher. That patch is on my desk and I plan to integrate it and even 
> make it generic to allow its use for all different cipher types. I have not 
> yet integrated it to allow a small patch set to be reviewed. If it is the 
> will 
> of the council, I can surely add that code to the initial patch set and 
> resubmit.

For symmetric ciphers this is awesome. For asymmetric ciphers I have no idea on 
how this would work. Once you provided the key handle, then the crypto 
subsystem would have to select the cipher. However that is not how it is 
designed. You are binding a cipher early one. In addition, depending on the 
key, you would need to choose the right hardware to execute on (in case of 
hardware bound keys). It is also not designed for this either.

So I have no idea how you want to overcome the design limitations / choices of 
AF_ALG when it comes to supporting assymetric ciphers correctly.

Regards

Marcel

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Stephan,

>> AF_ALG is best suited for crypto use cases where a socket is set up once
>> and there are lots of reads and writes to justify the setup cost. With
>> asymmetric crypto, the setup cost is high when you might only use the
>> socket for a brief time to do one verify or encrypt operation.
> 
> To me, the entire AF_ALG purpose is solely to export hardware support to user 
> space. That said, if user space wants an accelerator, a socket would be 
> opened 
> once followed by numerous read/write requests.
> 
> Besides, I am aware of Tadeusz' patch to link algif_akcipher to the keyring 
> and I planned to port it to the current implementation. But I thought I offer 
> a small patch focusing on the externalization of the akcipher API first.
> 
> I think the keyctl and AF_ALG are no opponents, but rather are orthogonal to 
> each other. The statement I made for the KPP AF_ALG RFC applies here too:
> 
> """
> I am aware and in fact supported development of the DH support in the keys
> subsystem. The question will be raised whether the AF_ALG KPP interface is
> superfluous in light of the keys DH support. My answer is that AF_ALG KPP is
> orthogonal to the keys DH support. The keys DH support use case is that
> the keys are managed inside the kernel and thus has the focus on the
> key management. Contrary, AF_ALG KPP does not really focus on key management
> but simply externalizes the DH/ECDH support found in the kernel including
> hardware acceleration. User space is in full control of the key life cycle.
> This way, AF_ALG could be used to complement user-space network protocol
> implementations like TLS or IKE to offload the resource intense DH
> calculations to accelerators.
> “""

we do not need two interfaces for doing the same thing. Especially not one that 
can not handle hardware backed keys. And more important if you can not abstract 
an accelerator that doesn’t expose the private key at all.

If you only have a hammer, everything looks like a nail. Luckily we have more 
tools than just a hammer :)

Regards

Marcel

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Stephan,

>>> The last round of reviews for AF_ALG akcipher left off at an impasse
>>> around a year ago: the consensus was that hardware key support was
>>> needed, but that requirement was in conflict with the "always have a
>>> software fallback" rule for the crypto subsystem. For example, a private
>>> key securely generated by and stored in a TPM could not be copied out for
>>> use by a software algorithm. Has anything come about to resolve this
>>> impasse?
>>> 
>>> There were some patches around to add keyring support by associating a key
>>> ID with an akcipher socket, but that approach ran in to a mismatch
>>> between the proposed keyring API for the verify operation and the
>>> semantics of AF_ALG verify.
>>> 
>>> AF_ALG is best suited for crypto use cases where a socket is set up once
>>> and there are lots of reads and writes to justify the setup cost. With
>>> asymmetric crypto, the setup cost is high when you might only use the
>>> socket for a brief time to do one verify or encrypt operation.
>>> 
>>> Given the efficiency and hardware key issues, AF_ALG seems to be
>>> mismatched with asymmetric crypto. Have you looked at the proposed
>>> keyctl() support for crypto operations?
>> we have also seen hardware now where the private key will never leave the
>> crypto hardware. They public and private key is only generated for key
>> exchange purposes and later on discarded again. Asymmetric ciphers are
>> really not a good fit for AF_ALG and they should be solely supported via
>> keyctl.
> 
> Thanks for the reminder. I have looked at that but I am unsure about whether 
> this one covers asym crypto appropriately, too.
> 
> The issue is that some hardware may only offer accelerators without a full 
> blown RSA siggen/ver logic (that pulls in PKCS or OAEP or others). How do you 
> propose to cover raw primitives with keyctl?

where is such a hardware? And what is the usage of it? Look at what we are 
using asymmetric crypto for at the moment. It is either for sign and verify 
with secure boot etc. Or it is for key exchange purposes.

The asymmetric crypto is a means to an end. If it is not for certification 
verification, then it for is creating a symmetric key to be used with a 
symmetric cipher. We have the the asymmetric_keys subsystem for representing 
the nature of this crypto. Also the list of asymmetric ciphers is a lot smaller 
than the symmetric ones.

What raw primitives? When we are using for example ECDH for Bluetooth where you 
need to create a pairwise symmetric key, then what you really want from the 
cryptographic primitives is this:

1) Create public/private key pair
2) Give public key to applications and store the private key safely
3) Retrieve public key from remote side and challenge
4) Compute key exchange magic (like DH) from remote public key
5) Tell the key generator to throw away the private key

So I do not want to load any private key into the kernel. I want the kernel to 
give me a public key and allow me to feed the key exchange primitive with the 
remote public key. This is clearly not AF_ALG. We had this discussion during 
the KPP design and I made it clear multiple times that this is totally 
different.

This is clearly keyctl area of interfaces since the main operation is key 
generation. I am not saying that keyctl is perfect just yet, but we are working 
on it. We however have to accept that it is more suitable than AF_ALG. You will 
never have stream based data feed into an asymmetric cipher. That is what 
stream ciphers are for.

Regards

Marcel

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Mat,

>> This patch set adds the AF_ALG user space API to externalize the
>> asymmetric cipher API recently added to the kernel crypto API.
> 
> ...
> 
>> Changes v8:
>> * port to kernel 4.13
>> * port to consolidated AF_ALG code
>> 
>> Stephan Mueller (4):
>> crypto: AF_ALG -- add sign/verify API
>> crypto: AF_ALG -- add setpubkey setsockopt call
>> crypto: AF_ALG -- add asymmetric cipher
>> crypto: algif_akcipher - enable compilation
>> 
>> crypto/Kconfig  |   9 +
>> crypto/Makefile |   1 +
>> crypto/af_alg.c |  28 ++-
>> crypto/algif_aead.c |  36 ++--
>> crypto/algif_akcipher.c | 466 
>> 
>> crypto/algif_skcipher.c |  26 ++-
>> include/crypto/if_alg.h |   7 +-
>> include/uapi/linux/if_alg.h |   3 +
>> 8 files changed, 543 insertions(+), 33 deletions(-)
>> create mode 100644 crypto/algif_akcipher.c
>> 
>> -- 
>> 2.13.4
> 
> The last round of reviews for AF_ALG akcipher left off at an impasse around a 
> year ago: the consensus was that hardware key support was needed, but that 
> requirement was in conflict with the "always have a software fallback" rule 
> for the crypto subsystem. For example, a private key securely generated by 
> and stored in a TPM could not be copied out for use by a software algorithm. 
> Has anything come about to resolve this impasse?
> 
> There were some patches around to add keyring support by associating a key ID 
> with an akcipher socket, but that approach ran in to a mismatch between the 
> proposed keyring API for the verify operation and the semantics of AF_ALG 
> verify.
> 
> AF_ALG is best suited for crypto use cases where a socket is set up once and 
> there are lots of reads and writes to justify the setup cost. With asymmetric 
> crypto, the setup cost is high when you might only use the socket for a brief 
> time to do one verify or encrypt operation.
> 
> Given the efficiency and hardware key issues, AF_ALG seems to be mismatched 
> with asymmetric crypto. Have you looked at the proposed keyctl() support for 
> crypto operations?

we have also seen hardware now where the private key will never leave the 
crypto hardware. They public and private key is only generated for key exchange 
purposes and later on discarded again. Asymmetric ciphers are really not a good 
fit for AF_ALG and they should be solely supported via keyctl.

Regards

Marcel

Re: KPP questions and confusion

[Marcel Holtmann]

Hi Tudor,

>>> I am confused about several things in the new key agreement code.
>>> 
>>> net/bluetooth/smp.c in two places generates random bytes for the
>>> private_key argument to
>>> net/bluetooth/ecdh_helper.c:generate_ecdh_keys, which suggests the
>>> private key is static within the function. However, there is a do ...
>>> while(true) loop within generate_ecdh_keys, with the following near
>>> the end:
>>> 
>>> /* Private key is not valid. Regenerate */
>>> if (err == -EINVAL)
>>>continue;
>>> 
>>> which suggests that it expects a different private key to be generated
>>> on each iteration of the loop. But it looks like it runs through the
>>> loop yet again with the same private key generated in the caller,
>>> which suggests it would infinitely loop on a bad private key value. Is
>>> this incorrect?
>> actually it seems that I screwed that up with commit 
>> 71653eb64bcca6110c42aadfd50b8d54d3a88079 where I moved the seeding of 
>> private_key outside of generate_ecdh_keys() function.
>>> Furthermore, since req->src == NULL via the call to
>>> kpp_request_set_input, ecc_make_pub_key will always be called in
>>> ecdh.c:ecdh_compute_value, in which case -EINVAL would be returned
>>> only by invalid input (!private_key or bad curve_id) that AFAICT
>>> cannot happen, or at least wouldn't be resolved by another run through
>>> the loop.
>>> 
>>> OTOH, -EAGAIN would be returned by ecc_make_pub_key if the public key
>>> turns out to be the zero point, which is at least one reason why you'd
>>> want to generate a new private key (if that loop were to do that.)
>>> 
>>> I'm a little confused about some other things:
>>> 
>>> * The bluetooth code doesn't seem to use ecc_gen_privkey, opting to
>>> instead just generate random bytes and hope for the best.
>>> * There doesn't appear to be any way for ecc_gen_privkey to get called
>>> from crypto/ecdh.c:ecdh_set_secret, as it starts with a call to
>>> crypto/ecdh_helper.c:crypto_ecdh_decode_key that returns -EINVAL if
>>> decoding fails, meaning that both params.key != NULL and (if I'm
>>> reading this correctly) params.key_size > 0. Is that dead code, or is
>>> there a way it is intended to be used?
>> I am fine switching to ecc_gen_privkey. Care to provide a patch for it?
> 
> Marcel, regarding the ecdh offload to hw from Bluetooth SMP code, this
> is not possible as of know because SMP code uses it's own private keys.
> atmel-ecc driver will fallback to the ecdh software implementation if
> user wants to use it's own private keys.
> 
> I can jump in the Bluetooth's ecdh handling, but at best effort, my
> primary goal is to add support for KPP in af_alg.

then we need a better abstraction inside KPP. Since even for Bluetooth SMP the 
keys are temporary. The only thing we have to check is against a well know 
public/private key pair that was designated as debug key for sniffer purposes.

Essentially we do what all other key exchange procedure do. Generate a 
private/public key pair, give the public key to the other side, run DH with the 
value from the other side. That Bluetooth SMP knows about the private key is 
really pointless. Since the detection of debug key usage is actually via the 
public key portion.

Regards

Marcel

Re: KPP questions and confusion

[Marcel Holtmann]

Hi Kyle,

> I am confused about several things in the new key agreement code.
> 
> net/bluetooth/smp.c in two places generates random bytes for the
> private_key argument to
> net/bluetooth/ecdh_helper.c:generate_ecdh_keys, which suggests the
> private key is static within the function. However, there is a do ...
> while(true) loop within generate_ecdh_keys, with the following near
> the end:
> 
> /* Private key is not valid. Regenerate */
> if (err == -EINVAL)
>continue;
> 
> which suggests that it expects a different private key to be generated
> on each iteration of the loop. But it looks like it runs through the
> loop yet again with the same private key generated in the caller,
> which suggests it would infinitely loop on a bad private key value. Is
> this incorrect?

actually it seems that I screwed that up with commit 
71653eb64bcca6110c42aadfd50b8d54d3a88079 where I moved the seeding of 
private_key outside of generate_ecdh_keys() function.

> Furthermore, since req->src == NULL via the call to
> kpp_request_set_input, ecc_make_pub_key will always be called in
> ecdh.c:ecdh_compute_value, in which case -EINVAL would be returned
> only by invalid input (!private_key or bad curve_id) that AFAICT
> cannot happen, or at least wouldn't be resolved by another run through
> the loop.
> 
> OTOH, -EAGAIN would be returned by ecc_make_pub_key if the public key
> turns out to be the zero point, which is at least one reason why you'd
> want to generate a new private key (if that loop were to do that.)
> 
> I'm a little confused about some other things:
> 
> * The bluetooth code doesn't seem to use ecc_gen_privkey, opting to
> instead just generate random bytes and hope for the best.
> * There doesn't appear to be any way for ecc_gen_privkey to get called
> from crypto/ecdh.c:ecdh_set_secret, as it starts with a call to
> crypto/ecdh_helper.c:crypto_ecdh_decode_key that returns -EINVAL if
> decoding fails, meaning that both params.key != NULL and (if I'm
> reading this correctly) params.key_size > 0. Is that dead code, or is
> there a way it is intended to be used?

I am fine switching to ecc_gen_privkey. Care to provide a patch for it?

> The context for this email is that I have need for the same basic
> functionality in net/bluetooth/ecdh_helper.c for a non-BT purpose, so
> it seems like this should be part of crypto/ecdh_helper.c (abstracted
> to work for any curve). Basically, I need to do basic ECDH key
> agreement:
> 
> * Generate a new (valid) ephemeral private key, or potentially re-use
> an existing one
> * Compute the corresponding public key for the curve
> * Compute the shared secret based on my private and peer's public
> 
> Is KPP intended to be an abstract interface to all of the above, e.g.,
> for both ECDH and FFDH? Right now it seems like layer violations are
> required as there doesn't appear to be any way to (for example)
> generate a fresh private key via kpp_*.

I think the whole goal is to abstract this. Mainly so that ECDH can also be 
offload to hardware.

Regards

Marcel

Re: [PATCH 0/3] crypto: introduce Microchip / Atmel ECC driver

[Marcel Holtmann]

Hi Tudor,

>>> This patch set introduces Microchip / Atmel ECC driver.
>>> 
>>> The first patch adds some helpers that will be used by fallbacks to
>>> kpp software implementations.
>>> 
>>> The second patch adds ECDH support for the ATECC508A (I2C)
>>> cryptographic engine. The I2C interface is designed to operate
>>> at a maximum clock speed of 1MHz.
>>> 
>>> The device features hardware acceleration for the NIST standard
>>> P256 prime curve and supports the complete key life cycle from
>>> private key generation to ECDH key agreement.
>>> 
>>> Random private key generation is supported internally within
>>> the device to ensure that the private key can never be known
>>> outside of the device. If the user wants to use its own private
>>> keys, the driver will fallback to the ecdh software implementation.
>> can we get this testing with the Bluetooth SMP code? I would really like to 
>> see this being offloaded to hardware. For Bluetooth SMP we never really need 
>> the private key either. The end result is an symmetric 128-bit key for AES. 
>> And we throw the generated key pairs away.
>> With the limitation of private is not available to Linux directly, we should 
>> make sure that KPP users that don’t require the private key are working 
>> properly and can utilize the offload.
> 
> The driver was tested with testmgr, the offload worked.
> 
> I've extended recently the ecdh software implementation with
> ecc privkey generation support. I also added a kpp test in
> testmgr to prove that it works correctly (see [1]).
> 
> I will take a look at Bluetooth SMP code.

you can test this without Bluetooth hardware, just need to make sure you have 
hci_vhci module available. And then from the BlueZ user space source code, you 
run ./tools/smp-tester (no need to install) to exercise the pairing with ECDH 
P-256. If you run ./monitor/btmon in a separate terminal, then it will show you 
the public keys exchanged.

Regards

Marcel

Re: [PATCH 0/3] crypto: introduce Microchip / Atmel ECC driver

[Marcel Holtmann]

Hi Tudor,

> This patch set introduces Microchip / Atmel ECC driver.
> 
> The first patch adds some helpers that will be used by fallbacks to
> kpp software implementations.
> 
> The second patch adds ECDH support for the ATECC508A (I2C)
> cryptographic engine. The I2C interface is designed to operate
> at a maximum clock speed of 1MHz.
> 
> The device features hardware acceleration for the NIST standard
> P256 prime curve and supports the complete key life cycle from
> private key generation to ECDH key agreement.
> 
> Random private key generation is supported internally within
> the device to ensure that the private key can never be known
> outside of the device. If the user wants to use its own private
> keys, the driver will fallback to the ecdh software implementation.

can we get this testing with the Bluetooth SMP code? I would really like to see 
this being offloaded to hardware. For Bluetooth SMP we never really need the 
private key either. The end result is an symmetric 128-bit key for AES. And we 
throw the generated key pairs away.

With the limitation of private is not available to Linux directly, we should 
make sure that KPP users that don’t require the private key are working 
properly and can utilize the offload.

Regards

Marcel

Re: [PATCH v4 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Marcel Holtmann]

Hi Jason,

>> yes, there are plenty of commands needed before a controller becomes usable.
> 
> That doesn't clearly address with precision what Ted was wondering.
> Specifically, the inquiry is: can you confirm with certainty whether
> or not all calls to get_random_bytes() in the bluetooth directory are
> *necessarily* going to come after a call to hci_power_on()?

on a powered down controller, you can not do any crypto. SMP is only during a 
connection and the RPAs are only generated when needed. So yes, doing this once 
in hci_power_on is plenty. However we might want to limit this to LE capable 
controllers since for BR/EDR only controllers this is not needed. For A2MP I 
need to check that we need the random numbers seeded there. However this hidden 
behind the high speed feature.

Regards

Marcel

Re: [PATCH v4 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Marcel Holtmann]

Hi Ted,

>> This protocol uses lots of complex cryptography that relies on securely
>> generated random numbers. Thus, it's important that the RNG is actually
>> seeded before use. Fortuantely, it appears we're always operating in
>> process context (there are many GFP_KERNEL allocations and other
>> sleeping operations), and so we can simply demand that the RNG is seeded
>> before we use it.
>> 
>> We take two strategies in this commit. The first is for the library code
>> that's called from other modules like hci or mgmt: here we just change
>> the call to get_random_bytes_wait, and return the result of the wait to
>> the caller, along with the other error codes of those functions like
>> usual. Then there's the SMP protocol handler itself, which makes many
>> many many calls to get_random_bytes during different phases. For this,
>> rather than have to change all the calls to get_random_bytes_wait and
>> propagate the error result, it's actually enough to just put a single
>> call to wait_for_random_bytes() at the beginning of the handler, to
>> ensure that all the subsequent invocations are safe, without having to
>> actually change them. Likewise, for the random address changing
>> function, we'd rather know early on in the function whether the RNG
>> initialization has been interrupted, rather than later, so we call
>> wait_for_random_bytes() at the top, so that later on the call to
>> get_random_bytes() is acceptable.
> 
> Do we need to do all of this?  Bluetooth folks, is it fair to assume
> that hci_power_on() has to be called before any bluetooth functions
> can be done?  If so, adding a wait_for_random_bytes() in
> hci_power_on() might be all that is necessary.

yes, there are plenty of commands needed before a controller becomes usable. 
When plugging in new Bluetooth hardware, we have to power it up and read the 
initial settings and configuration out of.

Also all the cryptographic features only apply to LE enabled controllers. The 
classic BR/EDR controllers have this all in hardware. So if you are not LE 
enabled, then there is not even a point in waiting for any seeding. However 
that said, also all LE controllers have an extra random number function we 
could call if we need extra seeding. We never bothered to hook this up since we 
thought that the kernel has enough sources.

Regards

Marcel

Re: ecdh: generation and retention of ecc privkey in kernel/hardware

[Marcel Holtmann]

Hi Tudor,

> I'm working with a crypto accelerator that is capable of generating and
> retaining ecc private keys in hardware and further use them for ecdh.
> The private keys can not be read from the device. This is good because
> the less software has access to secrets, the better.
> 
> Generation and retention of ecc private keys are also helpful in a user
> space to kernel ecdh offload. The privkey can be generated in kernel and 
> never revealed to user space.
> 
> I propose to extend the ecc software support to allow the generation of
> private keys. ECDH software implementation and drivers will permit the
> users to provide NULL keys. In this case, the kernel (or the device, if
> possible) will generate the ecc private key and further use it for ecdh.
> 
> What's your feeling on this?

can we represent these keys via keyctl as part of the kernel keyring? I think 
when it comes to asymmetric crypto and its keys, we need to have these as keys 
represented in kernel keyring. Recently we added keyctl features to sign, 
verify, encrypt and decrypt operations.

The crypto subsystem concept is broken when it comes to keys in hardware since 
it enforces the concept of always being able to fallback on a software 
implementation of the algorithm.

Regards

Marcel

Re: [PATCH v6 0/3] Key-agreement Protocol Primitives (KPP) API

[Marcel Holtmann]

Hi Herbert,

> the following patchset introduces a new API for abstracting key-agreement
> protocols such as DH and ECDH. It provides the primitives required for 
> implementing
> the protocol, thus the name KPP (Key-agreement Protocol Primitives).
> 
> Regards,
> Salvatore
> 
> Changes from v5:
> * Fix ecdh loading in fips mode.
> 
> Changes from v4:
> * If fips_enabled is set allow only P256 (or higher) as Stephan suggested
> * Pass ndigits as argument to ecdh_make_pub_key and ecdh_shared_secret
>  so that VLA can be used like in the rest of the module
> 
> Changes from v3:
> * Move curve ID definition to public header ecdh.h as users need to
>  have access to those ids when selecting the curve
> 
> Changes from v2:
> * Add support for ECDH (curve P192 and P256). I reused the ecc module
>  already present in net/bluetooth and extended it in order to select
>  different curves at runtime. Code for P192 was taken from tinycrypt.
> 
> Changes from v1:
> * Change check in dh_check_params_length based on Stephan review
> 
> 
> Salvatore Benedetto (3):
>  crypto: Key-agreement Protocol Primitives API (KPP)
>  crypto: kpp - Add DH software implementation
>  crypto: kpp - Add ECDH software support

we have tested this with the Bluetooth subsystem to use ECDH for key generation 
and shared secret generation. This seems to work as expected. Feel free to 
merge this patchset.

Acked-by: Marcel Holtmann <mar...@holtmann.org>

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Bluetooth: convert smp module to crypto kpp API

[Marcel Holtmann]

Hi Salvatore,

> This patch has *not* been tested as I don't have the hardware.
> It's purpose is to show how to use the kpp API.
> 
> Based on https://patchwork.kernel.org/patch/9022371/

actually you should be able to verify this without hardware. The BlueZ 
userspace package contains tools/mgmt-tester and tools/smp-tester which should 
both exercise most of the Bluetooth Security Manager (SMP) pieces.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2 v2] crypto: Key-agreement Protocol Primitives API (KPP)

[Marcel Holtmann]

Hi Herbert,

>> Add key-agreement protocol primitives (kpp) API which allows to
>> implement primitives required by protocols such as DH and ECDH.
>> The API is composed mainly by the following functions
>> * set_params() - It allows the user to set the parameters known to
>>   both parties involved in the key-agreement session
>> * set_secret() - It allows the user to set his secret, also
>>   referred to as his private key
>> * generate_public_key() - It generates the public key to be sent to
>>   the other counterpart involved in the key-agreement session. The
>>   function has to be called after set_params() and set_secret()
>> * generate_secret() - It generates the shared secret for the session
>> 
>> Other functions such as init() and exit() are provided for allowing
>> cryptographic hardware to be inizialized properly before use
>> 
>> Signed-off-by: Salvatore Benedetto 
> 
> I don't have any strong objections to this interface.
> 
> However, I'd like to see it along with an actual user.  Because
> otherwise I'm afraid that I'll soon start receiving patches adding
> drivers using this interface even before we settle on what the
> user interface looks like.  And what the user interface looks
> like is very important because it may impact how we structure
> this.

actually if we have support for ECDH P-256, then Bluetooth could be converted 
easily and we get an internal user of this API.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Marcel Holtmann]

Hi Tadeusz,

>>> In this way we can define a generic user side of the key exchange interface,
 and on the the driver side of the akcipher, the implementations would 
 overload
 the existing akcipher encrypt(), decrypt(), set_pub_key(), set_priv_key() 
 methods
 and do what should be done for a given algorithm. We just need to agree on 
 the
 format of the input parameters to these operations.
>> I think trying to heavily map this to encrypt and decrypt is a bad idea. 
>> These callbacks are not really designed to return what you need them to 
>> return. Key exchange is different.
>> 
>> So why are we trying to push this into akcipher in the first place? I mean 
>> lets just create a keyexch (or similar name) and design it explicitly for 
>> key exchange handling. We are also not trying to map hashes into skcipher. I 
>> think the clear distinction of semantics calls for a different API.
>> 
> 
> I just think that before introducing new API we should try to reuse what's 
> already
> there. Looking at RSA and DH, they basically perform x = a^b mod p, so we 
> should
> have what's needed for both and we just could overload the methods already 
> defined.
> I'm fine either way. Herbert, what's your preference?

I do not think that it is a valid argument because the operations are the same 
to use akcipher. While operations are similar, the inputs are totally 
different. And it is really not performing encrypt or decrypt operations here. 
You use different inputs and get different outputs. You are not encrypting or 
decrypting anything. Especially key generation via decrypt is something I can 
not wrap my mind around.

>>> Agree, we need to support all the cases, but dealing with different type of 
>>> keys
 needs to happen above this API. This API should solely be an interface to 
 math
 primitives of certain operations, which can be implemented in SW or can be 
 HW
 accelerated. Modules like public_key or afalg_akcipher need to be smart 
 enough
 and know what key types they deal with and do the right thing depending on 
 that
 knowledge.
>> I am not sure this can be that easily generalized. Just pushing the problem 
>> of key types and formats off to userspace just forces complexity and extra 
>> knowledge there. We need to be really carefully here. And I really dislike 
>> introducing custom ASN.1 structures that are not backed by an actual RFC 
>> document.
> 
> I'm not saying that we should push this to user space. I'm just saying this 
> should
> be handled above the crypto API.
> 
>>> Agree, and to make this work with afalg_akcipher, I have proposed new
 ALG_SET_KEY_ID and ALG_SET_KEY_ID operations. See here: 
 https://lkml.org/lkml/2015/12/26/65
 The plan is that in this case the afalg_akcipher will not use akcipher api 
 at all.
 In this case the algif_akcipher will use the request_key() interface to 
 retrieve
 the key type info and will use the operations that the new TPM interface 
 will define.
>> The ALG_SET_KEY_ID is great for skcipher where you key is a fixed size blob. 
>> There is works really well. And you will need if we let keyctl derive the 
>> session key from a set of asymmetric keys.
>> 
>> I get that we want to enable OpenSSL and others to gain access to kernel 
>> crypto operations. We want the same actually for ELL as well. However what 
>> is dangerous here is that AF_ALG can never support hardware based keys. So 
>> that means support in OpenSSL is broken by design. See earlier emails from 
>> David Woodhouse. If you have your certificates / keys in hardware then you 
>> need to be able to make them work with OpenSSL. We can not ignore this. It 
>> is a design we need to consider from the beginning.
> 
> If we will have the TPM interface that allows to invoke functions for HW keys 
> and
> the *_KEY_ID, then I don't see why AF_ALG would not work. As I said, in case 
> when
> the keys will be in HW the algif_akcipher will not use akcipher api, but use 
> the
> TPM defined functions in the same way as the keyctl would use it.
> So I think both AF_ALG and keyctl should work just fine.

When David and I talked to Herbert at the kernel summit in Seoul, he said that 
the crypto subsystem (and with that AF_ALG) always needs to support software 
fallback crypto. In case of hardware based keys, there is no software fallback 
possible. If you have a hardware key, then it always needs to use the hardware 
crypto that comes with the key.

And that is precisely my point in going forward with keyctl for all of these. 
We need _one_ interface that can handle software and hardware keys / key pairs.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Marcel Holtmann]

Hi Tadeusz,

>> And I have the feeling that akcipher is not the best approach for adding a 
>> key exchange method. I think we need a new method for doing exactly that. At 
>> the base of it, the key exchange is fundamentally different.
> 
> It is unfortunate that, unlike the symmetric ciphers, not all the
> asymmetric ciphers have the same simple encrypt/decrypt interface.
> As you said key exchange algorithms are different. To solve that we should
> define new methods in akcipher.h for key exchange as follows:
> 
> diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
> index c37cc59..d50d834 100644
> --- a/include/crypto/akcipher.h
> +++ b/include/crypto/akcipher.h
> @@ -383,4 +383,41 @@ static inline int crypto_akcipher_set_priv_key(struct 
> crypto_akcipher *tfm,
> 
>   return alg->set_priv_key(tfm, key, keylen);
> }
> +
> +/**
> + * crypto_akcipher_gen_public() - Invoke appropriate key exchange function
> + *
> + * Function invokes the specific key exchange function, which calculates
> + * the public component.
> + *
> + * @req: asymmetric key request
> + *
> + * Return: zero on success; error code in case of error
> + */
> +static inline int crypto_akcipher_gen_public(struct akcipher_request *req)
> +{
> + struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
> + struct akcipher_alg *alg = crypto_akcipher_alg(tfm);
> +
> + return alg->encrypt(req);
> +}
> +
> +/**
> + * crypto_akcipher_gen_shared() - Invoke appropriate key exchange function
> + *
> + * Function invokes the specific key exchange function, which calculates
> + * the shared secret component.
> + *
> + * @req: asymmetric key request
> + *
> + * Return: zero on success; error code in case of error
> + */
> +static inline int crypto_akcipher_gen_shared(struct akcipher_request *req)
> +{
> + struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
> + struct akcipher_alg *alg = crypto_akcipher_alg(tfm);
> +
> + return alg->decrypt(req);
> +}
> +
> #endif
> 
> In this way we can define a generic user side of the key exchange interface,
> and on the the driver side of the akcipher, the implementations would overload
> the existing akcipher encrypt(), decrypt(), set_pub_key(), set_priv_key() 
> methods
> and do what should be done for a given algorithm. We just need to agree on the
> format of the input parameters to these operations.

I think trying to heavily map this to encrypt and decrypt is a bad idea. These 
callbacks are not really designed to return what you need them to return. Key 
exchange is different.

So why are we trying to push this into akcipher in the first place? I mean lets 
just create a keyexch (or similar name) and design it explicitly for key 
exchange handling. We are also not trying to map hashes into skcipher. I think 
the clear distinction of semantics calls for a different API.

>> From an API point of view, I am also not convinced that it is a good idea to 
>> generate the private keys used on the fly. I think this all needs to be a 
>> lot more deterministic and flexible. In addition there are cases where you 
>> want to point to specific private / public key pair that you locally have. 
>> There are even protocols like Bluetooth that have defined fixed debug key 
>> pairs. If we can not support that, then this approach is not generic enough.
> 
> Agree, we need to support all the cases, but dealing with different type of 
> keys
> needs to happen above this API. This API should solely be an interface to math
> primitives of certain operations, which can be implemented in SW or can be HW
> accelerated. Modules like public_key or afalg_akcipher need to be smart enough
> and know what key types they deal with and do the right thing depending on 
> that
> knowledge.

I am not sure this can be that easily generalized. Just pushing the problem of 
key types and formats off to userspace just forces complexity and extra 
knowledge there. We need to be really carefully here. And I really dislike 
introducing custom ASN.1 structures that are not backed by an actual RFC 
document.

>> So my thinking actually is that we need a new key exchange abstraction in 
>> the crypto stack. However I am not sure that an userspace facing API should 
>> be done via AF_ALG. I think that does not fit. I think that doing it via 
>> keyctl is a lot more logical place.
> 
> The advantage of exposing akcipher via AF_ALG is that we can support generic
> applications like OpenSSL or web server. I agree that for some use cases 
> keyctl
> will make more sense, but this shouldn't prevent us from allowing the 
> mentioned
> applications using hardware crypto accelerators that implement e.g. RSA, and 
> can
> work with SW keys. The two interfaces should coexist, and they should work 
> together.
> Keyctl should internally use akcipher for SW keys instead of introducing its 
> own
> implementation, and algif_akcipher should use keyring to deal with HW keys.
> I'm working with David Howells to 

Re: [PATCH V2] crypto: implement DH primitives under akcipher API

[Marcel Holtmann]

Hi Salvatore,

> Implement Diffie-Hellman primitives required by the scheme under the
> akcipher API. Here is how it works.
> 1) Call set_pub_key() by passing DH parameters (p,g) in PKCS3 format
> 2) Call set_priv_key() to set your own private key (xa) in raw format
> 3) Call decrypt() without passing any data as input to get back the
>   public part which will be computed as g^xa mod p
> 4) Call encrypt() by passing the counter part public key (yb) in raw format
>   as input to get back the shared secret calculated as zz = yb^xa mod p

I am still not convinced that akcipher is good match for key exchange methods. 
I think we should try to introduce a new abstraction here.

Overloading set_pub_key() with DH params and using decrypt() for private/public 
key pair generation seems not a good fit. It does not really match.

And as I said before, we know for certain that ECDH has to happen as well. So 
we need to forward look into making that fit as well.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Marcel Holtmann]

Hi Stephan,

>>> +static int dh_check_params_length(unsigned int p_len)
>>> +{
>>> +   switch (p_len) {
>>> +   case 768:
>>> +   case 1024:
>>> +   case 1536:
>>> +   case 2048:
>>> +   case 3072:
>>> +   case 4096:
>>> +   return 0;
>>> +   }
>>> +   return -EINVAL;
>>> +}
>> 
>> As far back as 1999, the FreeS/WAN project refused to
>> implement the 768-bit IPsec group 1 (even though it was
>> the only one required by the RFCs) because it was not thought
>> secure enough. I think the most-used group was 1536-bit
>> group 5; it wasn't in the original RFCs but nearly everyone
>> implemented it.
>> 
 And besides, I would like to disallow all < 2048 right from the start.
>> 
>> I'm not up-to-date on the performance of attacks. You may be right,
>> or perhaps the minimum should be even higher. Certainly there is
>> no reason to support 768 or 1024-bit groups.
>> 
>> On the other hand, we should consider keeping the 1536-bit
>> group since it is very widely used, likely including by people
>> we'll want to interoperate with.
> 
> Considering the recent attacks which kind of broke DH <= 768, all smaller 
> than 
> 1024 is not to be used any more. Even 1024 bit is not too far off from 768 so 
> I would consider not using 1024 to keep a safety margin.
> 
> It is widely suggested to use 2048 and higher. But for compatibility, I agree 
> with 1536.
> 
> However, that leaves us with the open question for the upper bound. I have no 
> idea which sizes hardware may implement. But considering that OpenSSH uses DH 
> up to and including 8192 these days (see /etc/ssh/moduli), I would suggest to 
> allow 8192 too.
> 
> In addition, we should all be prepared to increase the limit with a 
> reasonable 
> effort. Thus, if we have implementations covering hardware support, they 
> should cope with reasonable efforts.

and why is this not an userspace policy decision on what minimum it allows? Why 
are we discussing this in the context of the kernel in the first place? The 
kernel should just expose the support for it.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Marcel Holtmann]

Hi Salvatore,

>>> Implement Diffie-Hellman primitives required by the scheme under the
>>> akcipher API. Here is how it works.
>>> 1) Call set_pub_key() by passing DH parameters (p,g) in PKCS3 format
>>> 2) Call set_priv_key() to set your own private key (xa) in raw format
>> 
>> this combination seems odd since it is normally the remote public key and 
>> the local private key. Generally the public key and private key are both 
>> remote ones.
> 
> I'm not sure I understand what you mean here. Usually the public key is
> remote and the private key is local. How can the private key be remote?

I accidentally mistyped. I meant of course local.

>> 
>> For using PKCS3 format is this standardized somewhere? I don't think it is a 
>> good idea to invent new ones here.
> 
> PKCS3 is the format used by openssl for genating DH params, that's why I
> used it.

Is that OpenSSL specific or backed up by a RFC?

>> 
>> In addition, how would this work for ECDH?
> 
> Don't know. There is not even ECC support right now.

If you call something dh-generic, then we need to think about how it would work 
for all the other ciphers that it might be used with. Making this RSA specific 
is not a good idea.

> 
>> 
>>> 3) Call decrypt() without passing any data as input to get back the
>>>  public part which will be computed as g^xa mod p
>>> 4) Call encrypt() by passing the counter part public key (yb) in raw format
>>>  as input to get back the shared secret calculated as zz = yb^xa mod p
>>> 
>>> A test is included in the patch. Test vector has been generated with
>>> openssl
>>> 
>>> Signed-off-by: Salvatore Benedetto 
>>> ---
>>> crypto/Kconfig|   8 ++
>>> crypto/Makefile   |   7 ++
>>> crypto/dh.c   | 264 
>>> ++
>>> crypto/pkcs3.asn1 |   5 ++
>>> crypto/tcrypt.c   |   4 +
>>> crypto/testmgr.c  | 140 +++--
>>> crypto/testmgr.h  | 208 +-
>>> 7 files changed, 627 insertions(+), 9 deletions(-)
>>> create mode 100644 crypto/dh.c
>>> create mode 100644 crypto/pkcs3.asn1
>>> 
>>> diff --git a/crypto/Kconfig b/crypto/Kconfig
>>> index f6bfdda..fd5b78d 100644
>>> --- a/crypto/Kconfig
>>> +++ b/crypto/Kconfig
>>> @@ -101,6 +101,14 @@ config CRYPTO_RSA
>>> help
>>>   Generic implementation of the RSA public key algorithm.
>>> 
>>> +config CRYPTO_DH
>>> +   tristate "Diffie-Hellman algorithm"
>>> +   select CRYPTO_AKCIPHER
>>> +   select MPILIB
>>> +   select ASN1
>> 
>> I really wonder that depending on ASN1 is a good idea here. As mentioned 
>> above ECDH would make sense to actually have supported from the beginning. 
>> The Bluetooth subsystem could be then converted to utilize in kernel ECC key 
>> generation and ECDH shared secret computation. It would be good to show this 
>> is truly generic DH.
>> 
> 
> This is an RFC. I understand it is not the best approach, but
> the idea behind was to try to reuse the akcipher for DH.

And I have the feeling that akcipher is not the best approach for adding a key 
exchange method. I think we need a new method for doing exactly that. At the 
base of it, the key exchange is fundamentally different.

>From an API point of view, I am also not convinced that it is a good idea to 
>generate the private keys used on the fly. I think this all needs to be a lot 
>more deterministic and flexible. In addition there are cases where you want to 
>point to specific private / public key pair that you locally have. There are 
>even protocols like Bluetooth that have defined fixed debug key pairs. If we 
>can not support that, then this approach is not generic enough.

So my thinking actually is that we need a new key exchange abstraction in the 
crypto stack. However I am not sure that an userspace facing API should be done 
via AF_ALG. I think that does not fit. I think that doing it via keyctl is a 
lot more logical place.

It also means that we need a separate keyctl to actually generate the local 
private / public key pairs first. I think that makes sense no matter what. You 
can generate the keys, the private key stays in kernel memory forever and you 
can read out the public key. Some protocols will throw away the keys after 
single use, but others might actually reuse them. Or as mentioned above has 
fixed keys for debugging purposes. Using keyctl should then also make it easy 
to handle RSA vs ECC for the key generation since we need to be able to store 
both types at some point anyway. Also in cases where keys are RSA keys in ASN.1 
format in the first place or are learned from certificates are already present 
and uniquely presented in the kernel. No need to invent yet another format for 
keys.

Especially in the case where you create a session key based out of certificates 
and existing public / private key pairs, it makes sense that keyctl can turn 
them directly into a new key. In most cases these are symmetric keys that can 

Re: [PATCH] crypto: implement DH primitives under akcipher API

[Marcel Holtmann]

Hi Salvatore,

> Implement Diffie-Hellman primitives required by the scheme under the
> akcipher API. Here is how it works.
> 1) Call set_pub_key() by passing DH parameters (p,g) in PKCS3 format
> 2) Call set_priv_key() to set your own private key (xa) in raw format

this combination seems odd since it is normally the remote public key and the 
local private key. Generally the public key and private key are both remote 
ones.

For using PKCS3 format is this standardized somewhere? I don't think it is a 
good idea to invent new ones here.

In addition, how would this work for ECDH?

> 3) Call decrypt() without passing any data as input to get back the
>   public part which will be computed as g^xa mod p
> 4) Call encrypt() by passing the counter part public key (yb) in raw format
>   as input to get back the shared secret calculated as zz = yb^xa mod p
> 
> A test is included in the patch. Test vector has been generated with
> openssl
> 
> Signed-off-by: Salvatore Benedetto 
> ---
> crypto/Kconfig|   8 ++
> crypto/Makefile   |   7 ++
> crypto/dh.c   | 264 ++
> crypto/pkcs3.asn1 |   5 ++
> crypto/tcrypt.c   |   4 +
> crypto/testmgr.c  | 140 +++--
> crypto/testmgr.h  | 208 +-
> 7 files changed, 627 insertions(+), 9 deletions(-)
> create mode 100644 crypto/dh.c
> create mode 100644 crypto/pkcs3.asn1
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index f6bfdda..fd5b78d 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -101,6 +101,14 @@ config CRYPTO_RSA
>   help
> Generic implementation of the RSA public key algorithm.
> 
> +config CRYPTO_DH
> + tristate "Diffie-Hellman algorithm"
> + select CRYPTO_AKCIPHER
> + select MPILIB
> + select ASN1

I really wonder that depending on ASN1 is a good idea here. As mentioned above 
ECDH would make sense to actually have supported from the beginning. The 
Bluetooth subsystem could be then converted to utilize in kernel ECC key 
generation and ECDH shared secret computation. It would be good to show this is 
truly generic DH.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/26] Bluetooth: Use skcipher and hash

[Marcel Holtmann]

Hi Herbert,

> This patch replaces uses of blkcipher with skcipher and the long
> obsolete hash interface with shash.
> 
> Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au>

Acked-by: Marcel Holtmann <mar...@holtmann.org>

> ---
> 
> net/bluetooth/smp.c |  135 
> 
> 1 file changed, 63 insertions(+), 72 deletions(-)

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] crypto: RSA padding algorithm

[Marcel Holtmann]

Hi Stephan,

>> This patch adds PKCS#1 v1.5 standard RSA padding as a separate template.
>> This way an RSA cipher with padding can be obtained by instantiating
>> "pkcs1pad(rsa)".  The reason for adding this is that RSA is almost
>> never used without this padding (or OAEP) so it will be needed for
>> either certificate work in the kernel or the userspace, and also I hear
>> that it is likely implemented by hardware RSA in which case an
>> implementation of the whole "pkcs1pad(rsa)" can be provided.
> 
> In general, I think that there is a PKCS 1 implementation in the kernel in 
> crypto/asymmetric_keys/rsa.c
> 
> Shouldn't that all somehow being synchronized?
> 
> Maybe this patch should go in but then crypto/asymmetric_keys/rsa.c should 
> kind of being removed or point to kernel crypto API?

I think crypto/asymmetric_keys/ needs to move to security/keys/asymmetric/ and 
then utilize akcipher and also PKCS 1 from crypto/

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] crypto: AF_ALG -- add setpubkey setsockopt call

[Marcel Holtmann]

Hi Stephan,

> For supporting asymmetric ciphers, user space must be able to set the
> public key. The patch adds a new setsockopt call for setting the public
> key.
> 
> Signed-off-by: Stephan Mueller 
> ---
> crypto/af_alg.c | 14 +++---
> include/crypto/if_alg.h |  1 +
> 2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index a8e7aa3..bf6528e 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -173,13 +173,16 @@ static int alg_bind(struct socket *sock, struct 
> sockaddr *uaddr, int addr_len)
> }
> 
> static int alg_setkey(struct sock *sk, char __user *ukey,
> -   unsigned int keylen)
> +   unsigned int keylen, bool pubkey)
> {
>   struct alg_sock *ask = alg_sk(sk);
>   const struct af_alg_type *type = ask->type;
>   u8 *key;
>   int err;
> 
> + if (pubkey && !type->setpubkey)
> + return -EOPNOTSUPP;
> +
>   key = sock_kmalloc(sk, keylen, GFP_KERNEL);
>   if (!key)
>   return -ENOMEM;
> @@ -188,7 +191,10 @@ static int alg_setkey(struct sock *sk, char __user *ukey,
>   if (copy_from_user(key, ukey, keylen))
>   goto out;
> 
> - err = type->setkey(ask->private, key, keylen);
> + if (pubkey)
> + err = type->setpubkey(ask->private, key, keylen);
> + else
> + err = type->setkey(ask->private, key, keyless);

why is this kind of hackery needed? Why not just introduce alg_setpubkey to 
keep this a lot cleaner.

> 
> out:
>   sock_kzfree_s(sk, key, keylen);
> @@ -212,12 +218,14 @@ static int alg_setsockopt(struct socket *sock, int 
> level, int optname,
> 
>   switch (optname) {
>   case ALG_SET_KEY:
> + case ALG_SET_PUBKEY:
>   if (sock->state == SS_CONNECTED)
>   goto unlock;
>   if (!type->setkey)
>   goto unlock;
> 
> - err = alg_setkey(sk, optval, optlen);
> + err = alg_setkey(sk, optval, optlen,
> +  (optname == ALG_SET_PUBKEY) ? true : false);
>   break;

Same here. Why not give ALG_SET_PUBKEY a separate case statement. Especially 
since you have to check type->setkey vs type->setpubkey.

>   case ALG_SET_AEAD_AUTHSIZE:
>   if (sock->state == SS_CONNECTED)
> diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> index 018afb2..ca4dc72 100644
> --- a/include/crypto/if_alg.h
> +++ b/include/crypto/if_alg.h
> @@ -49,6 +49,7 @@ struct af_alg_type {
>   void *(*bind)(const char *name, u32 type, u32 mask);
>   void (*release)(void *private);
>   int (*setkey)(void *private, const u8 *key, unsigned int keylen);
> + int (*setpubkey)(void *private, const u8 *key, unsigned int keylen);
>   int (*accept)(void *private, struct sock *sk);
>   int (*setauthsize)(void *private, unsigned int authorize);

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] crypto: RSA padding transform

[Marcel Holtmann]

Hi Andrzej,

>>> I can see now that with all these padding schemes there will be more buffer
>>> copied on top of this, so I wonder if it still make sense.
>>> Herbert?
>> 
>> When the padding stuff comes into play, I think the SGL approach avoids
>> memcpy() invocations.
>> 
>> For example, Andrzej's approach currently is to copy the *entire* source data
>> into a buffer where the padding is added.
>> 
>> With SGLs, Andrzej only needs a buffer with the padding (which I would think
>> could even reside on the stack, provided some bounds checks are done), and
>> modify the SGL by preprending the padding buffer to the SGL with the source
>> data.
> 
> Yes, in the case of the padding schemes, using sgl's would actually
> save a memcpy both on encrypt/sign and decrypt/verify.  Whether it'd
> actually help I'm not sure -- the number of pointers you need to
> touch, etc. may add up to around 128/256 bytes of memory access
> anyway.

I think we have akcipher patches using sgl now. Would it make sense to update 
this patch against them.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/5] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Stephan,

>>> Albeit that all sounds like the crown jewel, how do you propose that shall
>>> happen?
>>> 
>>> Assume that you have a web server that has a pub and priv key in its
>>> current configuration -- I guess that is the vast majority of configs.
>>> 
>>> Can you please elaborate how the process for such a web server shall
>>> really
>>> work?
>> 
>> 1. Create a kernel-side key.
>> 2. Use it.
>> 
>> That may require adding an API similar to the one you're proposing, but
>> working with kernel keys instead of directly with akcipher. Or perhaps
>> the key subsystem can already offer what you need in userspace. David?
> 
> Ohh, I see. So, you are saying that there should not be a setpub/privkey for 
> the akcipher AF_ALG interface?!

I tested support for adding ALG_SET_KEY_ID which takes the 32-bit key serial 
and then using AF_ALG with an skcipher. That works so far so fine. For making 
this super clean and get it upstream, it needs a symmetric key type (which is 
planned to be added by David). I can post my current patch as RFC since it is 
dead simple actually.

> If somebody wants to use akcipher, he shall set the key via the keyring and 
> akcipher shall obtain it from the keyring?
> 
> However, for the actual data shoveling, AF_ALG should still be used?

There is no massive data shoveling by an akcipher. All data shoveling for TLS 
is done via the symmetric session key that is negotiated. Actually with 
asymmetric ciphers, your keys will be normally larger than your clear text 
anyway.

So if a server has public/private key pair, then the first thing that should 
the server do is load this key pair into the kernel and retrieve a key serial 
for it. And then use this key id to derive the session key. That session key 
can then be used with AF_ALG and skcipher for the data shoveling.

However that all said, the keys should never leave the kernel. Neither the 
private key nor the session key. There is no point in sending keys through 
userspace. We actually do not want this at all. That is especially important if 
your actual private/public key pair is in hardware. So maybe your RSA 
accelerator might expose secure storage for the keys. Loading them over and 
over again from userspace makes no sense.

As David mentioned, we need to take a deep look at what the userspace API for 
asymmetric cipher suites (and we also have needs for ECDH etc. and not just 
RSA) should look like. Just exposing akcipher via AF_ALG is premature. If we 
expose it now, it is not an API that we can take back. Having two userspace 
APIs for the exactly the same functionality is a bad thing. Especially if one 
is limited to software only keys.

We also need to look at the larger picture here. And that is TLS support in the 
kernel. Potentially via AF_KCM or something similar.

And with TLS in mind, we actually do want to just load the whole X.509 
certificate into the key subsystem. It will then allow you retrieve the key id, 
validate the certificate and use the key. Current kernels already support this 
kind of functionality and we want to enable this and extend it. Having random 
pieces in userspace and/or kernel space to extract keys from the containers is 
a pretty bad idea. We really want this centralized. Here the same goal applies, 
to not have multiple userspace APIs doing the same thing.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: Add support for ALG_SET_KEY_ID for skcipher

[Marcel Holtmann]

This adds support for a new socket options called ALG_SET_KEY_ID that
allows providing the symmetric key via a key serial from the keys
subsystem.

NOTE: Currently we do not have a dedicated symmetric key type and using
the user key type is not optional. Also lookup_user_key() is currently
private to the keys subsystem and might need to be exposed to usage by
the crypto subystem first. This is just a RFC and not for merging !!!

Signed-off-by: Marcel Holtmann <mar...@holtmann.org>
---
 crypto/af_alg.c | 14 ++
 crypto/algif_skcipher.c | 25 +
 include/crypto/if_alg.h |  2 ++
 include/uapi/linux/if_alg.h |  1 +
 4 files changed, 42 insertions(+)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index a8e7aa3e257b..48ddbb4063aa 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -203,6 +203,7 @@ static int alg_setsockopt(struct socket *sock, int level, 
int optname,
struct alg_sock *ask = alg_sk(sk);
const struct af_alg_type *type;
int err = -ENOPROTOOPT;
+   key_serial_t keyid;
 
lock_sock(sk);
type = ask->type;
@@ -225,6 +226,19 @@ static int alg_setsockopt(struct socket *sock, int level, 
int optname,
if (!type->setauthsize)
goto unlock;
err = type->setauthsize(ask->private, optlen);
+   break;
+   case ALG_SET_KEY_ID:
+   if (sock->state == SS_CONNECTED)
+   goto unlock;
+   if (!type->setkeyid)
+   goto unlock;
+
+   err = -EFAULT;
+   if (get_user(keyid, (key_serial_t __user *) optval))
+   goto unlock;
+
+   err = type->setkeyid(ask->private, keyid);
+   break;
}
 
 unlock:
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 945075292bc9..5dfae3cb6e20 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -22,6 +22,8 @@
 #include 
 #include 
 #include 
+#include 
+#include "../security/keys/internal.h"
 
 struct skcipher_sg_list {
struct list_head list;
@@ -764,6 +766,28 @@ static int skcipher_setkey(void *private, const u8 *key, 
unsigned int keylen)
return crypto_ablkcipher_setkey(private, key, keylen);
 }
 
+static int skcipher_setkeyid(void *private, key_serial_t id)
+{
+   key_ref_t key_ref;
+   int err = -EPROTOTYPE;
+
+   key_ref = lookup_user_key(id, 0, KEY_NEED_READ);
+   if (IS_ERR(key_ref))
+   return PTR_ERR(key_ref);
+
+   if (key_ref_to_ptr(key_ref)->type == _type_user) {
+   struct user_key_payload *upayload;
+
+   upayload = rcu_dereference_key(key_ref_to_ptr(key_ref));
+
+   err = crypto_ablkcipher_setkey(private, upayload->data,
+  upayload->datalen);
+   }
+
+   key_ref_put(key_ref);
+   return err;
+}
+
 static void skcipher_wait(struct sock *sk)
 {
struct alg_sock *ask = alg_sk(sk);
@@ -832,6 +856,7 @@ static const struct af_alg_type algif_type_skcipher = {
.bind   =   skcipher_bind,
.release=   skcipher_release,
.setkey =   skcipher_setkey,
+   .setkeyid   =   skcipher_setkeyid,
.accept =   skcipher_accept_parent,
.ops=   _skcipher_ops,
.name   =   "skcipher",
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index 018afb264ac2..f71d88162326 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -51,6 +52,7 @@ struct af_alg_type {
int (*setkey)(void *private, const u8 *key, unsigned int keylen);
int (*accept)(void *private, struct sock *sk);
int (*setauthsize)(void *private, unsigned int authsize);
+   int (*setkeyid)(void *private, key_serial_t id);
 
struct proto_ops *ops;
struct module *owner;
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index d81dcca5bdd7..28cdc05695c0 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -34,6 +34,7 @@ struct af_alg_iv {
 #define ALG_SET_OP 3
 #define ALG_SET_AEAD_ASSOCLEN  4
 #define ALG_SET_AEAD_AUTHSIZE  5
+#define ALG_SET_KEY_ID 6
 
 /* Operations */
 #define ALG_OP_DECRYPT 0
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: Add support for ALG_SET_KEY_ID for skcipher

[Marcel Holtmann]

Hi Stephan,

>> This adds support for a new socket options called ALG_SET_KEY_ID that
>> allows providing the symmetric key via a key serial from the keys
>> subsystem.
>> 
>> NOTE: Currently we do not have a dedicated symmetric key type and using
>> the user key type is not optional. Also lookup_user_key() is currently
>> private to the keys subsystem and might need to be exposed to usage by
>> the crypto subystem first. This is just a RFC and not for merging !!!
> 
> First, thanks for sharing.
> 
> Albeit I have not had a deep look into that code, but I think your patch is 
> incomplete: you have to tie the kernel crypto API to the key retention system 
> in the Kconfig.
> 
> I guess that is one of the concerns that Herbert may have? See my other email 
> regarding this.

of course we have to tie this together. And I need to deal with Kconfig once we 
have symmetric key type support.

However I am not too much worried since reality is that the keys subsystem is 
pretty much mandatory if you use module signing (or firmware signing in the 
future). And with moving the keys subsystem to use akcipher and consolidate on 
a single RSA implementation in the kernel, I am not convinced that this is 
actually a real problem.

Also it is perfectly valid to return EOPNOTSUPP when using ALG_SET_KEY_ID and 
you do not have the keys subsystem configured. I do not see that as a problem.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/5] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Stephan,

> This patch set adds the AF_ALG user space API to externalize the
> asymmetric cipher API recently added to the kernel crypto API.
> 
> The patch set is tested with the user space library of libkcapi [1].
> Use [1] test/test.sh for a full test run. The test covers the
> following scenarios:
> 
>   * sendmsg of one IOVEC
> 
>   * sendmsg of 16 IOVECs with non-linear buffer
> 
>   * vmsplice of one IOVEC
> 
>   * vmsplice of 15 IOVECs with non-linear buffer
> 
>   * invoking multiple separate cipher operations with one
> open cipher handle
> 
>   * encryption with private key (using vector from testmgr.h)
> 
>   * encryption with public key (using vector from testmgr.h)
> 
>   * decryption with private key (using vector from testmgr.h)

after having discussions with David Howells and David Woodhouse, I don't think 
we should expose akcipher via AF_ALG at all. I think the akcipher operations 
for sign/verify/encrypt/decrypt should operate on asymmetric keys in the first 
place. With akcipher you are pretty much bound to public and private keys and 
the key is the important part and not the akcipher itself. Especially since we 
want to support private keys in hardware (like TPM for example).

It seems more appropriate to use keyctl to derive the symmetric session key 
from your asymmetric key. And then use the symmetric session key id with 
skcipher via AF_ALG. Especially once symmetric key type has been introduced 
this seems to be trivial then.

I am not really in favor of having two userspace facing APIs for asymmetric 
cipher usage. And we need to have an API that is capable to work with hardware 
keys.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: Fix ASN.1 key handling for RSA akcipher

[Marcel Holtmann]

Hi Stephan,

 The RSA algorithm provides two ASN.1 key types. One for RSA Private Key
 and another for RSA Public Key. Use these two already defined ASN.1
 definitions instead of inventing a new one.
 
 Signed-off-by: Marcel Holtmann mar...@holtmann.org
 ---
 crypto/Makefile   |  9 ++---
 crypto/rsa_helper.c   | 13 +
 crypto/rsakey.asn1|  5 -
 crypto/rsaprivatekey.asn1 | 13 +
 crypto/rsapublickey.asn1  |  4 
 5 files changed, 32 insertions(+), 12 deletions(-)
 delete mode 100644 crypto/rsakey.asn1
 create mode 100644 crypto/rsaprivatekey.asn1
 create mode 100644 crypto/rsapublickey.asn1
 
 diff --git a/crypto/Makefile b/crypto/Makefile
 index 3cc91c3301c7..0b056c411aa7 100644
 --- a/crypto/Makefile
 +++ b/crypto/Makefile
 @@ -31,10 +31,13 @@ obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
 obj-$(CONFIG_CRYPTO_PCOMP2) += pcompress.o
 obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o
 
 -$(obj)/rsakey-asn1.o: $(obj)/rsakey-asn1.c $(obj)/rsakey-asn1.h
 -clean-files += rsakey-asn1.c rsakey-asn1.h
 +$(obj)/rsapublickey-asn1.o: $(obj)/rsapublickey-asn1.c
 $(obj)/rsapublickey-asn1.h +clean-files += rsapublickey-asn1.c
 rsapublickey-asn1.h
 
 -rsa_generic-y := rsakey-asn1.o
 +$(obj)/rsaprivatekey-asn1.o: $(obj)/rsaprivatekey-asn1.c
 $(obj)/rsaprivatekey-asn1.h +clean-files += rsaprivatekey-asn1.c
 rsaprivatekey-asn1.h
 +
 +rsa_generic-y := rsapublickey-asn1.o rsaprivatekey-asn1.o
 rsa_generic-y += rsa.o
 rsa_generic-y += rsa_helper.o
 obj-$(CONFIG_CRYPTO_RSA) += rsa_generic.o
 diff --git a/crypto/rsa_helper.c b/crypto/rsa_helper.c
 index 8d96ce969b44..26617e3132fb 100644
 --- a/crypto/rsa_helper.c
 +++ b/crypto/rsa_helper.c
 @@ -15,7 +15,8 @@
 #include linux/err.h
 #include linux/fips.h
 #include crypto/internal/rsa.h
 -#include rsakey-asn1.h
 +#include rsapublickey-asn1.h
 +#include rsaprivatekey-asn1.h
 
 int rsa_get_n(void *context, size_t hdrlen, unsigned char tag,
const void *value, size_t vlen)
 @@ -109,9 +110,13 @@ int rsa_parse_key(struct rsa_key *rsa_key, const void
 *key, int ret;
 
  free_mpis(rsa_key);
 -ret = asn1_ber_decoder(rsakey_decoder, rsa_key, key, key_len);
 -if (ret  0)
 -goto error;
 +ret = asn1_ber_decoder(rsapublickey_decoder, rsa_key, key, key_len);
 +if (ret  0) {
 +ret = asn1_ber_decoder(rsaprivatekey_decoder, rsa_key,
 +   key, key_len);
 
 
 Wouldn't it be better to have 2 parse_key functions here? We (will) have 2 
 setkey functions which are the callers of parse_key.

I am no longer convinced that two setkey function will actually help. The RSA 
Private Key data structure contains the public and private key. And the RSA 
Public Key data structure contains just the private key.

 The reason is that the added if requires CPU cycles that can be easily 
 avoided.
 
 Hence I propose:
 
 rsa_parse_pubkey()
   rsapublickey_decoder
 
 rsa_parse_privkey()
   rsaprivatekey_decoder
 
 to avoid parsing a key as pub key even though we already know it is a priv 
 key.

If we really care about CPU cycles when setting the key, then we should not 
even be discussing ASN.1 parsing here. I think we really need to add support 
for setkeyid callback and use struct key / key serial to reference a given key.

This whole parsing of DER over and over again is a bad API design anyway. The 
kernel has a keys subsystem that knows how to handle asymmetric keys and parse 
key credentials out of it. Especially akcipher should learn how to use these 
keys.

It is also the only real way to unify the existing RSA implementations and 
allowing future asymmetric ciphers like ECC to utilize this correctly. I mean, 
we really need to stop handing DER encoded keys around. The amount of encoding 
and decoding that needs to be done in the kernel is the real waste of CPU 
cycles.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: Fix ASN.1 key handling for RSA akcipher

[Marcel Holtmann]

Hi Tadeusz,

 +++ b/crypto/rsaprivatekey.asn1
 @@ -0,0 +1,13 @@
 +RSAPrivateKey ::= SEQUENCE {
 +version Version,
 +modulus INTEGER ({ rsa_get_n }),-- n
 +publicExponent  INTEGER ({ rsa_get_e }),-- e
 +privateExponent INTEGER ({ rsa_get_d }),-- d
 +prime1  INTEGER,-- p
 +prime2  INTEGER,-- q
 +exponent1   INTEGER,-- d mod (p-1)
 +exponent2   INTEGER,-- d mod (q-1)
 +coefficient INTEGER -- (inverse of q) mod p
 +}
 +
 +Version ::= INTEGER
 
 If you want to do this you should also update the existing RSA test vectors, 
 because
 they are failing after this patch is applied.

the test vectors have been failing no matter what. The crypto/rsakey.asn1 is 
actually broken as I explained in previous emails. That the ASN.1 parser 
accepted the test vectors was a bug which has been fixed now.

 The reason is that there is no version in the private keys in crypto/testmgr.h
 And the QAT RSA implementation should also be updated so they are consistent.

The QAT code should be updated indeed, but honestly I am still maintaining the 
case that akcipher should just only operate on setkeyid with struct key / key 
serial and some ASN.1 blob that we have to decode and recode all the time.

 I have already started to do the changes proposed for the akcipher api to add 
 SGLs
 support and to split the set_key for set_publickey and set_privatekey so I 
 will
 take care of this.

I am not in favor of just hacking in this split until the semantics are 
actually understood. As said, the right solution from my point of view is to 
remove setkey from akcipher and replace it with setkeyid instead.

Remember that a private key contains the public key as well. Meaning operations 
are mutually exclusive. And there is really no point in forcing the caller to 
split things into two. With asymmetric cryptography you either have private and 
public key or you just have the public key. The case where you only have the 
private key is pointless.

We could keep the setkey as it is to load the private + public key information 
and add an extra setpubkey for loading only the public key. Then again a 
setkeyid would solve both of these problems since the key material would be 
nicely represented in a struct key.

However we actually want to load the keys into the asymmetric key type and use 
it from there. The asymmetric key type should be the only entity that has to 
deal with ASN.1 encoding. Having an akcipher deal with ASN.1 is just wrong.

What we really want is to be able to use keys from certificates to verify and 
encrypt information. And we want this all in one single place and not duplicate 
this whole ASN.1 stuff in the keys subsystem and the crypto subsystem. This 
means I am strongly against trying to add setkey, setpubkey, setcert, 
setkeychain or whatever else you might need when it comes to akcipher. There is 
one thing that is needed and that is setkeyid and have to reference the key 
serial from the keys subsystem.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: Fix ASN.1 key handling for RSA akcipher

[Marcel Holtmann]

The RSA algorithm provides two ASN.1 key types. One for RSA Private Key
and another for RSA Public Key. Use these two already defined ASN.1
definitions instead of inventing a new one.

Signed-off-by: Marcel Holtmann mar...@holtmann.org
---
 crypto/Makefile   |  9 ++---
 crypto/rsa_helper.c   | 13 +
 crypto/rsakey.asn1|  5 -
 crypto/rsaprivatekey.asn1 | 13 +
 crypto/rsapublickey.asn1  |  4 
 5 files changed, 32 insertions(+), 12 deletions(-)
 delete mode 100644 crypto/rsakey.asn1
 create mode 100644 crypto/rsaprivatekey.asn1
 create mode 100644 crypto/rsapublickey.asn1

diff --git a/crypto/Makefile b/crypto/Makefile
index 3cc91c3301c7..0b056c411aa7 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -31,10 +31,13 @@ obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
 obj-$(CONFIG_CRYPTO_PCOMP2) += pcompress.o
 obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o
 
-$(obj)/rsakey-asn1.o: $(obj)/rsakey-asn1.c $(obj)/rsakey-asn1.h
-clean-files += rsakey-asn1.c rsakey-asn1.h
+$(obj)/rsapublickey-asn1.o: $(obj)/rsapublickey-asn1.c 
$(obj)/rsapublickey-asn1.h
+clean-files += rsapublickey-asn1.c rsapublickey-asn1.h
 
-rsa_generic-y := rsakey-asn1.o
+$(obj)/rsaprivatekey-asn1.o: $(obj)/rsaprivatekey-asn1.c 
$(obj)/rsaprivatekey-asn1.h
+clean-files += rsaprivatekey-asn1.c rsaprivatekey-asn1.h
+
+rsa_generic-y := rsapublickey-asn1.o rsaprivatekey-asn1.o
 rsa_generic-y += rsa.o
 rsa_generic-y += rsa_helper.o
 obj-$(CONFIG_CRYPTO_RSA) += rsa_generic.o
diff --git a/crypto/rsa_helper.c b/crypto/rsa_helper.c
index 8d96ce969b44..26617e3132fb 100644
--- a/crypto/rsa_helper.c
+++ b/crypto/rsa_helper.c
@@ -15,7 +15,8 @@
 #include linux/err.h
 #include linux/fips.h
 #include crypto/internal/rsa.h
-#include rsakey-asn1.h
+#include rsapublickey-asn1.h
+#include rsaprivatekey-asn1.h
 
 int rsa_get_n(void *context, size_t hdrlen, unsigned char tag,
  const void *value, size_t vlen)
@@ -109,9 +110,13 @@ int rsa_parse_key(struct rsa_key *rsa_key, const void *key,
int ret;
 
free_mpis(rsa_key);
-   ret = asn1_ber_decoder(rsakey_decoder, rsa_key, key, key_len);
-   if (ret  0)
-   goto error;
+   ret = asn1_ber_decoder(rsapublickey_decoder, rsa_key, key, key_len);
+   if (ret  0) {
+   ret = asn1_ber_decoder(rsaprivatekey_decoder, rsa_key,
+  key, key_len);
+   if (ret  0)
+   goto error;
+   }
 
return 0;
 error:
diff --git a/crypto/rsakey.asn1 b/crypto/rsakey.asn1
deleted file mode 100644
index 3c7b5df7b428..
--- a/crypto/rsakey.asn1
+++ /dev/null
@@ -1,5 +0,0 @@
-RsaKey ::= SEQUENCE {
-   n INTEGER ({ rsa_get_n }),
-   e INTEGER ({ rsa_get_e }),
-   d INTEGER ({ rsa_get_d })
-}
diff --git a/crypto/rsaprivatekey.asn1 b/crypto/rsaprivatekey.asn1
new file mode 100644
index ..58dddc7c1536
--- /dev/null
+++ b/crypto/rsaprivatekey.asn1
@@ -0,0 +1,13 @@
+RSAPrivateKey ::= SEQUENCE {
+   version Version,
+   modulus INTEGER ({ rsa_get_n }),-- n
+   publicExponent  INTEGER ({ rsa_get_e }),-- e
+   privateExponent INTEGER ({ rsa_get_d }),-- d
+   prime1  INTEGER,-- p
+   prime2  INTEGER,-- q
+   exponent1   INTEGER,-- d mod (p-1)
+   exponent2   INTEGER,-- d mod (q-1)
+   coefficient INTEGER -- (inverse of q) mod p
+}
+
+Version ::= INTEGER
diff --git a/crypto/rsapublickey.asn1 b/crypto/rsapublickey.asn1
new file mode 100644
index ..8f7f8760f2a9
--- /dev/null
+++ b/crypto/rsapublickey.asn1
@@ -0,0 +1,4 @@
+RSAPublicKey ::= SEQUENCE {
+   modulus INTEGER ({ rsa_get_n }),-- n
+   publicExponent  INTEGER ({ rsa_get_e }) -- e
+}
-- 
2.4.3

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Proposal for adding setpubkey callback to akcipher_alg

[Marcel Holtmann]

Hi Herbert,

 We already have an interface that can handle asymmetric keys and it is easy 
 to extend with new key formats and key types. So lets use that. I can 
 clearly see that after RSA, we get DSA, ECDH etc. So having a simple way to 
 handle these key formats is a good idea. That infrastructure is already in 
 place and easy to extend if needed. Especially with the background that some 
 keys might be actually in hardware or compiled into the kernel, the current 
 asymmetric key interface has the right abstraction. It is also the right 
 abstraction to deal with crypto hardware like TPM or even UEFI.
 
 The crypto API akcipher interface is never going to be used for TPM
 or UEFI.  This is a purely algorithmic interface intended for
 hardware acceleration devices.  If your key is embedded into the
 hardware or otherwise hidden then this is not the interface for you.

I think it actually is the correct interface. And it will still stay a purely 
algorithmic interface. It is just that the algorithm is bound to specific 
hardware with a specific key. I really do not understand your distinction here.

Seriously where is the difference. Lets say you have AES-CCM offload in one PCI 
card and AES-ECB offload in another PCI card. The main job of skcipher here is 
to pick the right engine. So RSA-key1 in one PCI card compared to RSA-key2 in 
another PCI card is pretty much the same concept. So really explain to me where 
you are deriving your difference from.

If the hardware can offload the operation for you, then that is what it is 
doing. Not everything is about speed. Some things are actually about security. 
And treating akcipher the same as skcipher is not going to work. They are two 
different concepts and they are used differently.

Why would you advocate that we duplicate akcipher abstraction once for hardware 
acceleration and once for security key storage operation. At the end of the day 
it is the same abstraction. As I mentioned before, for skcipher the 
acceleration aspects are clear first and foremost. However just extending that 
reasoning blindly to akcipher is short sighted. I think we have a great 
opportunity to create a really powerful and simple API for asymmetric 
cryptography and hardware assisted asymmetric cryptography.

And if you think about how RSA for example is used these days, you spent more 
time loading the certificate and the keys, then you actually spent in the 
cipher operation. It is just the stepping stone for creating the session key 
that you are using with skcipher like AES.

So I do not want to waste time setting up my keys over and over again for a 
single RSA operation. I want to set them up quickly and then run my RSA cipher 
operation. I also want to set them up securely where my private key is protect 
and not copied for every single instance.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Proposal for adding setpubkey callback to akcipher_alg

[Marcel Holtmann]

Hi Herbert,

 RSA Private Key is n + e + d (including 6 other fields). RSA Public Key is n 
 + e (no other fields).
 
 So for RSA you would make setkey to take RSA Private Key and setpubkey to 
 take RSA Public Key. Meaning you only have to use one of them since if you 
 have the private key, you always have the public key.
 
 This real difference here is that you can provide the key in two different 
 key formats. As explained RSA uses two different format.
 
 I don't have a problem with a setpubkey/setprivkey split interface.
 
 However, I'm totally against importing MPI keys which is just silly.
 The BER-encoded keys are just raw integers.  Most of the hardware
 out there take raw integers.  So it makes no sense to have our
 interface take MPIs instead of raw integers, as this would mean
 converting into MPIs and then straight back into raw integers
 for hardware devices.


after looking further into this, the whole akcipher setkey should not operate 
on BER-encoded key blobs nor MPIs. Instead it should take a struct key as input 
and reference it.

For skcipher the key as binary blob thing works nicely, but for akcipher this 
is not a good interface. I prefer to have one interface for loading 
public/private keys and not have 2 or more different ways of loading and 
parsing these keys. That will get messy really quick. They are just magnitude 
more complicated than skcipher keys since they come in more complicated 
packaging.

We already have an interface that can handle asymmetric keys and it is easy to 
extend with new key formats and key types. So lets use that. I can clearly see 
that after RSA, we get DSA, ECDH etc. So having a simple way to handle these 
key formats is a good idea. That infrastructure is already in place and easy to 
extend if needed. Especially with the background that some keys might be 
actually in hardware or compiled into the kernel, the current asymmetric key 
interface has the right abstraction. It is also the right abstraction to deal 
with crypto hardware like TPM or even UEFI.

The nice advantage is that keys do not have to be copied around. The struct key 
supports referencing keys and it also ties nicely into process and user 
permissions. So essentially you reference your key for akcipher by struct key 
or from userspace indirectly via key_serial_t. No need to duplicate the key for 
each instance of the akcipher. There is only a single key. Especially when 
handling private keys, I prefer to keep them nicely in one place and not spread 
copies around kernel memory. Duplicating sensitive key material is a bad 
security practice.

It also avoids having to duplicate the ASN.1 parsing over and over again (I 
found 3 places that parse RSA keys at the moment). Have the asymmetric key 
interface parse the key once and stop duplicating this on every single attempt. 
This is especially helpful if your key comes in form of a certificate. We can 
validate the certificate and provide a key reference. More important we can 
also validate it against the kernel system keyring or a hardware provided 
keyring or certificate.

I really want to avoid that we are getting into the business of converting from 
format a to format b to format c because everybody invents their own ones. That 
is a bad API for me. And lets face it, when it comes to public key 
cryptography, there are standards we should follow. If the Linux kernel starts 
making up yet another format and interface, then we are doing a pretty bad job.

With the support for different sub-keytypes, we can really easily optimize the 
handling of struct key for the actual hardware in question. So instead of using 
an interim BER format or MPI or whatever, the keys can be parsed right into the 
correct format of the hardware that will eventually run the offload. This will 
be done once and we can avoid all the extra work.

Further more the important part for me is that when keys are actually bound to 
hardware, meaning we will never see the actual private key in kernel memory, we 
can still reference them. The selection of a hardware provided private key with 
a RSA cipher then can automatically select the right implementation for usage 
of that key. No point in trying a software RSA cipher if the key is only in 
hardware.

If the hardware supports RSA offload and has a private key storage of its own, 
why should that be treated any different than AES hardware engine. It is just 
that its usage is limited to its keys. However the interface of akcipher should 
handle this. If it does not, then we boxed ourselves into a corner and end up 
inventing new interfaces for these kinds of hardware. TPM and UEFI are such 
devices / services already out there. For me they are no different than 
skcipher offload. It is just that they are bound to a key and not just an 
instruction or hardware block.

The keys subsystem with the asymmetric key type should be responsible for 
loading and managing the keys. The crypto subsystem with akcipher 

Re: Proposal for adding setpubkey callback to akcipher_alg

[Marcel Holtmann]

Hi Stephan,

 I think we need to split the akcipher_alg setkey callback into a setkey
 and
 setpubkey.
 
 diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
 index 69d163e39101..ca93952b6d19 100644
 --- a/include/crypto/akcipher.h
 +++ b/include/crypto/akcipher.h
 @@ -91,6 +91,8 @@ struct akcipher_alg {
 
  int (*decrypt)(struct akcipher_request *req);
  int (*setkey)(struct crypto_akcipher *tfm, const void *key,
 
unsigned int keylen);
 
 +   int (*setpubkey)(struct crypto_akcipher *tfm, const void *key,
 +unsigned int keylen);
 
  int (*init)(struct crypto_akcipher *tfm);
  void (*exit)(struct crypto_akcipher *tfm);
 
 If the cipher actually uses two different formats for the public +
 private
 
 The public key is n + e.
 
 The private key is n + d.
 
 for RSA Public Key it is just n and e. However for RSA Private Key it is n
 and e and d and also version, primes etc. So the RSA Public Key contains a
 sequence of 2 integers and the RSA Private Key contains a sequence of 9
 integers.
 Both are encoded in the BER structure the current API requires. It is
 perfectly valid to provide only n + e when you do public key operations.
 
 And from an API perspective that is fully wrong from my point of view. We
 just invented another format that is not in any standard. The two standard
 key formats for RSA are RSA Private Key and RSA Public Key. These are the
 ones we should support.
 
 The format with n plus e and optionally d is total Linux invention as far as
 I can tell. And I do not want this exposed to userspace.
 
 For a clean separation I think splitting this into setkey for the RSA
 Private Key and setpubkey for the RSA Public Key is pretty obvious choice.
 
 Please define exactly what your pubkey and your privkey contains. Even when I 
 think of the DER keys from OpenSSL, we once have n+e and once n+e+d (see 
 asn1dump), no?

RSA Private Key is n + e + d (including 6 other fields). RSA Public Key is n + 
e (no other fields).

So for RSA you would make setkey to take RSA Private Key and setpubkey to take 
RSA Public Key. Meaning you only have to use one of them since if you have the 
private key, you always have the public key.

This real difference here is that you can provide the key in two different key 
formats. As explained RSA uses two different format.

 Please see in the testmgr.h for the 2048 bit key test vector (i.e. the one
 with public_key_vec = true). The BER structure has nice comments from
 Tadeusz to indicate it only contains n and e without d.
 
 And it is totally made up format. Why would you force conversion of a RSA
 
 BER is a made up implementation? I do not think so: 
 https://en.wikipedia.org/wiki/Basic_Encoding_Rules
 
 Or do you say that the kernel's implementation of BER is broken?

BER is an encoding format. It does NOT define a key format. You can use BER to 
define a key format, but that still means that our defined format that is 
currently used for setkey with RSA is made up. It is Linux specific.

The standards for key formats for RSA are RSA Public Key and RSA Private Key.

 Public Key or RSA Private Key in DER format into this format. This Linux
 only input format makes it just complicated for no reason. It is also not
 documented anywhere as I can tell. I had to dig this out of the code and
 rsakey.asn1.
 
 As mentioned above, splitting this into two functions makes this simpler.
 For all intense and purposes this is akcipher so we always either have
 public/private key pair or we just have the public key. And at least with
 RSA they are defined as two independent formats.
 
 I can see that user space (e.g. libkcapi) has such two functions. But 
 currently I do not see such distinction necessary in the kernel as mentioned 
 above.

Then how do you tell the two key formats apart? Try one, fail, try the other? I 
do not like these things. Just tell the kernel clearly what format you have. 
Simple and easy.

 Since the parsing of the key data is not a generic handling, I do not see a
 good enough reason to invent new formats. Use the format the cipher you
 implement already has defined.
 Thus, I do not currently understand your request. May I ask you to give
 more explanation why the use of BER is insufficient?
 
 Tell me how you create this Linux specific BER encoded key. I would like
 someone to provide the magic OpenSSL conversion command line to get this.
 
 Again: there is no DER to BER converter that I am aware of. Agreed, that 
 should be there. But currently I do not see that the kernel should do that.

For all intense and purposes DER is valid BER. Why are discussing this?

 Hand crafting such keys when there is a standard format for RSA Private Key
 and RSA Public Key makes no sense whatsoever.
 
 Fully agreed. Thus, a BER encoder is on my agenda for libkcapi.

You are missing my point here. You should not need such a thing. The RSA keys 
are already provided in two well defined DER 

Re: Proposal for adding setpubkey callback to akcipher_alg

[Marcel Holtmann]

Hi Stephan,

 I think we need to split the akcipher_alg setkey callback into a setkey and
 setpubkey.
 
 diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
 index 69d163e39101..ca93952b6d19 100644
 --- a/include/crypto/akcipher.h
 +++ b/include/crypto/akcipher.h
 @@ -91,6 +91,8 @@ struct akcipher_alg {
   int (*decrypt)(struct akcipher_request *req);
   int (*setkey)(struct crypto_akcipher *tfm, const void *key,
 unsigned int keylen);
 +   int (*setpubkey)(struct crypto_akcipher *tfm, const void *key,
 +unsigned int keylen);
   int (*init)(struct crypto_akcipher *tfm);
   void (*exit)(struct crypto_akcipher *tfm);
 
 If the cipher actually uses two different formats for the public + private
 
 The public key is n + e.
 
 The private key is n + d.

for RSA Public Key it is just n and e. However for RSA Private Key it is n and 
e and d and also version, primes etc. So the RSA Public Key contains a sequence 
of 2 integers and the RSA Private Key contains a sequence of 9 integers.

 Both are encoded in the BER structure the current API requires. It is 
 perfectly valid to provide only n + e when you do public key operations.

And from an API perspective that is fully wrong from my point of view. We just 
invented another format that is not in any standard. The two standard key 
formats for RSA are RSA Private Key and RSA Public Key. These are the ones we 
should support.

The format with n plus e and optionally d is total Linux invention as far as I 
can tell. And I do not want this exposed to userspace.

For a clean separation I think splitting this into setkey for the RSA Private 
Key and setpubkey for the RSA Public Key is pretty obvious choice.

 Please see in the testmgr.h for the 2048 bit key test vector (i.e. the one 
 with public_key_vec = true). The BER structure has nice comments from Tadeusz 
 to indicate it only contains n and e without d.

And it is totally made up format. Why would you force conversion of a RSA 
Public Key or RSA Private Key in DER format into this format. This Linux only 
input format makes it just complicated for no reason. It is also not documented 
anywhere as I can tell. I had to dig this out of the code and rsakey.asn1.

As mentioned above, splitting this into two functions makes this simpler. For 
all intense and purposes this is akcipher so we always either have 
public/private key pair or we just have the public key. And at least with RSA 
they are defined as two independent formats.

Since the parsing of the key data is not a generic handling, I do not see a 
good enough reason to invent new formats. Use the format the cipher you 
implement already has defined.

 Thus, I do not currently understand your request. May I ask you to give more 
 explanation why the use of BER is insufficient?

Tell me how you create this Linux specific BER encoded key. I would like 
someone to provide the magic OpenSSL conversion command line to get this. Hand 
crafting such keys when there is a standard format for RSA Private Key and RSA 
Public Key makes no sense whatsoever.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Stephan,

 It does not. The RSA Private Key has a different format.
 
  RSAPrivateKey ::= SEQUENCE {
  version   Version,
  modulus   INTEGER,  -- n
  publicExponentINTEGER,  -- e
  privateExponent   INTEGER,  -- d
  prime1INTEGER,  -- p
  prime2INTEGER,  -- q
  exponent1 INTEGER,  -- d mod (p-1)
  exponent2 INTEGER,  -- d mod (q-1)
  coefficient   INTEGER,  -- (inverse of q) mod p
  }
 
 And honestly that the RSA Public Key magically matches seems more luck then
 clear intention.
 
  RSAPublicKey ::= SEQUENCE {
  modulus   INTEGER,  -- n
  publicExponentINTEGER   -- e
  }
 
 I think here we may have the issue: the ASN.1 structure the kernel uses 
 should 
 be changed to implement that commonly used ASN.1 structure. If this change 
 would allow a DER to be used, I think we have the solution.

as you can clearly see. There are two formats defined here. There is no single 
ASN.1 structure that can decode both of these.

It is what it is, RSA Public Key and RSA Private Key formats are two different 
key formats. And OpenSSL also treats it like this. You can extract the public 
key from a private key (same way you can extract it from a certificate), but 
you can not create a private key structure that only contains the public key.

For RSA we need to support the two formats as listed above. To make this really 
easy from an API point of view, I would have setkey and setpubkey function. And 
also expose them as ALG_SET_KEY and ALG_SET_PUBKEY socket options for AF_ALG.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Stephan,

 I have been working with the AF_ALG patches for akcipher lately and I find
 the RSA set key function way too limited. Especially the fact that it uses a
 format that I can not find a single reference / standard for worries me.
 
 RsaKey ::= SEQUENCE {
   n INTEGER ({ rsa_get_n }),
   e INTEGER ({ rsa_get_e }),
   d INTEGER ({ rsa_get_d })
 }
 
 Note, the kernel uses BER encoding.

so what? DER is still valid BER.

 So where is this format coming from? I can find the RSA Public Key format
 which is a sequence of n and e. If you have a DER encoded RSA public key,
 then you can use it to encrypt and verify. So that is okay.
 
 However if you have a standard Public Key that OpenSSL would create by
 default, then things do not work since that is actually a more complicated
 DER encoded format. However in the end, I would expect that we could also
 load such a key here. The RSA set key function should auto detect it and
 extract the right information if it is marked as rsaEncryption type.
 
 Do you propose to add a DER parser to the kernel? I feel that the BER parser 
 is complex enough. If somebody has a DER key, user space should have the code 
 to convert it to BER.

Why would you do that? DER is still valid BER.

 My biggest concern however is that this does not reassemble the RSA Private
 Key at all. That key format is a sequence of 9 integers starting with a
 version. So logically I would expect that I can just set a RSA Private Key
 and then utilize the encrypt and decrypt features of the RSA cipher.
 
 Why do you think this is not possible? testmgr.h seems to exactly do that.

It does not. The RSA Private Key has a different format.

  RSAPrivateKey ::= SEQUENCE {
  version   Version,
  modulus   INTEGER,  -- n
  publicExponentINTEGER,  -- e
  privateExponent   INTEGER,  -- d
  prime1INTEGER,  -- p
  prime2INTEGER,  -- q
  exponent1 INTEGER,  -- d mod (p-1)
  exponent2 INTEGER,  -- d mod (q-1)
  coefficient   INTEGER,  -- (inverse of q) mod p
  }

And honestly that the RSA Public Key magically matches seems more luck then 
clear intention.

  RSAPublicKey ::= SEQUENCE {
  modulus   INTEGER,  -- n
  publicExponentINTEGER   -- e
  }

So what you have in testmgr.h is neither RSA Private Key nor RSA Public Key. It 
is a brand new format that is sadly Linux specific.

 When it comes to exposing RSA via AF_ALG and akcipher, I really want standard
 format for the set key operation. Asking userspace to construct this Linux
 kernel only key format is not helpful. You want to be able to just load the
 RSA Private Key in DER format and be done with it.
 
 I am all for it. BER is a standard, is it not? Yes, I have not yet seen a 
 converter for DER to BER (and I have not searched for one either). But I feel 
 DER should be left to user space.

DER is valid BER. However just saying the key is BER has no meaning whatsoever. 
It does not define an actual key format.

What I would like to see is support for RSA Private Key (DER encoded) and RSA 
Public Key (DER encoded) when using the RSA cipher. That makes sense to me. 
Everything else is just insanity since we force the user to convert well known 
formats into new ones for no apparent reason.

 Any ideas on how we can fix this to allow a sensible userspace API?
 
 I actually planned to add a BER encoder to my libkcapi. I feel that should be 
 right place to provide that code, including a potential DER to BER converter.

No idea what DER to BER would give you since DER is always valid BER. I see no 
need for BER or DER encoders if the keys and also certificates are already 
available in DER format your system (or can be easily converted from PEM to DER 
with standard tools).

Seriously, I want to take the RSA Private Key that comes out of OpenSSL and use 
it. I want to be able to extract the RSA Public Key out of a certificate and 
use it. Plain and simple. No other gimmicks, conversions or tricks to play. 
Using existing key format standards is really the key here.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Tadeusz,

 I already have patches for that actually. The question is just which 
 approach to take?
 
 My current proposal is to separate the current crypto_akcipher_setkey into 
 two functions. Use the crypto_akcipher_setkey for loading combined private 
 and public key formats and crypto_akcipher_setpubkey for just loading the 
 public key only format.
 
 I would prefer to have one setkey function if possible.
 
 
 One other option is actually for crypto_akcipher_setkey to use struct 
 public_key from include/crypto/public_key.h. I mean we have this all 
 defined. Why do we operate on binary blobs for the keys in the first place. 
 For example if the key comes from a certificate in the keyring, lets just 
 use that data. Most likely the asymmetric key type already decoded it nicely 
 for us. No need to do this again.
 
 That was the first proposal to use this struct, but it was rejected. Looks 
 like MPIs are not acceptable on the API.
 I think it make sense now as it will only be useful for the SW 
 implementation. For hardware the easiest way is to take
 it in this form.

actually I think this reasoning needs to be revisited. When I look at this, 
this makes no sense whatsoever. The end result is that we have keys in multiple 
formats in the kernel and have to convert between them or parse them again.

If you do not want to use struct public_key, then lets go for struct key as I 
proposed in my other response. That should in the end be able to represent 
hardware keys as well. There is really no good reason to move things around and 
parse them again or create new formats out of it.

My take is that we want to store a key once in a single location and single 
location only. Storing the same key in different formats in different locations 
is just a plain bad idea. Keep it secure and locked in one place.

 I mean there is more than RSA out there and this would allow us to express 
 struct public_key_algorithm in addition to the allowed applications of said 
 key as well. This would properly also allow us to combine the 
 crypto/asymmetric_keys/rsa.c and crypto/rsa.c so that we only have a single 
 RSA implementation. Not to mention that we also have lib/digsig.c in the 
 kernel.
 
 The crypto/asymmetric_keys/ will be converted to the new api and 
 crypto/asymmetric_keys/rsa.c removed soon.

I hope someone is looking at lib/digsig.c as well and gets this all cleaned up 
into a single implementation. However for this to happen, we really need to get 
this key stuff figured out. Right now this looks to me like the left hand does 
not know what the right hand is doing.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Tadeusz,

 as you can clearly see. There are two formats defined here. There is no 
 single ASN.1 structure that can decode both of these.
 
 It is what it is, RSA Public Key and RSA Private Key formats are two 
 different key formats. And OpenSSL also treats it like this. You can extract 
 the public key from a private key (same way you can extract it from a 
 certificate), but you can not create a private key structure that only 
 contains the public key.
 
 For RSA we need to support the two formats as listed above. To make this 
 really easy from an API point of view, I would have setkey and setpubkey 
 function. And also expose them as ALG_SET_KEY and ALG_SET_PUBKEY socket 
 options for AF_ALG.
 
 I'll have a look what will be the easiest way to get the openSSL generated  
 unmodified private key working.

I already have patches for that actually. The question is just which approach 
to take?

My current proposal is to separate the current crypto_akcipher_setkey into two 
functions. Use the crypto_akcipher_setkey for loading combined private and 
public key formats and crypto_akcipher_setpubkey for just loading the public 
key only format.

One other option is actually for crypto_akcipher_setkey to use struct 
public_key from include/crypto/public_key.h. I mean we have this all defined. 
Why do we operate on binary blobs for the keys in the first place. For example 
if the key comes from a certificate in the keyring, lets just use that data. 
Most likely the asymmetric key type already decoded it nicely for us. No need 
to do this again.

I mean there is more than RSA out there and this would allow us to express 
struct public_key_algorithm in addition to the allowed applications of said key 
as well. This would properly also allow us to combine the 
crypto/asymmetric_keys/rsa.c and crypto/rsa.c so that we only have a single RSA 
implementation. Not to mention that we also have lib/digsig.c in the kernel.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Tadeusz,

 as you can clearly see. There are two formats defined here. There is no 
 single ASN.1 structure that can decode both of these.
 
 It is what it is, RSA Public Key and RSA Private Key formats are two 
 different key formats. And OpenSSL also treats it like this. You can 
 extract the public key from a private key (same way you can extract it from 
 a certificate), but you can not create a private key structure that only 
 contains the public key.
 
 For RSA we need to support the two formats as listed above. To make this 
 really easy from an API point of view, I would have setkey and setpubkey 
 function. And also expose them as ALG_SET_KEY and ALG_SET_PUBKEY socket 
 options for AF_ALG.
 
 I'll have a look what will be the easiest way to get the openSSL generated  
 unmodified private key working.
 
 I already have patches for that actually. The question is just which approach 
 to take?
 
 My current proposal is to separate the current crypto_akcipher_setkey into 
 two functions. Use the crypto_akcipher_setkey for loading combined private 
 and public key formats and crypto_akcipher_setpubkey for just loading the 
 public key only format.
 
 One other option is actually for crypto_akcipher_setkey to use struct 
 public_key from include/crypto/public_key.h. I mean we have this all defined. 
 Why do we operate on binary blobs for the keys in the first place. For 
 example if the key comes from a certificate in the keyring, lets just use 
 that data. Most likely the asymmetric key type already decoded it nicely for 
 us. No need to do this again.

actually taking this one step further, why don't we integrate directly with the 
keys.

int crypto_akcipher_setkey(struct crypto_akcipher *tfm, struct key *key)

Or maybe key_ref_t or key_serial_t depending on what is the preferred method of 
referencing keys. So for every key loaded via the asymmetric key type, we can a 
have nice clean way to get the key from various different parts of the kernel 
and it is also reference counted.

It is also by definition cipher agnostic. So this would work for DSA, ECDH or 
whatever comes next. The only check needed is that the provided key is of type 
asymmetric and of subtype public key.

This means there is no need to parse it twice or store an extra copy of it or 
invent some new reference counting around struct public_key.

And then it would be dead simple to introduce ALG_SET_KEY_ID that takes a 
key_serial_t for usage with AF_ALG. I have implemented ALG_SET_KEY_ID for 
skcipher and it works great. However for akcipher this would show even more 
benefits. Especially when certificates and public keys come from the system 
keyring or via TPM / UEFI.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Tadeusz,

I have been working with the AF_ALG patches for akcipher lately and I find the 
RSA set key function way too limited. Especially the fact that it uses a format 
that I can not find a single reference / standard for worries me.

RsaKey ::= SEQUENCE {
n INTEGER ({ rsa_get_n }),
e INTEGER ({ rsa_get_e }),
d INTEGER ({ rsa_get_d })
}

So where is this format coming from? I can find the RSA Public Key format which 
is a sequence of n and e. If you have a DER encoded RSA public key, then you 
can use it to encrypt and verify. So that is okay.

However if you have a standard Public Key that OpenSSL would create by default, 
then things do not work since that is actually a more complicated DER encoded 
format. However in the end, I would expect that we could also load such a key 
here. The RSA set key function should auto detect it and extract the right 
information if it is marked as rsaEncryption type.

My biggest concern however is that this does not reassemble the RSA Private Key 
at all. That key format is a sequence of 9 integers starting with a version. So 
logically I would expect that I can just set a RSA Private Key and then utilize 
the encrypt and decrypt features of the RSA cipher.

When it comes to exposing RSA via AF_ALG and akcipher, I really want standard 
format for the set key operation. Asking userspace to construct this Linux 
kernel only key format is not helpful. You want to be able to just load the RSA 
Private Key in DER format and be done with it.

Any ideas on how we can fix this to allow a sensible userspace API?

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Proposal for adding setpubkey callback to akcipher_alg

[Marcel Holtmann]

Hi Tadeusz,

I think we need to split the akcipher_alg setkey callback into a setkey and 
setpubkey.

diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
index 69d163e39101..ca93952b6d19 100644
--- a/include/crypto/akcipher.h
+++ b/include/crypto/akcipher.h
@@ -91,6 +91,8 @@ struct akcipher_alg {
int (*decrypt)(struct akcipher_request *req);
int (*setkey)(struct crypto_akcipher *tfm, const void *key,
  unsigned int keylen);
+   int (*setpubkey)(struct crypto_akcipher *tfm, const void *key,
+unsigned int keylen);
int (*init)(struct crypto_akcipher *tfm);
void (*exit)(struct crypto_akcipher *tfm);
 
If the cipher actually uses two different formats for the public + private key 
data compared to just the public key data, then it is useful to have these 
independent. That way we can use standard formats for the keys and do not have 
to have a Linux kernel specific key format.

My definition would be that setkey sets the private and public key. And the 
setpubkey only sets the public key. So depending on which format of keys you 
have, you call the proper function and it will do the rest for you. At least 
for RSA this solves the problem that I described in my previous email and we 
could use RSA standard ASN.1 formats for each of the key files.

For obvious reasons, when you only call setpubkey, then only encrypt and verify 
will work. However if you call setkey, then you can sign, verify, encrypt and 
decrypt.

When exposing akcipher via AF_ALG, I would also propose to add a ALG_SET_PUBKEY 
so that userspace can clearly tell the kernel which part of the keys it has. 
This would map nicely and we then know which ASN.1 decoder to call instead of 
having to guess what format userspace provided. In case of RSA, the user 
already selected RSA as cipher. So it either has RSA Public Key and would use 
ALG_SET_PUBKEY or it has RSA Private Key and would use ALG_SET_KEY. Since the 
key formats do not describe themselves, I think this is the cleaner solution 
from an API point of view.

On a side note, that the ASN.1 decoder accepts a key with two integers even 
while the format describes three integers seems like a bug in the decoder and 
not a feature. If the third integer is not marked as optional, the decoder 
should just fail the parsing.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Using separate initcall level for crypto subsystem

[Marcel Holtmann]

Hi Herbert,

 we can easily run them later on. However when the Bluetooth subsystem is 
 built as module, then I would prefer to have the module loading fail in case 
 one of the selftest fails. I can hack around this with a lot of ifdef config 
 magic. If we would have all crypto, cipher etc. modules as crypto_initcall, 
 then I would have to add nothing extra on my side. It would reduce the ifdef 
 config magic on our side a lot.
 
 My personal take is that the crypto subsystem has become such a basic 
 feature that it might make sense to ensure that all pieces (including 
 ciphers) are loaded before we initialize any other subsystem.
 
 I don't think moving the crypto initcalls up is the answer because
 moving the subsystem itself isn't enough if you actually want to
 use crypto algorithms.

that is indeed true. All the crypto algorithm need to be moved as well. I 
considered that already since I had debugged the initcall order with a kernel 
without modules.

 You'd need to move the algorithms too which would be a nightmare.

Actually I was thinking to convert the algorithms to a newly introduced 
module_crypto_alg() and module_crypto_shash() helpers.

This would be similar to module_usb_driver() and module_pci_driver(). And then 
changing the initcall level would be trivial. The module_init/module_exit is 
just all the same boilerplate anyway and we could get rid of it this way.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Using separate initcall level for crypto subsystem

[Marcel Holtmann]

Hi,

we wanted to add some crypto selftests to the Bluetooth subsystem that checks 
our usage of the crypto handling we use for Bluetooth Low Energy Legacy Pairing 
and Secure Connections.

Since the Crypto subsystem and Bluetooth subsystem both use subsys_initcall 
that goes horrible wrong when running it built-in. So I wonder if it would make 
sense to introduce a crypto_initcall that comes before the subsys_initcall.

Any thoughts on this?

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/6] LLVMLinux: Patches to enable the kernel to be compiled with clang/LLVM

[Marcel Holtmann]

Hi Behan,

 These patches remove the use of Variable Length Arrays In Structs (VLAIS) in
 crypto related code. Presented here for comments as a whole (since they all do
 the same thing in the same way). Once everyone is happy I will submit them
 individually to their appropriate maintainers.
 
 The LLVMLinux project aims to fully build the Linux kernel using both gcc and
 clang (the C front end for the LLVM compiler infrastructure project). 
 
 
 Jan-Simon Möller (4):
  crypto, dm: LLVMLinux: Remove VLAIS usage from dm-crypt
  crypto: LLVMLinux: Remove VLAIS usage from crypto/hmac.c
  crypto: LLVMLinux: Remove VLAIS usage from libcrc32c.c
  crypto: LLVMLinux: Remove VLAIS usage from crypto/testmgr.c
 
 Vinícius Tinti (2):
  apparmor: LLVMLinux: Remove VLAIS
  btrfs: LLVMLinux: Remove VLAIS
 
 crypto/hmac.c  | 27 +--
 crypto/testmgr.c   | 16 
 drivers/md/dm-crypt.c  | 38 ++
 fs/btrfs/hash.c| 18 +-
 lib/libcrc32c.c| 18 +-
 security/apparmor/crypto.c | 19 +--
 6 files changed, 66 insertions(+), 70 deletions(-)

are you sure these are all of them? I know for a fact that we are using the 
same construct in net/bluetooth/amp.c as well.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Support for ECDH P-192 and P-256

[Marcel Holtmann]

Hello,

we are looking at adding support for Bluetooth Secure Connections to the 
Security Manager of the Bluetooth subsystem. For that we would need support for 
ECDH P-256 and eventually also P-192.

Right now we are bit lost on how this could be achieved best. I saw that the 
symmetric_keys feature has support for public_keys, but as far as I can tell 
that requires that userspace loads the public keys into the kernel and the 
private keys stay in userspace.

What we need is to generate private/public key pairs using elliptic curve with 
P-192 and P-256. We only need the private/public key pair for the Bluetooth 
pairing. After successful pairing, we derive link keys or long term keys and we 
can throw the private/public key pair away. Any further authentication between 
Bluetooth devices is done via their link keys or long term keys.

Has anybody looked into extending the kernel crypto framework to support ECDH 
P-192 and P-256. If nobody has, what are the best starting points to do so.

Regards

Marcel

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html