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

2021-03-05 Thread Stefan Berger 

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 

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 
Cc: keyri...@vger.kernel.org
Signed-off-by: Stefan Berger 
Reviewed-by: Tianjia Zhang 
---
  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 
  #include 
  #include 
+#include 
  #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.

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

2021-03-05 Thread Jarkko Sakkinen 
On Thu, Mar 04, 2021 at 07:51:59PM -0500, Stefan Berger wrote:
> From: Stefan Berger 
> 
> 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 
> Cc: keyri...@vger.kernel.org
> Signed-off-by: Stefan Berger 
> Reviewed-by: Tianjia Zhang 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  #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

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

2021-03-04 Thread Tianjia Zhang 

Hi,

On 3/5/21 8:51 AM, Stefan Berger wrote:

From: Stefan Berger 

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 
Cc: keyri...@vger.kernel.org
Signed-off-by: Stefan Berger 
Reviewed-by: Tianjia Zhang 
---
  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 
  #include 
  #include 
+#include 
  #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 

Best regards,
Tianjia

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

2021-03-04 Thread Stefan Berger 
From: Stefan Berger 

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 
Cc: keyri...@vger.kernel.org
Signed-off-by: Stefan Berger 
Reviewed-by: Tianjia Zhang 
---
 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 
 #include 
 #include 
+#include 
 #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