Thank you Daniel.
> On 13 Aug 2025, at 9:12 PM, Daniel Kiper <[email protected]> wrote:
>
> On Tue, Jul 29, 2025 at 08:21:52PM +0530, Sudhakar Kuppusamy wrote:
>> Introducing the following db and dbx commands
>>
>> 1. append_list_db:
>> Show the list of trusted certificates and binary hashes
>> from the db list.
>> 2. append_list_dbx:
>> Show the list of distrusted certificates and binary/certificate
>> hashes from the dbx list.
>> 3. append_add_db_cert:
>> Add the trusted certificate to the db list.
>> 4. append_add_db_hash:
>> Add the trusted binary hash to the db list.
>> 5. append_add_dbx_cert:
>> Add the distrusted certificate to the dbx list.
>> 6. append_add_dbx_hash:
>> Add the distrusted certificate/binary hash to the dbx list.
>>
>> Note that if signature verification (check_appended_signature)
>> is set to enforce,
>> 1. When append_add_db_cert or append_add_dbx_cert executes,
>> then the certificate file must be properly signed.
>> 2. When append_add_db_hash executes, then the binary hash file
>> must be properly signed.
>> 3. When append_add_dbx_hash executes, then the certificate/binary
>> hash file must be signed.
>>
>> Signed-off-by: Sudhakar Kuppusamy <[email protected]>
>> Reviewed-by: Avnish Chouhan <[email protected]>
>> ---
>> grub-core/commands/appendedsig/appendedsig.c | 377 ++++++++++++++++++-
>> include/grub/file.h | 2 +
>> 2 files changed, 362 insertions(+), 17 deletions(-)
>>
>> diff --git a/grub-core/commands/appendedsig/appendedsig.c
>> b/grub-core/commands/appendedsig/appendedsig.c
>> index fa908b963..1d7dc7ba1 100644
>> --- a/grub-core/commands/appendedsig/appendedsig.c
>> +++ b/grub-core/commands/appendedsig/appendedsig.c
>> @@ -42,6 +42,9 @@ GRUB_MOD_LICENSE ("GPLv3+");
>> /* Public key type. */
>> #define GRUB_PKEY_ID_PKCS7 2
>>
>> +#define OPTION_BINARY_HASH 0
>> +#define OPTION_CERT_HASH 1
>> +
>> /* Appended signature magic string. */
>> static const char magic[] = "~Module signature appended~\n";
>>
>> @@ -106,6 +109,13 @@ static grub_size_t append_sig_len = 0;
>> */
>> static bool check_sigs = false;
>>
>> +static const struct grub_arg_option options[] =
>> +{
>> + {"binary-hash", 'b', 0, N_("hash file of the binary."), 0,
>> ARG_TYPE_PATHNAME},
>> + {"cert-hash", 'c', 1, N_("hash file of the certificate."), 0,
>> ARG_TYPE_PATHNAME},
>> + {0, 0, 0, 0, 0, 0}
>> +};
>> +
>> static const char *
>> grub_env_read_sec (struct grub_env_var *var __attribute__ ((unused)),
>> const char *val __attribute__ ((unused)))
>> @@ -648,8 +658,8 @@ is_cert_present_in_db (const struct x509_certificate
>> *cert_in)
>> return false;
>> }
>>
>> -static bool
>> -is_cert_removed_from_db (const struct x509_certificate *cert)
>> +static void
>> +remove_cert_from_db (const struct x509_certificate *cert)
>
> This...
>
>> {
>> int i = 1;
>> struct x509_certificate *curr_cert, *prev_cert;
>> @@ -664,17 +674,64 @@ is_cert_removed_from_db (const struct x509_certificate
>> *cert)
>> prev_cert->next = curr_cert->next;
>>
>> grub_dprintf ("appendedsig",
>> - "removed certificate with CN: %s from the db
>> list\n", curr_cert->subject);
>> + "removed distrused certificate with CN: %s from the
>> db list\n",
>> + curr_cert->subject);
>
> ... this...
>
>> curr_cert->next = NULL;
>> certificate_release (curr_cert);
>> grub_free (curr_cert);
>> - return true;
>> + break;
>
> ... and this change does not belong to this patch…
Sure, will move it to patch 10.
>
>> }
>> else
>> prev_cert = curr_cert;
>>
>> i++;
>> }
>> +}
>> +
>> +/*
>> + * We cannot use hexdump() to display hash data because it is typically
>> + * displayed in hexadecimal format, along with an ASCII representation of
>> + * the same data.
>> + * Example: sha256 hash data
>> + * 00000000 52 b5 90 49 64 de 22 d7 4e 5f 4f b4 1b 51 9c 34
>> |R..Id.".N_O..Q.4|
>> + * 00000010 b1 96 21 7c 91 78 a5 0d 20 8c e9 5c 22 54 53 f7 |..!|.x..
>> ..\"TS.|
>> + *
>> + * An appended signature only required to display the hexadecimal of the
>> hash data
>> + * by separating each byte with ":". So, we introduced a new method
>> dump_ascii_to_hex
>> + * to display it.
>> + * Example: Sha256 hash data
>> + * 52:b5:90:49:64:de:22:d7:4e:5f:4f:b4:1b:51:9c:34:
>> + * b1:96:21:7c:91:78:a5:0d:20:8c:e9:5c:22:54:53:f7
>> + */
>> +static void
>> +dump_ascii_to_hex (const grub_uint8_t *data, const grub_size_t length)
>
> s/dump_ascii_to_hex/dump_data_hex/
Will do it.
>
>> +{
>> + grub_size_t i, count = 0;
>> +
>> + for (i = 0; i < length - 1; i++)
>> + {
>> + grub_printf ("%02x:", data[i]);
>> + count++;
>> + if (count == 16)
>> + {
>> + grub_printf ("\n\t ");
>> + count = 0;
>> + }
>> + }
>> +
>> + grub_printf ("%02x\n", data[i]);
>> +}
>> +
>> +static bool
>> +is_cert_present_in_dbx (const struct x509_certificate *cert_in)
>> +{
>> + struct x509_certificate *cert;
>> +
>> + for (cert = dbx.certs; cert; cert = cert->next)
>> + {
>> + if (is_cert_match (cert, cert_in) == true)
>> + return true;
>> + }
>>
>> return false;
>> }
>> @@ -738,12 +795,21 @@ grub_cmd_db_cert (grub_command_t cmd __attribute__
>> ((unused)), int argc, char **
>> return err;
>> }
>>
>> - if (is_cert_present_in_db (cert) == true)
>> + if (is_cert_present_in_dbx (cert) == false)
>> + {
>> + if (is_cert_present_in_db (cert) == true)
>> + err = grub_error (GRUB_ERR_STILL_REFERENCED,
>
> s/GRUB_ERR_STILL_REFERENCED/GRUB_ERR_EXISTS/? And probably in other
> similar places too…
Sure, will do it.
>
>> + "could not add trusted certificate, as it is
>> present in the db list");
>> + }
>> + else
>> + err = grub_error (GRUB_ERR_STILL_REFERENCED,
>
> s/GRUB_ERR_STILL_REFERENCED/GRUB_ERR_ACCESS_DENIED/? And probably in other
> similar places too…
Sure will do it.
>
>> + "could not add trusted certificate, as it is present
>> in the dbx list");
>> +
>> + if (err != GRUB_ERR_NONE)
>> {
>> certificate_release (cert);
>> grub_free (cert);
>> - return grub_error (GRUB_ERR_STILL_REFERENCED,
>> - "could not add the certificate, as it is present
>> in the db list");
>> + return err;
>> }
>>
>> grub_dprintf ("appendedsig", "added certificate with CN: %s\n",
>> cert->subject);
>> @@ -765,7 +831,7 @@ grub_cmd_dbx_cert (grub_command_t cmd __attribute__
>> ((unused)), int argc, char *
>> if (argc != 1)
>> return grub_error (GRUB_ERR_BAD_ARGUMENT,
>> "a distrusted X.509 certificate file is expected in
>> DER format\n"
>> - "Example:\n\tappend_rm_dbx_cert
>> <X509_CERTIFICATE>\n");
>> + "Example:\n\tappend_add_dbx_cert
>> <X509_CERTIFICATE>\n");
>
> Hmmm... This looks strange for me…
Patches 1 to 10 are static key management, which has only db. So the
append_rm_dbx_cert command removes the given cert from the db. However, patches
11 to 16 are dynamic key management, which has db and dbx, so here the
append_add_dbx_cert command inserts a given cert into dbx and removes it from
the db. So, the command name was changed from "append_rm_dbx_cert" to
"append_add_dbx_cert" in this patch.
>
>>
>> if (*args == NULL)
>> return grub_error (GRUB_ERR_BAD_FILENAME, "missing distrusted X.509
>> certificate file");
>> @@ -780,12 +846,24 @@ grub_cmd_dbx_cert (grub_command_t cmd __attribute__
>> ((unused)), int argc, char *
>> if (err != GRUB_ERR_NONE)
>> grub_free (cert);
>>
>> - if (is_cert_removed_from_db (cert) == false)
>> - err = grub_error (GRUB_ERR_EOF,
>> - "not found certificate with CN:%s in the db list",
>> cert->subject);
>> + if (is_cert_present_in_dbx (cert) == true)
>> + {
>> + certificate_release (cert);
>> + grub_free (cert);
>> + return grub_error (GRUB_ERR_STILL_REFERENCED,
>> + "could not add distrusted certificate, as it is
>> present in the dbx list");
>> + }
>> +
>> + /* remove distrusted certificate from the db list if present. */
>
> s/remove/Remove/
Will do it.
>
>> + remove_cert_from_db (cert);
>> +
>> + grub_dprintf ("appendedsig", "added distrusted certificate with CN: %s to
>> the dbx list\n",
>> + cert->subject);
>> +
>> + cert->next = dbx.certs;
>> + dbx.certs = cert;
>> + dbx.cert_entries++;
>>
>> - certificate_release (cert);
>> - grub_free (cert);
>>
>> return err;
>> }
>> @@ -812,9 +890,255 @@ grub_cmd_list_db (grub_command_t cmd __attribute__
>> ((unused)), int argc __attrib
>> cert_num++;
>> }
>>
>> + for (i = 0; i < db.signature_entries; i++)
>> + {
>> + if (db.signatures[i] != NULL)
>> + {
>> + grub_printf ("Binary hash %" PRIuGRUB_SIZE ":\n", i + 1);
>> + grub_printf ("\thash: ");
>> + dump_ascii_to_hex (db.signatures[i], db.signature_size[i]);
>> + }
>> + }
>> +
>> + return GRUB_ERR_NONE;
>> +}
>> +
>> +static grub_err_t
>> +grub_cmd_list_dbx (grub_command_t cmd __attribute__((unused)),
>> + int argc __attribute__((unused)), char **args
>> __attribute__((unused)))
>> +{
>> + struct x509_certificate *cert;
>> + grub_size_t i, cert_num = 1;
>
> It seems to me this is defined as grub_size_t due to
> dbx.signature_entries being grub_size_t. However, I do not think it
> should be larger than grub_uint32_t. So, please change all of this
> everywhere to grub_uint32_t…
Sure, will do it.
>
>> + for (cert = dbx.certs; cert; cert = cert->next)
>
> for (cert = dbx.certs; cert; cert = cert->next, cert_num++)
>
> ... and you can drop "cert_num++" below…
Sure, will drop it.
>
>> + {
>> + grub_printf ("Certificate %" PRIuGRUB_SIZE ":\n", cert_num);
>> + grub_printf ("\tserial: ");
>> +
>> + for (i = 0; i < cert->serial_len - 1; i++)
>> + grub_printf ("%02x:", cert->serial[i]);
>> +
>> + grub_printf ("%02x\n", cert->serial[cert->serial_len - 1]);
>> + grub_printf ("\tissuer: %s\n", cert->issuer);
>> + grub_printf ("\tCN: %s\n\n", cert->subject);
>> + cert_num++;
>> + }
>> +
>> + for (i = 0; i < dbx.signature_entries; i++)
>> + {
>> + if (dbx.signatures[i] != NULL)
>> + {
>> + grub_printf ("Certificate/Binary hash %" PRIuGRUB_SIZE ":\n", i +
>> 1);
>> + grub_printf ("\thash: ");
>> + dump_ascii_to_hex (dbx.signatures[i], dbx.signature_size[i]);
>> + }
>> + }
>> +
>> return GRUB_ERR_NONE;
>> }
>>
>> +static grub_err_t
>> +read_hash_from_file (char *file_path, grub_uint8_t **hash_data, grub_size_t
>> *hash_data_size)
>> +{
>> + grub_err_t rc;
>> + grub_file_t hash_file;
>> +
>> + hash_file = grub_file_open (file_path, GRUB_FILE_TYPE_HASH_TRUST |
>> GRUB_FILE_TYPE_NO_DECOMPRESS);
>> + if (hash_file == NULL)
>> + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "unable to open %s file",
>> file_path);
>> +
>> + rc = file_read_whole (hash_file, hash_data, hash_data_size);
>> + grub_file_close (hash_file);
>> +
>> + if (check_sigs == true)
>> + *hash_data_size -= append_sig_len;
>
> Please comment this "if"…
Sure, will add comments.
>
>> + return rc;
>> +}
>> +
>> +static bool
>> +is_hash_present_in_db (const grub_uint8_t *hash_data, const grub_size_t
>> hash_data_size)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < db.signature_entries; i++)
>> + {
>> + if (grub_memcmp (db.signatures[i], hash_data, hash_data_size) == 0)
>> + return true;
>> + }
>
> You can drop curly braces here…
Will do it.
>
>> + return false;
>> +}
>> +
>> +static bool
>> +is_hash_present_in_dbx (const grub_uint8_t *hash_data, const grub_size_t
>> hash_data_size)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < dbx.signature_entries; i++)
>> + {
>> + if (grub_memcmp (dbx.signatures[i], hash_data, hash_data_size) == 0)
>> + return true;
>> + }
>
> Ditto…
Will do it.
>
>> + return false;
>> +}
>> +
>> +static void
>> +remove_hash_from_db (const grub_uint8_t *hash_data, const grub_size_t
>> hash_data_size)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < db.signature_entries; i++)
>> + {
>> + if (grub_memcmp (db.signatures[i], hash_data, hash_data_size) == 0)
>> + {
>> + grub_dprintf ("appendedsig", "removed distrusted hash
>> %02x%02x%02x%02x.. from the db list\n",
>> + db.signatures[i][0], db.signatures[i][1],
>> db.signatures[i][2],
>> + db.signatures[i][3]);
>> + grub_free (db.signatures[i]);
>> + db.signatures[i] = NULL;
>> + db.signature_size[i] = 0;
>> + break;
>> + }
>
> Something is off with indention here…
Will correct it.
>
>> + }
>> +}
>> +
>> +static grub_err_t
>> +grub_cmd_db_hash (grub_command_t cmd __attribute__((unused)), int argc,
>> char**args)
>> +{
>> + grub_err_t rc;
>> + grub_uint8_t *hash_data = NULL;
>> + grub_size_t hash_data_size = 0;
>> +
>> + if (argc != 1)
>> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
>> + "a trusted binary hash file is expected in ASCII
>> text format\n"
>> + "Example:\n\tappend_add_db_hash <BINARY HASH
>> FILE>\n");
>> +
>> + if (*args == NULL)
>> + return grub_error (GRUB_ERR_BAD_FILENAME, "missing trusted binary hash
>> file");
>> +
>> + rc = read_hash_from_file (args[0], &hash_data, &hash_data_size);
>> + if (rc == GRUB_ERR_NONE)
>> + {
>> + grub_dprintf ("appendedsig",
>> + "adding a trusted binary hash %02x%02x%02x%02x...\n
>> with size of %" PRIuGRUB_SIZE "\n",
>> + hash_data[0], hash_data[1], hash_data[2], hash_data[3],
>> hash_data_size);
>> +
>> + /* Only accept SHA256, SHA384 and SHA512 binary hash */
>> + if (hash_data_size != 32 && hash_data_size != 48 && hash_data_size !=
>> 64)
>> + {
>> + grub_free (hash_data);
>> + return grub_error (GRUB_ERR_BAD_SIGNATURE, "unacceptable trusted
>> binary hash type");
>> + }
>> +
>> + if (is_hash_present_in_dbx (hash_data, hash_data_size) == true)
>> + {
>> + grub_free (hash_data);
>> + return grub_error (GRUB_ERR_STILL_REFERENCED,
>> + "could not add trusted binary hash, as it is
>> present in the dbx list");
>> + }
>> +
>> + if (is_hash_present_in_db (hash_data, hash_data_size) == false)
>> + {
>> + rc = add_hash ((const grub_uint8_t **) &hash_data,
>> hash_data_size, &db.signatures,
>> + &db.signature_size, &db.signature_entries);
>> + if (rc != GRUB_ERR_NONE)
>> + {
>> + free_db_list ();
>> + free_dbx_list ();
>
> Again, please do not free partial db/dbx... I think this should not be
> done in almost everywhere…
Sure, will do it.
Thanks,
Sudhakar
>
>> + rc = grub_error (rc, "adding of trusted binary hash failed");
>> + }
>> + else
>> + grub_dprintf ("appendedsig",
>> + "added trusted binary hash %02x%02x%02x%02x...\n
>> to the db list\n",
>> + hash_data[0], hash_data[1], hash_data[2],
>> hash_data[3]);
>> + }
>> + else
>> + rc = grub_error (GRUB_ERR_STILL_REFERENCED,
>> + "could not add trusted binary hash, as it is
>> present in the db list");
>> + }
>> +
>> + grub_free (hash_data);
>> +
>> + return rc;
>> +}
>
> Daniel
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
