Re: [PATCH 2/2 v2] efi: print appropriate status message when loading certificates

[jlee]

On Wed, Mar 27, 2019 at 03:23:55PM -0400, Mimi Zohar wrote:
> On Sun, 2019-03-24 at 08:26 +0800, Lee, Chun-Yi wrote:
> > When loading certificates list from UEFI variable, the original error
> > message direct shows the efi status code from UEFI firmware. It looks
> > ugly:
> > 
> > [2.335031] Couldn't get size: 0x800e
> > [2.335032] Couldn't get UEFI MokListRT
> > [2.339985] Couldn't get size: 0x800e
> > [2.339987] Couldn't get UEFI dbx list
> > 
> > So, this patch shows the status string instead of status code.
> > 
> > On the other hand, the "Couldn't get UEFI" message doesn't need
> > to be exposed when db/dbx/mok variable do not exist. So, this
> > patch set the message level to debug.
> > 
> > v2.
> > Setting the MODSIGN messagse level to debug.
> > 
> > Link: 
> > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> > Cc: James Morris 
> > Cc: Serge E. Hallyn" 
> > Cc: David Howells 
> > Cc: Nayna Jain 
> > Cc: Josh Boyer 
> > Cc: Mimi Zohar 
> > Signed-off-by: "Lee, Chun-Yi" 
> > ---
> >  security/integrity/platform_certs/load_uefi.c | 13 -
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/security/integrity/platform_certs/load_uefi.c 
> > b/security/integrity/platform_certs/load_uefi.c
> > index 81b19c52832b..e65244b31f04 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -48,7 +48,9 @@ static __init void *get_cert_list(efi_char16_t *name, 
> > efi_guid_t *guid,
> >  
> > status = efi.get_variable(name, guid, NULL, , );
> > if (status != EFI_BUFFER_TOO_SMALL) {
> > -   pr_err("Couldn't get size: 0x%lx\n", status);
> > +   if (status != EFI_NOT_FOUND)
> > +   pr_err("Couldn't get size: %s\n",
> > +   efi_status_to_str(status));
> > return NULL;
> > }
> >  
> > @@ -59,7 +61,8 @@ static __init void *get_cert_list(efi_char16_t *name, 
> > efi_guid_t *guid,
> > status = efi.get_variable(name, guid, NULL, , db);
> > if (status != EFI_SUCCESS) {
> > kfree(db);
> > -   pr_err("Error reading db var: 0x%lx\n", status);
> > +   pr_err("Error reading db var: %s\n",
> > +   efi_status_to_str(status));
> > return NULL;
> > }
> >  
> > @@ -155,7 +158,7 @@ static int __init load_uefi_certs(void)
> > if (!uefi_check_ignore_db()) {
> > db = get_cert_list(L"db", _var, );
> > if (!db) {
> > -   pr_err("MODSIGN: Couldn't get UEFI db list\n");
> > +   pr_debug("MODSIGN: Couldn't get UEFI db list\n");
> 
> Sure, this is fine.
> 
> > } else {
> > rc = parse_efi_signature_list("UEFI:db",
> > db, dbsize, get_handler_for_db);
> > @@ -168,7 +171,7 @@ static int __init load_uefi_certs(void)
> >  
> > mok = get_cert_list(L"MokListRT", _var, );
> > if (!mok) {
> > -   pr_info("Couldn't get UEFI MokListRT\n");
> > +   pr_debug("Couldn't get UEFI MokListRT\n");
> 
> This is fine too.
> 
> > } else {
> > rc = parse_efi_signature_list("UEFI:MokListRT",
> >   mok, moksize, get_handler_for_db);
> > @@ -179,7 +182,7 @@ static int __init load_uefi_certs(void)
> >  
> > dbx = get_cert_list(L"dbx", _var, );
> > if (!dbx) {
> > -   pr_info("Couldn't get UEFI dbx list\n");
> > +   pr_debug("Couldn't get UEFI dbx list\n");
> 
> If there isn't a db or moklist, then this is fine.  My concern is not
> having an indication that the dbx wasn't installed, when it should
> have been.
> 
> Perhaps similar to the "Loading compiled-in X.509 certificates"
> informational message there should informational messages for db, mok,
> and dbx as well.
>

OK. I will add message when kernel found db, dbx and mok. It will
just like the informaton message for ACPI S0,S3,S4 support.

Thanks
Joey Lee

Re: [PATCH 2/2] efi: print appropriate status message when loading certificates

[jlee]

Hi Ard,

On Fri, Mar 22, 2019 at 03:27:46PM +0100, Ard Biesheuvel wrote:
> On Fri, 22 Mar 2019 at 11:34, Lee, Chun-Yi  wrote:
> >
> > When loading certificates list from UEFI variable, the original error
> > message direct shows the efi status code from UEFI firmware. It looks
> > ugly:
> >
> > [2.335031] Couldn't get size: 0x800e
> > [2.335032] Couldn't get UEFI MokListRT
> > [2.339985] Couldn't get size: 0x800e
> > [2.339987] Couldn't get UEFI dbx list
> >
> 
> Why is it an error in the first place if these EFI variables do not exist?
> 

As you said, the error message should only be exposed when
these EFI variables exist. I will change it in next version of
my patch.

Thanks a lot!
Joey Lee

> 
> > So, this patch shows the status string instead of status code.
> >
> > On the other hand, the error message of EFI_NOT_FOUND
> > (0x800e) doesn't need to be exposed because kernel
> > already prints "Couldn't get UEFI..." message. This patch also
> > filtered out it.
> >
> > Link: 
> > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> > Cc: James Morris 
> > Cc: Serge E. Hallyn" 
> > Cc: David Howells 
> > Cc: Nayna Jain 
> > Cc: Josh Boyer 
> > Cc: Mimi Zohar 
> > Signed-off-by: "Lee, Chun-Yi" 
> > ---
> >  security/integrity/platform_certs/load_uefi.c | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/integrity/platform_certs/load_uefi.c 
> > b/security/integrity/platform_certs/load_uefi.c
> > index 81b19c52832b..fe261166621f 100644
> > --- a/security/integrity/platform_certs/load_uefi.c
> > +++ b/security/integrity/platform_certs/load_uefi.c
> > @@ -48,7 +48,9 @@ static __init void *get_cert_list(efi_char16_t *name, 
> > efi_guid_t *guid,
> >
> > status = efi.get_variable(name, guid, NULL, , );
> > if (status != EFI_BUFFER_TOO_SMALL) {
> > -   pr_err("Couldn't get size: 0x%lx\n", status);
> > +   if (status != EFI_NOT_FOUND)
> > +   pr_err("Couldn't get size: %s\n",
> > +   efi_status_to_str(status));
> > return NULL;
> > }
> >
> > @@ -59,7 +61,8 @@ static __init void *get_cert_list(efi_char16_t *name, 
> > efi_guid_t *guid,
> > status = efi.get_variable(name, guid, NULL, , db);
> > if (status != EFI_SUCCESS) {
> > kfree(db);
> > -   pr_err("Error reading db var: 0x%lx\n", status);
> > +   pr_err("Error reading db var: %s\n",
> > +   efi_status_to_str(status));
> > return NULL;
> > }
> >
> > --
> > 2.16.4
> >

Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)

[jlee]

Hi Mimi,

On Thu, Apr 05, 2018 at 10:01:09AM -0400, Mimi Zohar wrote:
> On Thu, 2018-04-05 at 10:16 +0800, joeyli wrote:
> > Hi David, 
> > 
> > On Wed, Apr 04, 2018 at 05:17:24PM +0100, David Howells wrote:
> > > Andy Lutomirski  wrote:
> > > 
> > > > Since this thread has devolved horribly, I'm going to propose a 
> > > > solution.
> > > > 
> > > > 1. Split the "lockdown" state into three levels:  (please don't
> > > > bikeshed about the names right now.)
> > > > 
> > > > LOCKDOWN_NONE: normal behavior
> > > > 
> > > > LOCKDOWN_PROTECT_INTEGREITY: kernel tries to keep root from writing to
> > > > kernel memory
> > > > 
> > > > LOCKDOWN_PROTECT_INTEGRITY_AND_SECRECY: kernel tries to keep root from
> > > > reading or writing kernel memory.
> > > 
> > > In theory, it's good idea, but in practice it's not as easy to implement 
> > > as I
> > > think you think.
> > > 
> > > Let me list here the things that currently get restricted by lockdown:
> > > 
> > [...snip]
> > >  (5) Kexec.
> > >
> > 
> > About IMA with kernel module signing and kexec(not on x86_64 yet)...
> 
> Only carrying the measurement list across kexec is architecture
> specific, but everything else should work.  
> 
> > Because IMA can be used to verify the integrity of kernel module or even
> > the image for kexec. I think that the
> > IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY must be enabled at 
> > runtime
> > when kernel is locked-down.
> 
> I think we need to understand the problem a bit better.  Is the
> problem that you're using the secondary keyring and loading the UEFI
> keys onto the secondary keyring?
>

Sorry for my mistake. I want to write INTEGRITY_TRUSTED_KEYRING in
above but not IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY.

My brain is not clear when writing the mail.
 
> > Because the root can enroll master key to keyring then IMA trusts the ima 
> > key
> > derived from master key. It causes that the arbitrary signed module can be 
> > loaded
> > when the root compromised.
> 
> With only the builtin keyring, only keys signed by a builtin key can
> be added to the IMA keyring.
> 

Thanks for your description. I saw that the IMA_LOAD_X509 already depends
on IMA_TRUSTED_KEYRING (INTEGRITY_TRUSTED_KEYRING). Please ignore my concern.

Thanks a lot!
Joey Lee

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[jlee]

On Fri, Oct 20, 2017 at 05:03:22PM +0100, David Howells wrote:
> j...@suse.com wrote:
> 
> > I think that we don't need to lock down sys_bpf() now because
> > we didn't lock down other interfaces for reading arbitrary
> > address like /dev/mem and /dev/kmem.
> 
> Ummm...  See patch 4.  You even gave me a Reviewed-by for it ;-)
> 
> David

hm... patch 4 only prevents write_mem() but not read_mem().
Or I missed anything?

Thanks
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[jlee]

On Fri, Oct 20, 2017 at 09:08:48AM +0100, David Howells wrote:
> Hi Joey,
> 
> Should I just lock down sys_bpf() entirely for now?  We can always free it up
> somewhat later.
> 
> David

OK~~ Please just remove my patch until we find out a way to
verify bpf code or protect sensitive data in memory.

I think that we don't need to lock down sys_bpf() now because
we didn't lock down other interfaces for reading arbitrary
address like /dev/mem and /dev/kmem.

Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html