by Stephan Müller

[permalink] [raw]

Subject: Re: [PATCH 4/8] crypto: KPP - add API crypto_kpp_set_params

Am Mittwoch, 19. April 2017, 16:51:54 CEST schrieb Benedetto, Salvatore:

Hi Salvatore,

> Hi Stephan,
>
> > -----Original Message-----
> > From: [email protected] [mailto:keyrings-
> > [email protected]] On Behalf Of Stephan M?ller
> > Sent: Wednesday, April 19, 2017 12:06 AM
> > To: [email protected]
> > Cc: [email protected]
> > Subject: [PATCH 4/8] crypto: KPP - add API crypto_kpp_set_params
> >
> > KPP mechanisms like DH require a parameter set to be provided by the
> > caller. That parameter set may be provided by the crypto_kpp_set_secret
> > function. Yet, the parameters hare handled independently from the secret
> > key which implies that they should be able to be set independently from
> > the key.
> >
> > The new API allows KPP mechanisms to register a callback allowing to set
> > such parameters.
> >
> > Signed-off-by: Stephan Mueller <[email protected]>
> > ---
> >
> > Documentation/crypto/api-kpp.rst | 2 +-
> > include/crypto/kpp.h | 28 ++++++++++++++++++++++++++++
> > 2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/crypto/api-kpp.rst b/Documentation/crypto/api-
> > kpp.rst
> > index 7d86ab9..7b2c0d4 100644
> > --- a/Documentation/crypto/api-kpp.rst
> > +++ b/Documentation/crypto/api-kpp.rst
> > @@ -11,7 +11,7 @@ Key-agreement Protocol Primitives (KPP) Cipher API
> >
> > :doc: Generic Key-agreement Protocol Primitives API
> >
> > .. kernel-doc:: include/crypto/kpp.h
> >
> > - :functions: crypto_alloc_kpp crypto_free_kpp crypto_kpp_set_secret
> > crypto_kpp_generate_public_key crypto_kpp_compute_shared_secret
> > crypto_kpp_maxsize
> > + :functions: crypto_alloc_kpp crypto_free_kpp crypto_kpp_set_params
> > + crypto_kpp_set_secret crypto_kpp_generate_public_key
> > + crypto_kpp_compute_shared_secret crypto_kpp_maxsize
> >
> > Key-agreement Protocol Primitives (KPP) Cipher Request Handle
> > -------------------------------------------------------------
> >
> > diff --git a/include/crypto/kpp.h b/include/crypto/kpp.h index
> > ce8e1f7..5931364 100644
> > --- a/include/crypto/kpp.h
> > +++ b/include/crypto/kpp.h
> > @@ -51,6 +51,9 @@ struct crypto_kpp {
> >
> > /**
> >
> > * struct kpp_alg - generic key-agreement protocol primitives
> > *
> >
> > + * @set_params: Function allows the caller to set the parameters
> > + * separately from the key. The format of the
> > parameters
> > + * is protocol specific.
> >
> > * @set_secret: Function invokes the protocol specific
> >
> > function to
> >
> > * store the secret private key along with parameters.
> > * The implementation knows how to decode thie
> >
> > buffer
> > @@ -74,6 +77,8 @@ struct crypto_kpp {
> >
> > * @base: Common crypto API algorithm data structure
> > */
> >
> > struct kpp_alg {
> >
> > + int (*set_params)(struct crypto_kpp *tfm, const void *buffer,
> > + unsigned int len);
> >
> > int (*set_secret)(struct crypto_kpp *tfm, const void *buffer,
> >
> > unsigned int len);
> >
> > int (*generate_public_key)(struct kpp_request *req); @@ -259,6
> >
> > +264,29 @@ struct kpp_secret { };
> >
> > /**
> >
> > + * crypto_kpp_set_params() - Set parameters needed for kpp operation
> > + *
> > + * Function invokes the specific kpp operation for a given alg.
> > + *
> > + * @tfm: tfm handle
> > + * @buffer: Buffer holding the protocol specific representation of the
> > + * parameters (e.g. PKCS#3 DER for DH)
> > + * @len: Length of the parameter buffer.
> > + *
> > + * Return: zero on success; error code in case of error */ static
> > +inline int crypto_kpp_set_params(struct crypto_kpp *tfm,
> > + const void *buffer, unsigned int len) {
> > + struct kpp_alg *alg = crypto_kpp_alg(tfm);
> > +
> > + if (alg->set_params)
> > + return alg->set_params(tfm, buffer, len);
> > + else
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +/**
> >
> > * crypto_kpp_set_secret() - Invoke kpp operation
> > *
> > * Function invokes the specific kpp operation for a given alg.
> >
> > --
> > 2.9.3
>
> I'm not really in favor of having two ways for setting the params.

I am in full agreement. But setting the key together with the parameters is
not good, IMHO. Every other crypto lib implements a separate setting.

> As you are probably aware, PKCS3 and set_params was my intial
> approach, but then Herbert suggested a lighter approach like rtnetlink
> which I actually prefer.

I was not aware of that. Considering that PKCS#3 or X9.42 are the common
formats for setting parameters, I am not in favor of "inventing" a special
format just for the kernel user space interface. If we would have such special
format, user space would need to add a conversion step from a common to the
kernel-special format.

You see with https://github.com/smuellerDD/libkcapi/blob/kpp/test/kcapi-main.c
starting at line 2500 a full description how the OpenSSL-generated DH
parameters can be used directly by the AF_ALG interface.

I have no concern to use rtnetlink for in-kernel use cases.
>
> Can't you expose that through AF_ALG?

As mentioned, exposing rtnetlink means that we have to have an ASN.1-based
converter from PKCS#3 to rtnetlink. As the kernel already has an ASN.1 parser
and the additional code to use PKCS#3 in the kernel is very minimal compared
to adding a ASN.1 parser in user space, I opted for implementing PKCS#3 in
kernel space.
>
> Regards,
> Salvatore

Ciao
Stephan

2017-08-28 10:34:54

by Tudor Ambarus

[permalink] [raw]

Subject: Re: [PATCH 7/8] crypto: ecdh - constify key

On 04/19/2017 02:07 AM, Stephan Müller wrote:
> Signed-off-by: Stephan Mueller <[email protected]>
> ---
> include/crypto/ecdh.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/crypto/ecdh.h b/include/crypto/ecdh.h
> index 03a64f6..b5bb149 100644
> --- a/include/crypto/ecdh.h
> +++ b/include/crypto/ecdh.h
> @@ -40,7 +40,7 @@
> */
> struct ecdh {
> unsigned short curve_id;
> - char *key;
> + const char *key;
> unsigned short key_size;
> };
>
>

I just came across this and remembered that Stephan already
made a patch, so:

Acked-by: Tudor Ambarus <[email protected]>