Re: [PATCH 2/2] crypto: kpp - Add DH software implementation

[Stephan Mueller]

Am Mittwoch, 13. April 2016, 09:07:38 schrieb Benedetto, Salvatore:

Hi Salvatore,
> 
> I don't see any particular benefit in replacing this check with a lower
> boundary check only. Values other than those listed are very unlikely.
> Anyway, if you feel so strong about it and other people think the same I'm
> OK with either check :-) Either case shouldn't harm.

The kernel is not supposed to policy user decisions. It is only there to 
perform operations. It is allowed to enforce policies to cover known 
weaknesses though (hence the check for the lower boundary).

So, if a user wants to use 2040 bit DH keys, what reason has the kernel to 
object?

Note, with the advancements of quantum computers is may be likely that we all 
want to use very large keys for asymmetric ciphers in the not too distant 
future.

Ciao
Stephan
--
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 2/2] crypto: kpp - Add DH software implementation

[Benedetto, Salvatore]

> -Original Message-
> From: linux-crypto-ow...@vger.kernel.org [mailto:linux-crypto-
> ow...@vger.kernel.org] On Behalf Of Stephan Mueller
> Sent: Tuesday, April 12, 2016 2:21 PM
> To: Benedetto, Salvatore <salvatore.benede...@intel.com>
> Cc: herb...@gondor.apana.org.au; linux-crypto@vger.kernel.org
> Subject: Re: [PATCH 2/2] crypto: kpp - Add DH software implementation
> 
> > >
> > > >  include/crypto/dh.h |  23 ++
> > > >  6 files changed, 631 insertions(+)  create mode 100644
> > > > crypto/dh.c  create mode 100644 include/crypto/dh.h
> > > >
> > > > +
> > > > +static int dh_check_params_length(unsigned int p_len) {
> > > > +   switch (p_len) {
> > > > +   case 1536:
> > > > +   case 2048:
> > > > +   case 3072:
> > > > +   case 4096:
> > > > +   case 6144:
> > > > +   case 8192:
> > > > +   return 0;
> > >
> > > Does the math require this check?
> > >
> > > Wouldn't it be better to implement limits to the low side (i.e.
> > > p_len <
> > > 1536) and then add a real limit due to the implementation (e.g. it
> > > must be multiple of full bytes)?
> >
> > The math itself does not require any check that I'm aware of.
> > As for the real limit, I think we have to add that as an hardware that
> > is only capable of handling up to 4096 bytes, should fall back to the
> > software implementation if a bigger param is used.
> 
> Then why not leave that check to the respective HW implementation and
> provide support for all parameters in software? I.e. simply replace this check
> with a lower boundary check only?
> 

I don't see any particular benefit in replacing this check with a lower boundary
check only. Values other than those listed are very unlikely.
Anyway, if you feel so strong about it and other people think the same I'm
OK with either check :-) Either case shouldn't harm.

Thanks,
Salvatore
--
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 2/2] crypto: kpp - Add DH software implementation

[Stephan Mueller]

Am Dienstag, 12. April 2016, 13:18:42 schrieb Benedetto, Salvatore:

Hi Salvatore,

> Hi Stephan,
> 
> > -Original Message-
> > From: Stephan Mueller [mailto:s...@eperm.de]
> > Sent: Tuesday, April 12, 2016 2:01 PM
> > To: Benedetto, Salvatore <salvatore.benede...@intel.com>
> > Cc: herb...@gondor.apana.org.au; linux-crypto@vger.kernel.org
> > Subject: Re: [PATCH 2/2] crypto: kpp - Add DH software implementation
> > 
> > Am Dienstag, 12. April 2016, 11:39:16 schrieb Salvatore Benedetto:
> > 
> > Hi Salvatore,
> > 
> > >  * Implement MPI based Diffie-Hellman under kpp API
> > >  * Add test with data generad by OpenSSL
> > > 
> > > Signed-off-by: Salvatore Benedetto <salvatore.benede...@intel.com>
> > > ---
> > > 
> > >  crypto/Kconfig  |   8 ++
> > >  crypto/Makefile |   2 +
> > >  crypto/dh.c | 233
> > > 
> > > 
> > 
> > crypto/testmgr.c|
> > 
> > > 157 +++
> > > 
> > >  crypto/testmgr.h| 208
> > 
> > ++
> > 
> > >  include/crypto/dh.h |  23 ++
> > >  6 files changed, 631 insertions(+)
> > >  create mode 100644 crypto/dh.c
> > >  create mode 100644 include/crypto/dh.h
> > > 
> > > +
> > > +static int dh_check_params_length(unsigned int p_len)
> > > +{
> > > + switch (p_len) {
> > > + case 1536:
> > > + case 2048:
> > > + case 3072:
> > > + case 4096:
> > > + case 6144:
> > > + case 8192:
> > > + return 0;
> > 
> > Does the math require this check?
> > 
> > Wouldn't it be better to implement limits to the low side (i.e. p_len <
> > 1536) and then add a real limit due to the implementation (e.g. it must
> > be multiple of full bytes)?
> 
> The math itself does not require any check that I'm aware of.
> As for the real limit, I think we have to add that as an hardware
> that is only capable of handling up to 4096 bytes, should fall back
> to the software implementation if a bigger param is used.

Then why not leave that check to the respective HW implementation and provide 
support for all parameters in software? I.e. simply replace this check with a 
lower boundary check only?


Ciao
Stephan
--
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 2/2] crypto: kpp - Add DH software implementation

[Benedetto, Salvatore]

Hi Stephan,

> -Original Message-
> From: Stephan Mueller [mailto:s...@eperm.de]
> Sent: Tuesday, April 12, 2016 2:01 PM
> To: Benedetto, Salvatore <salvatore.benede...@intel.com>
> Cc: herb...@gondor.apana.org.au; linux-crypto@vger.kernel.org
> Subject: Re: [PATCH 2/2] crypto: kpp - Add DH software implementation
> 
> Am Dienstag, 12. April 2016, 11:39:16 schrieb Salvatore Benedetto:
> 
> Hi Salvatore,
> 
> >  * Implement MPI based Diffie-Hellman under kpp API
> >  * Add test with data generad by OpenSSL
> >
> > Signed-off-by: Salvatore Benedetto <salvatore.benede...@intel.com>
> > ---
> >  crypto/Kconfig  |   8 ++
> >  crypto/Makefile |   2 +
> >  crypto/dh.c | 233
> > 
> crypto/testmgr.c|
> > 157 +++
> >  crypto/testmgr.h| 208
> ++
> >  include/crypto/dh.h |  23 ++
> >  6 files changed, 631 insertions(+)
> >  create mode 100644 crypto/dh.c
> >  create mode 100644 include/crypto/dh.h
> >
> > +
> > +static int dh_check_params_length(unsigned int p_len)
> > +{
> > +   switch (p_len) {
> > +   case 1536:
> > +   case 2048:
> > +   case 3072:
> > +   case 4096:
> > +   case 6144:
> > +   case 8192:
> > +   return 0;
> 
> Does the math require this check?
> 
> Wouldn't it be better to implement limits to the low side (i.e. p_len < 1536)
> and then add a real limit due to the implementation (e.g. it must be multiple
> of full bytes)?
> 

The math itself does not require any check that I'm aware of.
As for the real limit, I think we have to add that as an hardware
that is only capable of handling up to 4096 bytes, should fall back
to the software implementation if a bigger param is used.

Thanks,
Salvatore
--
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 2/2] crypto: kpp - Add DH software implementation

[Stephan Mueller]

Am Dienstag, 12. April 2016, 11:39:16 schrieb Salvatore Benedetto:

Hi Salvatore,

>  * Implement MPI based Diffie-Hellman under kpp API
>  * Add test with data generad by OpenSSL
> 
> Signed-off-by: Salvatore Benedetto 
> ---
>  crypto/Kconfig  |   8 ++
>  crypto/Makefile |   2 +
>  crypto/dh.c | 233
>  crypto/testmgr.c|
> 157 +++
>  crypto/testmgr.h| 208 ++
>  include/crypto/dh.h |  23 ++
>  6 files changed, 631 insertions(+)
>  create mode 100644 crypto/dh.c
>  create mode 100644 include/crypto/dh.h
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 31bf962..89db25c 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -110,6 +110,14 @@ config CRYPTO_RSA
>   help
> Generic implementation of the RSA public key algorithm.
> 
> +config CRYPTO_DH
> + tristate "Diffie-Hellman algorithm"
> + select CRYPTO_KPP
> + select MPILIB
> + help
> +   Generic implementation of the Diffie-Hellman algorithm.
> +
> +
>  config CRYPTO_MANAGER
>   tristate "Cryptographic algorithm manager"
>   select CRYPTO_MANAGER2
> diff --git a/crypto/Makefile b/crypto/Makefile
> index 5b60890..101f8fd 100644
> --- a/crypto/Makefile
> +++ b/crypto/Makefile
> @@ -32,6 +32,8 @@ obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
>  obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o
>  obj-$(CONFIG_CRYPTO_KPP2) += kpp.o
> 
> +obj-$(CONFIG_CRYPTO_DH) += dh.o
> +
>  $(obj)/rsapubkey-asn1.o: $(obj)/rsapubkey-asn1.c $(obj)/rsapubkey-asn1.h
>  $(obj)/rsaprivkey-asn1.o: $(obj)/rsaprivkey-asn1.c $(obj)/rsaprivkey-asn1.h
> clean-files += rsapubkey-asn1.c rsapubkey-asn1.h
> diff --git a/crypto/dh.c b/crypto/dh.c
> new file mode 100644
> index 000..b701848
> --- /dev/null
> +++ b/crypto/dh.c
> @@ -0,0 +1,233 @@
> +/*  Diffie-Hellman Key Agreement Method [RFC2631]
> + *
> + * Copyright (c) 2016, Intel Corporation
> + * Authors: Salvatore Benedetto 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct dh_ctx {
> + MPI p;
> + MPI g;
> + MPI xa;
> +};
> +
> +static void dh_free_ctx(struct dh_ctx *ctx)
> +{
> + mpi_free(ctx->p);
> + mpi_free(ctx->g);
> + mpi_free(ctx->xa);
> + ctx->p = NULL;
> + ctx->g = NULL;
> + ctx->xa = NULL;
> +}
> +
> +/*
> + * Public key generation function [RFC2631 sec 2.1.1]
> + * ya = g^xa mod p;
> + */
> +static int _generate_public_key(const struct dh_ctx *ctx, MPI ya)
> +{
> + /* ya = g^xa mod p */
> + return mpi_powm(ya, ctx->g, ctx->xa, ctx->p);
> +}
> +
> +/*
> + * ZZ generation function [RFC2631 sec 2.1.1]
> + * ZZ = yb^xa mod p;
> + */
> +static int _compute_shared_secret(const struct dh_ctx *ctx, MPI yb,
> +   MPI zz)
> +{
> + /* ZZ = yb^xa mod p */
> + return mpi_powm(zz, yb, ctx->xa, ctx->p);
> +}
> +
> +static inline struct dh_ctx *dh_get_ctx(struct crypto_kpp *tfm)
> +{
> + return kpp_tfm_ctx(tfm);
> +}
> +
> +static int dh_check_params_length(unsigned int p_len)
> +{
> + switch (p_len) {
> + case 1536:
> + case 2048:
> + case 3072:
> + case 4096:
> + case 6144:
> + case 8192:
> + return 0;

Does the math require this check?

Wouldn't it be better to implement limits to the low side (i.e. p_len < 1536) 
and then add a real limit due to the implementation (e.g. it must be multiple 
of full bytes)?

> + }
> + return -EINVAL;
> +}
> +
> +static int dh_set_params(struct crypto_kpp *tfm, void *buffer,
> +  unsigned int len)
> +{
> + struct dh_ctx *ctx = dh_get_ctx(tfm);
> + struct dh_params *params = (struct dh_params *)buffer;
> +
> + if (unlikely(!buffer || !len))
> + return -EINVAL;
> +
> + if (unlikely(!params->p || !params->g))
> + return -EINVAL;
> +
> + if (dh_check_params_length(params->p_size << 3))
> + return -EINVAL;
> +
> + ctx->p = mpi_read_raw_data(params->p, params->p_size);
> + if (!ctx->p)
> + return -EINVAL;
> +
> + ctx->g = mpi_read_raw_data(params->g, params->g_size);
> + if (!ctx->g) {
> + mpi_free(ctx->p);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int dh_set_secret(struct crypto_kpp *tfm, void *buffer,
> +  unsigned int len)
> +{
> + struct dh_ctx *ctx = dh_get_ctx(tfm);
> +
> + if (unlikely(!buffer || !len))
> + return -EINVAL;
> +
> + ctx->xa = mpi_read_raw_data(buffer, len);
> +
> + if (!ctx->xa)
> +