Re: [RFC PATCH v2 1/4] crypto: ecc - add privkey generation support

[Tudor Ambarus]

Hi, Stephan,

On 29.05.2017 12:56, Stephan Müller wrote:

Am Montag, 29. Mai 2017, 11:47:48 CEST schrieb Tudor Ambarus:

Hi Tudor,


Hm, there should be no blocking for the DRBG to initialize.

What happens if you compile that as a module and insmod it at runtime?


We will have a nop:

#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS

/* a perfect nop */
int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
{
printk(KERN_ERR "no op in alg_test");
return 0;
}

#else
...
#endif

If I further mangle it and change #ifdef with #ifndef then the tests are
passing.


Can you please enable the testmgr in the kernel config and compile the DH/ECDH
code as a module. Then I would insmod the dh/ecdh kernel module to see whether
the self tests work or not.


dh/ecdh self tests are passing when I insert dh/ecdh modules.



Note, if you use the calls of crypto_get_default_rng and friends, the DRBG
must be present in the kernel at the time of calling. I.e. if you statically
compile dh/ecdh but compile the RNG support as a module, the RNG will not load
during self test. If you compile dh/ecdh as a module, the RNG can stay as a
module.


However, when the self tests are enabled and DH/ECDH code is built-in, 
the kernel blocks.


I have to ensure that the drivers are loaded in the right order. 
Deferring the probe or just simply changing the order of the object

files from crypto/Makefile solves the issue. I'll go with the second
approach.

Thanks,
ta

Re: [RFC PATCH v2 1/4] crypto: ecc - add privkey generation support

[Stephan Müller]

Am Montag, 29. Mai 2017, 11:47:48 CEST schrieb Tudor Ambarus:

Hi Tudor,

> > Hm, there should be no blocking for the DRBG to initialize.
> > 
> > What happens if you compile that as a module and insmod it at runtime?
> 
> We will have a nop:
> 
> #ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
> 
> /* a perfect nop */
> int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
> {
>   printk(KERN_ERR "no op in alg_test");
>   return 0;
> }
> 
> #else
> ...
> #endif
> 
> If I further mangle it and change #ifdef with #ifndef then the tests are
> passing.

Can you please enable the testmgr in the kernel config and compile the DH/ECDH 
code as a module. Then I would insmod the dh/ecdh kernel module to see whether 
the self tests work or not.

Note, if you use the calls of crypto_get_default_rng and friends, the DRBG 
must be present in the kernel at the time of calling. I.e. if you statically 
compile dh/ecdh but compile the RNG support as a module, the RNG will not load 
during self test. If you compile dh/ecdh as a module, the RNG can stay as a 
module.

Ciao
Stephan

Re: [RFC PATCH v2 1/4] crypto: ecc - add privkey generation support

[Tudor Ambarus]

Hi, Stephan,

On 29.05.2017 12:23, Stephan Müller wrote:

Am Montag, 29. Mai 2017, 11:08:38 CEST schrieb Tudor Ambarus:

Hi Tudor,



+   unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
+
+   get_random_bytes(priv, nbytes);


Can you please use crypto_get_default_rng / crypto_rng_get_bytes /
crypto_put_default_rng?


Actually I tried this and I encountered some problems, I'm currently
debugging it.

When using the default rng and the run-time self tests are enabled,
the kernel is in a blocking state. What's worse is that the kernel
blocks before the console has the chance to be enabled and I can't see
anything :).

I suspect that the kernel blocks because the rng does not have enough
entropy. Could you please give me some hints?


Hm, there should be no blocking for the DRBG to initialize.

What happens if you compile that as a module and insmod it at runtime?


We will have a nop:

#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS

/* a perfect nop */
int alg_test(const char *driver, const char *alg, u32 type, u32 mask)
{
printk(KERN_ERR "no op in alg_test");
return 0;
}

#else
...
#endif

If I further mangle it and change #ifdef with #ifndef then the tests are
passing.

Thanks,
ta

Re: [RFC PATCH v2 1/4] crypto: ecc - add privkey generation support

[Stephan Müller]

Am Montag, 29. Mai 2017, 11:08:38 CEST schrieb Tudor Ambarus:

Hi Tudor,
> 
> >> +  unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
> >> +
> >> +  get_random_bytes(priv, nbytes);
> > 
> > Can you please use crypto_get_default_rng / crypto_rng_get_bytes /
> > crypto_put_default_rng?
> 
> Actually I tried this and I encountered some problems, I'm currently
> debugging it.
> 
> When using the default rng and the run-time self tests are enabled,
> the kernel is in a blocking state. What's worse is that the kernel
> blocks before the console has the chance to be enabled and I can't see
> anything :).
> 
> I suspect that the kernel blocks because the rng does not have enough
> entropy. Could you please give me some hints?

Hm, there should be no blocking for the DRBG to initialize.

What happens if you compile that as a module and insmod it at runtime?
> 
> >> +
> >> +  if (vli_is_zero(priv, ndigits))
> >> +  return -EINVAL;
> >> +
> >> +  /* Make sure the private key is in the range [1, n-1]. */
> >> +  if (vli_cmp(curve->n, priv, ndigits) != 1)
> >> +  return -EINVAL;
> >> +
> >> +  ecc_swap_digits(priv, privkey, ndigits);
> > 
> > Is a byteswap faster than a copy operation by looping through priv/privkey
> > and simply assinging the value?
> 
> Maybe not, but I am consistent with the rest of the code. Can we change
> this in a latter patch, if necessary?

Ok, fine with me.


Ciao
Stephan

Re: [RFC PATCH v2 1/4] crypto: ecc - add privkey generation support

[Tudor Ambarus]

Hi, Stephan,

Thank you for the review. Please see inline.

On 28.05.2017 21:44, Stephan Müller wrote:

Am Mittwoch, 17. Mai 2017, 17:26:50 CEST schrieb Tudor Ambarus:

Hi Tudor,


Add support for generating ecc private keys.

Generation of ecc private keys is helpful in a user-space to kernel
ecdh offload because the keys are not revealed to user-space. Private
key generation is also helpful to implement forward secrecy.

Signed-off-by: Tudor Ambarus 
---
  crypto/ecc.c | 20 
  crypto/ecc.h | 14 ++
  2 files changed, 34 insertions(+)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 414c78a..a591907 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -927,6 +927,26 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned
int ndigits, return 0;
  }

+int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64
*privkey) +{
+   const struct ecc_curve *curve = ecc_get_curve(curve_id);


Shouldn't there be a check that a curve is selected? I.e. a check for an error
should be added?


We already checked the curve_id before generating the key. The function
assumes that curve_id is checked before calling it, like 
ecc_is_key_valid() does.





+   u64 priv[ndigits];


Shouldn't there be a size check of ndigits?


Already checked when setting the secret. But FIPS 186-4, B.4.1
recommends to check the bit length of n. It should be greater or equal 
with 160. I will add this check, thanks.





+   unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
+
+   get_random_bytes(priv, nbytes);


Can you please use crypto_get_default_rng / crypto_rng_get_bytes /
crypto_put_default_rng?


Actually I tried this and I encountered some problems, I'm currently
debugging it.

When using the default rng and the run-time self tests are enabled,
the kernel is in a blocking state. What's worse is that the kernel
blocks before the console has the chance to be enabled and I can't see
anything :).

I suspect that the kernel blocks because the rng does not have enough
entropy. Could you please give me some hints?


+
+   if (vli_is_zero(priv, ndigits))
+   return -EINVAL;
+
+   /* Make sure the private key is in the range [1, n-1]. */
+   if (vli_cmp(curve->n, priv, ndigits) != 1)
+   return -EINVAL;
+
+   ecc_swap_digits(priv, privkey, ndigits);


Is a byteswap faster than a copy operation by looping through priv/privkey and
simply assinging the value?


Maybe not, but I am consistent with the rest of the code. Can we change
this in a latter patch, if necessary?

Thanks,
ta

Re: [RFC PATCH v2 1/4] crypto: ecc - add privkey generation support

[Stephan Müller]

Am Mittwoch, 17. Mai 2017, 17:26:50 CEST schrieb Tudor Ambarus:

Hi Tudor,

> Add support for generating ecc private keys.
> 
> Generation of ecc private keys is helpful in a user-space to kernel
> ecdh offload because the keys are not revealed to user-space. Private
> key generation is also helpful to implement forward secrecy.
> 
> Signed-off-by: Tudor Ambarus 
> ---
>  crypto/ecc.c | 20 
>  crypto/ecc.h | 14 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 414c78a..a591907 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -927,6 +927,26 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned
> int ndigits, return 0;
>  }
> 
> +int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64
> *privkey) +{
> + const struct ecc_curve *curve = ecc_get_curve(curve_id);

Shouldn't there be a check that a curve is selected? I.e. a check for an error 
should be added?

> + u64 priv[ndigits];

Shouldn't there be a size check of ndigits?

> + unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
> +
> + get_random_bytes(priv, nbytes);

Can you please use crypto_get_default_rng / crypto_rng_get_bytes / 
crypto_put_default_rng?
> +
> + if (vli_is_zero(priv, ndigits))
> + return -EINVAL;
> +
> + /* Make sure the private key is in the range [1, n-1]. */
> + if (vli_cmp(curve->n, priv, ndigits) != 1)
> + return -EINVAL;
> +
> + ecc_swap_digits(priv, privkey, ndigits);

Is a byteswap faster than a copy operation by looping through priv/privkey and 
simply assinging the value?
> +
> + return 0;
> +}
> +
>  int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
> const u8 *private_key, unsigned int private_key_len,
> u8 *public_key, unsigned int public_key_len)
> diff --git a/crypto/ecc.h b/crypto/ecc.h
> index 663d598..b94b7ce 100644
> --- a/crypto/ecc.h
> +++ b/crypto/ecc.h
> @@ -44,6 +44,20 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int
> ndigits, const u8 *private_key, unsigned int private_key_len);
> 
>  /**
> + * ecc_gen_privkey() -  Generates an ECC private key.
> + * The private key is a random integer in the range 0 < random < n, where n
> is a + * prime that is the order of the cyclic subgroup generated by the
> distinguished + * point G.
> + * @curve_id:id representing the curve to use
> + * @ndigits: curve number of digits
> + * @private_key: buffer for storing the generated private key
> + *
> + * Returns 0 if the private key was generated successfully, a negative
> value + * if an error occurred.
> + */
> +int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64
> *privkey); +
> +/**
>   * ecdh_make_pub_key() - Compute an ECC public key
>   *
>   * @curve_id:id representing the curve to use


Ciao
Stephan

[RFC PATCH v2 1/4] crypto: ecc - add privkey generation support

[Tudor Ambarus]

Add support for generating ecc private keys.

Generation of ecc private keys is helpful in a user-space to kernel
ecdh offload because the keys are not revealed to user-space. Private
key generation is also helpful to implement forward secrecy.

Signed-off-by: Tudor Ambarus 
---
 crypto/ecc.c | 20 
 crypto/ecc.h | 14 ++
 2 files changed, 34 insertions(+)

diff --git a/crypto/ecc.c b/crypto/ecc.c
index 414c78a..a591907 100644
--- a/crypto/ecc.c
+++ b/crypto/ecc.c
@@ -927,6 +927,26 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int 
ndigits,
return 0;
 }
 
+int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey)
+{
+   const struct ecc_curve *curve = ecc_get_curve(curve_id);
+   u64 priv[ndigits];
+   unsigned int nbytes = ndigits << ECC_DIGITS_TO_BYTES_SHIFT;
+
+   get_random_bytes(priv, nbytes);
+
+   if (vli_is_zero(priv, ndigits))
+   return -EINVAL;
+
+   /* Make sure the private key is in the range [1, n-1]. */
+   if (vli_cmp(curve->n, priv, ndigits) != 1)
+   return -EINVAL;
+
+   ecc_swap_digits(priv, privkey, ndigits);
+
+   return 0;
+}
+
 int ecdh_make_pub_key(unsigned int curve_id, unsigned int ndigits,
  const u8 *private_key, unsigned int private_key_len,
  u8 *public_key, unsigned int public_key_len)
diff --git a/crypto/ecc.h b/crypto/ecc.h
index 663d598..b94b7ce 100644
--- a/crypto/ecc.h
+++ b/crypto/ecc.h
@@ -44,6 +44,20 @@ int ecc_is_key_valid(unsigned int curve_id, unsigned int 
ndigits,
 const u8 *private_key, unsigned int private_key_len);
 
 /**
+ * ecc_gen_privkey() -  Generates an ECC private key.
+ * The private key is a random integer in the range 0 < random < n, where n is 
a
+ * prime that is the order of the cyclic subgroup generated by the 
distinguished
+ * point G.
+ * @curve_id:  id representing the curve to use
+ * @ndigits:   curve number of digits
+ * @private_key:   buffer for storing the generated private key
+ *
+ * Returns 0 if the private key was generated successfully, a negative value
+ * if an error occurred.
+ */
+int ecc_gen_privkey(unsigned int curve_id, unsigned int ndigits, u64 *privkey);
+
+/**
  * ecdh_make_pub_key() - Compute an ECC public key
  *
  * @curve_id:  id representing the curve to use
-- 
2.7.4