Thank you Daniel for the review.
> On 21 Aug 2025, at 8:53 PM, Daniel Kiper <dki...@net-space.pl> wrote:
>
> On Thu, Aug 21, 2025 at 01:25:03PM +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 verifier that requires that Linux kernels and
>> GRUB modules have appended signatures, and commands to manage the
>> list of trusted certificates for verification.
>
> This suggests that this patch should be split into two separate ones.
I will split the patch into two separate ones.
>
>> Verification must be enabled by setting check_appended_signatures. If
>> secure boot is enabled with enforced mode when the module is loaded,
>> verification will be enabled and locked automatically. If verification
>> is enabled, extract trusted keys from the GRUB ELF Note and store them in
>> the db.
>>
>> As with the PGP verifier, it is not a complete secure-boot solution:
>> other mechanisms, such as a password or lockdown, must be used to ensure
>> that a user cannot drop to the GRUB shell and disable verification.
>>
>> Introducing the following GRUB commands to access db.
>>
>> 1. append_list_db:
>> Show the list of trusted certificates from the db list
>> 2. append_add_db_cert:
>> Add the trusted certificate to the db list
>> 3. append_rm_dbx_cert:
>> Remove the distrusted certificate from the db list
>> 4. append_verify:
>> Verify the signed file using db list
>>
>> Signed-off-by: Daniel Axtens <d...@axtens.net>
>> Signed-off-by: Sudhakar Kuppusamy <sudha...@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stef...@linux.ibm.com>
>> Reviewed-by: Avnish Chouhan <avn...@linux.ibm.com>
>> ---
>> grub-core/Makefile.core.def | 15 +
>> grub-core/commands/appendedsig/appendedsig.c | 793 +++++++++++++++++++
>> include/grub/err.h | 3 +-
>> include/grub/file.h | 2 +
>> 4 files changed, 812 insertions(+), 1 deletion(-)
>> create mode 100644 grub-core/commands/appendedsig/appendedsig.c
>>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index b72f322b1..d91694de0 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -980,6 +980,21 @@ module = {
>> cppflags = '$(CPPFLAGS_GCRY)';
>> };
>>
>> +module = {
>> + name = appendedsig;
>> + common = commands/appendedsig/appendedsig.c;
>> + common = commands/appendedsig/x509.c;
>> + common = commands/appendedsig/pkcs7.c;
>> + common = commands/appendedsig/asn1util.c;
>> + common = commands/appendedsig/gnutls_asn1_tab.c;
>> + common = commands/appendedsig/pkix_asn1_tab.c;
>> + enable = emu;
>> + enable = powerpc_ieee1275;
>> + cflags = '$(CFLAGS_GCRY) -Wno-redundant-decls';
>> + cppflags = '$(CPPFLAGS_GCRY) -I$(srcdir)/lib/libtasn1-grub';
>> + depends = crypto, gcry_rsa, gcry_sha256, gcry_sha512, mpi, asn1;
>> +};
>> +
>> module = {
>> name = hdparm;
>> common = commands/hdparm.c;
>> diff --git a/grub-core/commands/appendedsig/appendedsig.c
>> b/grub-core/commands/appendedsig/appendedsig.c
>> new file mode 100644
>> index 000000000..2ddbb3a15
>> --- /dev/null
>> +++ b/grub-core/commands/appendedsig/appendedsig.c
>> @@ -0,0 +1,793 @@
>> +/*
>> + * 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+");
>> +
>> +/* Public key type. */
>> +#define GRUB_PKEY_ID_PKCS7 2
>> +
>> +/* Appended signature magic string. */
>> +static const char magic[] = "~Module signature appended~\n";
>
> This could be a constant... Two useful constants...
>
> #define SIG_MAGIC "~Module signature appended~\n"
> #define SIG_MAGIC_SIZE ((sizeof(SIG_MAGIC) - 1)
Sure, will do it.
>
>> +/*
>> + * 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;
>> +
>> +/* 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};
>> +
>> +/* Appended signature size. */
>> +static grub_size_t append_sig_len = 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 void
>> +register_appended_signatures_cmd (void);
>> +static void
>> +unregister_appended_signatures_cmd (void);
>> +static void
>> +free_db_list (void);
>> +static void
>> +build_static_db_list (void);
>> +
>> +static const char *
>> +grub_env_read_sec (struct grub_env_var *var __attribute__ ((unused)),
>> + const char *val __attribute__ ((unused)))
>> +{
>> + if (check_sigs == true)
>> + return "enforce";
>> +
>> + return "no";
>> +}
>> +
>> +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
>> + * (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;
>> +}
>> +
>> +static bool
>> +is_cert_match (const struct x509_certificate *distrusted_cert,
>> + const struct x509_certificate *db_cert)
>> +{
>> + if (grub_memcmp (distrusted_cert->subject, db_cert->subject,
>> db_cert->subject_len) == 0
>> + && grub_memcmp (distrusted_cert->issuer, db_cert->issuer,
>> db_cert->issuer_len) == 0
>> + && grub_memcmp (distrusted_cert->serial, db_cert->serial,
>> db_cert->serial_len) == 0
>> + && grub_memcmp (distrusted_cert->mpis[0], db_cert->mpis[0], sizeof
>> (db_cert->mpis[0])) == 0
>> + && grub_memcmp (distrusted_cert->mpis[1], db_cert->mpis[1], sizeof
>> (db_cert->mpis[1])) == 0)
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +static grub_err_t
>> +file_read_whole (grub_file_t file, grub_uint8_t **buf, grub_size_t *len)
>> +{
>> + grub_off_t full_file_size;
>> + grub_size_t file_size, total_read_size = 0;
>> + grub_ssize_t read_size;
>> +
>> + full_file_size = grub_file_size (file);
>> + if (full_file_size == GRUB_FILE_SIZE_UNKNOWN)
>> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
>> + "cannot read a file of unknown size into a buffer");
>> +
>> + if (full_file_size > GRUB_SIZE_MAX)
>> + return grub_error (GRUB_ERR_OUT_OF_RANGE,
>> + "file is too large to read: %" PRIuGRUB_UINT64_T "
>> bytes",
>
> s/PRIuGRUB_UINT64_T/PRIuGRUB_OFFSET/
will change it.
>
>> + full_file_size);
>> +
>> + file_size = (grub_size_t) full_file_size;
>> + *buf = grub_malloc (file_size);
>> + if (*buf == NULL)
>> + return grub_error (GRUB_ERR_OUT_OF_MEMORY,
>> + "could not allocate file data buffer size %"
>> PRIuGRUB_SIZE,
>> + file_size);
>> +
>> + while (total_read_size < file_size)
>> + {
>> + read_size = grub_file_read (file, *buf + total_read_size, file_size -
>> total_read_size);
>> + if (read_size < 0)
>> + {
>> + grub_free (*buf);
>> + return grub_errno;
>> + }
>> + else if (read_size == 0)
>> + {
>> + grub_free (*buf);
>> + return grub_error (GRUB_ERR_IO,
>> + "could not read full file size "
>> + "(%" PRIuGRUB_SIZE "), only %" PRIuGRUB_SIZE "
>> bytes read",
>> + file_size, total_read_size);
>> + }
>> +
>> + total_read_size += read_size;
>> + }
>> +
>> + *len = file_size;
>> +
>> + return GRUB_ERR_NONE;
>> +}
>> +
>> +static grub_err_t
>> +extract_appended_signature (const grub_uint8_t *buf, grub_size_t bufsize,
>> + struct grub_appended_signature *sig)
>> +{
>> + grub_size_t pkcs7_size;
>> + grub_size_t remaining_len;
>> + const grub_uint8_t *appsigdata = buf + bufsize - grub_strlen (magic);
>
> s/grub_strlen (magic)/sizeof (magic) - 1/
>
>> +
>> + if (bufsize < grub_strlen (magic))
>
> s/grub_strlen (magic)/sizeof (magic) - 1/
>
> Please be more consistent…
Sure, will do it.
>
>> + return grub_error (GRUB_ERR_BAD_SIGNATURE, "file too short for
>> signature magic");
>> +
>> + if (grub_strncmp ((const char *) appsigdata, magic, sizeof (magic) - 1))
>> + return grub_error (GRUB_ERR_BAD_SIGNATURE, "missing or invalid
>> signature magic");
>> +
>> + remaining_len = bufsize - grub_strlen (magic);
>
> Ditto...
>
> Or even define a constant, as above, and use it everywhere...
Sure, will do it.
>
>> + if (remaining_len < sizeof (struct module_signature))
>> + return grub_error (GRUB_ERR_BAD_SIGNATURE, "file too short for
>> signature metadata");
>> +
>> + appsigdata -= sizeof (struct module_signature);
>> + /* Extract the metadata. */
>> + grub_memcpy (&(sig->sig_metadata), appsigdata, sizeof (struct
>> module_signature));
>> + remaining_len -= sizeof (struct module_signature);
>> +
>> + if (sig->sig_metadata.id_type != GRUB_PKEY_ID_PKCS7)
>> + return grub_error (GRUB_ERR_BAD_SIGNATURE, "wrong signature type");
>> +
>> + pkcs7_size = grub_be_to_cpu32 (sig->sig_metadata.sig_len);
>> +
>> + if (pkcs7_size > remaining_len)
>> + return grub_error (GRUB_ERR_BAD_SIGNATURE, "file too short for PKCS#7
>> message");
>> +
>> + grub_dprintf ("appendedsig", "sig len %" PRIuGRUB_SIZE "\n", pkcs7_size);
>> +
>> + sig->signature_len = grub_strlen (magic) + sizeof (struct
>> module_signature) + pkcs7_size;
>
> Ditto…
>
>> + /* Rewind pointer and parse pkcs7 data. */
>> + appsigdata -= pkcs7_size;
>> +
>> + return parse_pkcs7_signedData (appsigdata, pkcs7_size, &sig->pkcs7);
>> +}
>> +
>> +/*
>> + * Given a hash value 'hval', of hash specification 'hash', prepare
>> + * the S-expressions (sexp) and perform the signature verification.
>> + */
>> +static grub_err_t
>> +verify_signature (const gcry_mpi_t *pkmpi, const gcry_mpi_t hmpi,
>> + const gcry_md_spec_t *hash, const grub_uint8_t *hval)
>> +{
>> + gcry_sexp_t hsexp, pubkey, sig;
>> + grub_size_t errof;
>> +
>> + if (_gcry_sexp_build(&hsexp, &errof, "(data (flags %s) (hash %s %b))",
>> "pkcs1",
>> + hash->name, hash->mdlen, hval) != GPG_ERR_NO_ERROR)
>
> Missing space after "_gcry_sexp_build" here and below…
Sure, will do it.
Thanks,
Sudhakar
>
>> + return GRUB_ERR_BAD_SIGNATURE;
>> +
>> + if (_gcry_sexp_build(&pubkey, &errof, "(public-key (dsa (n %M) (e %M)))",
>> + pkmpi[0], pkmpi[1]) != GPG_ERR_NO_ERROR)
>> + return GRUB_ERR_BAD_SIGNATURE;
>> +
>> + if (_gcry_sexp_build(&sig, &errof, "(sig-val (rsa (s %M)))", hmpi) !=
>> GPG_ERR_NO_ERROR)
>> + return GRUB_ERR_BAD_SIGNATURE;
>> +
>> + _gcry_sexp_dump(sig);
>> + _gcry_sexp_dump(hsexp);
>> + _gcry_sexp_dump(pubkey);
>> +
>> + if (grub_crypto_pk_rsa->verify (sig, hsexp, pubkey) != GPG_ERR_NO_ERROR)
>> + return GRUB_ERR_BAD_SIGNATURE;
>> +
>> + return GRUB_ERR_NONE;
>> +}
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
