[PATCH v3 15/25] mbedtls: add X509 cert parser porting layer

[Ilias Apalodimas]

On Tue, 28 May 2024 at 17:15, Raymond Mao <raymond.mao at linaro.org> wrote:
>
> Add porting layer for X509 cert parser on top of MbedTLS X509
> library.
>
> Signed-off-by: Raymond Mao <raymond.mao at linaro.org>
> ---
> Changes in v2
> - Move the porting layer to MbedTLS dir.
> Changes in v3
> - None.
>
>  lib/mbedtls/Makefile           |   1 +
>  lib/mbedtls/x509_cert_parser.c | 497 +++++++++++++++++++++++++++++++++
>  2 files changed, 498 insertions(+)
>  create mode 100644 lib/mbedtls/x509_cert_parser.c
>
> diff --git a/lib/mbedtls/Makefile b/lib/mbedtls/Makefile
> index cd0144eac1c..e7cba1ad17c 100644
> --- a/lib/mbedtls/Makefile
> +++ b/lib/mbedtls/Makefile
> @@ -24,6 +24,7 @@ hash_mbedtls-$(CONFIG_$(SPL_)SHA512) += sha512.o
>  # x509 libraries
>  obj-$(CONFIG_MBEDTLS_LIB_X509) += x509_mbedtls.o
>  x509_mbedtls-$(CONFIG_$(SPL_)ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += public_key.o
> +x509_mbedtls-$(CONFIG_$(SPL_)X509_CERTIFICATE_PARSER) += x509_cert_parser.o
>
>  obj-$(CONFIG_MBEDTLS_LIB_CRYPTO) += mbedtls_lib_crypto.o
>  mbedtls_lib_crypto-y := \
> diff --git a/lib/mbedtls/x509_cert_parser.c b/lib/mbedtls/x509_cert_parser.c
> new file mode 100644
> index 00000000000..b0867d31047
> --- /dev/null
> +++ b/lib/mbedtls/x509_cert_parser.c
> @@ -0,0 +1,497 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * X509 cert parser using MbedTLS X509 library
> + *
> + * Copyright (c) 2024 Linaro Limited
> + * Author: Raymond Mao <raymond.mao at linaro.org>
> + */
> +
> +#include <linux/err.h>
> +#include <crypto/public_key.h>
> +#include <crypto/x509_parser.h>
> +
> +static void x509_free_mbedtls_ctx(struct x509_cert_mbedtls_ctx *ctx)
> +{

Switch the logic here so we avoid the extra indentation
if (!ctx)
    return;

kfree(...)
etc

> +       if (ctx) {
> +               kfree(ctx->tbs);
> +               kfree(ctx->raw_serial);
> +               kfree(ctx->raw_issuer);
> +               kfree(ctx->raw_subject);
> +               kfree(ctx->raw_skid);
> +               kfree(ctx);
> +       }
> +}
> +
> +static int x509_set_cert_flags(struct x509_certificate *cert)
> +{
> +       struct public_key_signature *sig = cert->sig;
> +
> +       if (!sig || !cert->pub) {
> +               pr_err("Signature or public key is not initialized\n");
> +               return -ENOPKG;
> +       }
> +
> +       if (!cert->pub->pkey_algo)
> +               cert->unsupported_key = true;
> +
> +       if (!sig->pkey_algo)
> +               cert->unsupported_sig = true;
> +
> +       if (!sig->hash_algo)
> +               cert->unsupported_sig = true;
> +
> +       /* TODO: is_hash_blacklisted()? */

Is this supported by our current implementation?

> +
> +       /* Detect self-signed certificates and set self_signed flag */
> +       return x509_check_for_self_signed(cert);
> +}
> +
> +/*
> + * Check for self-signedness in an X.509 cert and if found, check the signature
> + * immediately if we can.
> + */
> +int x509_check_for_self_signed(struct x509_certificate *cert)
> +{
> +       int ret = 0;
> +
> +       if (cert->raw_subject_size != cert->raw_issuer_size ||
> +           memcmp(cert->raw_subject, cert->raw_issuer,
> +                  cert->raw_issuer_size) != 0)

We usually do !memcp

> +               goto not_self_signed;
> +
> +       if (cert->sig->auth_ids[0] || cert->sig->auth_ids[1]) {
> +               /* If the AKID is present it may have one or two parts.  If
> +                * both are supplied, both must match.
> +                */

The comment needs an empty line at the start

> +               bool a = asymmetric_key_id_same(cert->skid, cert->sig->auth_ids[1]);
> +               bool b = asymmetric_key_id_same(cert->id, cert->sig->auth_ids[0]);
> +
> +               if (!a && !b)
> +                       goto not_self_signed;
> +
> +               ret = -EKEYREJECTED;
> +               if (((a && !b) || (b && !a)) &&
> +                   cert->sig->auth_ids[0] && cert->sig->auth_ids[1])
> +                       goto out;
> +       }
> +
> +       ret = -EKEYREJECTED;
> +       if (strcmp(cert->pub->pkey_algo, cert->sig->pkey_algo) != 0)
> +               goto out;

!strcmp

> +
> +       ret = public_key_verify_signature(cert->pub, cert->sig);
> +       if (ret < 0) {
> +               if (ret == -ENOPKG) {
> +                       cert->unsupported_sig = true;
> +                       ret = 0;

goto not_self_signed; is a bit easier to read.
I know we already do that in the existing code, but is it correct to
treat a cert as not self-signed?
We will return -ENOPKG if it's not an RSA cert or the algorithm is unsupported.

> +               }
> +               goto out;
> +       }
> +
> +       pr_devel("Cert Self-signature verified");
> +       cert->self_signed = true;
> +
> +out:
> +       return ret;
> +
> +not_self_signed:
> +       return 0;
> +}

the whole function looks like a copy of lib/crypto/x509_public_key.c.
Can you move all the c/p ones to a common file that the existing and
mbedTLS implementations can use?

[..]

Thanks
/Ilias