Re: [PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[Eric Snowberg <eric.snowberg@oracle.com>]

> On Mar 12, 2021, at 4:53 PM, Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:
> 
> On 12/03/2021 21:49, Eric Snowberg wrote:
>> 
>>> On Mar 12, 2021, at 11:39 AM, Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:
>>> 
>>> On 25/02/2021 20:59, David Howells wrote:
>>>> From: Eric Snowberg <eric.snowberg@oracle.com>
>>>> 
>>>> During boot the Secure Boot Forbidden Signature Database, dbx,
>>>> is loaded into the blacklist keyring.  Systems booted with shim
>>>> have an equivalent Forbidden Signature Database called mokx.
>>>> Currently mokx is only used by shim and grub, the contents are
>>>> ignored by the kernel.
>>>> 
>>>> Add the ability to load mokx into the blacklist keyring during boot.
>>>> 
>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>>> cc: Jarkko Sakkinen <jarkko@kernel.org>
>>>> Link: https://lore.kernel.org/r/20210122181054.32635-5-eric.snowberg@oracle.com/ # v5
>>>> Link: https://lore.kernel.org/r/c33c8e3839a41e9654f41cc92c7231104931b1d7.camel@HansenPartnership.com/
>>>> ---
>>>> 
>>>> security/integrity/platform_certs/load_uefi.c |   20 ++++++++++++++++++--
>>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
>>>> index ee4b4c666854..f290f78c3f30 100644
>>>> --- a/security/integrity/platform_certs/load_uefi.c
>>>> +++ b/security/integrity/platform_certs/load_uefi.c
>>>> @@ -132,8 +132,9 @@ static int __init load_moklist_certs(void)
>>>> static int __init load_uefi_certs(void)
>>>> {
>>>> 	efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
>>>> -	void *db = NULL, *dbx = NULL;
>>>> -	unsigned long dbsize = 0, dbxsize = 0;
>>>> +	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
>>>> +	void *db = NULL, *dbx = NULL, *mokx = NULL;
>>>> +	unsigned long dbsize = 0, dbxsize = 0, mokxsize = 0;
>>>> 	efi_status_t status;
>>>> 	int rc = 0;
>>>> 
>>>> @@ -175,6 +176,21 @@ static int __init load_uefi_certs(void)
>>>> 		kfree(dbx);
>>>> 	}
>>>> 
>>>> +	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status);
>>>> +	if (!mokx) {
>>>> +		if (status == EFI_NOT_FOUND)
>>>> +			pr_debug("mokx variable wasn't found\n");
>>>> +		else
>>>> +			pr_info("Couldn't get mokx list\n");
>>>> +	} else {
>>>> +		rc = parse_efi_signature_list("UEFI:MokListXRT",
>>>> +					      mokx, mokxsize,
>>>> +					      get_handler_for_dbx);
>>>> +		if (rc)
>>>> +			pr_err("Couldn't parse mokx signatures %d\n", rc);
>>>> +		kfree(mokx);
>>>> +	}
>>>> +
>>> 
>>> 
>>> My preference would be if the above hunk was moved into the
>>> load_moklist_certs() function which is called just below. Such that
>>> loading of MokListRT & MOkListXRT are done next to each other.
>>> 
>>> And also implement loading the same way it is done for MokListRT -
>>> specifically via the EFI MOKvar config table & then via a variable.
>>> 
>>> See 726bd8965a5f112d9601f7ce68effa1e46e02bf2 otherwise large MokListXRT
>>> will fail to parse.
>> 
>> Is this support available from shim now?  Previously I thought only
>> MOK could be loaded from the config table, not MOKx.
>> 
> 
> It is about to become available across all distributions with the next
> shim as everyone is about to ship SBAT capable shims.

When I tested this change, I thought it was around 25+ certs and
hundreds of hashes before shim started having problems. Someone
needing the config list must really have a large list. It would
be nice of the MOKx in shim would support a TBS certificate hash,
it would really save space.

If MOKx will be available thru a config table in the next shim, 
I’ll prepare a follow on patch to add this support. 

> From my system with the next shim & 5.10 kernel I have:
> 
> $ ls /sys/firmware/efi/mok-variables/
> MokIgnoreDB  MokListRT  MokListXRT  MokSBStateRT  SbatRT
> 
> It's not just a single Mok variable, but _all_ mok variables are
> available from the mok-table that are used to determine mok state.
> Including whether or not db should be ignored, whether or not signature
> verification is turned off, and what are the SBAT generation revocations
> are, in addition to MokListRT & MokListXRT.
> 
> For example, kernel could gain further functionality to honor the user
> choices and disable loading db controlled by MokIgnoreDB especially
> since shim chooses to not consider db certificates & hashes as trust-worthy.

Isn’t this already handled by uefi_check_ignore_db()?