Message-ID: <20190322194231.ih6itzctanyheoi4@altlinux.org>
Date:   Fri, 22 Mar 2019 22:42:31 +0300
From:   Vitaly Chikunov <vt@...linux.org>
To:     David Howells <dhowells@...hat.com>, keyrings@...r.kernel.org
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 06/11] X.509: parse public key parameters from x509
 for akcipher

David,

Can you please Ack this patch or suggest modifications as it includes
changes to ASYMMETRIC KEYS tree?

Thanks,

On Fri, Mar 01, 2019 at 08:59:13PM +0300, Vitaly Chikunov wrote:
> Some public key algorithms (like EC-DSA) keep in parameters field
> important data such as digest and curve OIDs (possibly more for
> different EC-DSA variants). Thus, just setting a public key (as
> for RSA) is not enough.
> 
> Append parameters into the key stream for akcipher_set_{pub,priv}_key.
> Appended data is: (u32) algo OID, (u32) parameters length, parameters
> data.
> 
> This does not affect current akcipher API nor RSA ciphers (they could
> ignore it). Idea of appending parameters to the key stream is by Herbert
> Xu.
> 
> Cc: David Howells <dhowells@...hat.com>
> Cc: keyrings@...r.kernel.org
> Signed-off-by: Vitaly Chikunov <vt@...linux.org>
> ---
>  crypto/asymmetric_keys/asym_tpm.c         | 43 ++++++++++++++++--
>  crypto/asymmetric_keys/public_key.c       | 72 ++++++++++++++++++++++++-------
>  crypto/asymmetric_keys/x509.asn1          |  2 +-
>  crypto/asymmetric_keys/x509_cert_parser.c | 31 +++++++++++++
>  crypto/testmgr.c                          | 24 +++++++++--
>  crypto/testmgr.h                          |  5 +++
>  include/crypto/akcipher.h                 | 18 ++++----
>  include/crypto/public_key.h               |  4 ++
>  8 files changed, 168 insertions(+), 31 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
> index 402fc34ca044..d95d7ec50e5a 100644
> --- a/crypto/asymmetric_keys/asym_tpm.c
> +++ b/crypto/asymmetric_keys/asym_tpm.c
> @@ -395,6 +395,12 @@ static int determine_akcipher(const char *encoding, const char *hash_algo,
>  	return -ENOPKG;
>  }
>  
> +static u8 *tpm_pack_u32(u8 *dst, u32 val)
> +{
> +	memcpy(dst, &val, sizeof(val));
> +	return dst + sizeof(val);
> +}
> +
>  /*
>   * Query information about a key.
>   */
> @@ -407,6 +413,7 @@ static int tpm_key_query(const struct kernel_pkey_params *params,
>  	struct crypto_akcipher *tfm;
>  	uint8_t der_pub_key[PUB_KEY_BUF_SIZE];
>  	uint32_t der_pub_key_len;
> +	u8 *pkey, *ptr;
>  	int len;
>  
>  	/* TPM only works on private keys, public keys still done in software */
> @@ -421,7 +428,16 @@ static int tpm_key_query(const struct kernel_pkey_params *params,
>  	der_pub_key_len = derive_pub_key(tk->pub_key, tk->pub_key_len,
>  					 der_pub_key);
>  
> -	ret = crypto_akcipher_set_pub_key(tfm, der_pub_key, der_pub_key_len);
> +	pkey = kmalloc(der_pub_key_len + sizeof(u32) * 2, GFP_KERNEL);
> +	if (!pkey)
> +		goto error_free_tfm;
> +	memcpy(pkey, der_pub_key, der_pub_key_len);
> +	ptr = pkey + der_pub_key_len;
> +	/* Set dummy parameters to satisfy set_pub_key ABI. */
> +	ptr = tpm_pack_u32(ptr, 0); /* algo */
> +	ptr = tpm_pack_u32(ptr, 0); /* parameter length */
> +
> +	ret = crypto_akcipher_set_pub_key(tfm, pkey, der_pub_key_len);
>  	if (ret < 0)
>  		goto error_free_tfm;
>  
> @@ -440,6 +456,7 @@ static int tpm_key_query(const struct kernel_pkey_params *params,
>  
>  	ret = 0;
>  error_free_tfm:
> +	kfree(pkey);
>  	crypto_free_akcipher(tfm);
>  	pr_devel("<==%s() = %d\n", __func__, ret);
>  	return ret;
> @@ -460,6 +477,7 @@ static int tpm_key_encrypt(struct tpm_key *tk,
>  	struct scatterlist in_sg, out_sg;
>  	uint8_t der_pub_key[PUB_KEY_BUF_SIZE];
>  	uint32_t der_pub_key_len;
> +	u8 *pkey, *ptr;
>  	int ret;
>  
>  	pr_devel("==>%s()\n", __func__);
> @@ -475,7 +493,15 @@ static int tpm_key_encrypt(struct tpm_key *tk,
>  	der_pub_key_len = derive_pub_key(tk->pub_key, tk->pub_key_len,
>  					 der_pub_key);
>  
> -	ret = crypto_akcipher_set_pub_key(tfm, der_pub_key, der_pub_key_len);
> +	pkey = kmalloc(der_pub_key_len + sizeof(u32) * 2, GFP_KERNEL);
> +	if (!pkey)
> +		goto error_free_tfm;
> +	memcpy(pkey, der_pub_key, der_pub_key_len);
> +	ptr = pkey + der_pub_key_len;
> +	ptr = tpm_pack_u32(ptr, 0); /* algo */
> +	ptr = tpm_pack_u32(ptr, 0); /* parameter length */
> +
> +	ret = crypto_akcipher_set_pub_key(tfm, pkey, der_pub_key_len);
>  	if (ret < 0)
>  		goto error_free_tfm;
>  
> @@ -500,6 +526,7 @@ static int tpm_key_encrypt(struct tpm_key *tk,
>  
>  	akcipher_request_free(req);
>  error_free_tfm:
> +	kfree(pkey);
>  	crypto_free_akcipher(tfm);
>  	pr_devel("<==%s() = %d\n", __func__, ret);
>  	return ret;
> @@ -748,6 +775,7 @@ static int tpm_key_verify_signature(const struct key *key,
>  	char alg_name[CRYPTO_MAX_ALG_NAME];
>  	uint8_t der_pub_key[PUB_KEY_BUF_SIZE];
>  	uint32_t der_pub_key_len;
> +	u8 *pkey, *ptr;
>  	int ret;
>  
>  	pr_devel("==>%s()\n", __func__);
> @@ -770,7 +798,15 @@ static int tpm_key_verify_signature(const struct key *key,
>  	der_pub_key_len = derive_pub_key(tk->pub_key, tk->pub_key_len,
>  					 der_pub_key);
>  
> -	ret = crypto_akcipher_set_pub_key(tfm, der_pub_key, der_pub_key_len);
> +	pkey = kmalloc(der_pub_key_len + sizeof(u32) * 2, GFP_KERNEL);
> +	if (!pkey)
> +		goto error_free_tfm;
> +	memcpy(pkey, der_pub_key, der_pub_key_len);
> +	ptr = pkey + der_pub_key_len;
> +	ptr = tpm_pack_u32(ptr, 0); /* algo */
> +	ptr = tpm_pack_u32(ptr, 0); /* parameter length */
> +
> +	ret = crypto_akcipher_set_pub_key(tfm, pkey, der_pub_key_len);
>  	if (ret < 0)
>  		goto error_free_tfm;
>  
> @@ -792,6 +828,7 @@ static int tpm_key_verify_signature(const struct key *key,
>  
>  	akcipher_request_free(req);
>  error_free_tfm:
> +	kfree(pkey);
>  	crypto_free_akcipher(tfm);
>  	pr_devel("<==%s() = %d\n", __func__, ret);
>  	if (WARN_ON_ONCE(ret > 0))
> diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
> index 4dcfe281b898..0564951f8760 100644
> --- a/crypto/asymmetric_keys/public_key.c
> +++ b/crypto/asymmetric_keys/public_key.c
> @@ -45,6 +45,7 @@ void public_key_free(struct public_key *key)
>  {
>  	if (key) {
>  		kfree(key->key);
> +		kfree(key->params);
>  		kfree(key);
>  	}
>  }
> @@ -94,6 +95,12 @@ int software_key_determine_akcipher(const char *encoding,
>  	return -ENOPKG;
>  }
>  
> +static u8 *pkey_pack_u32(u8 *dst, u32 val)
> +{
> +	memcpy(dst, &val, sizeof(val));
> +	return dst + sizeof(val);
> +}
> +
>  /*
>   * Query information about a key.
>   */
> @@ -103,6 +110,7 @@ static int software_key_query(const struct kernel_pkey_params *params,
>  	struct crypto_akcipher *tfm;
>  	struct public_key *pkey = params->key->payload.data[asym_crypto];
>  	char alg_name[CRYPTO_MAX_ALG_NAME];
> +	u8 *key, *ptr;
>  	int ret, len;
>  
>  	ret = software_key_determine_akcipher(params->encoding,
> @@ -115,14 +123,22 @@ static int software_key_query(const struct kernel_pkey_params *params,
>  	if (IS_ERR(tfm))
>  		return PTR_ERR(tfm);
>  
> +	key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> +		      GFP_KERNEL);
> +	if (!key)
> +		goto error_free_tfm;
> +	memcpy(key, pkey->key, pkey->keylen);
> +	ptr = key + pkey->keylen;
> +	ptr = pkey_pack_u32(ptr, pkey->algo);
> +	ptr = pkey_pack_u32(ptr, pkey->paramlen);
> +	memcpy(ptr, pkey->params, pkey->paramlen);
> +
>  	if (pkey->key_is_private)
> -		ret = crypto_akcipher_set_priv_key(tfm,
> -						   pkey->key, pkey->keylen);
> +		ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen);
>  	else
> -		ret = crypto_akcipher_set_pub_key(tfm,
> -						  pkey->key, pkey->keylen);
> +		ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen);
>  	if (ret < 0)
> -		goto error_free_tfm;
> +		goto error_free_key;
>  
>  	len = crypto_akcipher_maxsize(tfm);
>  	info->key_size = len * 8;
> @@ -143,6 +159,8 @@ static int software_key_query(const struct kernel_pkey_params *params,
>  	}
>  	ret = 0;
>  
> +error_free_key:
> +	kfree(key);
>  error_free_tfm:
>  	crypto_free_akcipher(tfm);
>  	pr_devel("<==%s() = %d\n", __func__, ret);
> @@ -161,6 +179,7 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
>  	struct crypto_wait cwait;
>  	struct scatterlist in_sg, out_sg;
>  	char alg_name[CRYPTO_MAX_ALG_NAME];
> +	char *key, *ptr;
>  	int ret;
>  
>  	pr_devel("==>%s()\n", __func__);
> @@ -179,14 +198,23 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
>  	if (!req)
>  		goto error_free_tfm;
>  
> +	key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> +		      GFP_KERNEL);
> +	if (!key)
> +		goto error_free_req;
> +
> +	memcpy(key, pkey->key, pkey->keylen);
> +	ptr = key + pkey->keylen;
> +	ptr = pkey_pack_u32(ptr, pkey->algo);
> +	ptr = pkey_pack_u32(ptr, pkey->paramlen);
> +	memcpy(ptr, pkey->params, pkey->paramlen);
> +
>  	if (pkey->key_is_private)
> -		ret = crypto_akcipher_set_priv_key(tfm,
> -						   pkey->key, pkey->keylen);
> +		ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen);
>  	else
> -		ret = crypto_akcipher_set_pub_key(tfm,
> -						  pkey->key, pkey->keylen);
> +		ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen);
>  	if (ret)
> -		goto error_free_req;
> +		goto error_free_key;
>  
>  	sg_init_one(&in_sg, in, params->in_len);
>  	sg_init_one(&out_sg, out, params->out_len);
> @@ -216,6 +244,8 @@ static int software_key_eds_op(struct kernel_pkey_params *params,
>  	if (ret == 0)
>  		ret = req->dst_len;
>  
> +error_free_key:
> +	kfree(key);
>  error_free_req:
>  	akcipher_request_free(req);
>  error_free_tfm:
> @@ -235,6 +265,7 @@ int public_key_verify_signature(const struct public_key *pkey,
>  	struct akcipher_request *req;
>  	struct scatterlist src_sg[2];
>  	char alg_name[CRYPTO_MAX_ALG_NAME];
> +	char *key, *ptr;
>  	int ret;
>  
>  	pr_devel("==>%s()\n", __func__);
> @@ -258,14 +289,23 @@ int public_key_verify_signature(const struct public_key *pkey,
>  	if (!req)
>  		goto error_free_tfm;
>  
> +	key = kmalloc(pkey->keylen + sizeof(u32) * 2 + pkey->paramlen,
> +		      GFP_KERNEL);
> +	if (!key)
> +		goto error_free_req;
> +
> +	memcpy(key, pkey->key, pkey->keylen);
> +	ptr = key + pkey->keylen;
> +	ptr = pkey_pack_u32(ptr, pkey->algo);
> +	ptr = pkey_pack_u32(ptr, pkey->paramlen);
> +	memcpy(ptr, pkey->params, pkey->paramlen);
> +
>  	if (pkey->key_is_private)
> -		ret = crypto_akcipher_set_priv_key(tfm,
> -						   pkey->key, pkey->keylen);
> +		ret = crypto_akcipher_set_priv_key(tfm, key, pkey->keylen);
>  	else
> -		ret = crypto_akcipher_set_pub_key(tfm,
> -						  pkey->key, pkey->keylen);
> +		ret = crypto_akcipher_set_pub_key(tfm, key, pkey->keylen);
>  	if (ret)
> -		goto error_free_req;
> +		goto error_free_key;
>  
>  	sg_init_table(src_sg, 2);
>  	sg_set_buf(&src_sg[0], sig->s, sig->s_size);
> @@ -278,6 +318,8 @@ int public_key_verify_signature(const struct public_key *pkey,
>  				      crypto_req_done, &cwait);
>  	ret = crypto_wait_req(crypto_akcipher_verify(req), &cwait);
>  
> +error_free_key:
> +	kfree(key);
>  error_free_req:
>  	akcipher_request_free(req);
>  error_free_tfm:
> diff --git a/crypto/asymmetric_keys/x509.asn1 b/crypto/asymmetric_keys/x509.asn1
> index aae0cde414e2..5c9f4e4a5231 100644
> --- a/crypto/asymmetric_keys/x509.asn1
> +++ b/crypto/asymmetric_keys/x509.asn1
> @@ -22,7 +22,7 @@ CertificateSerialNumber ::= INTEGER
>  
>  AlgorithmIdentifier ::= SEQUENCE {
>  	algorithm		OBJECT IDENTIFIER ({ x509_note_OID }),
> -	parameters		ANY OPTIONAL
> +	parameters		ANY OPTIONAL ({ x509_note_params })
>  }
>  
>  Name ::= SEQUENCE OF RelativeDistinguishedName
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 991f4d735a4e..b2cdf2db1987 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -26,6 +26,9 @@ struct x509_parse_context {
>  	const void	*cert_start;		/* Start of cert content */
>  	const void	*key;			/* Key data */
>  	size_t		key_size;		/* Size of key data */
> +	const void	*params;		/* Key parameters */
> +	size_t		params_size;		/* Size of key parameters */
> +	enum OID	key_algo;		/* Public key algorithm */
>  	enum OID	last_oid;		/* Last OID encountered */
>  	enum OID	algo_oid;		/* Algorithm OID */
>  	unsigned char	nr_mpi;			/* Number of MPIs stored */
> @@ -109,6 +112,13 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
>  
>  	cert->pub->keylen = ctx->key_size;
>  
> +	cert->pub->params = kmemdup(ctx->params, ctx->params_size, GFP_KERNEL);
> +	if (!cert->pub->params)
> +		goto error_decode;
> +
> +	cert->pub->paramlen = ctx->params_size;
> +	cert->pub->algo = ctx->key_algo;
> +
>  	/* Grab the signature bits */
>  	ret = x509_get_sig_params(cert);
>  	if (ret < 0)
> @@ -401,6 +411,27 @@ int x509_note_subject(void *context, size_t hdrlen,
>  }
>  
>  /*
> + * Extract the parameters for the public key
> + */
> +int x509_note_params(void *context, size_t hdrlen,
> +		     unsigned char tag,
> +		     const void *value, size_t vlen)
> +{
> +	struct x509_parse_context *ctx = context;
> +
> +	/*
> +	 * AlgorithmIdentifier is used three times in the x509, we should skip
> +	 * first and ignore third, using second one which is after subject and
> +	 * before subjectPublicKey.
> +	 */
> +	if (!ctx->cert->raw_subject || ctx->key)
> +		return 0;
> +	ctx->params = value - hdrlen;
> +	ctx->params_size = vlen + hdrlen;
> +	return 0;
> +}
> +
> +/*
>   * Extract the data for the public key algorithm
>   */
>  int x509_extract_key_data(void *context, size_t hdrlen,
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 443d5b6f1045..50396b3a2a47 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -2493,6 +2493,12 @@ static int alg_test_kpp(const struct alg_test_desc *desc, const char *driver,
>  	return err;
>  }
>  
> +static u8 *test_pack_u32(u8 *dst, u32 val)
> +{
> +	memcpy(dst, &val, sizeof(val));
> +	return dst + sizeof(val);
> +}
> +
>  static int test_akcipher_one(struct crypto_akcipher *tfm,
>  			     const struct akcipher_testvec *vecs)
>  {
> @@ -2507,6 +2513,7 @@ static int test_akcipher_one(struct crypto_akcipher *tfm,
>  	const char *m, *c;
>  	unsigned int m_size, c_size;
>  	const char *op;
> +	u8 *key, *ptr;
>  
>  	if (testmgr_alloc_buf(xbuf))
>  		return err;
> @@ -2517,12 +2524,20 @@ static int test_akcipher_one(struct crypto_akcipher *tfm,
>  
>  	crypto_init_wait(&wait);
>  
> +	key = kmalloc(vecs->key_len + sizeof(u32) * 2 + vecs->param_len,
> +		      GFP_KERNEL);
> +	if (!key)
> +		goto free_xbuf;
> +	memcpy(key, vecs->key, vecs->key_len);
> +	ptr = key + vecs->key_len;
> +	ptr = test_pack_u32(ptr, vecs->algo);
> +	ptr = test_pack_u32(ptr, vecs->param_len);
> +	memcpy(ptr, vecs->params, vecs->param_len);
> +
>  	if (vecs->public_key_vec)
> -		err = crypto_akcipher_set_pub_key(tfm, vecs->key,
> -						  vecs->key_len);
> +		err = crypto_akcipher_set_pub_key(tfm, key, vecs->key_len);
>  	else
> -		err = crypto_akcipher_set_priv_key(tfm, vecs->key,
> -						   vecs->key_len);
> +		err = crypto_akcipher_set_priv_key(tfm, key, vecs->key_len);
>  	if (err)
>  		goto free_req;
>  
> @@ -2652,6 +2667,7 @@ static int test_akcipher_one(struct crypto_akcipher *tfm,
>  	kfree(outbuf_enc);
>  free_req:
>  	akcipher_request_free(req);
> +	kfree(key);
>  free_xbuf:
>  	testmgr_free_buf(xbuf);
>  	return err;
> diff --git a/crypto/testmgr.h b/crypto/testmgr.h
> index f267633cf13a..75d8f8c3e203 100644
> --- a/crypto/testmgr.h
> +++ b/crypto/testmgr.h
> @@ -25,6 +25,8 @@
>  #ifndef _CRYPTO_TESTMGR_H
>  #define _CRYPTO_TESTMGR_H
>  
> +#include <linux/oid_registry.h>
> +
>  #define MAX_IVLEN		32
>  
>  /*
> @@ -135,13 +137,16 @@ struct drbg_testvec {
>  
>  struct akcipher_testvec {
>  	const unsigned char *key;
> +	const unsigned char *params;
>  	const unsigned char *m;
>  	const unsigned char *c;
>  	unsigned int key_len;
> +	unsigned int param_len;
>  	unsigned int m_size;
>  	unsigned int c_size;
>  	bool public_key_vec;
>  	bool siggen_sigver_test;
> +	enum OID algo;
>  };
>  
>  struct kpp_testvec {
> diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h
> index 28ffa9ef03a9..4ecbedddd9a1 100644
> --- a/include/crypto/akcipher.h
> +++ b/include/crypto/akcipher.h
> @@ -74,10 +74,10 @@ struct crypto_akcipher {
>   *		operation
>   * @set_pub_key: Function invokes the algorithm specific set public key
>   *		function, which knows how to decode and interpret
> - *		the BER encoded public key
> + *		the BER encoded public key and parameters
>   * @set_priv_key: Function invokes the algorithm specific set private key
>   *		function, which knows how to decode and interpret
> - *		the BER encoded private key
> + *		the BER encoded private key and parameters
>   * @max_size:	Function returns dest buffer size required for a given key.
>   * @init:	Initialize the cryptographic transformation object.
>   *		This function is used to initialize the cryptographic
> @@ -388,11 +388,12 @@ static inline int crypto_akcipher_verify(struct akcipher_request *req)
>   * crypto_akcipher_set_pub_key() - Invoke set public key operation
>   *
>   * Function invokes the algorithm specific set key function, which knows
> - * how to decode and interpret the encoded key
> + * how to decode and interpret the encoded key and parameters
>   *
>   * @tfm:	tfm handle
> - * @key:	BER encoded public key
> - * @keylen:	length of the key
> + * @key:	BER encoded public key, algo OID, paramlen, BER encoded
> + *		parameters
> + * @keylen:	length of the key (not including other data)
>   *
>   * Return: zero on success; error code in case of error
>   */
> @@ -409,11 +410,12 @@ static inline int crypto_akcipher_set_pub_key(struct crypto_akcipher *tfm,
>   * crypto_akcipher_set_priv_key() - Invoke set private key operation
>   *
>   * Function invokes the algorithm specific set key function, which knows
> - * how to decode and interpret the encoded key
> + * how to decode and interpret the encoded keya and parameters
>   *
>   * @tfm:	tfm handle
> - * @key:	BER encoded private key
> - * @keylen:	length of the key
> + * @key:	BER encoded private key, algo OID, paramlen, BER encoded
> + *		parameters
> + * @keylen:	length of the key (not including other data)
>   *
>   * Return: zero on success; error code in case of error
>   */
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index be626eac9113..712fe1214b5f 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -15,6 +15,7 @@
>  #define _LINUX_PUBLIC_KEY_H
>  
>  #include <linux/keyctl.h>
> +#include <linux/oid_registry.h>
>  
>  /*
>   * Cryptographic data for the public-key subtype of the asymmetric key type.
> @@ -25,6 +26,9 @@
>  struct public_key {
>  	void *key;
>  	u32 keylen;
> +	enum OID algo;
> +	void *params;
> +	u32 paramlen;
>  	bool key_is_private;
>  	const char *id_type;
>  	const char *pkey_algo;
> -- 
> 2.11.0

Powered by blists - more mailing lists