Thank you Daniel for the review.
> On 27 Aug 2025, at 8:43 PM, Daniel Kiper <dki...@net-space.pl> wrote:
>
> On Mon, Aug 25, 2025 at 04:38:32PM +0530, Sudhakar Kuppusamy wrote:
>> Building on the parsers and the ability to embed X.509 certificates, as
>> well as the existing gcrypt functionality, add a module for verifying
>> appended signatures.
>>
>> This includes a signature verifier that requires that the Linux kernel and
>> GRUB modules have appended signatures for verification.
>>
>> Signature verification must be enabled by setting check_appended_signatures.
>> If secure boot is enabled with enforced mode when the appendedsig
>> module is loaded, signature verification will be enabled, and trusted
>> keys will be extracted from the GRUB ELF Note and stored in the db and
>> locked automatically.
>>
>> Signed-off-by: Daniel Axtens <d...@axtens.net>
>> Signed-off-by: Sudhakar Kuppusamy <sudha...@linux.ibm.com>
>
> Except two nits below Reviewed-by: Daniel Kiper <daniel.ki...@oracle.com>...
>
> [...]
>
>> diff --git a/grub-core/commands/appendedsig/appendedsig.c
>> b/grub-core/commands/appendedsig/appendedsig.c
>> new file mode 100644
>> index 000000000..5eb7b768a
>> --- /dev/null
>> +++ b/grub-core/commands/appendedsig/appendedsig.c
>> @@ -0,0 +1,597 @@
>> +/*
>> + * GRUB -- GRand Unified Bootloader
>> + * Copyright (C) 2020, 2021, 2022 Free Software Foundation, Inc.
>> + * Copyright (C) 2020, 2021, 2022, 2025 IBM Corporation
>> + *
>> + * GRUB is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, either version 3 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * GRUB is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with GRUB. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <grub/types.h>
>> +#include <grub/misc.h>
>> +#include <grub/mm.h>
>> +#include <grub/err.h>
>> +#include <grub/dl.h>
>> +#include <grub/file.h>
>> +#include <grub/command.h>
>> +#include <grub/crypto.h>
>> +#include <grub/i18n.h>
>> +#include <grub/gcrypt/gcrypt.h>
>> +#include <grub/kernel.h>
>> +#include <grub/extcmd.h>
>> +#include <grub/verify.h>
>> +#include <libtasn1.h>
>> +#include <grub/env.h>
>> +#include <grub/lockdown.h>
>> +
>> +#include "appendedsig.h"
>> +
>> +GRUB_MOD_LICENSE ("GPLv3+");
>> +
>> +/* Max size of hash data. */
>> +#define MAX_HASH_SIZE 64
>> +
>> +/* Public key type. */
>> +#define PKEY_ID_PKCS7 2
>> +
>> +/* Appended signature magic string and size. */
>> +#define SIG_MAGIC "~Module signature appended~\n"
>> +#define SIG_MAGIC_SIZE ((sizeof(SIG_MAGIC) - 1))
>> +
>> +/*
>> + * This structure is extracted from scripts/sign-file.c in the linux kernel
>> + * source. It was licensed as LGPLv2.1+, which is GPLv3+ compatible.
>> + */
>> +struct module_signature
>> +{
>> + grub_uint8_t algo; /* Public-key crypto algorithm [0]. */
>> + grub_uint8_t hash; /* Digest algorithm [0]. */
>> + grub_uint8_t id_type; /* Key identifier type [GRUB_PKEY_ID_PKCS7]. */
>> + grub_uint8_t signer_len; /* Length of signer's name [0]. */
>> + grub_uint8_t key_id_len; /* Length of key identifier [0]. */
>> + grub_uint8_t __pad[3];
>> + grub_uint32_t sig_len; /* Length of signature data. */
>> +} GRUB_PACKED;
>> +
>> +#define SIG_METADATA_SIZE (sizeof (struct module_signature))
>> +#define APPENDED_SIG_SIZE(pkcs7_data_size) \
>> + (pkcs7_data_size + SIG_MAGIC_SIZE +
>> SIG_METADATA_SIZE)
>> +
>> +/* This represents an entire, parsed, appended signature. */
>> +struct grub_appended_signature
>> +{
>> + grub_size_t signature_len; /* Length of PKCS#7 data + metadata
>> + magic. */
>> + struct module_signature sig_metadata; /* Module signature metadata. */
>> + struct pkcs7_signedData pkcs7; /* Parsed PKCS#7 data. */
>> +};
>> +
>> +/* This represents a trusted certificates. */
>> +struct grub_database
>> +{
>> + struct x509_certificate *certs; /* Certificates. */
>> + grub_uint32_t cert_entries; /* Number of certificates. */
>> +};
>> +
>> +/* The db list is used to validate appended signatures. */
>> +struct grub_database db = {.certs = NULL, .cert_entries = 0};
>> +
>> +/*
>> + * Signature verification flag (check_sigs).
>> + * check_sigs: false
>> + * - No signature verification. This is the default.
>> + * check_sigs: true
>> + * - Enforce signature verification, and if signature verification fails,
>> + * post the errors and stop the boot.
>> + */
>> +static bool check_sigs = false;
>> +
>> +static grub_ssize_t
>> +pseudo_read (struct grub_file *file, char *buf, grub_size_t len)
>> +{
>> + grub_memcpy (buf, (grub_uint8_t *) file->data + file->offset, len);
>> + return len;
>> +}
>> +
>> +/* Filesystem descriptor. */
>> +static struct grub_fs pseudo_fs = {
>> + .name = "pseudo",
>> + .fs_read = pseudo_read
>> +};
>> +
>> +static void
>> +add_cert_fingerprint (const grub_uint8_t *data, const grub_size_t data_size,
>> + struct x509_certificate *const cert)
>> +{
>> + gcry_md_spec_t *hash_func = NULL;
>> +
>> + /* Add SHA256 hash of certificate. */
>> + hash_func = &_gcry_digest_spec_sha256;
>> + grub_memset (&cert->fingerprint[0], 0, MAX_HASH_SIZE);
>> + grub_crypto_hash (hash_func, &cert->fingerprint[0], data, data_size);
>
> grub_crypto_hash ((gcry_md_spec_t *) &_gcry_digest_spec_sha256, ...
>
> And you can drop hash_func vaiable then…
Sure, will do it.
>
>> +}
>
> [...]
>
>> +static char *
>> +grub_env_write_sec (struct grub_env_var *var __attribute__ ((unused)),
>> const char *val)
>> +{
>> + char *ret;
>> +
>> + /*
>> + * Do not allow the value to be changed If signature verification is
>
> s/If/if/
Sure, will change it.
Thanks,
Sudhakar
>
>> + * (check_sigs is set to enforce) enabled and GRUB is locked down.
>> + */
>> + if (check_sigs == true && grub_is_lockdown () == GRUB_LOCKDOWN_ENABLED)
>> + {
>> + ret = grub_strdup ("enforce");
>> + if (ret == NULL)
>> + grub_error (GRUB_ERR_OUT_OF_MEMORY, "could not duplicate a string
>> enforce");
>> +
>> + return ret;
>> + }
>> +
>> + if ((*val == '1') || (*val == 'e'))
>> + check_sigs = true;
>> + else if ((*val == '0') || (*val == 'n'))
>> + check_sigs = false;
>> +
>> + ret = grub_strdup (grub_env_read_sec (NULL, NULL));
>> + if (ret == NULL)
>> + grub_error (GRUB_ERR_OUT_OF_MEMORY, "could not duplicate a string %s",
>> + grub_env_read_sec (NULL, NULL));
>> +
>> + return ret;
>> +}
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
