On Tue, 28 May 2024 at 17:15, Raymond Mao <raymond....@linaro.org> wrote:
>
> Add porting layer for X509 cert parser on top of MbedTLS X509
> library.
>
> Signed-off-by: Raymond Mao <raymond....@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....@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
