| Message ID | 161428674320.677100.12637282414018170743.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | keys: Add EFI_CERT_X509_GUID support for dbx/mokx entries | expand |
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. > /* Load the MokListRT certs */ > rc = load_moklist_certs(); > > > >
> 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.
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. 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. Regards, Dimitri.
> 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()?
Eric Snowberg <eric.snowberg@oracle.com> wrote: > If MOKx will be available thru a config table in the next shim, > I'll prepare a follow on patch to add this support. Can this go separately, or would it be better rolled into the existing patchset? David
> On Mar 13, 2021, at 1:13 AM, David Howells <dhowells@redhat.com> wrote: > > Eric Snowberg <eric.snowberg@oracle.com> wrote: > >> If MOKx will be available thru a config table in the next shim, >> I'll prepare a follow on patch to add this support. > > Can this go separately, or would it be better rolled into the existing > patchset? IMHO, since you have already sent a pull request and this is not available yet in shim, it seems save to have it go separately. I should have time to send something out next week to address this change.
Eric Snowberg <eric.snowberg@oracle.com> wrote: > > Can this go separately, or would it be better rolled into the existing > > patchset? > > IMHO, since you have already sent a pull request and this is not available > yet in shim, it seems save to have it go separately. I should have time > to send something out next week to address this change. Ok, thanks. David
On Sat, Mar 13, 2021 at 2:40 PM Eric Snowberg <eric.snowberg@oracle.com> wrote: > > > > On Mar 13, 2021, at 1:13 AM, David Howells <dhowells@redhat.com> wrote: > > > > Eric Snowberg <eric.snowberg@oracle.com> wrote: > > > >> If MOKx will be available thru a config table in the next shim, > >> I'll prepare a follow on patch to add this support. > > > > Can this go separately, or would it be better rolled into the existing > > patchset? > > IMHO, since you have already sent a pull request and this is not available > yet in shim, it seems save to have it go separately. I should have time > to send something out next week to address this change. > I don't think that was correct to call it "not available yet in shim". Shim has always exported all the variables, it's just at the time most shims from most distros did not have any contents in MokX. And as usual with all EFI variables, empty ones do not exist. The shim from Ubuntu which is now public contains 1 cert & 378 on x86_64 and 1 cert & 170 hashes. Thus it is likely to fail to be read by kernel correctly unless it uses efi_mokvar_entry_find(). These patches are tagged with CVE number which it does not address completely in some configurations. Are you working on a patch to add efi_mokvar_entry_find() or do you want me to write it / test it / submit it for review?
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); + } + /* Load the MokListRT certs */ rc = load_moklist_certs();