On Tue, Jun 10, 2025 at 09:20:47PM +0530, Sudhakar wrote:
> From: Daniel Axtens <d...@axtens.net>
>
> Building on the parsers and the ability to embed x509 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.
>
> Verification must be enabled by setting check_appended_signatures. If
> GRUB is locked down when the module is loaded, verification will be
> enabled and locked automatically.
>
> 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.
>
> 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 | 14 +
> grub-core/commands/appendedsig/appendedsig.c | 624 +++++++++++++++++++
> include/grub/file.h | 2 +
> 3 files changed, 640 insertions(+)
> create mode 100644 grub-core/commands/appendedsig/appendedsig.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index f19071e22..232e97a37 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -979,6 +979,20 @@ module = {
> cppflags = '-I$(srcdir)/lib/posix_wrap';
> };
>
> +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;
> +
> + cflags = '$(CFLAGS_POSIX)';
> + cppflags = '-I$(srcdir)/lib/posix_wrap -I$(srcdir)/lib/libtasn1-grub';
> + depends = asn1, crypto, gcry_rsa, mpi, pkcs1_v15;
> +};
> +
> 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..5fc8e452b
> --- /dev/null
> +++ b/grub-core/commands/appendedsig/appendedsig.c
> @@ -0,0 +1,624 @@
> +/*
> + * 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/pkcs1_v15.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+");
> +
> +static const char magic[] = "~Module signature appended~\n";
> +
> +/*
> + * 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 [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 */
> +};
> +
> +/* Trusted certificates for verifying appended signatures */
> +struct x509_certificate *grub_trusted_key;
> +
> +/*
> + * Force gcry_rsa to be a module dependency.
> + *
> + * If we use grub_crypto_pk_rsa, then then the gcry_rsa module won't be built
> + * in if you add 'appendedsig' to grub-install --modules. You would need to
> + * add 'gcry_rsa' too. That's confusing and seems suboptimal, especially when
> + * we only support RSA.
> + *
> + * Dynamic loading also causes some concerns. We can't load gcry_rsa from the
> + * the filesystem after we install the verifier - we won't be able to verify
> + * it without having it already present. We also shouldn't load it before we
> + * install the verifier, because that would mean it wouldn't be verified - an
> + * attacker could insert any code they wanted into the module.
> + *
> + * So instead, reference the internal symbol from gcry_rsa. That creates a
> + * direct dependency on gcry_rsa, so it will be built in when this module
> + * is built in. Being built in (assuming the core image is itself signed!)
> + * also resolves our concerns about loading from the filesystem.
> + */
> +extern gcry_pk_spec_t _gcry_pubkey_spec_rsa;
This is not nice. Please take a look at the commit 6744840b1 (build:
Track explicit module dependencies in Makefile.core.def) how it should
be done properly.
> +static enum
> +{
> + CHECK_SIGS_NO = 0,
> + CHECK_SIGS_ENFORCE = 1,
> + CHECK_SIGS_FORCED = 2
> +} CHECK_SIGS = CHECK_SIGS_NO;
s/CHECK_SIGS = CHECK_SIGS_NO/check_sigs = CHECK_SIGS_NO/
> +static const char *
> +grub_env_read_sec (struct grub_env_var *var __attribute__ ((unused)),
> + const char *val __attribute__ ((unused)))
> +{
> + if (CHECK_SIGS == CHECK_SIGS_FORCED)
> + return "forced";
> + else if (CHECK_SIGS == CHECK_SIGS_ENFORCE)
> + 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 set to forced */
> + if (CHECK_SIGS == CHECK_SIGS_FORCED)
> + {
> + ret = grub_strdup ("forced");
> + if (ret == NULL)
> + grub_error (GRUB_ERR_OUT_OF_MEMORY, "could not duplicate a string
> forced");
> +
> + return ret;
> + }
> +
> + if ((*val == '2') || (*val == 'f'))
> + CHECK_SIGS = CHECK_SIGS_FORCED;
> + else if ((*val == '1') || (*val == 'e'))
> + CHECK_SIGS = CHECK_SIGS_ENFORCE;
> + else if ((*val == '0') || (*val == 'n'))
> + CHECK_SIGS = CHECK_SIGS_NO;
> +
> + ret = grub_strdup (grub_env_read_sec (NULL, NULL));
> + if (ret == NULL)
> + grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("could not duplicate a string
> %s"),
> + grub_env_read_sec (NULL, NULL));
> +
> + return ret;
> +}
> +
> +static grub_err_t
> +file_read_all (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",
> + 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
> +read_cert_from_file (grub_file_t f, struct x509_certificate *certificate)
> +{
> + grub_err_t err;
> + grub_uint8_t *buf;
> + grub_size_t file_size;
> +
> + err = file_read_all (f, &buf, &file_size);
> + if (err != GRUB_ERR_NONE)
> + return err;
> +
> + err = parse_x509_certificate (buf, file_size, certificate);
> + grub_free (buf);
> +
> + return err;
> +}
> +
> +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);
> +
> + if (bufsize < grub_strlen (magic))
> + return grub_error (GRUB_ERR_BAD_SIGNATURE, "file too short for signature
> magic");
> +
> + if (grub_memcmp (appsigdata, (grub_uint8_t *) magic, grub_strlen (magic)))
if (grub_strncmp (appsigdata, magic, sizeof (magic) - 1))
You compare/expect strings, not binary data...
> + return grub_error (GRUB_ERR_BAD_SIGNATURE, "missing or invalid signature
> magic");
> +
> + remaining_len = bufsize - grub_strlen (magic);
> +
> + 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 != 2)
What id_type 2 means? Please define constant or add a comment before "if".
I think (meaningful) constant would be better...
> + 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;
> + /* rewind pointer and parse pkcs7 data */
> + appsigdata -= pkcs7_size;
> +
> + return parse_pkcs7_signedData (appsigdata, pkcs7_size, &sig->pkcs7);
> +}
[...]
> +static grub_err_t
> +grub_cmd_distrust (grub_command_t cmd __attribute__ ((unused)), int argc,
> char **args)
> +{
> + unsigned long cert_num, i;
> + struct x509_certificate *cert, *prev;
> +
> + if (argc != 1)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "one argument expected");
> +
> + grub_errno = GRUB_ERR_NONE;
> + cert_num = grub_strtoul (args[0], NULL, 10);
> + if (grub_errno != GRUB_ERR_NONE)
Again, this is not reliable. Please fix this.
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + "non-numeric or certificate number is invalid");
> + else if (cert_num <= 0)
How can it be less than zero if it is "unsigned long"?
> + return grub_error (GRUB_ERR_OUT_OF_RANGE,
> + "certificate number too small - numbers start at 1");
> +
> + if (cert_num == 1)
> + {
> + cert = grub_trusted_key;
> + grub_trusted_key = cert->next;
> +
> + certificate_release (cert);
> + grub_free (cert);
> + return GRUB_ERR_NONE;
> + }
> + i = 2;
Where 2 comes from? It begs for a comment.
> + prev = grub_trusted_key;
> + cert = grub_trusted_key->next;
> + while (cert)
Taking into account above probably "for" instead of while() would be
better here.
> + {
> + if (i == cert_num)
> + {
> + prev->next = cert->next;
> + certificate_release (cert);
> + grub_free (cert);
> + return GRUB_ERR_NONE;
> + }
> + i++;
> + prev = cert;
> + cert = cert->next;
> + }
> +
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("no certificate number %lu found - only %lu
> certificates in the store"),
> + cert_num, i - 1);
> +}
> +
> +static grub_err_t
> +grub_cmd_trust (grub_command_t cmd __attribute__ ((unused)), int argc, char
> **args)
> +{
> + grub_file_t certf;
> + struct x509_certificate *cert = NULL;
> + grub_err_t err;
> +
> + if (argc != 1)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "one argument expected");
> +
> + certf = grub_file_open (args[0], GRUB_FILE_TYPE_CERTIFICATE_TRUST |
> GRUB_FILE_TYPE_NO_DECOMPRESS);
> + if (certf == NULL)
> + return grub_error (GRUB_ERR_BAD_FILE_TYPE, N_("could not open %s file"),
> args[0]);
> +
> + cert = grub_zalloc (sizeof (struct x509_certificate));
> + if (cert == NULL)
> + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "could not allocate memory
> for certificate");
> +
> + err = read_cert_from_file (certf, cert);
> + grub_file_close (certf);
> + if (err != GRUB_ERR_NONE)
> + {
> + grub_free (cert);
> + return err;
> + }
> +
> + grub_dprintf ("appendedsig", "loaded certificate with CN: %s\n",
> cert->subject);
> +
> + cert->next = grub_trusted_key;
> + grub_trusted_key = cert;
> +
> + return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_list (grub_command_t cmd __attribute__ ((unused)), int argc
> __attribute__ ((unused)),
> + char **args __attribute__ ((unused)))
> +{
> + struct x509_certificate *cert;
> + int cert_num = 1;
> + grub_size_t i;
> +
> + for (cert = grub_trusted_key; cert; cert = cert->next)
> + {
> + grub_printf ("Certificate %d:\n", cert_num);
> + grub_printf ("\tSerial: ");
> +
> + for (i = 0; i < cert->serial_len - 1; i++)
> + grub_printf ("%02x:", cert->serial[i]);
Something is off with indention here...
> + grub_printf ("%02x\n", cert->serial[cert->serial_len - 1]);
> + grub_printf ("\tCN: %s\n\n", cert->subject);
> + cert_num++;
> + }
> +
> + return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +appendedsig_init (grub_file_t io __attribute__ ((unused)), enum
> grub_file_type type,
> + void **context __attribute__ ((unused)), enum
> grub_verify_flags *flags)
> +{
> + if (CHECK_SIGS == CHECK_SIGS_NO)
> + {
> + *flags = GRUB_VERIFY_FLAGS_SKIP_VERIFICATION;
> + return GRUB_ERR_NONE;
> + }
> +
> + switch (type & GRUB_FILE_TYPE_MASK)
> + {
> + case GRUB_FILE_TYPE_CERTIFICATE_TRUST:
> + /*
> + * This is a certificate to add to trusted keychain.
> + *
> + * This needs to be verified or blocked. Ideally we'd write an x509
> + * verifier, but we lack the hubris required to take this on.
> Instead,
> + * require that it have an appended signature.
> + */
> + case GRUB_FILE_TYPE_LINUX_KERNEL:
> + case GRUB_FILE_TYPE_GRUB_MODULE:
> + /*
> + * Appended signatures are only defined for ELF binaries.
> + * Out of an abundance of caution, we only verify Linux kernels and
> + * GRUB modules at this point.
> + */
> + *flags = GRUB_VERIFY_FLAGS_SINGLE_CHUNK;
> + return GRUB_ERR_NONE;
> +
> + case GRUB_FILE_TYPE_ACPI_TABLE:
> + case GRUB_FILE_TYPE_DEVICE_TREE_IMAGE:
> + /*
> + * It is possible to use appended signature verification without
> + * lockdown - like the PGP verifier. When combined with an embedded
> + * config file in a signed grub binary, this could still be a
> meaningful
s/grub/GRUB/
> + * secure-boot chain - so long as it isn't subverted by something
> like a
> + * rouge ACPI table or DT image. Defer them explicitly.
> + */
> + *flags = GRUB_VERIFY_FLAGS_DEFER_AUTH;
> + return GRUB_ERR_NONE;
> +
> + default:
> + *flags = GRUB_VERIFY_FLAGS_SKIP_VERIFICATION;
> + return GRUB_ERR_NONE;
> + }
> +}
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
