Re: [PATCH 00/20] crypto: introduce crypto_shash_tfm_digest()
Hi Eric,
> This series introduces a helper function crypto_shash_tfm_digest() which
> replaces the following common pattern:
>
> {
> SHASH_DESC_ON_STACK(desc, tfm);
> int err;
>
> desc->tfm = tfm;
>
> err = crypto_shash_digest(desc, data, len, out);
>
> shash_desc_zero(desc);
> }
>
> with:
>
> err = crypto_shash_tfm_digest(tfm, data, len, out);
>
> Patch 1 introduces this helper function, and patches 2-20 convert all
> relevant users to use it.
>
> IMO, it would be easiest to take all these patches through the crypto
> tree. But taking just the "crypto:" ones and then me trying to get the
> rest merged later via subsystem trees is also an option.
I am fine if you take the net/bluetooth/smp.c change through the crypto tree.
Acked-by: Marcel Holtmann
Regards
Marcel
Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
Hi Eric, >> One of the issues that I would like to see addressed in the crypto API >> is they way the cipher abstraction is used. In general, a cipher should >> never be used directly, and so it would be much better to clean up the >> existing uses of ciphers outside of the crypto subsystem itself, so that >> we can make the cipher abstraction part of the internal API, only to >> be used by templates or crypto drivers that require them as a callback. >> >> As a first step, this series moves all users of the 'arc4' cipher to >> the ecb(arc4) skcipher, which happens to be implemented by the same >> driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE >> actually evaluates to 1. >> >> Next step would be to switch the users of the 'des' and 'aes' ciphers >> to other interfaces that are more appropriate, either ecb(...) or a >> library interface, which may be more appropriate in some cases. In any >> case, the end result should be that ciphers are no longer used outside >> of crypto/ and drivers/crypto/ >> >> This series is presented as an RFC, since I am mostly interested in >> discussing the above, but I prefer to do so in the context of actual >> patches rather than an abstract discussion. >> >> Ard Biesheuvel (3): >> net/mac80211: switch to skcipher interface for arc4 >> lib80211/tkip: switch to skcipher interface for arc4 >> lib80211/wep: switch to skcipher interface for arc4 >> > > The way the crypto API exposes ARC4 is definitely broken. It treats it as a > block cipher (with a block size of 1 byte...), when it's actually a stream > cipher. Also, it violates the API by modifying the key during each > encryption. > > Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be > using, and the users call it on virtual addresses, perhaps we should instead > remove it from the crypto API and provide a library function arc4_crypt()? > We'd > lose support for ARC4 in three hardware drivers, but are there real users who > really are using ARC4 and need those to get acceptable performance? Note that > they aren't being used in the cases where the 'cipher' API is currently being > used, so it would only be the current 'skcipher' users that might matter. > > Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but > it > seems unlikely… that is not unlikely, we use ecb(arc4) via AF_ALG in iwd. It is what the WiFi standard defines to be used. Regards Marcel
Re: [PATCH] crypto: af_alg - implement keyring support
Hi Ondrej, > This patch adds new socket options to AF_ALG that allow setting key from > kernel keyring. For simplicity, each keyring key type (logon, user, > trusted, encrypted) has its own socket option name and the value is just > the key description string that identifies the key to be used. The key > description doesn't need to be NULL-terminated, but bytes after the > first zero byte are ignored. why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful. Regards Marcel
Re: [v2 PATCH 5/5] Bluetooth: let the crypto subsystem generate the ecc privkey
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(&result.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(&p);
>>> - buf = kmalloc(buf_len, GFP_KERNEL);
>>> - if (!buf) {
>>> - err = -ENOMEM;
>>> - goto free_req;
>>> - }
>>> -
>>> - crypto_ecdh_encode_key(buf, buf_len, &p);
>>> -
>>> - /* 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 *)&public_key[32], (u64 *)&tmp[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(&p);
>>> + buf = kmalloc(buf_len, GFP_KERNEL);
>>> + if (!buf) {
>>> + err = -ENOMEM;
>>> + goto free_tmp;
>>> + }
>>> +
>>> + err = crypto_ecdh_encode_key(buf, buf_len, &p);
>>> + if (err)
>>> + goto free_all;
>>> +
>>> + err = crypto_kpp_set_secret(tfm, buf, buf_len);
>>> + /* fall thr
Re: [v2 PATCH 0/5] Bluetooth: let the crypto subsystem generate the ecc privkey
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
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(&result.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(&p);
> - buf = kmalloc(buf_len, GFP_KERNEL);
> - if (!buf) {
> - err = -ENOMEM;
> - goto free_req;
> - }
> -
> - crypto_ecdh_encode_key(buf, buf_len, &p);
> -
> - /* 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 *)&public_key[32], (u64 *)&tmp[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(&p);
> + buf = kmalloc(buf_len, GFP_KERNEL);
> + if (!buf) {
> + err = -ENOMEM;
> + goto free_tmp;
> + }
> +
> + err = crypto_ecdh_encode_key(buf, buf_len, &p);
> + 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 t
Re: [RESEND RFC PATCH 2/2] Bluetooth: let the crypto subsystem generate the ecc privkey
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(&result.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(&p);
> - buf = kmalloc(buf_len, GFP_KERNEL);
> - if (!buf)
> - goto free_req;
>
> - crypto_ecdh_encode_key(buf, buf_len, &p);
> + 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(&p);
> + buf = kmalloc(buf_len, GFP_KERNEL);
> + if (!buf)
> + goto free_req;
> +
> + crypto_ecdh_encode_key(buf, buf_len, &p);
> +
> + /* 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 *)&public_key[32], (u64 *)&tmp[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(&result.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(&p);
> 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, &p);
> - err = crypto_kpp_set_secret(tfm, buf, buf_len);
> - if (err)
> - goto free_all;
> -
> - sg_init_one(&dst, tmp, 64);
> - kpp_request_set_input(req, NULL, 0);
> - kpp_request_
Re: [RESEND RFC PATCH 1/2] Bluetooth: move ecdh allocation outside of ecdh_helper
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(&result.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(&result.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
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
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
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
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
Hi Stephan, >>> 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? > > Strangely, I see such support all the time in embedded devices where > asymmetric acceleration is really necessary. and where are the support patches to integrate them with KPP in the kernel. Maybe we should first have these before exposing anything to userspace. I said this in another thread before, we have Bluetooth Security Manager which uses ECDH. It has been re-written to use KPP and once we have that fully supporting hardware based ECC key generation and ECDH operation, I would be a lot more confident that we understand how asymmetric crypto accelerators actually do their job. Especially in the embedded world. >> 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. > > I understand that this is the core purpose of asymmetric ciphers. Yet, > asymmetric ciphers are complex and we as kernel developers do not know (or > shall not mandate?) where the complexity shall be implemented. By forcing all > into the keyctl, we would insist that the entire complexity of the full-blown > asym cipher is in the kernel without an option that user space may implement > it. > > What we are currently seemingly discuss is the choice of > > - keyctl: having all complexity of the entire asym logic including its key > handling in the kernel with the benefit of the kernel protection of the > private key Which we already have anyway since the kernel supports Secure Boot. > - algif_akcipher (maybe with a link to keyctl): only exporting the cipher > support and allow user space to decide where the complexity lies > > Just to give you an example: A full blown RSA operation (excluding the > hashing > part for signatures) consists of padding and the asymmetric operation. For > the > asymmetric operation, we have sign/verify and encrypt/decrypt (keywrap). > There > are a gazillion padding types out there: > > - PKCS1 > > - OAEP > > - SP800-56B: RSAEP, RSADP, RSASVE, RSA-OAEP, RSA-KEM-KWS > > And there may be others. > > When we talk about encryption/decryption we have to consider the KDFs > (SP800-108, RFC5689, SP800-56A). > > When we consider the KDFs, we have to think of the KDF data styles (ASN.1, > concatenation) > > With keyctl to me it seems that we need to integrate all that logic into the > kernel. As all of that is just processing logic, securing it in the kernel > may > not be the right way as this code does not need the elevated privileges in > the > kernel for that. The keyring subsystem has extensive user permissions available. Actually that is even more of a reason to not expose anything with private keys via AF_ALG. If I can use AF_ALG to access (meaning use a private key) that I am not suppose to use, then we have a real security nightmare on our hands. The keyring subsystem has permission that can be restricted on a per-thread level. On a side note, have a look at ELL and how it uses Mat’s crypto tree. For fun look at iwd and how it does WPA-Enterprise with mostly only kernel primitives. > Yet, some hardware may provide some/all of this logic. And we want to make > that available to user space. I don’t get the raw access to RSA for example. Nobody uses it in raw mode. There is always some sort of padding involved and needed. Otherwise RSA would be pretty insecure. It is also funny how RSA centric this is. There is more than RSA since especially ECDH and its friends are getting more and more traction in the IoT spaces. Secure key exchange without the usage of certificates is actually a valid use case as well. >> 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 > > See above, it is my opinion that with asym ciphers, there is a lot of > complexity and lots of options. I do not think that the kernel API should be > a > limiting factor here, because the kernel simply does not implement a specific > cipher type. What has cipher type to do with this? This confuses me. If the kernel supports any KPP backed a
Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API
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
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
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
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
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
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
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
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
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
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
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
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 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
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)
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
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
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
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
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
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 then be easily referenced by skcip
Re: [PATCH] crypto: implement DH primitives under akcipher API
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
Hi Herbert, > This patch replaces uses of blkcipher with skcipher and the long > obsolete hash interface with shash. > > Signed-off-by: Herbert Xu Acked-by: Marcel Holtmann > --- > > 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
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: [RFC PATCH] crypto: RSA padding transform
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 3/5] crypto: AF_ALG -- add setpubkey setsockopt call
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: [PATCH v2 0/5] crypto: add algif_akcipher user space API
Hi Stephan, >> 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 > > I personally do not fully agree here. For our day-to-day desktops and servers > I would fully and completely agree. But I see other use cases of Linux in > routers or other embedded systems where there may be other checks and > balances > in place where this hard demand is not warranted. actually in embedded devices (especially the tiny IoT stuff I am looking after) I really want keys in a central place and do not have to copy them around all the time. Keeping keys secure is actually a hard job. Doing that multiple places in the kernel and in userspace seems not a good idea. Especially not for an embedded device. The keys subsystem is great since it includes access control to your keys. If you have the right permissions you can get the keys back out if you want to. However if the key is locked down, you will not be able to read it again. I mean the keys subsystem can operate on per user, per process and per thread keys and access rights. So the policy that you are after is already done nicely via the keys subsystem. I really don't think we are limiting anything here. We are actually giving users more choice for policy. > Thus, I feel that this is a policy decision to be made in user space (see my > other email -- please answer on that topic there to keep a single thread). > >> 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. > > Do not get me wrong, my patch is shall be there for all to comment. I have no > issues when we find a better solution. And I also do not like multiple > interfaces that would not be needed if we would have thought better. >> >> 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. > > With all due respect, I would object here. When we say yes to TLS (even if it > is parts of TLS up to the point where the KDF happens), we invite all higher > level crypto implementations: IKE, SNMP, SSH -- I would not want to go down > that path that started by simply supporting accelerated asymmetric ciphers. Reality is that TLS in the kernel is happening. Reality is also that we do signing and certificate handling in the kernel. How much of the cipher negotiations and key derivation happens in kernel vs userspace is something to be still discussed. And there will be userspace involved for sure. However that does not mean that we keep moving keys through userspace to just load it back into another subsystem. That kind of API really needs to stop. One other note I make is that we are looking at such tiny systems that including OpenSSL or GnuTLS is not an option at all. However we do use secure boot and need to be able to handle certificates and TLS. > Look at user space crypto libs: where is the most fuzz happening? Not in the > cipher implementations, but in the network protocols. Ciphers itself are easy of course. What we are saying here is that we do not have the cipher as the main important input on what to do. We have the key itself. The key decides what options for ciphers or fallback ciphers we have. So let me repeat this, we need to think really hard about supporting hardware keys. We need to see the bigger picture. Just looking at ciphers is not enough. 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: Add support for ALG_SET_KEY_ID for skcipher
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
[PATCH] crypto: Add support for ALG_SET_KEY_ID for skcipher
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
---
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 == &key_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= &algif_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 v2 0/5] crypto: add algif_akcipher user space API
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
Re: [PATCH v2 0/5] crypto: add algif_akcipher user space API
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
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
Re: [PATCH] crypto: Fix ASN.1 key handling for RSA akcipher
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
>> ---
>> 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
>> #include
>> #include
>> -#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
[PATCH] crypto: Fix ASN.1 key handling for RSA akcipher
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
---
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
#include
#include
-#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
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
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 s
Re: Limited usefulness of RSA set key function
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
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
Re: Limited usefulness of RSA set key function
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
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: Proposal for adding setpubkey callback to akcipher_alg
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 s
Re: Limited usefulness of RSA set key function
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: Proposal for adding setpubkey callback to akcipher_alg
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
Proposal for adding setpubkey callback to akcipher_alg
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
Limited usefulness of RSA set key function
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
Re: Using separate initcall level for crypto subsystem
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
Re: Using separate initcall level for crypto subsystem
Hi Herbert, >> 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? > > If it's just for self-tests would it be possible for them to be > done after system boot-up? 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. 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
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
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
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
