2021-03-05 01:04:25

by Stefan Berger

Subject: [PATCH v10 5/9] x509: Detect sm2 keys by their parameters OID

From: Stefan Berger <[email protected]>

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 <[email protected]>
Cc: [email protected]
Signed-off-by: Stefan Berger <[email protected]>
Reviewed-by: Tianjia Zhang <[email protected]>
---
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 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)
+{
+ 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
--
2.29.2

2021-03-05 07:39:28

by Tianjia Zhang

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

Hi,

On 3/5/21 8:51 AM, Stefan Berger wrote:
> From: Stefan Berger <[email protected]>
>
> 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 <[email protected]>
> Cc: [email protected]
> Signed-off-by: Stefan Berger <[email protected]>
> Reviewed-by: Tianjia Zhang <[email protected]>
> ---
> 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 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)
> +{
> + 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
>

Tested-by: Tianjia Zhang <[email protected]>

Best regards,
Tianjia

2021-03-05 17:18:00

by Jarkko Sakkinen

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

On Thu, Mar 04, 2021 at 07:51:59PM -0500, Stefan Berger wrote:
> From: Stefan Berger <[email protected]>
>
> 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 <[email protected]>
> Cc: [email protected]
> Signed-off-by: Stefan Berger <[email protected]>
> Reviewed-by: Tianjia Zhang <[email protected]>
> ---
> 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.

>
> 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.

> +
> + *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
> --
> 2.29.2
>
>

/Jarkko

2021-03-05 18:03:17

by Stefan Berger

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 <[email protected]>
>>
>> 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 <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Stefan Berger <[email protected]>
>> Reviewed-by: Tianjia Zhang <[email protected]>
>> ---
>> 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.