Thank you Daniel.
> On 11 Aug 2025, at 10:51 PM, Daniel Kiper <dki...@net-space.pl> wrote:
>
> On Tue, Jul 29, 2025 at 08:21:48PM +0530, Sudhakar Kuppusamy wrote:
>> If secure boot is enabled with static key management mode, the trusted
>> certificates will be extracted from the GRUB ELF Note and added to db list.
>> This is introduced by a subsequent patch.
>>
>> If secure boot is enabled with dynamic key management mode, the trusted
>> certificates and certificate/binary hash will be extracted from the PKS
>> and added to db list. The distrusted certificates, certificate/binary hash
>> from the PKS and added to dbx list. Both dbx and db lists are introduced by
>> a subsequent patch.
>>
>> Note:-
>>
>> If the certificate or the certificate hash exists in the dbx list, then
>> do not add that certificate/certificate hash to the db list.
>>
>> 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>
>
> [...]
>
>> +/* Add the certificate into the db/dbx list */
>> +static grub_err_t
>> +add_certificate (const grub_uint8_t *data, const grub_size_t data_size,
>> + struct grub_database *database, const bool is_db)
>> +{
>> + struct x509_certificate *cert;
>> + grub_err_t rc;
>> + grub_size_t cert_entries = database->cert_entries;
>> +
>> + if (data == NULL || data_size == 0)
>> + return grub_error (GRUB_ERR_OUT_OF_RANGE, "certificate data or size is
>> not available");
>> +
>> + cert = grub_zalloc (sizeof (struct x509_certificate));
>> + if (cert == NULL)
>> + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "out of memory");
>> +
>> + rc = parse_x509_certificate (data, data_size, cert);
>> + if (rc != GRUB_ERR_NONE)
>> + {
>> + grub_dprintf ("appendedsig", "skipped %s certificate (%d)\n",
>> + ((is_db == true) ? "trusted" : "distrusted"), (int) rc);
>
> The rc value is meaningless for user because it may change from one GRUB
> version to another. Just add an error message which makes sense here...
Sure. Will do it.
>
>> + grub_free (cert);
>> + return rc;
>> + }
>> +
>> + if (is_db == true)
>> + {
>> + rc = is_dbx_cert (cert);
>> + if (rc != GRUB_ERR_NONE)
>> + {
>> + certificate_release (cert);
>> + grub_free (cert);
>> + return rc;
>> + }
>> + }
>> +
>> + grub_dprintf ("appendedsig", "add a %s certificate CN='%s'\n",
>> + ((is_db == true) ? "trusted" : "distrusted"),
>> cert->subject);
>
> grub_dprintf ("add a certificate CN='%s' to %s", ((is_db == true) ? "db" :
> "dbx"), cert->subject);
>
> I think the error message above should be changed in similar way, e.g.,
> "cannot add a certificate CN='%s' to %s due to an error"...
Sure. Will do it.
>
>> + cert_entries++;
>> + cert->next = database->certs;
>> + database->certs = cert;
>> + database->cert_entries = cert_entries;
>> +
>> + return rc;
>> +}
>> +
>> static grub_err_t
>> file_read_whole (grub_file_t file, grub_uint8_t **buf, grub_size_t *len)
>> {
>> @@ -272,7 +472,7 @@ grub_verify_appended_signature (const grub_uint8_t *buf,
>> grub_size_t bufsize)
>> struct pkcs7_signerInfo *si;
>> int i;
>>
>> - if (db == NULL)
>> + if (!db.cert_entries)
>> return grub_error (GRUB_ERR_BAD_SIGNATURE, "no trusted keys to verify
>> against");
>>
>> err = extract_appended_signature (buf, bufsize, &sig);
>> @@ -303,7 +503,7 @@ grub_verify_appended_signature (const grub_uint8_t *buf,
>> grub_size_t bufsize)
>> grub_dprintf ("appendedsig", "data size %" PRIuGRUB_SIZE ", signer %d
>> hash %02x%02x%02x%02x...\n",
>> datasize, i, hash[0], hash[1], hash[2], hash[3]);
>>
>> - for (pk = db; pk != NULL; pk = pk->next)
>> + for (pk = db.certs; pk != NULL; pk = pk->next)
>> {
>> err = verify_signature (pk->mpis, si->sig_mpi, si->hash, hash);
>> if (err == GRUB_ERR_NONE)
>> @@ -359,7 +559,7 @@ is_cert_present_in_db (const struct x509_certificate
>> *cert_in)
>> {
>> struct x509_certificate *cert;
>>
>> - for (cert = db; cert; cert = cert->next)
>> + for (cert = db.certs; cert; cert = cert->next)
>
> ...; cert != NULL;...??? If yes then it should be changed in patch which
> introduces the file_read_whole() function...
Sure. Will do it.
>
>> {
>> if (is_cert_match (cert, cert_in) == true)
>> return true;
>> @@ -374,12 +574,12 @@ is_cert_removed_from_db (const struct x509_certificate
>> *cert)
>> int i = 1;
>> struct x509_certificate *curr_cert, *prev_cert;
>>
>> - for (curr_cert = prev_cert = db; curr_cert != NULL; curr_cert =
>> curr_cert->next)
>> + for (curr_cert = prev_cert = db.certs; curr_cert != NULL; curr_cert =
>> curr_cert->next)
>> {
>> if (is_cert_match (curr_cert, cert) == true)
>> {
>> if (i == 1) /* Match with first certificate in the db list. */
>> - db = curr_cert->next;
>> + db.certs = curr_cert->next;
>> else
>> prev_cert->next = curr_cert->next;
>>
>> @@ -468,8 +668,9 @@ grub_cmd_db_cert (grub_command_t cmd __attribute__
>> ((unused)), int argc, char **
>>
>> grub_dprintf ("appendedsig", "added certificate with CN: %s\n",
>> cert->subject);
>>
>> - cert->next = db;
>> - db = cert;
>> + cert->next = db.certs;
>> + db.certs = cert;
>> + db.cert_entries++;
>>
>> return GRUB_ERR_NONE;
>> }
>> @@ -517,7 +718,7 @@ grub_cmd_list_db (grub_command_t cmd __attribute__
>> ((unused)), int argc __attrib
>> int cert_num = 1;
>> grub_size_t i;
>>
>> - for (cert = db; cert != NULL; cert = cert->next)
>> + for (cert = db.certs; cert != NULL; cert = cert->next)
>
> ... like here…
Will do it.
>
>> {
>> grub_printf ("Certificate %d:\n", cert_num);
>> grub_printf ("\tSerial: ");
>> @@ -609,6 +810,238 @@ static struct grub_fs pseudo_fs = {
>>
>> static grub_command_t cmd_verify, cmd_list_db, cmd_dbx_cert, cmd_db_cert;
>>
>> +/* Check the certificate hash presence in the PKS dbx list. */
>> +static bool
>> +is_dbx_cert_hash (const grub_uint8_t *data, const grub_size_t data_size)
>> +{
>> + grub_err_t rc;
>> + grub_size_t i, cert_hash_size = 0;
>> + grub_uint8_t cert_hash[GRUB_MAX_HASH_SIZE] = { 0 };
>> +
>> + for (i = 0; i < grub_pks_keystore.dbx_entries; i++)
>> + {
>> + if (grub_pks_keystore.dbx[i].data == NULL ||
>> + grub_pks_keystore.dbx[i].data_size == 0)
>> + continue;
>> +
>> + rc = get_hash (&grub_pks_keystore.dbx[i].guid, data, data_size,
>> + cert_hash, &cert_hash_size);
>> + if (rc != GRUB_ERR_NONE)
>> + continue;
>> +
>> + if (cert_hash_size == grub_pks_keystore.dbx[i].data_size &&
>> + grub_memcmp (grub_pks_keystore.dbx[i].data, cert_hash,
>> cert_hash_size) == 0)
>> + {
>> + grub_dprintf ("appendedsig", "a trusted certificate
>> (%02x%02x%02x%02x) is ignored "
>
> s/trusted//
Will do it.
>
>> + "because this certificate hash is on the dbx
>> list\n",
>> + cert_hash[0], cert_hash[1], cert_hash[2],
>> cert_hash[3]);
>> + return true;
>> + }
>> + }
>> +
>> + return false;
>> +}
>> +
>> +/* Check the binary hash presence in the PKS dbx list. */
>> +static bool
>> +is_dbx_binary_hash (const grub_uint8_t *binary_hash, const grub_size_t
>> binary_hash_size)
>> +{
>> + grub_size_t i;
>> +
>> + for (i = 0; i < grub_pks_keystore.dbx_entries; i++)
>> + {
>> + if (grub_pks_keystore.dbx[i].data == NULL ||
>> + grub_pks_keystore.dbx[i].data_size == 0)
>> + continue;
>> +
>> + if (binary_hash_size == grub_pks_keystore.dbx[i].data_size &&
>> + grub_memcmp (grub_pks_keystore.dbx[i].data, binary_hash,
>> binary_hash_size) == 0)
>> + {
>> + grub_dprintf ("appendedsig", "a trusted binary hash
>> (%02x%02x%02x%02x) is ignored"
>
> s/trusted//
Will do it.
>
>> + " because it is on the dbx list\n", binary_hash[0],
>> binary_hash[1],
>> + binary_hash[2], binary_hash[3]);
>> + return true;
>> + }
>> + }
>> +
>> + return false;
>> +}
>
> [...]
>
>> GRUB_MOD_INIT (appendedsig)
>> {
>> int rc;
>> @@ -621,7 +1054,6 @@ GRUB_MOD_INIT (appendedsig)
>> if (grub_ieee1275_is_secure_boot () == GRUB_SB_ENFORCED)
>> check_sigs = true;
>>
>> - db = NULL;
>> grub_register_variable_hook ("check_appended_signatures",
>> grub_env_read_sec, grub_env_write_sec);
>> grub_env_export ("check_appended_signatures");
>>
>> @@ -630,38 +1062,54 @@ GRUB_MOD_INIT (appendedsig)
>> grub_fatal ("error initing ASN.1 data structures: %d: %s\n", rc,
>> asn1_strerror (rc));
>>
>> /*
>> - * If signature verification is enabled,
>> - * extract trusted keys from ELF Note and store them in the db.
>> + * If signature verification is enabled with static key management mode,
>> + * extract trusted keys from ELF Note and store them in the db list.
>> */
>> - if (check_sigs == true)
>> + if (grub_pks_use_keystore == false && check_sigs == true)
>> {
>> FOR_MODULES (header)
>> - {
>> - struct grub_file pseudo_file;
>> - struct x509_certificate *pk = NULL;
>> - grub_err_t err;
>> -
>> - /* Not an X.509 certificate, skip. */
>> - if (header->type != OBJ_TYPE_X509_PUBKEY)
>> - continue;
>> -
>> - grub_memset (&pseudo_file, 0, sizeof (pseudo_file));
>> - pseudo_file.fs = &pseudo_fs;
>> - pseudo_file.size = header->size - sizeof (struct
>> grub_module_header);
>> - pseudo_file.data = (char *) header + sizeof (struct
>> grub_module_header);
>> -
>> - grub_dprintf ("appendedsig", "found an x509 key, size=%"
>> PRIuGRUB_UINT64_T "\n",
>> - pseudo_file.size);
>> -
>> - err = read_cert_from_file (&pseudo_file, &pk);
>> - if (err != GRUB_ERR_NONE)
>> - grub_fatal ("error loading initial key: %s", grub_errmsg);
>> -
>> - grub_dprintf ("appendedsig", "loaded certificate CN='%s'\n",
>> pk->subject);
>> -
>> - pk->next = db;
>> - db = pk;
>> - }
>> + {
>> + /* Not an ELF module, skip. */
>> + if (header->type != OBJ_TYPE_X509_PUBKEY)
>> + continue;
>> + rc = build_static_db_list (header);
>> + if (rc != GRUB_ERR_NONE)
>> + {
>> + free_db_list ();
>> + grub_error (rc, "static db list creation failed");
>> + }
>> + else
>> + grub_dprintf ("appendedsig", "the db list now has %"
>> PRIuGRUB_SIZE " static keys\n",
>> + db.cert_entries);
>> + }
>> + }
>> + /*
>> + * If signature verification is enabled with dynamic key management mode,
>> + * extract trusted and distrusted keys from PKS and store them in the db
>> and dbx list.
>> + */
>> + else if (grub_pks_use_keystore == true && check_sigs == true)
>> + {
>> + rc = create_db_list ();
>> + if (rc != GRUB_ERR_NONE)
>> + {
>> + free_db_list ();
>
> I would not call free_db_list() here because even partially populated
> list can be useful. Though we should be sure partial or even empty list
> does not lead to crashes.
Sure. Do I need to use partial or empty list even out of memory case?
>
>> + grub_error (rc, "db list creation failed");
>> + }
>> + else
>> + {
>> + rc = create_dbx_list ();
>
> The dbx should be populated regardless of create_db_list() failure.
>
>> + if (rc != GRUB_ERR_NONE)
>> + {
>> + free_db_list ();
>> + free_dbx_list ();
>
> Again, do not free lists in case of errors.
>
>> + grub_error (rc, "dbx list creation failed");
>> + }
>> + else
>> + grub_dprintf ("appendedsig", "the db list now has %"
>> PRIuGRUB_SIZE " keys\n"
>> + "the dbx list now has %" PRIuGRUB_SIZE " keys\n",
>> + db.signature_entries + db.cert_entries,
>> dbx.signature_entries);
>> + }
>> + grub_pks_free_keystore ();
>> }
>>
>> cmd_db_cert = grub_register_command ("append_add_db_cert",
>> grub_cmd_db_cert, N_("X509_CERTIFICATE"),
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
