On Thu, Mar 27, 2025 at 01:02:41AM +0530, Sudhakar Kuppusamy wrote:
> To support the following trusted and distrusted commands
>
> 1. trusted_list:
> It will show the list of trusted certificates and binary hashes
> 2. distrusted_list:
> It will show the list of distrusted certificates and
> binary/certificate hashes
> 3. trusted_certificate:
> It will add the trusted certificate to the trusted list
There are a few bugs in grub_cmd_distrusted_cert().
> 4. trusted_signature:
> It will add the certificate/binary hash to the trusted list
> 5. distrusted_certificate:
> It will remove the trusted certificate from trsuted list
> 6. distrusted_signature:
> It will add the certificate/binary hash to the distrsuted list
>
> Note:-
> The addition/deletion of trusted certificates and binary hashes
> are not allowed in grub command prompt while secure boot is enabled.
>
> Signed-off-by: Sudhakar Kuppusamy <sudha...@linux.ibm.com>
> Reviewed-by: Avnish Chouhan <avn...@linux.ibm.com>
> ---
> grub-core/commands/appendedsig/appendedsig.c | 518 +++++++++++++------
> 1 file changed, 351 insertions(+), 167 deletions(-)
>
> diff --git a/grub-core/commands/appendedsig/appendedsig.c
> b/grub-core/commands/appendedsig/appendedsig.c
> index 5631f0ab4..6f665aab2 100644
> --- a/grub-core/commands/appendedsig/appendedsig.c
> +++ b/grub-core/commands/appendedsig/appendedsig.c
> @@ -117,6 +117,36 @@ static enum
----->8-----
> +
> +static grub_err_t
> +grub_cmd_distrusted_cert (grub_command_t cmd __attribute__((unused)), int
> argc, char **args)
> +{
> + grub_size_t cert_num = 0, i = 1;
> + struct x509_certificate *current_cert = db.keys;
> + struct x509_certificate *previous_cert = db.keys;
> +
> + if (argc != 1)
> + {
> + grub_printf (N_("trusted certificate number is expected\n"
> + "Example:\n\tdistrusted_certificate <CERT_NUMER>\n"));
> + return GRUB_ERR_BAD_ARGUMENT;
> + }
> +
> + if (check_sigs == check_sigs_forced)
> + {
> + grub_printf ("Warning: since secure boot is enabled, "
> + "removing of trusted certificate is not permitted!\n");
> + return grub_errno;
> + }
> +
> + cert_num = grub_strtoul (args[0], NULL, 10);
> + if (cert_num < 1)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("trusted certificate number should to begin with
> 1"));
> +
> + if (cert_num > db.key_entries)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("trusted certificate number should not exceed %"
> PRIuGRUB_SIZE ""),
> + db.key_entries);
> + else if (cert_num < db.key_entries)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> + N_("there is no certificate on the trusted list. so,
> not permitted"));
The 'else if' looks wrong since it's totally fine to have a cert number
smaller than db.key_entries. Per the error message, it should be
'db.key_entries == 0'.
> +
> + for (i = 1; i < db.key_entries; i++)
It has to be 'i <= db.key_entries'. Otherwise, the last certificate in
the list is always ignored.
> + {
> + if (cert_num == 1)
> {
> - grub_printf ("%02x:", cert->serial[i]);
> + previous_cert = current_cert->next;
Changing 'previous_cert' here won't affect 'db.keys', so the next user
of 'db.keys' will get 'current_cert' instead of 'current_cert->next',
and this leads to crash. I'd suggest to add:
db.keys = current_cert->next;
> + break;
> + }
> + else if (cert_num == i)
> + {
> + previous_cert->next = current_cert->next;
> + break;
> }
> - grub_printf ("%02x\n", cert->serial[cert->serial_len - 1]);
>
> - grub_printf ("\tCN: %s\n\n", cert->subject);
> - cert_num++;
> + previous_cert = current_cert;
> + current_cert = current_cert->next;
> }
>
> + certificate_release (current_cert);
> + grub_free (current_cert);
> +
'db.key_entries' wasn't updated in the whole function.
db.key_entries--;
> return GRUB_ERR_NONE;
> }
>
Cheers,
Gary Lin
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
