mbox series

[v6,0/4] Add support for x509 certs with NIST p256 and p192 keys

Message ID 20210131233301.1301787-1-stefanb@linux.ibm.com
Headers show
Series Add support for x509 certs with NIST p256 and p192 keys | expand

Message

Stefan Berger Jan. 31, 2021, 11:32 p.m. UTC
This series of patches adds support for x509 certificates signed by a CA
that uses NIST p256 or p192 keys for signing. It also adds support for
certificates where the public key is a NIST p256 or p192 key. The math
for ECDSA signature verification is also added.

Since self-signed certificates are verified upon loading, the following
script can be used for testing:

k=$(keyctl newring test @u)

while :; do
	for hash in sha1 sha224 sha256 sha384 sha512; do
		openssl req \
			-x509 \
			-${hash} \
			-newkey ec \
			-pkeyopt ec_paramgen_curve:prime256v1 \
			-keyout key.pem \
			-days 365 \
			-subj '/CN=test' \
			-nodes \
			-outform der \
			-out cert.der
		keyctl padd asymmetric testkey $k < cert.der
		if [ $? -ne 0 ]; then
			echo "ERROR"
			exit 1
		fi
	done
done

It also works with restricted keyrings where an RSA key is used to sign
a NIST P256/P192 key. Scripts for testing are here:

https://github.com/stefanberger/eckey-testing

The ECDSA signature verification will be used by IMA Appraisal where ECDSA
file signatures stored in RPM packages will use substantially less space
than if RSA signatures were to be used.

   Stefan

v5->v6:
  - moved ecdsa code into its own module ecdsa_generic built from ecdsa.c
  - added script-generated test vectors for NIST P256 & P192 and all hashes
  - parsing of OID that contain header with new parse_oid()

v4->v5:
  - registering crypto support under names ecdsa-nist-p256/p192 following
    Hubert Xu's suggestion in other thread
  - appended IMA ECDSA support patch

v3->v4:
  - split off of ecdsa crypto part; registering akcipher as "ecdsa" and
    deriving used curve from digits in parsed key

v2->v3:
  - patch 2 now includes linux/scatterlist.h

v1->v2:
  - using faster vli_sub rather than newly added vli_mod_fast to 'reduce'
    result
  - rearranged switch statements to follow after RSA
  - 3rd patch from 1st posting is now 1st patch

Stefan Berger (4):
  crypto: Add support for ECDSA signature verification
  x509: Detect sm2 keys by their parameters OID
  x509: Add support for parsing x509 certs with ECDSA keys
  ima: Support EC keys for signature verification

 crypto/Kconfig                            |  10 +
 crypto/Makefile                           |   6 +
 crypto/asymmetric_keys/public_key.c       |  19 ++
 crypto/asymmetric_keys/x509_cert_parser.c |  44 ++-
 crypto/ecc.c                              |  13 +-
 crypto/ecc.h                              |  28 ++
 crypto/ecdsa.c                            | 361 ++++++++++++++++++++++
 crypto/ecdsasignature.asn1                |   4 +
 crypto/testmgr.c                          |  12 +
 crypto/testmgr.h                          | 267 ++++++++++++++++
 include/keys/asymmetric-type.h            |   6 +
 include/linux/oid_registry.h              |   7 +
 lib/oid_registry.c                        |  13 +
 security/integrity/digsig_asymmetric.c    |  29 +-
 14 files changed, 790 insertions(+), 29 deletions(-)
 create mode 100644 crypto/ecdsa.c
 create mode 100644 crypto/ecdsasignature.asn1

Comments

tianjia.zhang Feb. 1, 2021, 10:36 a.m. UTC | #1
On 2/1/21 7:33 AM, Stefan Berger wrote:
> Add support for IMA signature verification for EC keys. Since SHA type

> of hashes can be used by RSA and ECDSA signature schemes we need to

> look at the key and derive from the key which signature scheme to use.

> Since this can be applied to all types of keys, we change the selection

> of the encoding type to be driven by the key's signature scheme rather

> than by the hash type.

> 

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

> Reviewed-by: Vitaly Chikunov <vt@altlinux.org>

> Cc: Mimi Zohar <zohar@linux.ibm.com>

> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>

> Cc: linux-integrity@vger.kernel.org

> Cc: Vitaly Chikunov <vt@altlinux.org>

> Cc: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>

> Cc: David Howells <dhowells@redhat.com>

> Cc: keyrings@vger.kernel.org

> ---

>   include/keys/asymmetric-type.h         |  6 ++++++

>   security/integrity/digsig_asymmetric.c | 29 ++++++++++++--------------

>   2 files changed, 19 insertions(+), 16 deletions(-)

> 

> diff --git a/include/keys/asymmetric-type.h b/include/keys/asymmetric-type.h

> index a29d3ff2e7e8..c432fdb8547f 100644

> --- a/include/keys/asymmetric-type.h

> +++ b/include/keys/asymmetric-type.h

> @@ -72,6 +72,12 @@ const struct asymmetric_key_ids *asymmetric_key_ids(const struct key *key)

>   	return key->payload.data[asym_key_ids];

>   }

>   

> +static inline

> +const struct public_key *asymmetric_key_public_key(const struct key *key)

> +{

> +	return key->payload.data[asym_crypto];

> +}

> +

>   extern struct key *find_asymmetric_key(struct key *keyring,

>   				       const struct asymmetric_key_id *id_0,

>   				       const struct asymmetric_key_id *id_1,

> diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c

> index a662024b4c70..29002d016607 100644

> --- a/security/integrity/digsig_asymmetric.c

> +++ b/security/integrity/digsig_asymmetric.c

> @@ -84,6 +84,7 @@ int asymmetric_verify(struct key *keyring, const char *sig,

>   {

>   	struct public_key_signature pks;

>   	struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;

> +	const struct public_key *pk;

>   	struct key *key;

>   	int ret;

>   

> @@ -105,23 +106,19 @@ int asymmetric_verify(struct key *keyring, const char *sig,

>   	memset(&pks, 0, sizeof(pks));

>   

>   	pks.hash_algo = hash_algo_name[hdr->hash_algo];

> -	switch (hdr->hash_algo) {

> -	case HASH_ALGO_STREEBOG_256:

> -	case HASH_ALGO_STREEBOG_512:

> -		/* EC-RDSA and Streebog should go together. */

> -		pks.pkey_algo = "ecrdsa";

> -		pks.encoding = "raw";

> -		break;

> -	case HASH_ALGO_SM3_256:

> -		/* SM2 and SM3 should go together. */

> -		pks.pkey_algo = "sm2";

> -		pks.encoding = "raw";

> -		break;

> -	default:

> -		pks.pkey_algo = "rsa";

> +

> +	pk = asymmetric_key_public_key(key);

> +	pks.pkey_algo = pk->pkey_algo;

> +	if (!strcmp(pk->pkey_algo, "rsa"))

>   		pks.encoding = "pkcs1";

> -		break;

> -	}

> +	else if (!strcmp(pk->pkey_algo, "ecdsa"))

> +		pks.encoding = "x962";

> +	else if (!strcmp(pk->pkey_algo, "ecrdsa") ||

> +		   !strcmp(pk->pkey_algo, "sm2"))

> +		pks.encoding = "raw";

> +	else

> +		return -ENOPKG;

> +

>   	pks.digest = (u8 *)data;

>   	pks.digest_size = datalen;

>   	pks.s = hdr->sig;

> 


Looks good to me, so

Reviewed-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>


Thanks,
Tianjia
tianjia.zhang Feb. 1, 2021, 10:39 a.m. UTC | #2
On 2/1/21 7:32 AM, Stefan Berger wrote:
> Detect whether a key is an sm2 type of key by its OID in the parameters

> array rather than assuming that everything under OID_id_ecPublicKey

> is sm2, which is not the case.

> 

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

> Cc: David Howells <dhowells@redhat.com>

> Cc: keyrings@vger.kernel.org

> ---

>   crypto/asymmetric_keys/x509_cert_parser.c | 12 +++++++++++-

>   include/linux/oid_registry.h              |  1 +

>   lib/oid_registry.c                        | 13 +++++++++++++

>   3 files changed, 25 insertions(+), 1 deletion(-)

> 

> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c

> index 52c9b455fc7d..1621ceaf5c95 100644

> --- a/crypto/asymmetric_keys/x509_cert_parser.c

> +++ b/crypto/asymmetric_keys/x509_cert_parser.c

> @@ -459,6 +459,7 @@ int x509_extract_key_data(void *context, size_t hdrlen,

>   			  const void *value, size_t vlen)

>   {

>   	struct x509_parse_context *ctx = context;

> +	enum OID oid;

>   

>   	ctx->key_algo = ctx->last_oid;

>   	switch (ctx->last_oid) {

> @@ -470,7 +471,16 @@ int x509_extract_key_data(void *context, size_t hdrlen,

>   		ctx->cert->pub->pkey_algo = "ecrdsa";

>   		break;

>   	case OID_id_ecPublicKey:

> -		ctx->cert->pub->pkey_algo = "sm2";

> +		if (parse_OID(ctx->params, ctx->params_size, &oid) != 0)

> +			return -EBADMSG;

> +

> +		switch (oid) {

> +		case OID_sm2:

> +			ctx->cert->pub->pkey_algo = "sm2";

> +			break;

> +		default:

> +			return -ENOPKG;

> +		}

>   		break;

>   	default:

>   		return -ENOPKG;

> diff --git a/include/linux/oid_registry.h b/include/linux/oid_registry.h

> index 4462ed2c18cd..d4982e42c0d2 100644

> --- a/include/linux/oid_registry.h

> +++ b/include/linux/oid_registry.h

> @@ -117,6 +117,7 @@ enum OID {

>   };

>   

>   extern enum OID look_up_OID(const void *data, size_t datasize);

> +extern int parse_OID(const void *data, size_t datasize, enum OID *oid);

>   extern int sprint_oid(const void *, size_t, char *, size_t);

>   extern int sprint_OID(enum OID, char *, size_t);

>   

> diff --git a/lib/oid_registry.c b/lib/oid_registry.c

> index f7ad43f28579..508e0b34b5f0 100644

> --- a/lib/oid_registry.c

> +++ b/lib/oid_registry.c

> @@ -11,6 +11,7 @@

>   #include <linux/kernel.h>

>   #include <linux/errno.h>

>   #include <linux/bug.h>

> +#include <linux/asn1.h>

>   #include "oid_registry_data.c"

>   

>   MODULE_DESCRIPTION("OID Registry");

> @@ -92,6 +93,18 @@ enum OID look_up_OID(const void *data, size_t datasize)

>   }

>   EXPORT_SYMBOL_GPL(look_up_OID);

>   

> +int parse_OID(const void *data, size_t datasize, enum OID *oid)

> +{

> +	const unsigned char *v = data;

> +

> +	if (datasize < 2 || v[0] != ASN1_OID || v[1] != datasize - 2)

> +		return -EBADMSG;

> +

> +	*oid = look_up_OID(data + 2, datasize - 2);

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(parse_OID);

> +

>   /*

>    * sprint_OID - Print an Object Identifier into a buffer

>    * @data: The encoded OID to print

> 


Great job, I'm just curious why we need to add a new function, this 
seems unnecessary, if possible, please add

Reviewed-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>


Best regards,
Tianjia
Stefan Berger Feb. 1, 2021, 1:02 p.m. UTC | #3
On 2/1/21 5:39 AM, Tianjia Zhang wrote:
>

>> index f7ad43f28579..508e0b34b5f0 100644

>> --- a/lib/oid_registry.c

>> +++ b/lib/oid_registry.c

>> @@ -11,6 +11,7 @@

>>   #include <linux/kernel.h>

>>   #include <linux/errno.h>

>>   #include <linux/bug.h>

>> +#include <linux/asn1.h>

>>   #include "oid_registry_data.c"

>>     MODULE_DESCRIPTION("OID Registry");

>> @@ -92,6 +93,18 @@ enum OID look_up_OID(const void *data, size_t 

>> datasize)

>>   }

>>   EXPORT_SYMBOL_GPL(look_up_OID);

>>   +int parse_OID(const void *data, size_t datasize, enum OID *oid)

>> +{

>> +    const unsigned char *v = data;

>> +

>> +    if (datasize < 2 || v[0] != ASN1_OID || v[1] != datasize - 2)

>> +        return -EBADMSG;

>> +

>> +    *oid = look_up_OID(data + 2, datasize - 2);

>> +    return 0;

>> +}

>> +EXPORT_SYMBOL_GPL(parse_OID);

>> +

>>   /*

>>    * sprint_OID - Print an Object Identifier into a buffer

>>    * @data: The encoded OID to print

>>

>

> Great job, I'm just curious why we need to add a new function, this 

> seems unnecessary, if possible, please add



Thanks. I call this function in two places now. I thought it was 'worth it'.


>

> Reviewed-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>

>

> Best regards,

> Tianjia