linux-kernel - Re: [PATCH v10 5/9] x509: Detect sm2 keys by their parameters OID

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <85b2cdaa-52e1-f519-db85-2ba95dc42b3b@linux.ibm.com>
Date:   Fri, 5 Mar 2021 13:02:09 -0500
From:   Stefan Berger <stefanb@...ux.ibm.com>
To:     Jarkko Sakkinen <jarkko@...nel.org>,
        Stefan Berger <stefanb@...ux.vnet.ibm.com>
Cc:     keyrings@...r.kernel.org, linux-crypto@...r.kernel.org,
        davem@...emloft.net, herbert@...dor.apana.org.au,
        dhowells@...hat.com, zohar@...ux.ibm.com,
        linux-kernel@...r.kernel.org, patrick@...terwijk.org,
        linux-integrity@...r.kernel.org,
        Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>
Subject: Re: [PATCH v10 5/9] x509: Detect sm2 keys by their parameters OID

On 3/5/21 12:16 PM, Jarkko Sakkinen wrote:
> On Thu, Mar 04, 2021 at 07:51:59PM -0500, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@...ux.ibm.com>
>>
>> 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.
>>
>> Cc: David Howells <dhowells@...hat.com>
>> Cc: keyrings@...r.kernel.org
>> Signed-off-by: Stefan Berger <stefanb@...ux.ibm.com>
>> Reviewed-by: Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>
>> ---
>>   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;
> enum is not a real type, and it is hard to recall how much it allocates
> from stack. I would replace this with plain int.


That would be worse considering existing code: 
https://elixir.bootlin.com/linux/latest/source/lib/oid_registry.c#L25


>
>>   
>>   	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 b504e2f36b25..f32d91895e4d 100644
>> --- a/include/linux/oid_registry.h
>> +++ b/include/linux/oid_registry.h
>> @@ -121,6 +121,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)
> An exported function without kdoc.
>
>> +{
>> +	const unsigned char *v = data;
>> +
>> +	if (datasize < 2 || v[0] != ASN1_OID || v[1] != datasize - 2)
>> +		return -EBADMSG;
> '1' and '2' are magic numbers unless you either have constants defining
> them, or at least an inline comment with explanation.


I can add those.