linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
@ 2018-08-15 10:00 Yannik Sembritzki
  2018-08-15 16:54 ` Linus Torvalds
  0 siblings, 1 reply; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 10:00 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel
  Cc: Yannik Sembritzki

---
 arch/x86/kernel/kexec-bzimage64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 7326078e..eaaa125d 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
 static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
 {
 	return verify_pefile_signature(kernel, kernel_len,
-				       NULL,
+				       (void *)1UL,
 				       VERIFYING_KEXEC_PE_SIGNATURE);
 }
 #endif
-- 
2.17.1

The exact scenario under which this issue occurs is described here:
https://bugzilla.redhat.com/show_bug.cgi?id=1554113


^ permalink raw reply related	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 10:00 [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot Yannik Sembritzki
@ 2018-08-15 16:54 ` Linus Torvalds
  2018-08-15 17:27   ` Yannik Sembritzki
  0 siblings, 1 reply; 57+ messages in thread
From: Linus Torvalds @ 2018-08-15 16:54 UTC (permalink / raw)
  To: yannik, David Howells, Vivek Goyal
  Cc: Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List

This needs more people involved, and at least a sign-off.

It looks ok, but I think we need a #define for the magical (void *)1UL
thing. I see the use in verify_pkcs7_signature(), but still.

              Linus



On Wed, Aug 15, 2018 at 3:11 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
>
> ---
>  arch/x86/kernel/kexec-bzimage64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 7326078e..eaaa125d 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
>  static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
>  {
>         return verify_pefile_signature(kernel, kernel_len,
> -                                      NULL,
> +                                      (void *)1UL,
>                                        VERIFYING_KEXEC_PE_SIGNATURE);
>  }
>  #endif
> --
> 2.17.1
>
> The exact scenario under which this issue occurs is described here:
> https://bugzilla.redhat.com/show_bug.cgi?id=1554113
>

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 16:54 ` Linus Torvalds
@ 2018-08-15 17:27   ` Yannik Sembritzki
  2018-08-15 17:37     ` Yannik Sembritzki
                       ` (2 more replies)
  0 siblings, 3 replies; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 17:27 UTC (permalink / raw)
  To: Linus Torvalds, David Howells, Vivek Goyal
  Cc: Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List

Would this be okay?

diff --git a/arch/x86/kernel/kexec-bzimage64.c
b/arch/x86/kernel/kexec-bzimage64.c
index 7326078e..2ba47e24 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -41,6 +41,9 @@
 #define MIN_KERNEL_LOAD_ADDR   0x100000
 #define MIN_INITRD_LOAD_ADDR   0x1000000
 
+// Allow both builtin trusted keys and secondary trusted keys
+#define TRUST_FULL_KEYRING     (void *)1UL
+
 /*
  * This is a place holder for all boot loader specific data structure which
  * gets allocated in one call but gets freed much later during cleanup
@@ -532,7 +535,7 @@ static int bzImage64_cleanup(void *loader_data)
 static int bzImage64_verify_sig(const char *kernel, unsigned long
kernel_len)
 {
        return verify_pefile_signature(kernel, kernel_len,
-                                      NULL,
+                                      TRUST_FULL_KEYRING,
                                       VERIFYING_KEXEC_PE_SIGNATURE);
 }
 #endif
--

On 15.08.2018 18:54, Linus Torvalds wrote:
> This needs more people involved, and at least a sign-off.
>
> It looks ok, but I think we need a #define for the magical (void *)1UL
> thing. I see the use in verify_pkcs7_signature(), but still.
>
>               Linus
>
>
>
> On Wed, Aug 15, 2018 at 3:11 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
>> ---
>>  arch/x86/kernel/kexec-bzimage64.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
>> index 7326078e..eaaa125d 100644
>> --- a/arch/x86/kernel/kexec-bzimage64.c
>> +++ b/arch/x86/kernel/kexec-bzimage64.c
>> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
>>  static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
>>  {
>>         return verify_pefile_signature(kernel, kernel_len,
>> -                                      NULL,
>> +                                      (void *)1UL,
>>                                        VERIFYING_KEXEC_PE_SIGNATURE);
>>  }
>>  #endif
>> --
>> 2.17.1
>>
>> The exact scenario under which this issue occurs is described here:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1554113
>>


^ permalink raw reply related	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 17:27   ` Yannik Sembritzki
@ 2018-08-15 17:37     ` Yannik Sembritzki
  2018-08-15 17:42     ` Vivek Goyal
  2018-08-15 17:45     ` Linus Torvalds
  2 siblings, 0 replies; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 17:37 UTC (permalink / raw)
  To: Linus Torvalds, David Howells, Vivek Goyal
  Cc: Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List

I'm sorry, the sign-off was missing again (this is my first submission
to linux).

Signed-off-by: Yannik Sembritzki <yannik@sembritzki.me>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/kexec-bzimage64.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/kexec-bzimage64.c
b/arch/x86/kernel/kexec-bzimage64.c
index 7326078e..2ba47e24 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -41,6 +41,9 @@
 #define MIN_KERNEL_LOAD_ADDR    0x100000
 #define MIN_INITRD_LOAD_ADDR    0x1000000
 
+// Allow both builtin trusted keys and secondary trusted keys
+#define TRUST_FULL_KEYRING    (void *)1UL
+
 /*
  * This is a place holder for all boot loader specific data structure which
  * gets allocated in one call but gets freed much later during cleanup
@@ -532,7 +535,7 @@ static int bzImage64_cleanup(void *loader_data)
 static int bzImage64_verify_sig(const char *kernel, unsigned long
kernel_len)
 {
     return verify_pefile_signature(kernel, kernel_len,
-                       NULL,
+                       TRUST_FULL_KEYRING,
                        VERIFYING_KEXEC_PE_SIGNATURE);
 }
 #endif
-- 
2.17.1

On 15.08.2018 19:27, Yannik Sembritzki wrote:
> Would this be okay?
>
> diff --git a/arch/x86/kernel/kexec-bzimage64.c
> b/arch/x86/kernel/kexec-bzimage64.c
> index 7326078e..2ba47e24 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -41,6 +41,9 @@
>  #define MIN_KERNEL_LOAD_ADDR   0x100000
>  #define MIN_INITRD_LOAD_ADDR   0x1000000
>  
> +// Allow both builtin trusted keys and secondary trusted keys
> +#define TRUST_FULL_KEYRING     (void *)1UL
> +
>  /*
>   * This is a place holder for all boot loader specific data structure which
>   * gets allocated in one call but gets freed much later during cleanup
> @@ -532,7 +535,7 @@ static int bzImage64_cleanup(void *loader_data)
>  static int bzImage64_verify_sig(const char *kernel, unsigned long
> kernel_len)
>  {
>         return verify_pefile_signature(kernel, kernel_len,
> -                                      NULL,
> +                                      TRUST_FULL_KEYRING,
>                                        VERIFYING_KEXEC_PE_SIGNATURE);
>  }
>  #endif
> --
>
> On 15.08.2018 18:54, Linus Torvalds wrote:
>> This needs more people involved, and at least a sign-off.
>>
>> It looks ok, but I think we need a #define for the magical (void *)1UL
>> thing. I see the use in verify_pkcs7_signature(), but still.
>>
>>               Linus
>>
>>
>>
>> On Wed, Aug 15, 2018 at 3:11 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
>>> ---
>>>  arch/x86/kernel/kexec-bzimage64.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
>>> index 7326078e..eaaa125d 100644
>>> --- a/arch/x86/kernel/kexec-bzimage64.c
>>> +++ b/arch/x86/kernel/kexec-bzimage64.c
>>> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
>>>  static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
>>>  {
>>>         return verify_pefile_signature(kernel, kernel_len,
>>> -                                      NULL,
>>> +                                      (void *)1UL,
>>>                                        VERIFYING_KEXEC_PE_SIGNATURE);
>>>  }
>>>  #endif
>>> --
>>> 2.17.1
>>>
>>> The exact scenario under which this issue occurs is described here:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1554113
>>>


^ permalink raw reply related	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 17:27   ` Yannik Sembritzki
  2018-08-15 17:37     ` Yannik Sembritzki
@ 2018-08-15 17:42     ` Vivek Goyal
  2018-08-15 18:44       ` Yannik Sembritzki
                         ` (3 more replies)
  2018-08-15 17:45     ` Linus Torvalds
  2 siblings, 4 replies; 57+ messages in thread
From: Vivek Goyal @ 2018-08-15 17:42 UTC (permalink / raw)
  To: Yannik Sembritzki
  Cc: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin M. Forbes

On Wed, Aug 15, 2018 at 07:27:33PM +0200, Yannik Sembritzki wrote:
> Would this be okay?

[ CC dave young, Baoquan, Justin Forbes]

Hi Yannik,

I am reading that bug and wondering that what broke it. It used to work,
so some change broke it. 

Justin said that we have been signing fedora kernels with fedora keys so
looks like no change there.

Previously, I think all the keys used to go in system keyring and it
used to work. Is it somehow because of split in builtin keyring and
secondary system keyring. Could it be that fedora key used to show
up in system keyring previously and it worked but now it shows up
in secondary system keyring and by default we don't use keys from
that keyring for signature verification?

Thanks
Vivek

> 
> diff --git a/arch/x86/kernel/kexec-bzimage64.c
> b/arch/x86/kernel/kexec-bzimage64.c
> index 7326078e..2ba47e24 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -41,6 +41,9 @@
>  #define MIN_KERNEL_LOAD_ADDR   0x100000
>  #define MIN_INITRD_LOAD_ADDR   0x1000000
>  
> +// Allow both builtin trusted keys and secondary trusted keys
> +#define TRUST_FULL_KEYRING     (void *)1UL
> +
>  /*
>   * This is a place holder for all boot loader specific data structure which
>   * gets allocated in one call but gets freed much later during cleanup
> @@ -532,7 +535,7 @@ static int bzImage64_cleanup(void *loader_data)
>  static int bzImage64_verify_sig(const char *kernel, unsigned long
> kernel_len)
>  {
>         return verify_pefile_signature(kernel, kernel_len,
> -                                      NULL,
> +                                      TRUST_FULL_KEYRING,
>                                        VERIFYING_KEXEC_PE_SIGNATURE);
>  }
>  #endif
> --
> 
> On 15.08.2018 18:54, Linus Torvalds wrote:
> > This needs more people involved, and at least a sign-off.
> >
> > It looks ok, but I think we need a #define for the magical (void *)1UL
> > thing. I see the use in verify_pkcs7_signature(), but still.
> >
> >               Linus
> >
> >
> >
> > On Wed, Aug 15, 2018 at 3:11 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
> >> ---
> >>  arch/x86/kernel/kexec-bzimage64.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> >> index 7326078e..eaaa125d 100644
> >> --- a/arch/x86/kernel/kexec-bzimage64.c
> >> +++ b/arch/x86/kernel/kexec-bzimage64.c
> >> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> >>  static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
> >>  {
> >>         return verify_pefile_signature(kernel, kernel_len,
> >> -                                      NULL,
> >> +                                      (void *)1UL,
> >>                                        VERIFYING_KEXEC_PE_SIGNATURE);
> >>  }
> >>  #endif
> >> --
> >> 2.17.1
> >>
> >> The exact scenario under which this issue occurs is described here:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1554113
> >>
> 

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 17:27   ` Yannik Sembritzki
  2018-08-15 17:37     ` Yannik Sembritzki
  2018-08-15 17:42     ` Vivek Goyal
@ 2018-08-15 17:45     ` Linus Torvalds
  2018-08-15 18:19       ` Yannik Sembritzki
  2 siblings, 1 reply; 57+ messages in thread
From: Linus Torvalds @ 2018-08-15 17:45 UTC (permalink / raw)
  To: yannik
  Cc: David Howells, Vivek Goyal, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List

On Wed, Aug 15, 2018 at 10:27 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
>
> Would this be okay?

No, I meant that it would have to go into the proper header files, and
also be used by verify_pkcs7_signature() and pkcs7_preparse() etc, so
that you could actually grep for this, and understand what it does.

Right now you have to know about the magic. Or follow the call chain
down and look, like I did.

Side note, it should probably be

  #define TRUST_FULL_KEYRING ((struct key *)1ul)

instead, so that it is also type-safe (using "void *" means that it
would work almost anywhere, but it really should be a "struct key *".

And I'd like to see a comment from the kexec people too, I guess.

             Linus

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 17:45     ` Linus Torvalds
@ 2018-08-15 18:19       ` Yannik Sembritzki
  2018-08-15 18:22         ` Linus Torvalds
  0 siblings, 1 reply; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 18:19 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Howells, Vivek Goyal, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List

> No, I meant that it would have to go into the proper header files, and
> also be used by verify_pkcs7_signature() and pkcs7_preparse() etc, so
> that you could actually grep for this, and understand what it does.
Thanks, Linus, I'll take care of this right away.

This is my first patch and I'm not familiar with the kernel; can you
give me a quick hint which header file(s) would be the right place for
this #define?

Yannik

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 18:19       ` Yannik Sembritzki
@ 2018-08-15 18:22         ` Linus Torvalds
  2018-08-15 19:42           ` [PATCH 0/2] Fix kexec forbidding kernels signed with keys in the secondary keyring " Yannik Sembritzki
                             ` (2 more replies)
  0 siblings, 3 replies; 57+ messages in thread
From: Linus Torvalds @ 2018-08-15 18:22 UTC (permalink / raw)
  To: yannik
  Cc: David Howells, Vivek Goyal, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List

On Wed, Aug 15, 2018 at 11:19 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
>
> > No, I meant that it would have to go into the proper header files, and
> > also be used by verify_pkcs7_signature() and pkcs7_preparse() etc, so
> > that you could actually grep for this, and understand what it does.
> Thanks, Linus, I'll take care of this right away.
>
> This is my first patch and I'm not familiar with the kernel; can you
> give me a quick hint which header file(s) would be the right place for
> this #define?

I think

    include/linux/verification.h

is the right point, it's where verify_pkcs7_signature() is declared
too (and "struct key" is forward-declared), so it would seem to make
most sense there.

             Linus

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 17:42     ` Vivek Goyal
@ 2018-08-15 18:44       ` Yannik Sembritzki
  2018-08-15 18:58       ` Vivek Goyal
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 18:44 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin M. Forbes


> I am reading that bug and wondering that what broke it. It used to work,
> so some change broke it. 
>
> Previously, I think all the keys used to go in system keyring and it
> used to work. Is it somehow because of split in builtin keyring and
> secondary system keyring. Could it be that fedora key used to show
> up in system keyring previously and it worked but now it shows up
> in secondary system keyring and by default we don't use keys from
> that keyring for signature verification?
The fedora kernel is signed by the "Fedora Secure Boot CA", which is not
in the builtin keyring, but in the secondary keyring. So yes, that is
why kexec fails.
If this did indeed work earlier on, either "Fedora Secure Boot CA" was
in the system keyring previously, or the kernel was signed by the
"Fedora kernel signing key" (which is in the builtin keyring).

The fix I've introduced in my patch is necessary for everyone who uses
their own platform key in any case.

Yannik

>
>> diff --git a/arch/x86/kernel/kexec-bzimage64.c
>> b/arch/x86/kernel/kexec-bzimage64.c
>> index 7326078e..2ba47e24 100644
>> --- a/arch/x86/kernel/kexec-bzimage64.c
>> +++ b/arch/x86/kernel/kexec-bzimage64.c
>> @@ -41,6 +41,9 @@
>>  #define MIN_KERNEL_LOAD_ADDR   0x100000
>>  #define MIN_INITRD_LOAD_ADDR   0x1000000
>>  
>> +// Allow both builtin trusted keys and secondary trusted keys
>> +#define TRUST_FULL_KEYRING     (void *)1UL
>> +
>>  /*
>>   * This is a place holder for all boot loader specific data structure which
>>   * gets allocated in one call but gets freed much later during cleanup
>> @@ -532,7 +535,7 @@ static int bzImage64_cleanup(void *loader_data)
>>  static int bzImage64_verify_sig(const char *kernel, unsigned long
>> kernel_len)
>>  {
>>         return verify_pefile_signature(kernel, kernel_len,
>> -                                      NULL,
>> +                                      TRUST_FULL_KEYRING,
>>                                        VERIFYING_KEXEC_PE_SIGNATURE);
>>  }
>>  #endif
>> --
>>
>> On 15.08.2018 18:54, Linus Torvalds wrote:
>>> This needs more people involved, and at least a sign-off.
>>>
>>> It looks ok, but I think we need a #define for the magical (void *)1UL
>>> thing. I see the use in verify_pkcs7_signature(), but still.
>>>
>>>               Linus
>>>
>>>
>>>
>>> On Wed, Aug 15, 2018 at 3:11 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
>>>> ---
>>>>  arch/x86/kernel/kexec-bzimage64.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
>>>> index 7326078e..eaaa125d 100644
>>>> --- a/arch/x86/kernel/kexec-bzimage64.c
>>>> +++ b/arch/x86/kernel/kexec-bzimage64.c
>>>> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
>>>>  static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
>>>>  {
>>>>         return verify_pefile_signature(kernel, kernel_len,
>>>> -                                      NULL,
>>>> +                                      (void *)1UL,
>>>>                                        VERIFYING_KEXEC_PE_SIGNATURE);
>>>>  }
>>>>  #endif
>>>> --
>>>> 2.17.1
>>>>
>>>> The exact scenario under which this issue occurs is described here:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1554113
>>>>


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 17:42     ` Vivek Goyal
  2018-08-15 18:44       ` Yannik Sembritzki
@ 2018-08-15 18:58       ` Vivek Goyal
  2018-08-15 19:06         ` Yannik Sembritzki
  2018-08-16  0:52       ` Dave Young
  2018-08-16 12:13       ` David Howells
  3 siblings, 1 reply; 57+ messages in thread
From: Vivek Goyal @ 2018-08-15 18:58 UTC (permalink / raw)
  To: Yannik Sembritzki
  Cc: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin M. Forbes, Peter Jones,
	James Bottomley, Matthew Garrett

On Wed, Aug 15, 2018 at 01:42:47PM -0400, Vivek Goyal wrote:
> On Wed, Aug 15, 2018 at 07:27:33PM +0200, Yannik Sembritzki wrote:
> > Would this be okay?
> 
> [ CC dave young, Baoquan, Justin Forbes]
> 
> Hi Yannik,
> 
> I am reading that bug and wondering that what broke it. It used to work,
> so some change broke it. 
> 
> Justin said that we have been signing fedora kernels with fedora keys so
> looks like no change there.
> 
> Previously, I think all the keys used to go in system keyring and it
> used to work. Is it somehow because of split in builtin keyring and
> secondary system keyring. Could it be that fedora key used to show
> up in system keyring previously and it worked but now it shows up
> in secondary system keyring and by default we don't use keys from
> that keyring for signature verification?

I do strongly suspect it is due to the fact that .system_keyring got
split into .builtin_trusted_keys and .secondary_trusted_keys.

commit d3bfe84129f65e0af2450743ebdab33d161d01c9
Author: David Howells <dhowells@redhat.com>
Date:   Wed Apr 6 16:14:27 2016 +0100

    certs: Add a secondary system keyring that can be added to dynamically

int verify_pkcs7_signature(const void *data, size_t len,
        if (ret < 0)
                goto error;
 
-       if (!trusted_keys)
-               trusted_keys = system_trusted_keyring;
+       if (!trusted_keys) {
+               trusted_keys = builtin_trusted_keys;
+       } else if (trusted_keys == (void *)1UL) {


Previously we defaulted to using system_trusted_keyring, and that
contained both builtin keys and all the keys we imported from UEFI db,
MokList variable etc.

Now this patch changed it to trusting builtin_trusted_keys by default,
and all the other keys go to secondary_trusted_keys kerying. And that
probably explains why it broke.

So checking for keys in both the keyrings makes sense to me. 

I am wondering why did we have to split this keyring to begin with. 
So there are use cases where we want to trust builtin keys but
not the ones which came from other places (UEFI secure boot db, or
user loaded one)?

I am finding code in upstream kernel to add keys to secondary
keyring. I do see extra patches we are carrying in fedora which
add keys to secondary keyring from various sources.

Thinking loud to make sure if it safe to trust keys in .secondary_trusted_keys
keyring and there are no gotchas. I don't know secureboot and UEFI
parts that well. I have copied Peter Jones, James Bottomley and Matthew Garret
to cc as well, just to see if they can think of any issues in trusting
keys from .secondary_trusted_keys keyring for kernel signature
verification.

Thanks
Vivek

> 
> > 
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c
> > b/arch/x86/kernel/kexec-bzimage64.c
> > index 7326078e..2ba47e24 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -41,6 +41,9 @@
> >  #define MIN_KERNEL_LOAD_ADDR   0x100000
> >  #define MIN_INITRD_LOAD_ADDR   0x1000000
> >  
> > +// Allow both builtin trusted keys and secondary trusted keys
> > +#define TRUST_FULL_KEYRING     (void *)1UL
> > +
> >  /*
> >   * This is a place holder for all boot loader specific data structure which
> >   * gets allocated in one call but gets freed much later during cleanup
> > @@ -532,7 +535,7 @@ static int bzImage64_cleanup(void *loader_data)
> >  static int bzImage64_verify_sig(const char *kernel, unsigned long
> > kernel_len)
> >  {
> >         return verify_pefile_signature(kernel, kernel_len,
> > -                                      NULL,
> > +                                      TRUST_FULL_KEYRING,
> >                                        VERIFYING_KEXEC_PE_SIGNATURE);
> >  }
> >  #endif
> > --
> > 
> > On 15.08.2018 18:54, Linus Torvalds wrote:
> > > This needs more people involved, and at least a sign-off.
> > >
> > > It looks ok, but I think we need a #define for the magical (void *)1UL
> > > thing. I see the use in verify_pkcs7_signature(), but still.
> > >
> > >               Linus
> > >
> > >
> > >
> > > On Wed, Aug 15, 2018 at 3:11 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
> > >> ---
> > >>  arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > >> index 7326078e..eaaa125d 100644
> > >> --- a/arch/x86/kernel/kexec-bzimage64.c
> > >> +++ b/arch/x86/kernel/kexec-bzimage64.c
> > >> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> > >>  static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
> > >>  {
> > >>         return verify_pefile_signature(kernel, kernel_len,
> > >> -                                      NULL,
> > >> +                                      (void *)1UL,
> > >>                                        VERIFYING_KEXEC_PE_SIGNATURE);
> > >>  }
> > >>  #endif
> > >> --
> > >> 2.17.1
> > >>
> > >> The exact scenario under which this issue occurs is described here:
> > >> https://bugzilla.redhat.com/show_bug.cgi?id=1554113
> > >>
> > 

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 18:58       ` Vivek Goyal
@ 2018-08-15 19:06         ` Yannik Sembritzki
  2018-08-15 19:49           ` Vivek Goyal
  0 siblings, 1 reply; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 19:06 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin M. Forbes, Peter Jones,
	James Bottomley, Matthew Garrett


> I am wondering why did we have to split this keyring to begin with. 
> So there are use cases where we want to trust builtin keys but
> not the ones which came from other places (UEFI secure boot db, or
> user loaded one)?
>
"User loaded ones" should not be trusted in general to prevent rootkits
and similar from modifying the kernel (even if they have root).

According to the patch which introduced the secondary keyring (the one
you mentioned), the requirements for adding keys to the secondary
keyring are as follows:
"Add a secondary system keyring that can be added to by root whilst the
system is running - provided the key being added is vouched for by a key
built into the kernel or already added to the secondary keyring."

I personally don't see a reason for this split, as the requirements for
the secondary keyring are as strict as it can get. However, I'm new to
this, so feel free to correct me.

Yannik


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 0/2] Fix kexec forbidding kernels signed with keys in the secondary keyring to boot
  2018-08-15 18:22         ` Linus Torvalds
@ 2018-08-15 19:42           ` Yannik Sembritzki
  2018-08-16 18:02             ` Linus Torvalds
  2018-08-15 19:42           ` [PATCH 1/2] " Yannik Sembritzki
  2018-08-15 19:42           ` [PATCH 2/2] Replace magic for trusting the secondary keyring with #define Yannik Sembritzki
  2 siblings, 1 reply; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 19:42 UTC (permalink / raw)
  To: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin M. Forbes, Peter Jones,
	James Bottomley, Matthew Garrett, Vivek Goyal
  Cc: Yannik Sembritzki

I've written two patches for (a) the logical change of allowing kernels
signed with keys in the secondary keyring to be kexec'd (b) the refactoring
of the magic 1UL Linus requested.

Yannik Sembritzki (2):
  Fix kexec forbidding kernels signed with keys in the secondary keyring
    to boot
  Replace magic for trusting the secondary keyring with #define

 arch/x86/kernel/kexec-bzimage64.c       | 2 +-
 certs/system_keyring.c                  | 3 ++-
 crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
 include/linux/verification.h            | 4 ++++
 4 files changed, 8 insertions(+), 3 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 57+ messages in thread

* [PATCH 1/2] Fix kexec forbidding kernels signed with keys in the secondary keyring to boot
  2018-08-15 18:22         ` Linus Torvalds
  2018-08-15 19:42           ` [PATCH 0/2] Fix kexec forbidding kernels signed with keys in the secondary keyring " Yannik Sembritzki
@ 2018-08-15 19:42           ` Yannik Sembritzki
  2018-08-15 19:42           ` [PATCH 2/2] Replace magic for trusting the secondary keyring with #define Yannik Sembritzki
  2 siblings, 0 replies; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 19:42 UTC (permalink / raw)
  To: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin M. Forbes, Peter Jones,
	James Bottomley, Matthew Garrett, Vivek Goyal
  Cc: Yannik Sembritzki, stable

The split of .system_keyring to .builtin_trusted_keys and .secondary_trusted_keys broke kexec, as kernels signed by keys which are now in the secondary keyring cannot be kexec'd anymore.

Signed-off-by: Yannik Sembritzki <yannik@sembritzki.me>
CC: stable@vger.kernel.org
---
 arch/x86/kernel/kexec-bzimage64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 7326078e..74628275 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
 static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
 {
 	return verify_pefile_signature(kernel, kernel_len,
-				       NULL,
+				       ((struct key *)1UL),
 				       VERIFYING_KEXEC_PE_SIGNATURE);
 }
 #endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 57+ messages in thread

* [PATCH 2/2] Replace magic for trusting the secondary keyring with #define
  2018-08-15 18:22         ` Linus Torvalds
  2018-08-15 19:42           ` [PATCH 0/2] Fix kexec forbidding kernels signed with keys in the secondary keyring " Yannik Sembritzki
  2018-08-15 19:42           ` [PATCH 1/2] " Yannik Sembritzki
@ 2018-08-15 19:42           ` Yannik Sembritzki
  2018-08-15 21:14             ` kbuild test robot
  2 siblings, 1 reply; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 19:42 UTC (permalink / raw)
  To: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin M. Forbes, Peter Jones,
	James Bottomley, Matthew Garrett, Vivek Goyal
  Cc: Yannik Sembritzki

Signed-off-by: Yannik Sembritzki <yannik@sembritzki.me>
---
 arch/x86/kernel/kexec-bzimage64.c       | 2 +-
 certs/system_keyring.c                  | 3 ++-
 crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
 include/linux/verification.h            | 4 ++++
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 74628275..97d199a3 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
 static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
 {
 	return verify_pefile_signature(kernel, kernel_len,
-				       ((struct key *)1UL),
+				       TRUST_SECONDARY_KEYRING,
 				       VERIFYING_KEXEC_PE_SIGNATURE);
 }
 #endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b2..777ac7d2 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -15,6 +15,7 @@
 #include <linux/cred.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+#include <linux/verification.h>
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
 #include <crypto/pkcs7.h>
@@ -230,7 +231,7 @@ int verify_pkcs7_signature(const void *data, size_t len,
 
 	if (!trusted_keys) {
 		trusted_keys = builtin_trusted_keys;
-	} else if (trusted_keys == (void *)1UL) {
+	} else if (trusted_keys == TRUST_SECONDARY_KEYRING) {
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 		trusted_keys = secondary_trusted_keys;
 #else
diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c b/crypto/asymmetric_keys/pkcs7_key_type.c
index e284d9cb..0783e555 100644
--- a/crypto/asymmetric_keys/pkcs7_key_type.c
+++ b/crypto/asymmetric_keys/pkcs7_key_type.c
@@ -63,7 +63,7 @@ static int pkcs7_preparse(struct key_preparsed_payload *prep)
 
 	return verify_pkcs7_signature(NULL, 0,
 				      prep->data, prep->datalen,
-				      (void *)1UL, usage,
+				      TRUST_SECONDARY_KEYRING, usage,
 				      pkcs7_view_content, prep);
 }
 
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6..5a7f2053 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -12,6 +12,10 @@
 #ifndef _LINUX_VERIFICATION_H
 #define _LINUX_VERIFICATION_H
 
+// Allow both builtin trusted keys and secondary trusted keys
+#ifndef TRUST_SECONDARY_KEYRING
+#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
+
 /*
  * The use to which an asymmetric key is being put.
  */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 19:06         ` Yannik Sembritzki
@ 2018-08-15 19:49           ` Vivek Goyal
  2018-08-15 20:47             ` Linus Torvalds
  2018-08-16 13:51             ` David Howells
  0 siblings, 2 replies; 57+ messages in thread
From: Vivek Goyal @ 2018-08-15 19:49 UTC (permalink / raw)
  To: Yannik Sembritzki
  Cc: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin M. Forbes, Peter Jones,
	James Bottomley, Matthew Garrett

On Wed, Aug 15, 2018 at 09:06:13PM +0200, Yannik Sembritzki wrote:
> 
> > I am wondering why did we have to split this keyring to begin with. 
> > So there are use cases where we want to trust builtin keys but
> > not the ones which came from other places (UEFI secure boot db, or
> > user loaded one)?
> >
> "User loaded ones" should not be trusted in general to prevent rootkits
> and similar from modifying the kernel (even if they have root).
> 
> According to the patch which introduced the secondary keyring (the one
> you mentioned), the requirements for adding keys to the secondary
> keyring are as follows:
> "Add a secondary system keyring that can be added to by root whilst the
> system is running - provided the key being added is vouched for by a key
> built into the kernel or already added to the secondary keyring."
> 

Right.

So it will become a question of should we trust a key which is
possibly dynamically loaded into the kernel, and which has been
trusted by an existing key. So this sounds like extending chain of
trust to a key which is dynamically loaded later. I feels reasonable
to me to extend chain of trust for kexec kernel. (Until and unless
somebody has a use case in mind where this is not a good idea).

I see that module signing code trusts only builtin keys and
not the keys in secondary_trusted_keys keyring.

Dave, what's the reason behind having two keyrings. Is it because
module signing code does not want to trust keys other than built-in
ones?

Thanks
Vivek



> I personally don't see a reason for this split, as the requirements for
> the secondary keyring are as strict as it can get. However, I'm new to
> this, so feel free to correct me.
> 
> Yannik
> 

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 19:49           ` Vivek Goyal
@ 2018-08-15 20:47             ` Linus Torvalds
  2018-08-15 20:53               ` James Bottomley
  2018-08-15 21:08               ` Yannik Sembritzki
  2018-08-16 13:51             ` David Howells
  1 sibling, 2 replies; 57+ messages in thread
From: Linus Torvalds @ 2018-08-15 20:47 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: yannik, David Howells, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, James Bottomley,
	Matthew Garrett

On Wed, Aug 15, 2018 at 12:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> I see that module signing code trusts only builtin keys and
> not the keys in secondary_trusted_keys keyring.

This, I think, makes sense.

It basically says: we don't allow modules that weren't built with the
kernel. Adding a new key later and signing a module with it violates
that premise.

                  Linus

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 20:47             ` Linus Torvalds
@ 2018-08-15 20:53               ` James Bottomley
  2018-08-15 21:08               ` Yannik Sembritzki
  1 sibling, 0 replies; 57+ messages in thread
From: James Bottomley @ 2018-08-15 20:53 UTC (permalink / raw)
  To: Linus Torvalds, Vivek Goyal
  Cc: yannik, David Howells, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, Matthew Garrett

On Wed, 2018-08-15 at 13:47 -0700, Linus Torvalds wrote:
> On Wed, Aug 15, 2018 at 12:49 PM Vivek Goyal <vgoyal@redhat.com>
> wrote:
> > 
> > I see that module signing code trusts only builtin keys and
> > not the keys in secondary_trusted_keys keyring.
> 
> This, I think, makes sense.
> 
> It basically says: we don't allow modules that weren't built with the
> kernel. Adding a new key later and signing a module with it violates
> that premise.

Yes, the point is about layers of trust.  The UEFI secure boot keys for
a standard (i.e. system where the user hasn't taken ownership) contain
Microsoft and some ODM vendor keys.  You're forced to trust these keys
in the boot (or reboot) environment because that's consistent with the
way windows operates (any breach and microsoft will get annoyed and
help us sort it out).  You can't trust them for other kernel key
operations because that's outside the boot environment trust boundary
and if something goes wrong (say someone at an ODM signs a malicious
linux kernel module because the ODM keys are trusted to sign modules)
Microsoft won't care and we'll be on our own with no real ability to
sort it out.

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 20:47             ` Linus Torvalds
  2018-08-15 20:53               ` James Bottomley
@ 2018-08-15 21:08               ` Yannik Sembritzki
  2018-08-15 21:13                 ` James Bottomley
  2018-08-15 21:14                 ` Linus Torvalds
  1 sibling, 2 replies; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 21:08 UTC (permalink / raw)
  To: Linus Torvalds, Vivek Goyal
  Cc: David Howells, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, James Bottomley,
	Matthew Garrett

On 15.08.2018 22:47, Linus Torvalds wrote:
> It basically says: we don't allow modules that weren't built with the
> kernel. Adding a new key later and signing a module with it violates
> that premise.

Considering the following scenario:
A user is running a distro kernel, which is built by the distro, and has
the distro signing key builtin (i.e. fedora). Now, the user has taken
ownership of their system and provisioned their own platform key.
Accordingly, the user signs the distro kernel with their own key.

If I understand you correctly, modules signed by the users own key, but
not signed with the distro key, will stop working in this case?

IMO, this is not okay. The layer of trust should extend from the bottom
(user-provisioned platform key) up. Only trusting the kernel builtin key
later on (wrt. kernel modules) contradicts this principal.

Yannik


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 21:08               ` Yannik Sembritzki
@ 2018-08-15 21:13                 ` James Bottomley
  2018-08-15 21:31                   ` Yannik Sembritzki
  2018-08-15 21:52                   ` Vivek Goyal
  2018-08-15 21:14                 ` Linus Torvalds
  1 sibling, 2 replies; 57+ messages in thread
From: James Bottomley @ 2018-08-15 21:13 UTC (permalink / raw)
  To: Yannik Sembritzki, Linus Torvalds, Vivek Goyal
  Cc: David Howells, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, Matthew Garrett

On Wed, 2018-08-15 at 23:08 +0200, Yannik Sembritzki wrote:
> On 15.08.2018 22:47, Linus Torvalds wrote:
> > It basically says: we don't allow modules that weren't built with
> > the kernel. Adding a new key later and signing a module with it
> > violates that premise.
> 
> Considering the following scenario:
> A user is running a distro kernel, which is built by the distro, and
> has the distro signing key builtin (i.e. fedora). Now, the user has
> taken ownership of their system and provisioned their own platform
> key. Accordingly, the user signs the distro kernel with their own
> key.
> 
> If I understand you correctly, modules signed by the users own key,
> but not signed with the distro key, will stop working in this case?

They never actually would have worked, but yes.

> IMO, this is not okay. The layer of trust should extend from the
> bottom (user-provisioned platform key) up. Only trusting the kernel
> builtin key later on (wrt. kernel modules) contradicts this
> principal.

The kernel can't tell whether the UEFI user has taken ownership or not
so it has no basis on which to make a decision to trust the UEFI keys
or not, so we should *always* not trust them.

Consider a UEFI system for which a user has taken ownership, but which
has some signed ROMs which are UEFI secure boot verified.  Simply to
get their system to boot the user will be forced to add the ODM key to
the UEFI db ... and I'm sure in that situation the user wouldn't want
to trust the ODM key further than booting.

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/2] Replace magic for trusting the secondary keyring with #define
  2018-08-15 19:42           ` [PATCH 2/2] Replace magic for trusting the secondary keyring with #define Yannik Sembritzki
@ 2018-08-15 21:14             ` kbuild test robot
  2018-08-15 21:19               ` [PATCH 2/2] [FIXED] " Yannik Sembritzki
  0 siblings, 1 reply; 57+ messages in thread
From: kbuild test robot @ 2018-08-15 21:14 UTC (permalink / raw)
  To: Yannik Sembritzki
  Cc: kbuild-all, Linus Torvalds, David Howells, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He,
	Justin M. Forbes, Peter Jones, James Bottomley, Matthew Garrett,
	Vivek Goyal, Yannik Sembritzki

[-- Attachment #1: Type: text/plain, Size: 2265 bytes --]

Hi Yannik,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18 next-20180814]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Yannik-Sembritzki/Fix-kexec-forbidding-kernels-signed-with-keys-in-the-secondary-keyring-to-boot/20180816-042529
config: x86_64-randconfig-x019-201832 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from include/keys/asymmetric-type.h:18:0,
                    from crypto/asymmetric_keys/asymmetric_keys.h:12,
                    from crypto/asymmetric_keys/restrict.c:17:
>> include/linux/verification.h:12:0: error: unterminated #ifndef
    #ifndef _LINUX_VERIFICATION_H
    
--
   In file included from include/crypto/pkcs7.h:15:0,
                    from crypto/asymmetric_keys/pkcs7_parser.h:13,
                    from crypto/asymmetric_keys/pkcs7_parser.c:20:
>> include/linux/verification.h:12:0: error: unterminated #ifndef
    #ifndef _LINUX_VERIFICATION_H
    
   In file included from include/keys/asymmetric-type.h:18:0,
                    from crypto/asymmetric_keys/x509_parser.h:14,
                    from crypto/asymmetric_keys/pkcs7_parser.h:14,
                    from crypto/asymmetric_keys/pkcs7_parser.c:20:
>> include/linux/verification.h:12:0: error: unterminated #ifndef
    #ifndef _LINUX_VERIFICATION_H
    

vim +12 include/linux/verification.h

e68503bd David Howells 2016-04-06 @12  #ifndef _LINUX_VERIFICATION_H
e68503bd David Howells 2016-04-06  13  #define _LINUX_VERIFICATION_H
e68503bd David Howells 2016-04-06  14  

:::::: The code at line 12 was first introduced by commit
:::::: e68503bd6836ba765dc8e0ee77ea675fedc07e41 KEYS: Generalise system_verify_data() to provide access to internal content

:::::: TO: David Howells <dhowells@redhat.com>
:::::: CC: David Howells <dhowells@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29822 bytes --]

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 21:08               ` Yannik Sembritzki
  2018-08-15 21:13                 ` James Bottomley
@ 2018-08-15 21:14                 ` Linus Torvalds
  1 sibling, 0 replies; 57+ messages in thread
From: Linus Torvalds @ 2018-08-15 21:14 UTC (permalink / raw)
  To: yannik
  Cc: Vivek Goyal, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin Forbes, Peter Jones,
	James Bottomley, Matthew Garrett

On Wed, Aug 15, 2018 at 2:08 PM Yannik Sembritzki <yannik@sembritzki.me> wrote:
>
> IMO, this is not okay. The layer of trust should extend from the bottom
> (user-provisioned platform key) up. Only trusting the kernel builtin key
> later on (wrt. kernel modules) contradicts this principal.

This module loading case is not about trusting the *key*.

This is about trusting the *build system*.

For example, I build my kernels with one single randomly generated key
(that gets deleted afterwards). The modules get built with that key
too.

No amount of added keys later will make a module valid to load.

                  Linus

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/2] [FIXED] Replace magic for trusting the secondary keyring with #define
  2018-08-15 21:14             ` kbuild test robot
@ 2018-08-15 21:19               ` Yannik Sembritzki
  2018-08-15 22:01                 ` Linus Torvalds
  0 siblings, 1 reply; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 21:19 UTC (permalink / raw)
  To: kbuild test robot
  Cc: kbuild-all, Linus Torvalds, David Howells, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He,
	Justin M. Forbes, Peter Jones, James Bottomley, Matthew Garrett,
	Vivek Goyal

Signed-off-by: Yannik Sembritzki <yannik@sembritzki.me>
---
 arch/x86/kernel/kexec-bzimage64.c       | 2 +-
 certs/system_keyring.c                  | 3 ++-
 crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
 include/linux/verification.h            | 5 +++++
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/kexec-bzimage64.c
b/arch/x86/kernel/kexec-bzimage64.c
index 74628275..97d199a3 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
 static int bzImage64_verify_sig(const char *kernel, unsigned long
kernel_len)
 {
     return verify_pefile_signature(kernel, kernel_len,
-                       ((struct key *)1UL),
+                       TRUST_SECONDARY_KEYRING,
                        VERIFYING_KEXEC_PE_SIGNATURE);
 }
 #endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b2..777ac7d2 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -15,6 +15,7 @@
 #include <linux/cred.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+#include <linux/verification.h>
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
 #include <crypto/pkcs7.h>
@@ -230,7 +231,7 @@ int verify_pkcs7_signature(const void *data, size_t len,
 
     if (!trusted_keys) {
         trusted_keys = builtin_trusted_keys;
-    } else if (trusted_keys == (void *)1UL) {
+    } else if (trusted_keys == TRUST_SECONDARY_KEYRING) {
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
         trusted_keys = secondary_trusted_keys;
 #else
diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c
b/crypto/asymmetric_keys/pkcs7_key_type.c
index e284d9cb..0783e555 100644
--- a/crypto/asymmetric_keys/pkcs7_key_type.c
+++ b/crypto/asymmetric_keys/pkcs7_key_type.c
@@ -63,7 +63,7 @@ static int pkcs7_preparse(struct key_preparsed_payload
*prep)
 
     return verify_pkcs7_signature(NULL, 0,
                       prep->data, prep->datalen,
-                      (void *)1UL, usage,
+                      TRUST_SECONDARY_KEYRING, usage,
                       pkcs7_view_content, prep);
 }
 
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6..ad221ab2 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -12,6 +12,11 @@
 #ifndef _LINUX_VERIFICATION_H
 #define _LINUX_VERIFICATION_H
 
+// Allow both builtin trusted keys and secondary trusted keys
+#ifndef TRUST_SECONDARY_KEYRING
+#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
+#endif
+
 /*
  * The use to which an asymmetric key is being put.
  */
-- 
2.17.1

Sorry, I've fixed this now.

On 15.08.2018 23:14, kbuild test robot wrote:
> Hi Yannik,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.18 next-20180814]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Yannik-Sembritzki/Fix-kexec-forbidding-kernels-signed-with-keys-in-the-secondary-keyring-to-boot/20180816-042529
> config: x86_64-randconfig-x019-201832 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
>
> All errors (new ones prefixed by >>):
>
>    In file included from include/keys/asymmetric-type.h:18:0,
>                     from crypto/asymmetric_keys/asymmetric_keys.h:12,
>                     from crypto/asymmetric_keys/restrict.c:17:
>>> include/linux/verification.h:12:0: error: unterminated #ifndef
>     #ifndef _LINUX_VERIFICATION_H
>     
> --
>    In file included from include/crypto/pkcs7.h:15:0,
>                     from crypto/asymmetric_keys/pkcs7_parser.h:13,
>                     from crypto/asymmetric_keys/pkcs7_parser.c:20:
>>> include/linux/verification.h:12:0: error: unterminated #ifndef
>     #ifndef _LINUX_VERIFICATION_H
>     
>    In file included from include/keys/asymmetric-type.h:18:0,
>                     from crypto/asymmetric_keys/x509_parser.h:14,
>                     from crypto/asymmetric_keys/pkcs7_parser.h:14,
>                     from crypto/asymmetric_keys/pkcs7_parser.c:20:
>>> include/linux/verification.h:12:0: error: unterminated #ifndef
>     #ifndef _LINUX_VERIFICATION_H
>     
>
> vim +12 include/linux/verification.h
>
> e68503bd David Howells 2016-04-06 @12  #ifndef _LINUX_VERIFICATION_H
> e68503bd David Howells 2016-04-06  13  #define _LINUX_VERIFICATION_H
> e68503bd David Howells 2016-04-06  14  
>
> :::::: The code at line 12 was first introduced by commit
> :::::: e68503bd6836ba765dc8e0ee77ea675fedc07e41 KEYS: Generalise system_verify_data() to provide access to internal content
>
> :::::: TO: David Howells <dhowells@redhat.com>
> :::::: CC: David Howells <dhowells@redhat.com>
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 21:13                 ` James Bottomley
@ 2018-08-15 21:31                   ` Yannik Sembritzki
  2018-08-15 21:40                     ` James Bottomley
  2018-08-15 21:57                     ` Vivek Goyal
  2018-08-15 21:52                   ` Vivek Goyal
  1 sibling, 2 replies; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 21:31 UTC (permalink / raw)
  To: James Bottomley, Linus Torvalds, Vivek Goyal
  Cc: David Howells, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, Matthew Garrett

On 15.08.2018 23:13, James Bottomley wrote:
> Consider a UEFI system for which a user has taken ownership, but which
> has some signed ROMs which are UEFI secure boot verified.  Simply to
> get their system to boot the user will be forced to add the ODM key to
> the UEFI db ... and I'm sure in that situation the user wouldn't want
> to trust the ODM key further than booting.
I definitely agree with this point.

Is there any solution, except from building your own kernel, to the
scenario I described?
I think there should be.
(I've personally run into this with VirtualBox, which I IIRC couldn't
load, even though I provisioned my own PK, and signed both kernel and
VirtualBox module with my own key. I could've compiled my own kernel
with my //own key, but that is pretty impractical for most users.)

Yannik

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 21:31                   ` Yannik Sembritzki
@ 2018-08-15 21:40                     ` James Bottomley
  2018-08-15 21:50                       ` Yannik Sembritzki
  2018-08-15 21:57                     ` Vivek Goyal
  1 sibling, 1 reply; 57+ messages in thread
From: James Bottomley @ 2018-08-15 21:40 UTC (permalink / raw)
  To: Yannik Sembritzki, Linus Torvalds, Vivek Goyal
  Cc: David Howells, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, Matthew Garrett

On Wed, 2018-08-15 at 23:31 +0200, Yannik Sembritzki wrote:
> On 15.08.2018 23:13, James Bottomley wrote:
> > Consider a UEFI system for which a user has taken ownership, but
> > which
> > has some signed ROMs which are UEFI secure boot verified.  Simply
> > to
> > get their system to boot the user will be forced to add the ODM key
> > to
> > the UEFI db ... and I'm sure in that situation the user wouldn't
> > want
> > to trust the ODM key further than booting.
> 
> I definitely agree with this point.
> 
> Is there any solution, except from building your own kernel, to the
> scenario I described?
> I think there should be. (I've personally run into this with
> VirtualBox, which I IIRC couldn't load, even though I provisioned my
> own PK, and signed both kernel and VirtualBox module with my own key.
> I could've compiled my own kernel with my //own key, but that is
> pretty impractical for most users.)

What about the key linking patches:

https://lkml.org/lkml/2018/5/2/989

? They allow you to insert your own binary key into bzimage and then
resign the resulting blob for secure boot.  It's a fairly painless
process.  The patches have been languishing for an unstated reason but
it's suspected to have something to do with Red Hat not wanting to
support Enterprise users signing their own kernels.

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 21:40                     ` James Bottomley
@ 2018-08-15 21:50                       ` Yannik Sembritzki
  0 siblings, 0 replies; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 21:50 UTC (permalink / raw)
  To: James Bottomley, Linus Torvalds, Vivek Goyal
  Cc: David Howells, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, Matthew Garrett

On 15.08.2018 23:40, James Bottomley wrote:
> What about the key linking patches:
>
> https://lkml.org/lkml/2018/5/2/989
>
> ? They allow you to insert your own binary key into bzimage and then
> resign the resulting blob for secure boot.  It's a fairly painless
> process.  The patches have been languishing for an unstated reason but
> it's suspected to have something to do with Red Hat not wanting to
> support Enterprise users signing their own kernels.

Thanks, that's exactly what I was thinking about.

Yannik


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 21:13                 ` James Bottomley
  2018-08-15 21:31                   ` Yannik Sembritzki
@ 2018-08-15 21:52                   ` Vivek Goyal
  2018-08-15 21:57                     ` James Bottomley
  1 sibling, 1 reply; 57+ messages in thread
From: Vivek Goyal @ 2018-08-15 21:52 UTC (permalink / raw)
  To: James Bottomley
  Cc: Yannik Sembritzki, Linus Torvalds, David Howells,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, Matthew Garrett

On Wed, Aug 15, 2018 at 02:13:17PM -0700, James Bottomley wrote:
> On Wed, 2018-08-15 at 23:08 +0200, Yannik Sembritzki wrote:
> > On 15.08.2018 22:47, Linus Torvalds wrote:
> > > It basically says: we don't allow modules that weren't built with
> > > the kernel. Adding a new key later and signing a module with it
> > > violates that premise.
> > 
> > Considering the following scenario:
> > A user is running a distro kernel, which is built by the distro, and
> > has the distro signing key builtin (i.e. fedora). Now, the user has
> > taken ownership of their system and provisioned their own platform
> > key. Accordingly, the user signs the distro kernel with their own
> > key.
> > 
> > If I understand you correctly, modules signed by the users own key,
> > but not signed with the distro key, will stop working in this case?
> 
> They never actually would have worked, but yes.
> 
> > IMO, this is not okay. The layer of trust should extend from the
> > bottom (user-provisioned platform key) up. Only trusting the kernel
> > builtin key later on (wrt. kernel modules) contradicts this
> > principal.
> 
> The kernel can't tell whether the UEFI user has taken ownership or not
> so it has no basis on which to make a decision to trust the UEFI keys
> or not, so we should *always* not trust them.
> 
> Consider a UEFI system for which a user has taken ownership, but which
> has some signed ROMs which are UEFI secure boot verified.  Simply to
> get their system to boot the user will be forced to add the ODM key to
> the UEFI db ... and I'm sure in that situation the user wouldn't want
> to trust the ODM key further than booting.

IIUC, it is fine to trust these ODM keys, User keys and "foo" keys for loading
kernel but not for modules? If yes, then atleast we can enable trusting
keys in .secondary_trusted_keys keyring for kernel signature verificaton
and that will solve the kexec/kdump issue on distribution kernels.

Thanks
Vivek

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 21:52                   ` Vivek Goyal
@ 2018-08-15 21:57                     ` James Bottomley
  0 siblings, 0 replies; 57+ messages in thread
From: James Bottomley @ 2018-08-15 21:57 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Yannik Sembritzki, Linus Torvalds, David Howells,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, Matthew Garrett

On Wed, 2018-08-15 at 17:52 -0400, Vivek Goyal wrote:
> On Wed, Aug 15, 2018 at 02:13:17PM -0700, James Bottomley wrote:
> > On Wed, 2018-08-15 at 23:08 +0200, Yannik Sembritzki wrote:
> > > On 15.08.2018 22:47, Linus Torvalds wrote:
> > > > It basically says: we don't allow modules that weren't built
> > > > with
> > > > the kernel. Adding a new key later and signing a module with it
> > > > violates that premise.
> > > 
> > > Considering the following scenario:
> > > A user is running a distro kernel, which is built by the distro,
> > > and has the distro signing key builtin (i.e. fedora). Now, the
> > > user has taken ownership of their system and provisioned their
> > > own platform key. Accordingly, the user signs the distro kernel
> > > with their own key.
> > > 
> > > If I understand you correctly, modules signed by the users own
> > > key, but not signed with the distro key, will stop working in
> > > this case?
> > 
> > They never actually would have worked, but yes.
> > 
> > > IMO, this is not okay. The layer of trust should extend from the
> > > bottom (user-provisioned platform key) up. Only trusting the
> > > kernel builtin key later on (wrt. kernel modules) contradicts
> > > this principal.
> > 
> > The kernel can't tell whether the UEFI user has taken ownership or
> > not so it has no basis on which to make a decision to trust the
> > UEFI keys or not, so we should *always* not trust them.
> > 
> > Consider a UEFI system for which a user has taken ownership, but
> > which has some signed ROMs which are UEFI secure boot
> > verified.  Simply to get their system to boot the user will be
> > forced to add the ODM key to the UEFI db ... and I'm sure in that
> > situation the user wouldn't want to trust the ODM key further than
> > booting.
> 
> IIUC, it is fine to trust these ODM keys, User keys and "foo" keys
> for loading kernel but not for modules?

It's fine to trust the secure boot keys for the boot environment.  If
you argue kexec is linux booting linux then yes, that's a supported
use.

>  If yes, then atleast we can enable trusting keys in
> .secondary_trusted_keys keyring for kernel signature verificaton and
> that will solve the kexec/kdump issue on distribution kernels.

I think it's OK ... I can't think of any reason you'd want a signed
kernel to boot but not to be able to kexec to a kernel with the same
signer.

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 21:31                   ` Yannik Sembritzki
  2018-08-15 21:40                     ` James Bottomley
@ 2018-08-15 21:57                     ` Vivek Goyal
  2018-08-15 22:14                       ` Yannik Sembritzki
  1 sibling, 1 reply; 57+ messages in thread
From: Vivek Goyal @ 2018-08-15 21:57 UTC (permalink / raw)
  To: Yannik Sembritzki
  Cc: James Bottomley, Linus Torvalds, David Howells, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

On Wed, Aug 15, 2018 at 11:31:27PM +0200, Yannik Sembritzki wrote:
> On 15.08.2018 23:13, James Bottomley wrote:
> > Consider a UEFI system for which a user has taken ownership, but which
> > has some signed ROMs which are UEFI secure boot verified.  Simply to
> > get their system to boot the user will be forced to add the ODM key to
> > the UEFI db ... and I'm sure in that situation the user wouldn't want
> > to trust the ODM key further than booting.
> I definitely agree with this point.
> 
> Is there any solution, except from building your own kernel, to the
> scenario I described?
> I think there should be.
> (I've personally run into this with VirtualBox, which I IIRC couldn't
> load, even though I provisioned my own PK, and signed both kernel and
> VirtualBox module with my own key. I could've compiled my own kernel
> with my //own key, but that is pretty impractical for most users.)

Aha.., so that's your real problem. You are trying to load VirtualBox
module and that will not load even if you take ownership of platform
by adding your key and sign module with that key.

So this patch still will not fix the problem you are facing. It is still
good to fix the case of kexec/kdump broken on Fedora on secureboot
machines.

Thanks
Vivek

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/2] [FIXED] Replace magic for trusting the secondary keyring with #define
  2018-08-15 21:19               ` [PATCH 2/2] [FIXED] " Yannik Sembritzki
@ 2018-08-15 22:01                 ` Linus Torvalds
  2018-08-15 22:07                   ` [PATCH 2/2] [FIXED v2] " Yannik Sembritzki
  0 siblings, 1 reply; 57+ messages in thread
From: Linus Torvalds @ 2018-08-15 22:01 UTC (permalink / raw)
  To: yannik
  Cc: kbuild test robot, kbuild-all, David Howells, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, James Bottomley, Matthew Garrett, Vivek Goyal

On Wed, Aug 15, 2018 at 2:19 PM Yannik Sembritzki <yannik@sembritzki.me> wrote:
>
>
> +// Allow both builtin trusted keys and secondary trusted keys
> +#ifndef TRUST_SECONDARY_KEYRING
> +#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
> +#endif
>
> Sorry, I've fixed this now.

Actually, just remove the ifndef/endif entirely. There is no possible
other valid #define this could have, and the header itself is
protected from multi-inclusion the standard way.

                 Linus

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
  2018-08-15 22:01                 ` Linus Torvalds
@ 2018-08-15 22:07                   ` Yannik Sembritzki
  2018-08-16  1:11                     ` Dave Young
  0 siblings, 1 reply; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 22:07 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: David Howells, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, James Bottomley,
	Matthew Garrett, Vivek Goyal

Signed-off-by: Yannik Sembritzki <yannik@sembritzki.me>
---
 arch/x86/kernel/kexec-bzimage64.c       | 2 +-
 certs/system_keyring.c                  | 3 ++-
 crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
 include/linux/verification.h            | 3 +++
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/kexec-bzimage64.c
b/arch/x86/kernel/kexec-bzimage64.c
index 74628275..97d199a3 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
 static int bzImage64_verify_sig(const char *kernel, unsigned long
kernel_len)
 {
     return verify_pefile_signature(kernel, kernel_len,
-                       ((struct key *)1UL),
+                       TRUST_SECONDARY_KEYRING,
                        VERIFYING_KEXEC_PE_SIGNATURE);
 }
 #endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b2..777ac7d2 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -15,6 +15,7 @@
 #include <linux/cred.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+#include <linux/verification.h>
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
 #include <crypto/pkcs7.h>
@@ -230,7 +231,7 @@ int verify_pkcs7_signature(const void *data, size_t len,
 
     if (!trusted_keys) {
         trusted_keys = builtin_trusted_keys;
-    } else if (trusted_keys == (void *)1UL) {
+    } else if (trusted_keys == TRUST_SECONDARY_KEYRING) {
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
         trusted_keys = secondary_trusted_keys;
 #else
diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c
b/crypto/asymmetric_keys/pkcs7_key_type.c
index e284d9cb..0783e555 100644
--- a/crypto/asymmetric_keys/pkcs7_key_type.c
+++ b/crypto/asymmetric_keys/pkcs7_key_type.c
@@ -63,7 +63,7 @@ static int pkcs7_preparse(struct key_preparsed_payload
*prep)
 
     return verify_pkcs7_signature(NULL, 0,
                       prep->data, prep->datalen,
-                      (void *)1UL, usage,
+                      TRUST_SECONDARY_KEYRING, usage,
                       pkcs7_view_content, prep);
 }
 
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6..c00c1143 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -12,6 +12,9 @@
 #ifndef _LINUX_VERIFICATION_H
 #define _LINUX_VERIFICATION_H
 
+// Allow both builtin trusted keys and secondary trusted keys
+#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
+
 /*
  * The use to which an asymmetric key is being put.
  */
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 21:57                     ` Vivek Goyal
@ 2018-08-15 22:14                       ` Yannik Sembritzki
  0 siblings, 0 replies; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-15 22:14 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: James Bottomley, Linus Torvalds, David Howells, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

On 15.08.2018 23:57, Vivek Goyal wrote:
> Aha.., so that's your real problem. You are trying to load VirtualBox
> module and that will not load even if you take ownership of platform
> by adding your key and sign module with that key.
>
> So this patch still will not fix the problem you are facing. It is still
> good to fix the case of kexec/kdump broken on Fedora on secureboot
> machines.

No, I wrote this patch specifically because because of the kexec issue
and would like to seem them merged to fix exactly that :-)

I only replied to the module signing discussion as there were some
ongoing arguments with regard to that. IMO this is resolved with the
patches by Mehmet Kayaalp mentioned by James anyway.

Thanks
Yannik


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 17:42     ` Vivek Goyal
  2018-08-15 18:44       ` Yannik Sembritzki
  2018-08-15 18:58       ` Vivek Goyal
@ 2018-08-16  0:52       ` Dave Young
  2018-08-16  0:55         ` Dave Young
  2018-08-16 12:13       ` David Howells
  3 siblings, 1 reply; 57+ messages in thread
From: Dave Young @ 2018-08-16  0:52 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Yannik Sembritzki, Linus Torvalds, David Howells,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Baoquan He,
	Justin M. Forbes

On 08/15/18 at 01:42pm, Vivek Goyal wrote:
> On Wed, Aug 15, 2018 at 07:27:33PM +0200, Yannik Sembritzki wrote:
> > Would this be okay?
> 
> [ CC dave young, Baoquan, Justin Forbes]
> 
> Hi Yannik,
> 
> I am reading that bug and wondering that what broke it. It used to work,
> so some change broke it. 
> 
> Justin said that we have been signing fedora kernels with fedora keys so
> looks like no change there.
> 
> Previously, I think all the keys used to go in system keyring and it
> used to work. Is it somehow because of split in builtin keyring and
> secondary system keyring. Could it be that fedora key used to show
> up in system keyring previously and it worked but now it shows up
> in secondary system keyring and by default we don't use keys from
> that keyring for signature verification?

There was a Fedora bug below:
https://bugzilla.redhat.com/show_bug.cgi?id=1470995

I posted a fix here but bobody responsed, I think I obviously did not
consider the "trust build system only" point from Linus:
http://lists.infradead.org/pipermail/kexec/2017-November/019632.html

But either above patch or defining a macro for the "1UL" in cert header
file works.

Since nobody reviewed my patch so later I submitted a Fedora only patch
which is similar with Yannik's and merged in Fedora tree:
https://bugzilla.redhat.com/attachment.cgi?id=1450772&action=edit

> 
> Thanks
> Vivek
> 
> > 
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c
> > b/arch/x86/kernel/kexec-bzimage64.c
> > index 7326078e..2ba47e24 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -41,6 +41,9 @@
> >  #define MIN_KERNEL_LOAD_ADDR   0x100000
> >  #define MIN_INITRD_LOAD_ADDR   0x1000000
> >  
> > +// Allow both builtin trusted keys and secondary trusted keys
> > +#define TRUST_FULL_KEYRING     (void *)1UL
> > +
> >  /*
> >   * This is a place holder for all boot loader specific data structure which
> >   * gets allocated in one call but gets freed much later during cleanup
> > @@ -532,7 +535,7 @@ static int bzImage64_cleanup(void *loader_data)
> >  static int bzImage64_verify_sig(const char *kernel, unsigned long
> > kernel_len)
> >  {
> >         return verify_pefile_signature(kernel, kernel_len,
> > -                                      NULL,
> > +                                      TRUST_FULL_KEYRING,
> >                                        VERIFYING_KEXEC_PE_SIGNATURE);
> >  }
> >  #endif
> > --
> > 
> > On 15.08.2018 18:54, Linus Torvalds wrote:
> > > This needs more people involved, and at least a sign-off.
> > >
> > > It looks ok, but I think we need a #define for the magical (void *)1UL
> > > thing. I see the use in verify_pkcs7_signature(), but still.
> > >
> > >               Linus
> > >
> > >
> > >
> > > On Wed, Aug 15, 2018 at 3:11 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
> > >> ---
> > >>  arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > >> index 7326078e..eaaa125d 100644
> > >> --- a/arch/x86/kernel/kexec-bzimage64.c
> > >> +++ b/arch/x86/kernel/kexec-bzimage64.c
> > >> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> > >>  static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
> > >>  {
> > >>         return verify_pefile_signature(kernel, kernel_len,
> > >> -                                      NULL,
> > >> +                                      (void *)1UL,
> > >>                                        VERIFYING_KEXEC_PE_SIGNATURE);
> > >>  }
> > >>  #endif
> > >> --
> > >> 2.17.1
> > >>
> > >> The exact scenario under which this issue occurs is described here:
> > >> https://bugzilla.redhat.com/show_bug.cgi?id=1554113
> > >>
> > 

Thanks
Dave

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16  0:52       ` Dave Young
@ 2018-08-16  0:55         ` Dave Young
  0 siblings, 0 replies; 57+ messages in thread
From: Dave Young @ 2018-08-16  0:55 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Yannik Sembritzki, Linus Torvalds, David Howells,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Baoquan He,
	Justin M. Forbes

On 08/16/18 at 08:52am, Dave Young wrote:
> On 08/15/18 at 01:42pm, Vivek Goyal wrote:
> > On Wed, Aug 15, 2018 at 07:27:33PM +0200, Yannik Sembritzki wrote:
> > > Would this be okay?
> > 
> > [ CC dave young, Baoquan, Justin Forbes]
> > 
> > Hi Yannik,
> > 
> > I am reading that bug and wondering that what broke it. It used to work,
> > so some change broke it. 
> > 
> > Justin said that we have been signing fedora kernels with fedora keys so
> > looks like no change there.
> > 
> > Previously, I think all the keys used to go in system keyring and it
> > used to work. Is it somehow because of split in builtin keyring and
> > secondary system keyring. Could it be that fedora key used to show
> > up in system keyring previously and it worked but now it shows up
> > in secondary system keyring and by default we don't use keys from
> > that keyring for signature verification?

The commit introduced this issue is:

commit d3bfe84129f65e0af2450743ebdab33d161d01c9
Author: David Howells <dhowells@redhat.com>
Date:   Wed Apr 6 16:14:27 2016 +0100

    certs: Add a secondary system keyring that can be added to dynamically

> 
> There was a Fedora bug below:
> https://bugzilla.redhat.com/show_bug.cgi?id=1470995
> 
> I posted a fix here but bobody responsed, I think I obviously did not
> consider the "trust build system only" point from Linus:
> http://lists.infradead.org/pipermail/kexec/2017-November/019632.html
> 
> But either above patch or defining a macro for the "1UL" in cert header
> file works.
> 
> Since nobody reviewed my patch so later I submitted a Fedora only patch
> which is similar with Yannik's and merged in Fedora tree:
> https://bugzilla.redhat.com/attachment.cgi?id=1450772&action=edit
> 
> > 
> > Thanks
> > Vivek
> > 
> > > 
> > > diff --git a/arch/x86/kernel/kexec-bzimage64.c
> > > b/arch/x86/kernel/kexec-bzimage64.c
> > > index 7326078e..2ba47e24 100644
> > > --- a/arch/x86/kernel/kexec-bzimage64.c
> > > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > > @@ -41,6 +41,9 @@
> > >  #define MIN_KERNEL_LOAD_ADDR   0x100000
> > >  #define MIN_INITRD_LOAD_ADDR   0x1000000
> > >  
> > > +// Allow both builtin trusted keys and secondary trusted keys
> > > +#define TRUST_FULL_KEYRING     (void *)1UL
> > > +
> > >  /*
> > >   * This is a place holder for all boot loader specific data structure which
> > >   * gets allocated in one call but gets freed much later during cleanup
> > > @@ -532,7 +535,7 @@ static int bzImage64_cleanup(void *loader_data)
> > >  static int bzImage64_verify_sig(const char *kernel, unsigned long
> > > kernel_len)
> > >  {
> > >         return verify_pefile_signature(kernel, kernel_len,
> > > -                                      NULL,
> > > +                                      TRUST_FULL_KEYRING,
> > >                                        VERIFYING_KEXEC_PE_SIGNATURE);
> > >  }
> > >  #endif
> > > --
> > > 
> > > On 15.08.2018 18:54, Linus Torvalds wrote:
> > > > This needs more people involved, and at least a sign-off.
> > > >
> > > > It looks ok, but I think we need a #define for the magical (void *)1UL
> > > > thing. I see the use in verify_pkcs7_signature(), but still.
> > > >
> > > >               Linus
> > > >
> > > >
> > > >
> > > > On Wed, Aug 15, 2018 at 3:11 AM Yannik Sembritzki <yannik@sembritzki.me> wrote:
> > > >> ---
> > > >>  arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > > >> index 7326078e..eaaa125d 100644
> > > >> --- a/arch/x86/kernel/kexec-bzimage64.c
> > > >> +++ b/arch/x86/kernel/kexec-bzimage64.c
> > > >> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> > > >>  static int bzImage64_verify_sig(const char *kernel, unsigned long kernel_len)
> > > >>  {
> > > >>         return verify_pefile_signature(kernel, kernel_len,
> > > >> -                                      NULL,
> > > >> +                                      (void *)1UL,
> > > >>                                        VERIFYING_KEXEC_PE_SIGNATURE);
> > > >>  }
> > > >>  #endif
> > > >> --
> > > >> 2.17.1
> > > >>
> > > >> The exact scenario under which this issue occurs is described here:
> > > >> https://bugzilla.redhat.com/show_bug.cgi?id=1554113
> > > >>
> > > 
> 
> Thanks
> Dave

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
  2018-08-15 22:07                   ` [PATCH 2/2] [FIXED v2] " Yannik Sembritzki
@ 2018-08-16  1:11                     ` Dave Young
  2018-08-16  7:43                       ` Yannik Sembritzki
  2018-08-16 12:46                       ` Vivek Goyal
  0 siblings, 2 replies; 57+ messages in thread
From: Dave Young @ 2018-08-16  1:11 UTC (permalink / raw)
  To: Yannik Sembritzki
  Cc: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Baoquan He, Justin Forbes, Peter Jones, James Bottomley,
	Matthew Garrett, Vivek Goyal

On 08/16/18 at 12:07am, Yannik Sembritzki wrote:
> Signed-off-by: Yannik Sembritzki <yannik@sembritzki.me>
> ---
>  arch/x86/kernel/kexec-bzimage64.c       | 2 +-
>  certs/system_keyring.c                  | 3 ++-
>  crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
>  include/linux/verification.h            | 3 +++
>  4 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/kexec-bzimage64.c
> b/arch/x86/kernel/kexec-bzimage64.c
> index 74628275..97d199a3 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
>  static int bzImage64_verify_sig(const char *kernel, unsigned long
> kernel_len)
>  {
>      return verify_pefile_signature(kernel, kernel_len,
> -                       ((struct key *)1UL),
> +                       TRUST_SECONDARY_KEYRING,

Instead of fix your 1st patch in 2nd patch, I would suggest to
switch the patch order.  In 1st patch change the common code to use
the new macro and in 2nd patch you can directly fix the kexec code
with TRUST_SECONDARY_KEYRING.

>                         VERIFYING_KEXEC_PE_SIGNATURE);
>  }
>  #endif
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 6251d1b2..777ac7d2 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -15,6 +15,7 @@
>  #include <linux/cred.h>
>  #include <linux/err.h>
>  #include <linux/slab.h>
> +#include <linux/verification.h>
>  #include <keys/asymmetric-type.h>
>  #include <keys/system_keyring.h>
>  #include <crypto/pkcs7.h>
> @@ -230,7 +231,7 @@ int verify_pkcs7_signature(const void *data, size_t len,
>  
>      if (!trusted_keys) {
>          trusted_keys = builtin_trusted_keys;
> -    } else if (trusted_keys == (void *)1UL) {
> +    } else if (trusted_keys == TRUST_SECONDARY_KEYRING) {
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>          trusted_keys = secondary_trusted_keys;
>  #else
> diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c
> b/crypto/asymmetric_keys/pkcs7_key_type.c
> index e284d9cb..0783e555 100644
> --- a/crypto/asymmetric_keys/pkcs7_key_type.c
> +++ b/crypto/asymmetric_keys/pkcs7_key_type.c
> @@ -63,7 +63,7 @@ static int pkcs7_preparse(struct key_preparsed_payload
> *prep)
>  
>      return verify_pkcs7_signature(NULL, 0,
>                        prep->data, prep->datalen,
> -                      (void *)1UL, usage,
> +                      TRUST_SECONDARY_KEYRING, usage,
>                        pkcs7_view_content, prep);
>  }
>  
> diff --git a/include/linux/verification.h b/include/linux/verification.h
> index a10549a6..c00c1143 100644
> --- a/include/linux/verification.h
> +++ b/include/linux/verification.h
> @@ -12,6 +12,9 @@
>  #ifndef _LINUX_VERIFICATION_H
>  #define _LINUX_VERIFICATION_H
>  
> +// Allow both builtin trusted keys and secondary trusted keys

It would be better to use commenting style /*

> +#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
> +
>  /*
>   * The use to which an asymmetric key is being put.
>   */
> -- 
> 2.17.1
> 
> 

Thanks
Dave

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
  2018-08-16  1:11                     ` Dave Young
@ 2018-08-16  7:43                       ` Yannik Sembritzki
  2018-08-16  8:02                         ` Dave Young
  2018-08-16 12:46                       ` Vivek Goyal
  1 sibling, 1 reply; 57+ messages in thread
From: Yannik Sembritzki @ 2018-08-16  7:43 UTC (permalink / raw)
  To: Dave Young
  Cc: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Baoquan He, Justin Forbes, Peter Jones, James Bottomley,
	Matthew Garrett, Vivek Goyal

On 16.08.2018 03:11, Dave Young wrote:
> Instead of fix your 1st patch in 2nd patch, I would suggest to
> switch the patch order.  In 1st patch change the common code to use
> the new macro and in 2nd patch you can directly fix the kexec code
> with TRUST_SECONDARY_KEYRING.
My reasoning for doing it in this order was that the first patch which
fixes the bug itself should be merged into stable, while the refactoring
doesn't necessarily have to. I'm not familiar with the linux development
process, so please correct me if this should be done in another fashion.

Yannik

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
  2018-08-16  7:43                       ` Yannik Sembritzki
@ 2018-08-16  8:02                         ` Dave Young
  2018-08-16  8:20                           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 57+ messages in thread
From: Dave Young @ 2018-08-16  8:02 UTC (permalink / raw)
  To: Yannik Sembritzki
  Cc: Linus Torvalds, David Howells, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Baoquan He, Justin Forbes, Peter Jones, Greg Kroah-Hartman,
	James Bottomley, Matthew Garrett, Vivek Goyal

On 08/16/18 at 09:43am, Yannik Sembritzki wrote:
> On 16.08.2018 03:11, Dave Young wrote:
> > Instead of fix your 1st patch in 2nd patch, I would suggest to
> > switch the patch order.  In 1st patch change the common code to use
> > the new macro and in 2nd patch you can directly fix the kexec code
> > with TRUST_SECONDARY_KEYRING.
> My reasoning for doing it in this order was that the first patch which
> fixes the bug itself should be merged into stable, while the refactoring
> doesn't necessarily have to. I'm not familiar with the linux development
> process, so please correct me if this should be done in another fashion.

Frankly I'm not sure about the stable process.  But personally I do not 
like the order.

Cced Greg for opinions about stable concern.

> 
> Yannik

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
  2018-08-16  8:02                         ` Dave Young
@ 2018-08-16  8:20                           ` Greg Kroah-Hartman
  0 siblings, 0 replies; 57+ messages in thread
From: Greg Kroah-Hartman @ 2018-08-16  8:20 UTC (permalink / raw)
  To: Dave Young
  Cc: Yannik Sembritzki, Linus Torvalds, David Howells,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Baoquan He,
	Justin Forbes, Peter Jones, James Bottomley, Matthew Garrett,
	Vivek Goyal

On Thu, Aug 16, 2018 at 04:02:59PM +0800, Dave Young wrote:
> On 08/16/18 at 09:43am, Yannik Sembritzki wrote:
> > On 16.08.2018 03:11, Dave Young wrote:
> > > Instead of fix your 1st patch in 2nd patch, I would suggest to
> > > switch the patch order.  In 1st patch change the common code to use
> > > the new macro and in 2nd patch you can directly fix the kexec code
> > > with TRUST_SECONDARY_KEYRING.
> > My reasoning for doing it in this order was that the first patch which
> > fixes the bug itself should be merged into stable, while the refactoring
> > doesn't necessarily have to. I'm not familiar with the linux development
> > process, so please correct me if this should be done in another fashion.
> 
> Frankly I'm not sure about the stable process.  But personally I do not 
> like the order.
> 
> Cced Greg for opinions about stable concern.

It's up to what the maintainer of the subsystem wants to do here.  I
will take what they recommend.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 17:42     ` Vivek Goyal
                         ` (2 preceding siblings ...)
  2018-08-16  0:52       ` Dave Young
@ 2018-08-16 12:13       ` David Howells
  2018-08-16 14:22         ` James Bottomley
  2018-08-16 14:43         ` David Howells
  3 siblings, 2 replies; 57+ messages in thread
From: David Howells @ 2018-08-16 12:13 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: dhowells, Yannik Sembritzki, Linus Torvalds, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He,
	Justin M. Forbes, Peter Jones, James Bottomley, Matthew Garrett

Vivek Goyal <vgoyal@redhat.com> wrote:

> Now this patch changed it to trusting builtin_trusted_keys by default,
> and all the other keys go to secondary_trusted_keys kerying. And that
> probably explains why it broke.
> 
> So checking for keys in both the keyrings makes sense to me. 
> 
> I am wondering why did we have to split this keyring to begin with. 
> So there are use cases where we want to trust builtin keys but
> not the ones which came from other places (UEFI secure boot db, or
> user loaded one)?

IMA and the IMA authors.  They want everything separated into separate
keyrings out by source and usage as far as I can tell - though this just makes
it harder to use things.

One advantage of splitting things, though, is that you don't lose the built-in
keys if you load a conflicting one from another source.

One thing that's on my to-do list is to mark keys with the provenance, perhaps
something like:

	enum key_source {
	     key_added_by_user,
	     key_built_in_for_modsign,
	     key_added_to_image,
	     key_from_uefi_db,
	     key_from_uefi_dbx,
	     key_from_tpm,
	};

	struct key {
		...
		enum key_source	source;
	};

Then:

 (1) pass this information to LSMs to make use of

 (2) Make the verification code take a bitmask of what keys are permitted for
     the task at hand.

David

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
  2018-08-16  1:11                     ` Dave Young
  2018-08-16  7:43                       ` Yannik Sembritzki
@ 2018-08-16 12:46                       ` Vivek Goyal
  1 sibling, 0 replies; 57+ messages in thread
From: Vivek Goyal @ 2018-08-16 12:46 UTC (permalink / raw)
  To: Dave Young
  Cc: Yannik Sembritzki, Linus Torvalds, David Howells,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Baoquan He,
	Justin Forbes, Peter Jones, James Bottomley, Matthew Garrett

On Thu, Aug 16, 2018 at 09:11:06AM +0800, Dave Young wrote:
> On 08/16/18 at 12:07am, Yannik Sembritzki wrote:
> > Signed-off-by: Yannik Sembritzki <yannik@sembritzki.me>
> > ---
> >  arch/x86/kernel/kexec-bzimage64.c       | 2 +-
> >  certs/system_keyring.c                  | 3 ++-
> >  crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
> >  include/linux/verification.h            | 3 +++
> >  4 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c
> > b/arch/x86/kernel/kexec-bzimage64.c
> > index 74628275..97d199a3 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> >  static int bzImage64_verify_sig(const char *kernel, unsigned long
> > kernel_len)
> >  {
> >      return verify_pefile_signature(kernel, kernel_len,
> > -                       ((struct key *)1UL),
> > +                       TRUST_SECONDARY_KEYRING,
> 
> Instead of fix your 1st patch in 2nd patch, I would suggest to
> switch the patch order.  In 1st patch change the common code to use
> the new macro and in 2nd patch you can directly fix the kexec code
> with TRUST_SECONDARY_KEYRING.

I agree. It looks cleaner that first patch change the common code and
introduce the macro to replace 1UL. And second patch makes use of that
macro in kexec bzImage64 verification.

Thanks
Vivek

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-15 19:49           ` Vivek Goyal
  2018-08-15 20:47             ` Linus Torvalds
@ 2018-08-16 13:51             ` David Howells
  2018-08-16 15:16               ` James Bottomley
  2018-08-16 15:49               ` David Howells
  1 sibling, 2 replies; 57+ messages in thread
From: David Howells @ 2018-08-16 13:51 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: dhowells, Vivek Goyal, yannik, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin Forbes, Peter Jones,
	James Bottomley, Matthew Garrett

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > I see that module signing code trusts only builtin keys and
> > not the keys in secondary_trusted_keys keyring.
> 
> This, I think, makes sense.
> 
> It basically says: we don't allow modules that weren't built with the
> kernel. Adding a new key later and signing a module with it violates
> that premise.

At the risk of starting another flamewar...

The problem with that is that it means you can't load third party modules -
such as the NVidia driver.  That's fine if you absolutely reject the right of
people to produce third party drivers for the Linux kernel and absolutely
require that they open and upstream their code if they want in.

One of the reasons *for* letting modules be loaded using UEFI keys is that
this allows you to load signed third-party drivers without needing to manually
re-sign your shim, grub and kernel.

But this is not something we can ask most ordinary users to do (not least
because of key material security) - and they would have to at least partially
repeat the process every time one of those components is upgraded.  One of the
jobs of a distribution is to insulate the ordinary user from this.

And before anyone says "well, the distribution should just build and sign
$THIRD_PARTY_MODULE", there are issues with that that potentially come with
lawyers attached.

Further, if you want to go down the route of restricting the use of UEFI keys
to load modules because that might allow someone to run hacked code, then you
also can't let kexec use UEFI keys because that would then be able to run
hacked code too.

As an alternative, though, maybe it would make sense to allow TPM-based keys
to be used to verify modules.  The problem there is that it doesn't prevent
the signing process from being interfered with on an already-hacked box.

David

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 12:13       ` David Howells
@ 2018-08-16 14:22         ` James Bottomley
  2018-08-16 14:43         ` David Howells
  1 sibling, 0 replies; 57+ messages in thread
From: James Bottomley @ 2018-08-16 14:22 UTC (permalink / raw)
  To: David Howells, Vivek Goyal
  Cc: Yannik Sembritzki, Linus Torvalds, Thomas Gleixner, Ingo Molnar,
	Peter Anvin, the arch/x86 maintainers, Linux Kernel Mailing List,
	Dave Young, Baoquan He, Justin M. Forbes, Peter Jones,
	Matthew Garrett

On Thu, 2018-08-16 at 13:13 +0100, David Howells wrote:
> Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > Now this patch changed it to trusting builtin_trusted_keys by
> > default, and all the other keys go to secondary_trusted_keys
> > kerying. And that probably explains why it broke.
> > 
> > So checking for keys in both the keyrings makes sense to me. 
> > 
> > I am wondering why did we have to split this keyring to begin
> > with. So there are use cases where we want to trust builtin keys
> > but not the ones which came from other places (UEFI secure boot db,
> > or user loaded one)?
> 
> IMA and the IMA authors.  They want everything separated into
> separate keyrings out by source and usage as far as I can tell -
> though this just makes it harder to use things.

Hey, it's not just IMA people.  I've told you several times you can't
use the secure boot keys for any form of trust beyond boot, so if you
want to add them to the kernel they have to be in a lesser trusted
keyring.  Anyone who values their security wants to be very careful
what the UEFI keys are trusted for ... it's not just IMA signatures as
the module signing discussion shows.

Personally, I don't see any use for the UEFI keys in the kernel beyond
kexec (where you can actually get the key directly from the UEFI
variable), so I'd prefer not to load them and not to have a secondary
keyring.

> One advantage of splitting things, though, is that you don't lose the
> built-in keys if you load a conflicting one from another source.
> 
> One thing that's on my to-do list is to mark keys with the
> provenance, perhaps
> something like:
> 
> 	enum key_source {
> 	     key_added_by_user,
> 	     key_built_in_for_modsign,
> 	     key_added_to_image,
> 	     key_from_uefi_db,
> 	     key_from_uefi_dbx,
> 	     key_from_tpm,
> 	};
> 
> 	struct key {
> 		...
> 		enum key_source	source;
> 	};
> 
> Then:
> 
>  (1) pass this information to LSMs to make use of
> 
>  (2) Make the verification code take a bitmask of what keys are
> permitted for
>      the task at hand.

Sounds reasonable,

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 12:13       ` David Howells
  2018-08-16 14:22         ` James Bottomley
@ 2018-08-16 14:43         ` David Howells
  2018-08-16 14:59           ` James Bottomley
  1 sibling, 1 reply; 57+ messages in thread
From: David Howells @ 2018-08-16 14:43 UTC (permalink / raw)
  To: James Bottomley
  Cc: dhowells, Vivek Goyal, Yannik Sembritzki, Linus Torvalds,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin M. Forbes, Peter Jones, Matthew Garrett

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> I've told you several times you can't use the secure boot keys for any form
> of trust beyond boot,

Yes - and you've been told several times that you're wrong.

As far as I can tell, you seem to think that whilst keys from the UEFI storage
could be used to verify a hacked module, they couldn't be used to verify a
hacked boot-time component (shim, grub, kernel, etc.).

However, if you can load a hacked module, you can very likely replace the
shim, say, with a hacked one.  In fact, replacing the shim may be easier
because modules are tied to their parent kernel in other ways besides the
signing key, whereas a shim must be standalone.

I will grant, however, that it I can understand a desire to reduce the attack
surface by not trusting the UEFI keys beyond booting - but then you shouldn't
use them for kexec *either*.

> Personally, I don't see any use for the UEFI keys in the kernel beyond
> kexec

Allowing you to load the NVidia module, say, into the kernel without the
distribution having to build it in with the kernel.

David

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 14:43         ` David Howells
@ 2018-08-16 14:59           ` James Bottomley
  2018-08-17 17:00             ` Alan Cox
  0 siblings, 1 reply; 57+ messages in thread
From: James Bottomley @ 2018-08-16 14:59 UTC (permalink / raw)
  To: David Howells
  Cc: Vivek Goyal, Yannik Sembritzki, Linus Torvalds, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He,
	Justin M. Forbes, Peter Jones, Matthew Garrett

On Thu, 2018-08-16 at 15:43 +0100, David Howells wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > I've told you several times you can't use the secure boot keys for
> > any form
> > of trust beyond boot,
> 
> Yes - and you've been told several times that you're wrong.
> 
> As far as I can tell, you seem to think that whilst keys from the
> UEFI storage could be used to verify a hacked module, they couldn't
> be used to verify a hacked boot-time component (shim, grub, kernel,
> etc.).

I'm actually not talking about UEFI storage, just the UEFI secure boot
database.  I think we might come up with a viable model for adding keys
from a UEFI variable that isn't part of the secure boot database.

> However, if you can load a hacked module, you can very likely replace
> the shim, say, with a hacked one.  In fact, replacing the shim may be
> easier because modules are tied to their parent kernel in other ways
> besides the signing key, whereas a shim must be standalone.

I think our misunderstanding is around the granularity of security. 
You seem to be arguing that it's monolithic; that's true for compromise
(usually one compromise to anything breaks everything) but it's not
true for trust.  Trust goes in defined boundaries.  For the secure boot
keys that boundary ends after boot which is why trusting them into the
kernel runtime is wrong.

The reason for keeping this boundary is to do with the politics of
breaches.  If we get a breach to the secure boot boundary, Microsoft
and all the ODMs will help us hunt it down and plug it (They have no
option because Windows is threatened by any breach to that boundary). 
If we use the keys beyond the secure boot boundary and get a breach
that only affects our use case no-one will help us because no-one will
care.

> I will grant, however, that it I can understand a desire to reduce
> the attack surface by not trusting the UEFI keys beyond booting - but
> then you shouldn't use them for kexec *either*.

Depends whether you see kexec as a boot process or not, I think.

> > Personally, I don't see any use for the UEFI keys in the kernel
> > beyond kexec
> 
> Allowing you to load the NVidia module, say, into the kernel without
> the distribution having to build it in with the kernel.

How about I address that one in your invitation to a flamewar?

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 13:51             ` David Howells
@ 2018-08-16 15:16               ` James Bottomley
  2018-08-16 15:42                 ` James Bottomley
  2018-08-16 15:49               ` David Howells
  1 sibling, 1 reply; 57+ messages in thread
From: James Bottomley @ 2018-08-16 15:16 UTC (permalink / raw)
  To: David Howells, Linus Torvalds
  Cc: Vivek Goyal, yannik, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, Matthew Garrett

On Thu, 2018-08-16 at 14:51 +0100, David Howells wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > > I see that module signing code trusts only builtin keys and
> > > not the keys in secondary_trusted_keys keyring.
> > 
> > This, I think, makes sense.
> > 
> > It basically says: we don't allow modules that weren't built with
> > the kernel. Adding a new key later and signing a module with it
> > violates that premise.
> 
> At the risk of starting another flamewar...

OK, I'll bite, I can be technically polite.

> The problem with that is that it means you can't load third party
> modules - such as the NVidia driver.  That's fine if you absolutely
> reject the right of people to produce third party drivers for the
> Linux kernel and absolutely require that they open and upstream their
> code if they want in.

So if you build your own kernel and want to load the nVidia module, you
have the key to sign it.  If you're a distribution and want third party
modules to be loaded you can set up a third party signing process using
a distro key ... I don't see what the big problem is.

> One of the reasons *for* letting modules be loaded using UEFI keys is
> that this allows you to load signed third-party drivers without
> needing to manually re-sign your shim, grub and kernel.

At the cost of compromising the security of the entire system by
trusting a key for a use beyond its purpose.  That's why this is a daft
idea.

What is the use case for this?  I think I addressed all the ones I can
think of above (own or distro kernel).  If Red Hat wants to allow the
nVidia module in its kernel it needs a third party module signing
process.

> But this is not something we can ask most ordinary users to do (not
> least because of key material security) - and they would have to at
> least partially repeat the process every time one of those components
> is upgraded.  One of the jobs of a distribution is to insulate the
> ordinary user from this.
> 
> And before anyone says "well, the distribution should just build and
> sign $THIRD_PARTY_MODULE", there are issues with that that
> potentially come with lawyers attached.

So your lawyers tell you if you sign a third party module for your
kernel then you could get blamed for the damage it causes?  So this
whole escapade is about Red Hat trying to evade legal responsibility
for allowing customers to load third party modules.

Firstly, your lawyers are wrong: Microsoft took a lot of legal advice
before they agreed to become the third party signing authority for
UEFI.  They definitely believe they can't be sued if they sign
something that later breaches UEFI security.  However, I realise trying
to overcome overly cautious legal advice is a no win situation, so lets
move on.

What I don't understand is Red Hat's objection to Mehmet's patches
which seem to achieve this goal in a much more secure way than trusting
the UEFI database.  They allow a user to inject their own key into the
kernel in a way that it's verified.  The price is relinking and
resigning the kernel, but that whole process can be automated, thus it
would seem to achieve the goal of insulating the customer from the
process as much as possible.  I mean it's a security process so they
need some type of private key handling process (say a dongle) that
they'd need to plug in to resign the kernel and plug in to sign the
third party module, but if they care about security it's likely not too
high a price for them to pay and if they don't care about security then
why do they want signed modules in the first place?

> Further, if you want to go down the route of restricting the use of
> UEFI keys to load modules because that might allow someone to run
> hacked code, then you also can't let kexec use UEFI keys because that
> would then be able to run hacked code too.

Depends: if the hacked code could be booted directly then its within
the secure boot trust boundary.

> As an alternative, though, maybe it would make sense to allow TPM-
> based keys to be used to verify modules.  The problem there is that
> it doesn't prevent the signing process from being interfered with on
> an already-hacked box.

Hey, I'm the TPM person, so I'd be happy with this.  However, you're
going to find that the TPM process for doing this is hard: the TPM is
designed to hide private keys, not really to certify public keys for
specific uses, which is what the kernel needs.  It's not that it can't
be done but the certification key will have to be setup somehow and
probably installed with a physical presence policy so an attacker can't
certify a key remotely.  By  the time we've worked out how to do all
that, it would likely have been much easier and quicker to use a
Yubikey to follow Mehmet's key insertion method.

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 15:16               ` James Bottomley
@ 2018-08-16 15:42                 ` James Bottomley
  0 siblings, 0 replies; 57+ messages in thread
From: James Bottomley @ 2018-08-16 15:42 UTC (permalink / raw)
  To: David Howells, Linus Torvalds
  Cc: Vivek Goyal, yannik, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, Matthew Garrett

On Thu, 2018-08-16 at 08:16 -0700, James Bottomley wrote:
> So your lawyers tell you if you sign a third party module for your
> kernel then you could get blamed for the damage it causes?  So this
> whole escapade is about Red Hat trying to evade legal responsibility
> for allowing customers to load third party modules.
> 
> Firstly, your lawyers are wrong: Microsoft took a lot of legal advice
> before they agreed to become the third party signing authority for
> UEFI.  They definitely believe they can't be sued if they sign
> something that later breaches UEFI security.  However, I realise
> trying to overcome overly cautious legal advice is a no win
> situation, so lets move on.

Let me give you some advice from an old hand on this: You definitely
can't overcome a lawyer with a legal argument (well, unless you're
really good, pig headed and come spoiling for a fight), but you
definitely can with a business case.  Once you present a business case
for doing whatever it is the lawyer's have said no to, the next
instruction a good executive will issue is "quantify the legal risk so
we can balance it against the business benefit".  That's where a "no"
based on over caution usually gets overruled because the risks look
minor when exposed to scrutiny.

To generate that business case, why not merge Mehmet's patches?  If
other distributions start using them successfully, then you'll have
both direct and indirect business pressures for Red Hat to do the same
and it will force the re-evaluation you need.  If no-one uses them
there'll be no additional pressure and you'll be no worse off.

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 13:51             ` David Howells
  2018-08-16 15:16               ` James Bottomley
@ 2018-08-16 15:49               ` David Howells
  2018-08-16 15:56                 ` James Bottomley
  2018-08-16 20:31                 ` David Howells
  1 sibling, 2 replies; 57+ messages in thread
From: David Howells @ 2018-08-16 15:49 UTC (permalink / raw)
  To: James Bottomley
  Cc: dhowells, Linus Torvalds, Vivek Goyal, yannik, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> > The problem with that is that it means you can't load third party
> > modules - such as the NVidia driver.  That's fine if you absolutely
> > reject the right of people to produce third party drivers for the
> > Linux kernel and absolutely require that they open and upstream their
> > code if they want in.
> 
> So if you build your own kernel and want to load the nVidia module, you
> have the key to sign it.

I think you have to assume that doing this is beyond most people.  Further, as
a distribution we would prefer people didn't raise bugs against kernels that
we didn't build.

> If you're a distribution and want third party modules to be loaded you can
> set up a third party signing process using a distro key ... I don't see what
> the big problem is.

That's the problem is right there.  AIUI, we *don't* want to set up a third
party signing process.  As I said, it potentially comes with lawyers attached.

> So your lawyers tell you if you sign a third party module for your
> kernel then you could get blamed for the damage it causes?

There's more to it than that, but I feel I should discuss it with our legal
dept. before airing it here.

David

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 15:49               ` David Howells
@ 2018-08-16 15:56                 ` James Bottomley
  2018-08-16 16:56                   ` David Laight
  2018-08-16 20:31                 ` David Howells
  1 sibling, 1 reply; 57+ messages in thread
From: James Bottomley @ 2018-08-16 15:56 UTC (permalink / raw)
  To: David Howells
  Cc: Linus Torvalds, Vivek Goyal, yannik, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

On Thu, 2018-08-16 at 16:49 +0100, David Howells wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > > The problem with that is that it means you can't load third party
> > > modules - such as the NVidia driver.  That's fine if you
> > > absolutely reject the right of people to produce third party
> > > drivers for the Linux kernel and absolutely require that they
> > > open and upstream their code if they want in.
> > 
> > So if you build your own kernel and want to load the nVidia module,
> > you have the key to sign it.
> 
> I think you have to assume that doing this is beyond most people.

As a step by step process, I agree.  However, I think we can automate
it to the point where you install a package and it says "insert your
yubikey" every time you upgrade the kernel

>   Further, as a distribution we would prefer people didn't raise bugs
> against kernels that we didn't build.

Mehmet's patches don't require building a new kernel.  They merely
require the added key be linked into the Red Hat built bzImage and then
the resulting blob be signed.  You can still identify that the original
bzImage is the Red Hat one.

> > If you're a distribution and want third party modules to be loaded
> > you can set up a third party signing process using a distro key ...
> > I don't see what the big problem is.
> 
> That's the problem is right there.  AIUI, we *don't* want to set up a
> third party signing process.  As I said, it potentially comes with
> lawyers attached.

Right so generate a business pressure to overcome the legal one.  To be
honest that's what I believe happened at Microsoft: I'm sure their
lawyers initially said "no way" to being a third party signing
authority until their executives laid out the business cost of being
blamed for the first boot virus after having refused to implement
ecosystem protection for legal reasons.

> > So your lawyers tell you if you sign a third party module for your
> > kernel then you could get blamed for the damage it causes?
> 
> There's more to it than that, but I feel I should discuss it with our
> legal dept. before airing it here.

Sure; if you want me to be involved in the conversation I can do it
under NDA or whatever they require.

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* RE: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 15:56                 ` James Bottomley
@ 2018-08-16 16:56                   ` David Laight
  2018-08-16 17:15                     ` James Bottomley
  0 siblings, 1 reply; 57+ messages in thread
From: David Laight @ 2018-08-16 16:56 UTC (permalink / raw)
  To: 'James Bottomley', David Howells
  Cc: Linus Torvalds, Vivek Goyal, yannik, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

From: James Bottomley
> Sent: 16 August 2018 16:57
> On Thu, 2018-08-16 at 16:49 +0100, David Howells wrote:
> > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> >
> > > > The problem with that is that it means you can't load third party
> > > > modules - such as the NVidia driver.  That's fine if you
> > > > absolutely reject the right of people to produce third party
> > > > drivers for the Linux kernel and absolutely require that they
> > > > open and upstream their code if they want in.
> > >
> > > So if you build your own kernel and want to load the nVidia module,
> > > you have the key to sign it.
> >
> > I think you have to assume that doing this is beyond most people.
> 
> As a step by step process, I agree.  However, I think we can automate
> it to the point where you install a package and it says "insert your
> yubikey" every time you upgrade the kernel

What about 3rd parties that want to release drivers that can be loaded
into 'distribution' kernels?
We can do that for windows (provided we've paid the 'driver signing tax')
because windows has a stable kernel DDI/DKI.
For Linux we have to release most of the driver as a binary 'blob' and
compile wrapper code on the target system with the correct kernel headers.
There is no way we could generate signed copies for every 'distribution'
kernel - even if there was a way to get them signed.
Even if we agreed to make our source code available it wouldn't be
accepted for inclusion in the kernel source tree.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 16:56                   ` David Laight
@ 2018-08-16 17:15                     ` James Bottomley
  0 siblings, 0 replies; 57+ messages in thread
From: James Bottomley @ 2018-08-16 17:15 UTC (permalink / raw)
  To: David Laight, David Howells
  Cc: Linus Torvalds, Vivek Goyal, yannik, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

On Thu, 2018-08-16 at 16:56 +0000, David Laight wrote:
> From: James Bottomley
> > Sent: 16 August 2018 16:57
> > On Thu, 2018-08-16 at 16:49 +0100, David Howells wrote:
> > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > 
> > > > > The problem with that is that it means you can't load third
> > > > > party modules - such as the NVidia driver.  That's fine if
> > > > > you absolutely reject the right of people to produce third
> > > > > party drivers for the Linux kernel and absolutely require
> > > > > that they open and upstream their code if they want in.
> > > > 
> > > > So if you build your own kernel and want to load the nVidia
> > > > module, you have the key to sign it.
> > > 
> > > I think you have to assume that doing this is beyond most people.
> > 
> > As a step by step process, I agree.  However, I think we can
> > automate it to the point where you install a package and it says
> > "insert your yubikey" every time you upgrade the kernel
> 
> What about 3rd parties that want to release drivers that can be
> loadedinto 'distribution' kernels?

You'd follow the distributions external kernel module process, I think.

> We can do that for windows (provided we've paid the 'driver signing
> tax') because windows has a stable kernel DDI/DKI.
> For Linux we have to release most of the driver as a binary 'blob'
> and compile wrapper code on the target system with the correct kernel
> headers.

Right, that's the usual distribution kernel module approach.  I'm most
familiar with the SUSE DKMS packaging project, which has a lot of
automation around this.  I believe it's actually pretty easy to use
from talking to people about it.  Of course, SUSE doesn't currently use
signed kernel modules, which makes the key issue a non problem for them
...

> There is no way we could generate signed copies for every
> 'distribution' kernel - even if there was a way to get them signed.
> Even if we agreed to make our source code available it wouldn't be
> accepted for inclusion in the kernel source tree.

Well, I think for signed kernel modules the DKMS process could be
adapted to generate a private key and then install the public component
in the kernel, resign with the private key, put the private key in the
MoK database and that means the DKMS process now has a key with which
it could sign any kernel module at the request of the user.  Of course
the main problem will be guarding the private key.

However, in the above process signing is under the control of the end
user.  I'm guessing you want signing to be under the control of an
external third party or the distro, like the Microsoft case?  In that
case you have to either persuade the distros to run a signing service
or persuade them to trust a third party to run it.

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH 0/2] Fix kexec forbidding kernels signed with keys in the secondary keyring to boot
  2018-08-15 19:42           ` [PATCH 0/2] Fix kexec forbidding kernels signed with keys in the secondary keyring " Yannik Sembritzki
@ 2018-08-16 18:02             ` Linus Torvalds
  0 siblings, 0 replies; 57+ messages in thread
From: Linus Torvalds @ 2018-08-16 18:02 UTC (permalink / raw)
  To: yannik
  Cc: David Howells, Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin Forbes, Peter Jones, James Bottomley,
	Matthew Garrett, Vivek Goyal

On Wed, Aug 15, 2018 at 12:42 PM Yannik Sembritzki <yannik@sembritzki.me> wrote:
>
> I've written two patches for (a) the logical change of allowing kernels
> signed with keys in the secondary keyring to be kexec'd (b) the refactoring
> of the magic 1UL Linus requested.

Just coming back to this original submission: I've applied the
slightly modified version of this series from David Howells, so it's
upstream and marked for stable now.

                  Linus

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 15:49               ` David Howells
  2018-08-16 15:56                 ` James Bottomley
@ 2018-08-16 20:31                 ` David Howells
  2018-08-17  0:07                   ` James Bottomley
  2018-08-17  8:24                   ` David Howells
  1 sibling, 2 replies; 57+ messages in thread
From: David Howells @ 2018-08-16 20:31 UTC (permalink / raw)
  To: James Bottomley
  Cc: dhowells, Linus Torvalds, Vivek Goyal, yannik, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> As a step by step process, I agree.  However, I think we can automate
> it to the point where you install a package and it says "insert your
> yubikey" every time you upgrade the kernel

That's a very bad idea.  You train people to unlock their private key on
request.  It can be abused like one of those emails that tells you that your
account has been suspended - just follow the link and put in your password
please.

Further, you expose the unlocked key on a machine that might be compromised.

> Mehmet's patches don't require building a new kernel.  They merely
> require the added key be linked into the Red Hat built bzImage and then
> the resulting blob be signed.  You can still identify that the original
> bzImage is the Red Hat one.

No, you can't.  You might *think* that it is, but the signature you're
checking is after the image has being fiddled with by the key-adder - and that
means there's a window in which someone else can fiddle with the image too.

Mehmet's patches make sense from the reproducible kernel build PoV, but don't
actually help with the security so much since they're replacing, say, a RH
generated key with one you've generated locally, likely on the box that you
don't necessarily trust.

The bootloader would need to check the kernel image with the keys and the
image with the keys stripped off.  And if you're doing that, it doesn't make
sense to modify the kernel image, but rather load the keys separately,
possibly by an initrd=-like option on the bootloader or in your initrd.

> > That's the problem is right there.  AIUI, we *don't* want to set up a
> > third party signing process.  As I said, it potentially comes with lawyers
> > attached.
> 
> Right so generate a business pressure to overcome the legal one.

No.  We *really* don't want to do this, and the legal reasons are minor ones
like people suing us for GPL infringement[*] or because we said no.

The primary reason is that we aren't willing to just rubber stamp any old
kernel module.  The signature is, in effect, a certification.  We'd be putting
our name to something and we would want to be absolutely sure we know what it
does, review all the sources and have thoroughly tested it internally - and
we'd have to be willing to support it to some extent.  That takes a lot of
resources - and also requires the module vendor to be willing to open up their
sources to us.

We also don't want to sign the keys of third party vendors because then we
give away a certain degree of control over whatever they do with that, though
we would have the option of blacklisting it - after the fact.

[*] I've had people demand the transient private keys for Fedora kernels under
    the GPL.  They didn't seem to like the answer that I couldn't give them
    those keys because no one had them anymore; they seemed to think that this
    in some way violated their rights under the GPL.

David

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 20:31                 ` David Howells
@ 2018-08-17  0:07                   ` James Bottomley
  2018-08-17  8:24                   ` David Howells
  1 sibling, 0 replies; 57+ messages in thread
From: James Bottomley @ 2018-08-17  0:07 UTC (permalink / raw)
  To: David Howells
  Cc: Linus Torvalds, Vivek Goyal, yannik, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

On Thu, 2018-08-16 at 21:31 +0100, David Howells wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > As a step by step process, I agree.  However, I think we can
> > automate it to the point where you install a package and it says
> > "insert your yubikey" every time you upgrade the kernel
> 
> That's a very bad idea.  You train people to unlock their private key
> on request.  It can be abused like one of those emails that tells you
> that your account has been suspended - just follow the link and put
> in your password please.

It's exactly the same process those of us who use yubikeys for gpg or
ssh keys follow.  You insert your key, you activate the process that
needs the key, it asks for you to confirm your key, you press the
button and the operation gets performed.  Since it's what we as kernel
developers do, I don't see why it's a bad idea for others.

> Further, you expose the unlocked key on a machine that might be
> compromised.

No it doesn't; the point about using a yubikey (or any other HSM type
thing) is that the key is shielded inside the module so you get a
signature back and the key can't be compromised even if the machine is.

> > Mehmet's patches don't require building a new kernel.  They merely
> > require the added key be linked into the Red Hat built bzImage and
> > then the resulting blob be signed.  You can still identify that the
> > original bzImage is the Red Hat one.
> 
> No, you can't.  You might *think* that it is, but the signature
> you're checking is after the image has being fiddled with by the key-
> adder

Well, yes, you have to add a new signature to the combination. 
However, you can always verify that the hash without the added key is
the hash of the Red Hat supplied bzImage.

>  - and that means there's a window in which someone else can fiddle
> with the image too.

That window exists in any HSM system (including the kernel.org
yubikeys): if you can intercept my sending the authorization and hash
to be signed to the HSM, you can substitute your own hash and get back
a genuine signature over your bogus hash.  However, actually achieving
this requires a very sophisticated multi-layered attack.  I really
think if this is the level of attack you worry about then you probably
would use more sophisticated security than a yubikey.

> Mehmet's patches make sense from the reproducible kernel build PoV,
> but don't actually help with the security so much since they're
> replacing, say, a RH generated key with one you've generated locally,
> likely on the box that you don't necessarily trust.

That's why you'd use a yubikey or TPM to guard this key.  OK, I admit,
that's why I'd use one ... it's possible some users will use a simple
password protected key file and get hacked as a result.

> The bootloader would need to check the kernel image with the keys and
> the image with the keys stripped off.  And if you're doing that, it
> doesn't make sense to modify the kernel image, but rather load the
> keys separately, possibly by an initrd=-like option on the bootloader
> or in your initrd.

The reason it makes sense to add the key and resign the image is
because that's the part that's verified by secure boot, so the secure
boot system verifies and protects your added key.  The initrd is a bit
of a hole in our secure boot system because it's not signed and it can
be used to compromise the system.  However, if someone were to fix
that, absolutely you could pass the key in from a verified initrd.  I'm
also happy to add that mechanism now and hope that one day this
security hole gets plugged ...

> > > That's the problem is right there.  AIUI, we *don't* want to set
> > > up a third party signing process.  As I said, it potentially
> > > comes with lawyers attached.
> > 
> > Right so generate a business pressure to overcome the legal one.
> 
> No.  We *really* don't want to do this, and the legal reasons are
> minor ones like people suing us for GPL infringement[*] or because we
> said no.

So now you're telling me it's actually not a legal problem it's a
business process problem ... 

> The primary reason is that we aren't willing to just rubber stamp any
> old kernel module.  The signature is, in effect, a
> certification.  We'd be putting our name to something and we would
> want to be absolutely sure we know what it does, review all the
> sources and have thoroughly tested it internally - and we'd have to
> be willing to support it to some extent.  That takes a lot of
> resources - and also requires the module vendor to be willing to open
> up their sources to us.

OK, so I agree, depending on the level of assurance you want to give in
the signature that it's an effort.  However, the fact that Red Hat is
unwilling to undertake the effort yet still wants to claim security
based on module signatures means your only available policy is that
third party modules aren't allowed at all.  

What's wrong with simply telling users who want to get around this that
they have to build their own kernel?  Red Hat is definitely off the
hook in that case as well.  I know it's hard but surely you want to
deter customers from doing it.

> We also don't want to sign the keys of third party vendors because
> then we give away a certain degree of control over whatever they do
> with that, though we would have the option of blacklisting it - after
> the fact.

This would precisely mirror the reason why I don't trust the UEFI ODM
keys.

James


> [*] I've had people demand the transient private keys for Fedora
> kernels under
>     the GPL.  They didn't seem to like the answer that I couldn't
> give them
>     those keys because no one had them anymore; they seemed to think
> that this
>     in some way violated their rights under the GPL.



^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 20:31                 ` David Howells
  2018-08-17  0:07                   ` James Bottomley
@ 2018-08-17  8:24                   ` David Howells
  2018-08-17 14:58                     ` James Bottomley
  1 sibling, 1 reply; 57+ messages in thread
From: David Howells @ 2018-08-17  8:24 UTC (permalink / raw)
  To: James Bottomley
  Cc: dhowells, Linus Torvalds, Vivek Goyal, yannik, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> > > As a step by step process, I agree.  However, I think we can
> > > automate it to the point where you install a package and it says
> > > "insert your yubikey" every time you upgrade the kernel
> > 
> > That's a very bad idea.  You train people to unlock their private key
> > on request.  It can be abused like one of those emails that tells you
> > that your account has been suspended - just follow the link and put
> > in your password please.
> 
> It's exactly the same process those of us who use yubikeys for gpg or
> ssh keys follow.  You insert your key, you activate the process that
> needs the key, it asks for you to confirm your key, you press the
> button and the operation gets performed.  Since it's what we as kernel
> developers do, I don't see why it's a bad idea for others.

You've completely missed the point.

You need to think from the PoV of an ordinary user.  Imagine the system does
an automatic upgrade and wants to upgrade the kernel.  It pops up a dialogue
box saying "please put in your yubikey and enter your password here"[**].  It
might do this on a regular basis - and you can be sure that some users at
least will become accustomed to just doing this when their computer tells them
too.  *That* is the problem.

Now they follow a link to a dodgy website that causes some code to be
downloaded and run.  *It* now pops up a dialogue box that looks exactly like
the kernel installer's dialogue that says "please put in your yubikey and
enter your password here".  But now we've trained those users to do this on
demand...

PEBKAC[*].

[*] Note that I'm not trying to slight ordinary users here, it's more a fact
    of psychology.  As a distribution, it's our responsibility to try and
    protect them as best we can - and training them to unthinkingly bypass the
    security mechanisms isn't in anyone's best interests.

[**] Note also that I've never actually used a yubikey[***], so I'm not sure
     whether it takes a password or has some other mechanism to unlock the
     key.

[***] We also don't want to require that someone buys and keeps track of a
      yubikey to be able to use, say, the NVidia driver with Fedora/RHEL.
      Using the TPM if installed would be preferable because it's harder to
      lose.

We also don't necessarily want to encourage ordinary users to fiddle with the
system key databases unless they really know what they are doing.  There've
been cases where doing this has bricked a machine because the BIOS is buggy.
Now I will grant, since you'll probably raise it if I don't;-), that this
might be a good reason *for* having our own third party signing key as we
could then build the key into our kernels.

But if they use a yubikey, they have to get the public key from there into the
system key list or possibly the yubikey has to be accessed by the bootloader.
The same for the TPM.

> > Further, you expose the unlocked key on a machine that might be
> > compromised.
> 
> No it doesn't; the point about using a yubikey (or any other HSM type
> thing) is that the key is shielded inside the module so you get a
> signature back and the key can't be compromised even if the machine is.

Yes, you do.  Note that I don't mean necessarily exposing the actual key
material, but you do have to make it available for use - which means someone
can then use it and there's a window in which it is available for that use.
If there wasn't, it would be useless.

> > No, you can't.  You might *think* that it is, but the signature
> > you're checking is after the image has being fiddled with by the key-
> > adder
> 
> Well, yes, you have to add a new signature to the combination. 
> However, you can always verify that the hash without the added key is
> the hash of the Red Hat supplied bzImage.

At what point would you do this?  You have to assume that your userspace tools
can be compromised unless they are also verified (signature/IMA).  No, this
needs to be done by the bootloader.

Sometimes I feel I've spent too much time talking to people about security and
their paranoia is rubbing off... ;-)

David

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-17  8:24                   ` David Howells
@ 2018-08-17 14:58                     ` James Bottomley
  2018-08-17 15:42                       ` Justin Forbes
  0 siblings, 1 reply; 57+ messages in thread
From: James Bottomley @ 2018-08-17 14:58 UTC (permalink / raw)
  To: David Howells
  Cc: Linus Torvalds, Vivek Goyal, yannik, Thomas Gleixner,
	Ingo Molnar, Peter Anvin, the arch/x86 maintainers,
	Linux Kernel Mailing List, Dave Young, Baoquan He, Justin Forbes,
	Peter Jones, Matthew Garrett

On Fri, 2018-08-17 at 09:24 +0100, David Howells wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > > > As a step by step process, I agree.  However, I think we can
> > > > automate it to the point where you install a package and it
> > > > says "insert your yubikey" every time you upgrade the kernel
> > > 
> > > That's a very bad idea.  You train people to unlock their private
> > > key on request.  It can be abused like one of those emails that
> > > tells you that your account has been suspended - just follow the
> > > link and put in your password please.
> > 
> > It's exactly the same process those of us who use yubikeys for gpg
> > or ssh keys follow.  You insert your key, you activate the process
> > that needs the key, it asks for you to confirm your key, you press
> > the button and the operation gets performed.  Since it's what we as
> > kernel developers do, I don't see why it's a bad idea for others.
> 
> You've completely missed the point.
> 
> You need to think from the PoV of an ordinary user.  Imagine the
> system does an automatic upgrade and wants to upgrade the kernel.  It
> pops up a dialogue box saying "please put in your yubikey and enter
> your password here"[**].  It might do this on a regular basis - and
> you can be sure that some users at least will become accustomed to
> just doing this when their computer tells them too.  *That* is the
> problem.

I was assuming the kernel would get pinned, so when the system
automatically updates it installs everything else but tells you you
have to do the kernel manually.  Presumably installing the add your own
key package would do this.

The point I'm making isn't that everything will just magically work,
it's that we can design a process for a user to update a distro kernel
while installing their own key.  I'm sure you can imagine hundreds of
bad processes that encourage wrong behaviour, but the realistic answer
is we just wouldn't use them.


> Now they follow a link to a dodgy website that causes some code to be
> downloaded and run.  *It* now pops up a dialogue box that looks
> exactly like the kernel installer's dialogue that says "please put in
> your yubikey and enter your password here".  But now we've trained
> those users to do this on demand...
> 
> PEBKAC[*].
> 
> [*] Note that I'm not trying to slight ordinary users here, it's more
> a fact
>     of psychology.  As a distribution, it's our responsibility to try
> and
>     protect them as best we can - and training them to unthinkingly
> bypass the
>     security mechanisms isn't in anyone's best interests.
> 
> [**] Note also that I've never actually used a yubikey[***], so I'm
> not sure
>      whether it takes a password or has some other mechanism to
> unlock the
>      key.
> 
> [***] We also don't want to require that someone buys and keeps track
> of a
>       yubikey to be able to use, say, the NVidia driver with
> Fedora/RHEL.
>       Using the TPM if installed would be preferable because it's
> harder to
>       lose.

I'm perfectly happy to use the TPM as well, and to help design
processes around it (although I think we'll need both yubikey and TPM).
 I also have to confess whenever I say yubikey in the context of kernel
processes I'm making the caveat that everyone else uses a yubikey but I
use my TPM based keys.

> We also don't necessarily want to encourage ordinary users to fiddle
> with the system key databases unless they really know what they are
> doing.  There've been cases where doing this has bricked a machine
> because the BIOS is buggy. Now I will grant, since you'll probably
> raise it if I don't;-), that this might be a good reason *for* having
> our own third party signing key as we could then build the key into
> our kernels.
> 
> But if they use a yubikey, they have to get the public key from there
> into the system key list or possibly the yubikey has to be accessed
> by the bootloader. The same for the TPM.

For security reasons, a Yubikey should only be connected when you need
it to sign something.  The TPM you can assume is always available.

> > > Further, you expose the unlocked key on a machine that might be
> > > compromised.
> > 
> > No it doesn't; the point about using a yubikey (or any other HSM
> > type thing) is that the key is shielded inside the module so you
> > get a signature back and the key can't be compromised even if the
> > machine is.
> 
> Yes, you do.  Note that I don't mean necessarily exposing the actual
> key material, but you do have to make it available for use - which
> means someone can then use it and there's a window in which it is
> available for that use. If there wasn't, it would be useless.

Right so it's the byzantine exploit window shared by all HSMs
(interception between authorization and use).  We take that risk in our
kernel development security model because we think it's reasonably low
... the same is true for this use case.

> > > No, you can't.  You might *think* that it is, but the signature
> > > you're checking is after the image has being fiddled with by the
> > > key-adder
> > 
> > Well, yes, you have to add a new signature to the combination. 
> > However, you can always verify that the hash without the added key
> > is the hash of the Red Hat supplied bzImage.
> 
> At what point would you do this?  You have to assume that your
> userspace tools can be compromised unless they are also verified
> (signature/IMA).  No, this needs to be done by the bootloader.

? THe kernel is verified by the signature over the whole, which is
bzImage plus new key; that's all the security the system needs.  The
only verification needed of the original RH bzImage is presumably for
Red Hat to confirm support or some forensic diagnostic ... it's not a
security relevant thing.

> Sometimes I feel I've spent too much time talking to people about
> security and their paranoia is rubbing off... ;-)

When dealing with security people the trick is to separate reasonable
from unreasonable paranoia ...

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-17 14:58                     ` James Bottomley
@ 2018-08-17 15:42                       ` Justin Forbes
  2018-08-17 16:02                         ` James Bottomley
  0 siblings, 1 reply; 57+ messages in thread
From: Justin Forbes @ 2018-08-17 15:42 UTC (permalink / raw)
  To: James Bottomley
  Cc: David Howells, Linus Torvalds, Vivek Goyal, yannik,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Peter Jones, Matthew Garrett

On Fri, Aug 17, 2018 at 9:58 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2018-08-17 at 09:24 +0100, David Howells wrote:
>> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>>
>> > > > As a step by step process, I agree.  However, I think we can
>> > > > automate it to the point where you install a package and it
>> > > > says "insert your yubikey" every time you upgrade the kernel
>> > >
>> > > That's a very bad idea.  You train people to unlock their private
>> > > key on request.  It can be abused like one of those emails that
>> > > tells you that your account has been suspended - just follow the
>> > > link and put in your password please.
>> >
>> > It's exactly the same process those of us who use yubikeys for gpg
>> > or ssh keys follow.  You insert your key, you activate the process
>> > that needs the key, it asks for you to confirm your key, you press
>> > the button and the operation gets performed.  Since it's what we as
>> > kernel developers do, I don't see why it's a bad idea for others.
>>
>> You've completely missed the point.
>>
>> You need to think from the PoV of an ordinary user.  Imagine the
>> system does an automatic upgrade and wants to upgrade the kernel.  It
>> pops up a dialogue box saying "please put in your yubikey and enter
>> your password here"[**].  It might do this on a regular basis - and
>> you can be sure that some users at least will become accustomed to
>> just doing this when their computer tells them too.  *That* is the
>> problem.
>
> I was assuming the kernel would get pinned, so when the system
> automatically updates it installs everything else but tells you you
> have to do the kernel manually.  Presumably installing the add your own
> key package would do this.
>
> The point I'm making isn't that everything will just magically work,
> it's that we can design a process for a user to update a distro kernel
> while installing their own key.  I'm sure you can imagine hundreds of
> bad processes that encourage wrong behaviour, but the realistic answer
> is we just wouldn't use them.
>
>
>> Now they follow a link to a dodgy website that causes some code to be
>> downloaded and run.  *It* now pops up a dialogue box that looks
>> exactly like the kernel installer's dialogue that says "please put in
>> your yubikey and enter your password here".  But now we've trained
>> those users to do this on demand...
>>
>> PEBKAC[*].
>>
>> [*] Note that I'm not trying to slight ordinary users here, it's more
>> a fact
>>     of psychology.  As a distribution, it's our responsibility to try
>> and
>>     protect them as best we can - and training them to unthinkingly
>> bypass the
>>     security mechanisms isn't in anyone's best interests.
>>
>> [**] Note also that I've never actually used a yubikey[***], so I'm
>> not sure
>>      whether it takes a password or has some other mechanism to
>> unlock the
>>      key.
>>
>> [***] We also don't want to require that someone buys and keeps track
>> of a
>>       yubikey to be able to use, say, the NVidia driver with
>> Fedora/RHEL.
>>       Using the TPM if installed would be preferable because it's
>> harder to
>>       lose.
>
> I'm perfectly happy to use the TPM as well, and to help design
> processes around it (although I think we'll need both yubikey and TPM).
>  I also have to confess whenever I say yubikey in the context of kernel
> processes I'm making the caveat that everyone else uses a yubikey but I
> use my TPM based keys.
>
>> We also don't necessarily want to encourage ordinary users to fiddle
>> with the system key databases unless they really know what they are
>> doing.  There've been cases where doing this has bricked a machine
>> because the BIOS is buggy. Now I will grant, since you'll probably
>> raise it if I don't;-), that this might be a good reason *for* having
>> our own third party signing key as we could then build the key into
>> our kernels.
>>
>> But if they use a yubikey, they have to get the public key from there
>> into the system key list or possibly the yubikey has to be accessed
>> by the bootloader. The same for the TPM.
>
> For security reasons, a Yubikey should only be connected when you need
> it to sign something.  The TPM you can assume is always available.
>
This is absolutely correct, the important word here being *should*.
The reality is, with weekly kernel updates and various uses of the
Yubikey, the average user is just going to leave it connected. We can
lay out best practices all we want, but it seems pretty obvious that
most users go with convenient or default.  I really wish we could
change that, but it is unlikely.  As a result, we have to make the
convenient or default path also fairly secure.

>> > > Further, you expose the unlocked key on a machine that might be
>> > > compromised.
>> >
>> > No it doesn't; the point about using a yubikey (or any other HSM
>> > type thing) is that the key is shielded inside the module so you
>> > get a signature back and the key can't be compromised even if the
>> > machine is.
>>
>> Yes, you do.  Note that I don't mean necessarily exposing the actual
>> key material, but you do have to make it available for use - which
>> means someone can then use it and there's a window in which it is
>> available for that use. If there wasn't, it would be useless.
>
> Right so it's the byzantine exploit window shared by all HSMs
> (interception between authorization and use).  We take that risk in our
> kernel development security model because we think it's reasonably low
> ... the same is true for this use case.
>
>> > > No, you can't.  You might *think* that it is, but the signature
>> > > you're checking is after the image has being fiddled with by the
>> > > key-adder
>> >
>> > Well, yes, you have to add a new signature to the combination.
>> > However, you can always verify that the hash without the added key
>> > is the hash of the Red Hat supplied bzImage.
>>
>> At what point would you do this?  You have to assume that your
>> userspace tools can be compromised unless they are also verified
>> (signature/IMA).  No, this needs to be done by the bootloader.
>
> ? THe kernel is verified by the signature over the whole, which is
> bzImage plus new key; that's all the security the system needs.  The
> only verification needed of the original RH bzImage is presumably for
> Red Hat to confirm support or some forensic diagnostic ... it's not a
> security relevant thing.
>
>> Sometimes I feel I've spent too much time talking to people about
>> security and their paranoia is rubbing off... ;-)
>
> When dealing with security people the trick is to separate reasonable
> from unreasonable paranoia ...
>
> James
>

^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-17 15:42                       ` Justin Forbes
@ 2018-08-17 16:02                         ` James Bottomley
  0 siblings, 0 replies; 57+ messages in thread
From: James Bottomley @ 2018-08-17 16:02 UTC (permalink / raw)
  To: Justin Forbes
  Cc: David Howells, Linus Torvalds, Vivek Goyal, yannik,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Peter Jones, Matthew Garrett

On Fri, 2018-08-17 at 10:42 -0500, Justin Forbes wrote:
> On Fri, Aug 17, 2018 at 9:58 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Fri, 2018-08-17 at 09:24 +0100, David Howells wrote:
[...]
> > > We also don't necessarily want to encourage ordinary users to
> > > fiddle with the system key databases unless they really know what
> > > they are doing.  There've been cases where doing this has bricked
> > > a machine because the BIOS is buggy. Now I will grant, since
> > > you'll probably raise it if I don't;-), that this might be a good
> > > reason *for* having our own third party signing key as we could
> > > then build the key into our kernels.
> > > 
> > > But if they use a yubikey, they have to get the public key from
> > > there into the system key list or possibly the yubikey has to be
> > > accessed by the bootloader. The same for the TPM.
> > 
> > For security reasons, a Yubikey should only be connected when you
> > need it to sign something.  The TPM you can assume is always
> > available.
> > 
> 
> This is absolutely correct, the important word here being *should*.
> The reality is, with weekly kernel updates and various uses of the
> Yubikey, the average user is just going to leave it connected. We can
> lay out best practices all we want, but it seems pretty obvious that
> most users go with convenient or default.  I really wish we could
> change that, but it is unlikely.  As a result, we have to make the
> convenient or default path also fairly secure.

Imagining the worst scenario with the most stupid user doesn't prove
anything.  We have sensible users who want to achieve security.  Our
job is to design processes that help them with that goal.  Just because
someone could do something stupid is no excuse for not helping the
sensible users.  I admit this "design usable processes to help sensible
users with security" is hard (it's the reason why we've never been able
to deploy the TPM successfully for the majority of users) but they're
not impossible.

Why don't I tell you how it currently works here so you can see how
easily it would slot into a cloud deployment process?  We're designing
CI/CD images for physical systems (i.e. images that are immutable, like
those of containers).  The signing occurs in a special secure area
using a code signing service (so we don't even use a local key dongle)
after the image has been tested in the DevOps cycle.  Today, because
the image is immutable, the initrd it built to support all the cloud
physical systems and thus it can actually be linked in to the bzImage
and the entire thing signed, which solves the initrd security problem
neatly for us.  Linking a key into the bzImage as well is a trivial
addition for us, as you can see.  For us, by the way, you can also see
that providing a key in the initrd would work fine as well.

Since our images are immutable, we don't allow automatic updates.  Even
for mutable images they're a huge security and downtime risk in a cloud
environment because you just don't know what impact they'll have
without testing them.  So even before we moved to immutable images, the
process has always been to disable updates, test out the distro
proposed update first and then deploy if all tests pass.

I'm fairly certain all cloud providers have similar processes.

If we can run this process at industrial scale, there's no reason a
simpler version can't be designed for a user to follow.

James


^ permalink raw reply	[flat|nested] 57+ messages in thread

* Re: [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot
  2018-08-16 14:59           ` James Bottomley
@ 2018-08-17 17:00             ` Alan Cox
  0 siblings, 0 replies; 57+ messages in thread
From: Alan Cox @ 2018-08-17 17:00 UTC (permalink / raw)
  To: James Bottomley
  Cc: David Howells, Vivek Goyal, Yannik Sembritzki, Linus Torvalds,
	Thomas Gleixner, Ingo Molnar, Peter Anvin,
	the arch/x86 maintainers, Linux Kernel Mailing List, Dave Young,
	Baoquan He, Justin M. Forbes, Peter Jones, Matthew Garrett

> I'm actually not talking about UEFI storage, just the UEFI secure boot
> database.  I think we might come up with a viable model for adding keys
> from a UEFI variable that isn't part of the secure boot database.

You also need to be able to remove or not trust the existing ones
including the Microsoft keys. Not trust probably makes the most sense.

If you trust those you are dead already, because there are existing
signed kernel images and Windows boot images that have known remote ring 0
exploits so I can just boot one of those and own you.

Since you are never going to make a Fedora update blacklist the previous
kernel image it's also not going to get fixed because it's a usability
problem.

> The reason for keeping this boundary is to do with the politics of
> breaches.  If we get a breach to the secure boot boundary, Microsoft
> and all the ODMs will help us hunt it down and plug it (They have no
> option because Windows is threatened by any breach to that boundary). 
> If we use the keys beyond the secure boot boundary and get a breach
> that only affects our use case no-one will help us because no-one will
> care.

Also the politics of control. A world where Red Hat, Microsoft and some
other parties together effectively control who gets to load modules into
a free OS for most users is not a good one - whatever the module licence.

Plus I'd have thought if your lawyers are scared of signing keys they'll
be even more terrified of the competition law impacts of gatekeeping 8)

Alan

^ permalink raw reply	[flat|nested] 57+ messages in thread

end of thread, other threads:[~2018-08-17 17:00 UTC | newest]

Thread overview: 57+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-15 10:00 [PATCH] Fix kexec forbidding kernels signed with custom platform keys to boot Yannik Sembritzki
2018-08-15 16:54 ` Linus Torvalds
2018-08-15 17:27   ` Yannik Sembritzki
2018-08-15 17:37     ` Yannik Sembritzki
2018-08-15 17:42     ` Vivek Goyal
2018-08-15 18:44       ` Yannik Sembritzki
2018-08-15 18:58       ` Vivek Goyal
2018-08-15 19:06         ` Yannik Sembritzki
2018-08-15 19:49           ` Vivek Goyal
2018-08-15 20:47             ` Linus Torvalds
2018-08-15 20:53               ` James Bottomley
2018-08-15 21:08               ` Yannik Sembritzki
2018-08-15 21:13                 ` James Bottomley
2018-08-15 21:31                   ` Yannik Sembritzki
2018-08-15 21:40                     ` James Bottomley
2018-08-15 21:50                       ` Yannik Sembritzki
2018-08-15 21:57                     ` Vivek Goyal
2018-08-15 22:14                       ` Yannik Sembritzki
2018-08-15 21:52                   ` Vivek Goyal
2018-08-15 21:57                     ` James Bottomley
2018-08-15 21:14                 ` Linus Torvalds
2018-08-16 13:51             ` David Howells
2018-08-16 15:16               ` James Bottomley
2018-08-16 15:42                 ` James Bottomley
2018-08-16 15:49               ` David Howells
2018-08-16 15:56                 ` James Bottomley
2018-08-16 16:56                   ` David Laight
2018-08-16 17:15                     ` James Bottomley
2018-08-16 20:31                 ` David Howells
2018-08-17  0:07                   ` James Bottomley
2018-08-17  8:24                   ` David Howells
2018-08-17 14:58                     ` James Bottomley
2018-08-17 15:42                       ` Justin Forbes
2018-08-17 16:02                         ` James Bottomley
2018-08-16  0:52       ` Dave Young
2018-08-16  0:55         ` Dave Young
2018-08-16 12:13       ` David Howells
2018-08-16 14:22         ` James Bottomley
2018-08-16 14:43         ` David Howells
2018-08-16 14:59           ` James Bottomley
2018-08-17 17:00             ` Alan Cox
2018-08-15 17:45     ` Linus Torvalds
2018-08-15 18:19       ` Yannik Sembritzki
2018-08-15 18:22         ` Linus Torvalds
2018-08-15 19:42           ` [PATCH 0/2] Fix kexec forbidding kernels signed with keys in the secondary keyring " Yannik Sembritzki
2018-08-16 18:02             ` Linus Torvalds
2018-08-15 19:42           ` [PATCH 1/2] " Yannik Sembritzki
2018-08-15 19:42           ` [PATCH 2/2] Replace magic for trusting the secondary keyring with #define Yannik Sembritzki
2018-08-15 21:14             ` kbuild test robot
2018-08-15 21:19               ` [PATCH 2/2] [FIXED] " Yannik Sembritzki
2018-08-15 22:01                 ` Linus Torvalds
2018-08-15 22:07                   ` [PATCH 2/2] [FIXED v2] " Yannik Sembritzki
2018-08-16  1:11                     ` Dave Young
2018-08-16  7:43                       ` Yannik Sembritzki
2018-08-16  8:02                         ` Dave Young
2018-08-16  8:20                           ` Greg Kroah-Hartman
2018-08-16 12:46                       ` Vivek Goyal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).