* Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys
@ 2019-01-09 1:33 ` Dave Young
0 siblings, 0 replies; 20+ messages in thread
From: Dave Young @ 2019-01-09 1:33 UTC (permalink / raw)
To: Mimi Zohar
Cc: jwboyer, Kairui Song, ebiggers, nayna, kexec, linux-kernel,
jmorris, dhowells, linux-security-module, keyrings,
linux-integrity, dwmw2, bauerman, serge
CC kexec list
On 01/08/19 at 10:18am, Mimi Zohar wrote:
> [Cc'ing the LSM and integrity mailing lists]
>
> Repeating my comment on PATCH 0/1 here with the expanded set of
> mailing lists.
>
> The builtin and secondary keyrings have a signature change of trust
> rooted in the signed kernel image. Adding the pre-boot keys to the
> secondary keyring breaks that signature chain of trust.
>
> Please do NOT add the pre-boot "platform" keys to the secondary
> keyring.
If we regard kexec as a bootloader, it sounds natural to use the
platform key to verify the signature with kexec_file_load syscall.
It will be hard for user to manually sign a kernel and import the key
then to reuse kexec_file_load.
I think we do not care if platform key can be added to secondary or not,
any suggestions how can kexec_file to use the platform key?
>
> Mimi
>
>
> On Tue, 2019-01-08 at 16:12 +0800, Kairui Song wrote:
> > Currently kexec may need to verify the kerne image, and the kernel image
> > could be signed with third part keys which are provided by paltform or
> > firmware (eg. stored in MokListRT EFI variable). And the same time,
> > kexec_file_load will only verify the image agains .builtin_trusted_keys
> > or .secondary_trusted_keys according to configuration, but there is no
> > way for kexec_file_load to verify the image against any third part keys
> > mentioned above.
> >
> > In ea93102f3224 ('integrity: Define a trusted platform keyring') a
> > .platform keyring is introduced to store the keys provided by platform
> > or firmware. And with a few following commits including 15ea0e1e3e185
> > ('efi: Import certificates from UEFI Secure Boot'), now keys required to
> > verify the image is being imported to .paltform keyring, and later
> > IMA-appraisal could access the keyring and verify the image.
> >
> > This patch links the .platform keyring to .secondary_trusted_keys so
> > kexec_file_load could also leverage the .platform keyring to verify the
> > kernel image.
> >
> > Signed-off-by: Kairui Song <kasong@redhat.com>
> > ---
> > certs/system_keyring.c | 30 ++++++++++++++++++++++++++++++
> > include/keys/platform_keyring.h | 12 ++++++++++++
> > security/integrity/digsig.c | 7 +++++++
> > 3 files changed, 49 insertions(+)
> > create mode 100644 include/keys/platform_keyring.h
> >
> > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > index 81728717523d..dcef0259e149 100644
> > --- a/certs/system_keyring.c
> > +++ b/certs/system_keyring.c
> > @@ -18,12 +18,14 @@
> > #include <linux/verification.h>
> > #include <keys/asymmetric-type.h>
> > #include <keys/system_keyring.h>
> > +#include <keys/platform_keyring.h>
> > #include <crypto/pkcs7.h>
> >
> > static struct key *builtin_trusted_keys;
> > #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> > static struct key *secondary_trusted_keys;
> > #endif
> > +static struct key *platform_keys = NULL;
> >
> > extern __initconst const u8 system_certificate_list[];
> > extern __initconst const unsigned long system_certificate_list_size;
> > @@ -67,6 +69,12 @@ int restrict_link_by_builtin_and_secondary_trusted(
> > /* Allow the builtin keyring to be added to the secondary */
> > return 0;
> >
> > + if (type == &key_type_keyring &&
> > + dest_keyring == secondary_trusted_keys &&
> > + payload == &platform_keys->payload)
> > + /* Allow the platform keyring to be added to the secondary */
> > + return 0;
> > +
> > return restrict_link_by_signature(dest_keyring, type, payload,
> > secondary_trusted_keys);
> > }
> > @@ -188,6 +196,28 @@ static __init int load_system_certificate_list(void)
> > }
> > late_initcall(load_system_certificate_list);
> >
> > +#if defined(CONFIG_INTEGRITY_PLATFORM_KEYRING) && defined(CONFIG_SECONDARY_TRUSTED_KEYRING)
> > +
> > +/*
> > + * Link .platform keyring to .secondary_trusted_key keyring
> > + */
> > +static __init int load_platform_certificate_list(void)
> > +{
> > + int ret = 0;
> > + platform_keys = integrity_get_platform_keyring();
> > + if (!platform_keys) {
> > + return 0;
> > + }
> > + ret = key_link(secondary_trusted_keys, platform_keys);
> > + if (ret < 0) {
> > + pr_err("Failed to link platform keyring: %d", ret);
> > + }
> > + return 0;
> > +}
> > +late_initcall(load_platform_certificate_list);
> > +
> > +#endif
> > +
> > #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> >
> > /**
> > diff --git a/include/keys/platform_keyring.h b/include/keys/platform_keyring.h
> > new file mode 100644
> > index 000000000000..4f92ed6c0b42
> > --- /dev/null
> > +++ b/include/keys/platform_keyring.h
> > @@ -0,0 +1,12 @@
> > +#ifndef _KEYS_PLATFORM_KEYRING_H
> > +#define _KEYS_PLATFORM_KEYRING_H
> > +
> > +#include <linux/key.h>
> > +
> > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > +
> > +extern const struct key* __init integrity_get_platform_keyring(void);
> > +
> > +#endif /* CONFIG_INTEGRITY_PLATFORM_KEYRING */
> > +
> > +#endif /* _KEYS_SYSTEM_KEYRING_H */
> > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > index f45d6edecf99..397758d4f12d 100644
> > --- a/security/integrity/digsig.c
> > +++ b/security/integrity/digsig.c
> > @@ -176,3 +176,10 @@ int __init integrity_load_cert(const unsigned int id, const char *source,
> > pr_info("Loading X.509 certificate: %s\n", source);
> > return integrity_add_key(id, data, len, perm);
> > }
> > +
> > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > +struct key* __init integrity_get_platform_keyring(void)
> > +{
> > + return keyring[INTEGRITY_KEYRING_PLATFORM];
> > +}
> > +#endif
>
Thanks
Dave
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys
@ 2019-01-09 1:33 ` Dave Young
0 siblings, 0 replies; 20+ messages in thread
From: Dave Young @ 2019-01-09 1:33 UTC (permalink / raw)
To: Mimi Zohar
Cc: Kairui Song, linux-kernel, dhowells, dwmw2, jwboyer, keyrings,
jmorris, serge, bauerman, ebiggers, nayna, linux-security-module,
linux-integrity, kexec
CC kexec list
On 01/08/19 at 10:18am, Mimi Zohar wrote:
> [Cc'ing the LSM and integrity mailing lists]
>
> Repeating my comment on PATCH 0/1 here with the expanded set of
> mailing lists.
>
> The builtin and secondary keyrings have a signature change of trust
> rooted in the signed kernel image. Adding the pre-boot keys to the
> secondary keyring breaks that signature chain of trust.
>
> Please do NOT add the pre-boot "platform" keys to the secondary
> keyring.
If we regard kexec as a bootloader, it sounds natural to use the
platform key to verify the signature with kexec_file_load syscall.
It will be hard for user to manually sign a kernel and import the key
then to reuse kexec_file_load.
I think we do not care if platform key can be added to secondary or not,
any suggestions how can kexec_file to use the platform key?
>
> Mimi
>
>
> On Tue, 2019-01-08 at 16:12 +0800, Kairui Song wrote:
> > Currently kexec may need to verify the kerne image, and the kernel image
> > could be signed with third part keys which are provided by paltform or
> > firmware (eg. stored in MokListRT EFI variable). And the same time,
> > kexec_file_load will only verify the image agains .builtin_trusted_keys
> > or .secondary_trusted_keys according to configuration, but there is no
> > way for kexec_file_load to verify the image against any third part keys
> > mentioned above.
> >
> > In ea93102f3224 ('integrity: Define a trusted platform keyring') a
> > .platform keyring is introduced to store the keys provided by platform
> > or firmware. And with a few following commits including 15ea0e1e3e185
> > ('efi: Import certificates from UEFI Secure Boot'), now keys required to
> > verify the image is being imported to .paltform keyring, and later
> > IMA-appraisal could access the keyring and verify the image.
> >
> > This patch links the .platform keyring to .secondary_trusted_keys so
> > kexec_file_load could also leverage the .platform keyring to verify the
> > kernel image.
> >
> > Signed-off-by: Kairui Song <kasong@redhat.com>
> > ---
> > certs/system_keyring.c | 30 ++++++++++++++++++++++++++++++
> > include/keys/platform_keyring.h | 12 ++++++++++++
> > security/integrity/digsig.c | 7 +++++++
> > 3 files changed, 49 insertions(+)
> > create mode 100644 include/keys/platform_keyring.h
> >
> > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > index 81728717523d..dcef0259e149 100644
> > --- a/certs/system_keyring.c
> > +++ b/certs/system_keyring.c
> > @@ -18,12 +18,14 @@
> > #include <linux/verification.h>
> > #include <keys/asymmetric-type.h>
> > #include <keys/system_keyring.h>
> > +#include <keys/platform_keyring.h>
> > #include <crypto/pkcs7.h>
> >
> > static struct key *builtin_trusted_keys;
> > #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> > static struct key *secondary_trusted_keys;
> > #endif
> > +static struct key *platform_keys = NULL;
> >
> > extern __initconst const u8 system_certificate_list[];
> > extern __initconst const unsigned long system_certificate_list_size;
> > @@ -67,6 +69,12 @@ int restrict_link_by_builtin_and_secondary_trusted(
> > /* Allow the builtin keyring to be added to the secondary */
> > return 0;
> >
> > + if (type == &key_type_keyring &&
> > + dest_keyring == secondary_trusted_keys &&
> > + payload == &platform_keys->payload)
> > + /* Allow the platform keyring to be added to the secondary */
> > + return 0;
> > +
> > return restrict_link_by_signature(dest_keyring, type, payload,
> > secondary_trusted_keys);
> > }
> > @@ -188,6 +196,28 @@ static __init int load_system_certificate_list(void)
> > }
> > late_initcall(load_system_certificate_list);
> >
> > +#if defined(CONFIG_INTEGRITY_PLATFORM_KEYRING) && defined(CONFIG_SECONDARY_TRUSTED_KEYRING)
> > +
> > +/*
> > + * Link .platform keyring to .secondary_trusted_key keyring
> > + */
> > +static __init int load_platform_certificate_list(void)
> > +{
> > + int ret = 0;
> > + platform_keys = integrity_get_platform_keyring();
> > + if (!platform_keys) {
> > + return 0;
> > + }
> > + ret = key_link(secondary_trusted_keys, platform_keys);
> > + if (ret < 0) {
> > + pr_err("Failed to link platform keyring: %d", ret);
> > + }
> > + return 0;
> > +}
> > +late_initcall(load_platform_certificate_list);
> > +
> > +#endif
> > +
> > #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> >
> > /**
> > diff --git a/include/keys/platform_keyring.h b/include/keys/platform_keyring.h
> > new file mode 100644
> > index 000000000000..4f92ed6c0b42
> > --- /dev/null
> > +++ b/include/keys/platform_keyring.h
> > @@ -0,0 +1,12 @@
> > +#ifndef _KEYS_PLATFORM_KEYRING_H
> > +#define _KEYS_PLATFORM_KEYRING_H
> > +
> > +#include <linux/key.h>
> > +
> > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > +
> > +extern const struct key* __init integrity_get_platform_keyring(void);
> > +
> > +#endif /* CONFIG_INTEGRITY_PLATFORM_KEYRING */
> > +
> > +#endif /* _KEYS_SYSTEM_KEYRING_H */
> > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > index f45d6edecf99..397758d4f12d 100644
> > --- a/security/integrity/digsig.c
> > +++ b/security/integrity/digsig.c
> > @@ -176,3 +176,10 @@ int __init integrity_load_cert(const unsigned int id, const char *source,
> > pr_info("Loading X.509 certificate: %s\n", source);
> > return integrity_add_key(id, data, len, perm);
> > }
> > +
> > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > +struct key* __init integrity_get_platform_keyring(void)
> > +{
> > + return keyring[INTEGRITY_KEYRING_PLATFORM];
> > +}
> > +#endif
>
Thanks
Dave
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys
2019-01-09 1:33 ` Dave Young
(?)
@ 2019-01-09 2:02 ` Kairui Song
-1 siblings, 0 replies; 20+ messages in thread
From: Kairui Song @ 2019-01-09 2:02 UTC (permalink / raw)
To: Dave Young
Cc: Mimi Zohar, linux-kernel, David Howells, dwmw2, jwboyer,
keyrings, jmorris, serge, bauerman, ebiggers, nayna,
linux-security-module, linux-integrity, kexec
Thanks for the explanation Dave, my second thought is to let kexec use
the platform keyring directly, that is let kexec verify the image with
secondary/builtin keyring first then try platform keyring. And better
to make platform keyring independent of integrity subsystem, so kexec
could verify the image and don't depend on integrity. Any thought?
On Wed, Jan 9, 2019 at 9:34 AM Dave Young <dyoung@redhat.com> wrote:
>
> CC kexec list
> On 01/08/19 at 10:18am, Mimi Zohar wrote:
> > [Cc'ing the LSM and integrity mailing lists]
> >
> > Repeating my comment on PATCH 0/1 here with the expanded set of
> > mailing lists.
> >
> > The builtin and secondary keyrings have a signature change of trust
> > rooted in the signed kernel image. Adding the pre-boot keys to the
> > secondary keyring breaks that signature chain of trust.
> >
> > Please do NOT add the pre-boot "platform" keys to the secondary
> > keyring.
>
> If we regard kexec as a bootloader, it sounds natural to use the
> platform key to verify the signature with kexec_file_load syscall.
>
> It will be hard for user to manually sign a kernel and import the key
> then to reuse kexec_file_load.
>
> I think we do not care if platform key can be added to secondary or not,
> any suggestions how can kexec_file to use the platform key?
>
> >
> > Mimi
> >
> >
> > On Tue, 2019-01-08 at 16:12 +0800, Kairui Song wrote:
> > > Currently kexec may need to verify the kerne image, and the kernel image
> > > could be signed with third part keys which are provided by paltform or
> > > firmware (eg. stored in MokListRT EFI variable). And the same time,
> > > kexec_file_load will only verify the image agains .builtin_trusted_keys
> > > or .secondary_trusted_keys according to configuration, but there is no
> > > way for kexec_file_load to verify the image against any third part keys
> > > mentioned above.
> > >
> > > In ea93102f3224 ('integrity: Define a trusted platform keyring') a
> > > .platform keyring is introduced to store the keys provided by platform
> > > or firmware. And with a few following commits including 15ea0e1e3e185
> > > ('efi: Import certificates from UEFI Secure Boot'), now keys required to
> > > verify the image is being imported to .paltform keyring, and later
> > > IMA-appraisal could access the keyring and verify the image.
> > >
> > > This patch links the .platform keyring to .secondary_trusted_keys so
> > > kexec_file_load could also leverage the .platform keyring to verify the
> > > kernel image.
> > >
> > > Signed-off-by: Kairui Song <kasong@redhat.com>
> > > ---
> > > certs/system_keyring.c | 30 ++++++++++++++++++++++++++++++
> > > include/keys/platform_keyring.h | 12 ++++++++++++
> > > security/integrity/digsig.c | 7 +++++++
> > > 3 files changed, 49 insertions(+)
> > > create mode 100644 include/keys/platform_keyring.h
> > >
> > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > > index 81728717523d..dcef0259e149 100644
> > > --- a/certs/system_keyring.c
> > > +++ b/certs/system_keyring.c
> > > @@ -18,12 +18,14 @@
> > > #include <linux/verification.h>
> > > #include <keys/asymmetric-type.h>
> > > #include <keys/system_keyring.h>
> > > +#include <keys/platform_keyring.h>
> > > #include <crypto/pkcs7.h>
> > >
> > > static struct key *builtin_trusted_keys;
> > > #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> > > static struct key *secondary_trusted_keys;
> > > #endif
> > > +static struct key *platform_keys = NULL;
> > >
> > > extern __initconst const u8 system_certificate_list[];
> > > extern __initconst const unsigned long system_certificate_list_size;
> > > @@ -67,6 +69,12 @@ int restrict_link_by_builtin_and_secondary_trusted(
> > > /* Allow the builtin keyring to be added to the secondary */
> > > return 0;
> > >
> > > + if (type = &key_type_keyring &&
> > > + dest_keyring = secondary_trusted_keys &&
> > > + payload = &platform_keys->payload)
> > > + /* Allow the platform keyring to be added to the secondary */
> > > + return 0;
> > > +
> > > return restrict_link_by_signature(dest_keyring, type, payload,
> > > secondary_trusted_keys);
> > > }
> > > @@ -188,6 +196,28 @@ static __init int load_system_certificate_list(void)
> > > }
> > > late_initcall(load_system_certificate_list);
> > >
> > > +#if defined(CONFIG_INTEGRITY_PLATFORM_KEYRING) && defined(CONFIG_SECONDARY_TRUSTED_KEYRING)
> > > +
> > > +/*
> > > + * Link .platform keyring to .secondary_trusted_key keyring
> > > + */
> > > +static __init int load_platform_certificate_list(void)
> > > +{
> > > + int ret = 0;
> > > + platform_keys = integrity_get_platform_keyring();
> > > + if (!platform_keys) {
> > > + return 0;
> > > + }
> > > + ret = key_link(secondary_trusted_keys, platform_keys);
> > > + if (ret < 0) {
> > > + pr_err("Failed to link platform keyring: %d", ret);
> > > + }
> > > + return 0;
> > > +}
> > > +late_initcall(load_platform_certificate_list);
> > > +
> > > +#endif
> > > +
> > > #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> > >
> > > /**
> > > diff --git a/include/keys/platform_keyring.h b/include/keys/platform_keyring.h
> > > new file mode 100644
> > > index 000000000000..4f92ed6c0b42
> > > --- /dev/null
> > > +++ b/include/keys/platform_keyring.h
> > > @@ -0,0 +1,12 @@
> > > +#ifndef _KEYS_PLATFORM_KEYRING_H
> > > +#define _KEYS_PLATFORM_KEYRING_H
> > > +
> > > +#include <linux/key.h>
> > > +
> > > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > > +
> > > +extern const struct key* __init integrity_get_platform_keyring(void);
> > > +
> > > +#endif /* CONFIG_INTEGRITY_PLATFORM_KEYRING */
> > > +
> > > +#endif /* _KEYS_SYSTEM_KEYRING_H */
> > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > > index f45d6edecf99..397758d4f12d 100644
> > > --- a/security/integrity/digsig.c
> > > +++ b/security/integrity/digsig.c
> > > @@ -176,3 +176,10 @@ int __init integrity_load_cert(const unsigned int id, const char *source,
> > > pr_info("Loading X.509 certificate: %s\n", source);
> > > return integrity_add_key(id, data, len, perm);
> > > }
> > > +
> > > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > > +struct key* __init integrity_get_platform_keyring(void)
> > > +{
> > > + return keyring[INTEGRITY_KEYRING_PLATFORM];
> > > +}
> > > +#endif
> >
>
> Thanks
> Dave
--
Best Regards,
Kairui Song
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys
@ 2019-01-09 2:02 ` Kairui Song
0 siblings, 0 replies; 20+ messages in thread
From: Kairui Song @ 2019-01-09 2:02 UTC (permalink / raw)
To: Dave Young
Cc: jwboyer, ebiggers, nayna, kexec, linux-kernel, Mimi Zohar,
jmorris, David Howells, linux-security-module, keyrings,
linux-integrity, dwmw2, bauerman, serge
Thanks for the explanation Dave, my second thought is to let kexec use
the platform keyring directly, that is let kexec verify the image with
secondary/builtin keyring first then try platform keyring. And better
to make platform keyring independent of integrity subsystem, so kexec
could verify the image and don't depend on integrity. Any thought?
On Wed, Jan 9, 2019 at 9:34 AM Dave Young <dyoung@redhat.com> wrote:
>
> CC kexec list
> On 01/08/19 at 10:18am, Mimi Zohar wrote:
> > [Cc'ing the LSM and integrity mailing lists]
> >
> > Repeating my comment on PATCH 0/1 here with the expanded set of
> > mailing lists.
> >
> > The builtin and secondary keyrings have a signature change of trust
> > rooted in the signed kernel image. Adding the pre-boot keys to the
> > secondary keyring breaks that signature chain of trust.
> >
> > Please do NOT add the pre-boot "platform" keys to the secondary
> > keyring.
>
> If we regard kexec as a bootloader, it sounds natural to use the
> platform key to verify the signature with kexec_file_load syscall.
>
> It will be hard for user to manually sign a kernel and import the key
> then to reuse kexec_file_load.
>
> I think we do not care if platform key can be added to secondary or not,
> any suggestions how can kexec_file to use the platform key?
>
> >
> > Mimi
> >
> >
> > On Tue, 2019-01-08 at 16:12 +0800, Kairui Song wrote:
> > > Currently kexec may need to verify the kerne image, and the kernel image
> > > could be signed with third part keys which are provided by paltform or
> > > firmware (eg. stored in MokListRT EFI variable). And the same time,
> > > kexec_file_load will only verify the image agains .builtin_trusted_keys
> > > or .secondary_trusted_keys according to configuration, but there is no
> > > way for kexec_file_load to verify the image against any third part keys
> > > mentioned above.
> > >
> > > In ea93102f3224 ('integrity: Define a trusted platform keyring') a
> > > .platform keyring is introduced to store the keys provided by platform
> > > or firmware. And with a few following commits including 15ea0e1e3e185
> > > ('efi: Import certificates from UEFI Secure Boot'), now keys required to
> > > verify the image is being imported to .paltform keyring, and later
> > > IMA-appraisal could access the keyring and verify the image.
> > >
> > > This patch links the .platform keyring to .secondary_trusted_keys so
> > > kexec_file_load could also leverage the .platform keyring to verify the
> > > kernel image.
> > >
> > > Signed-off-by: Kairui Song <kasong@redhat.com>
> > > ---
> > > certs/system_keyring.c | 30 ++++++++++++++++++++++++++++++
> > > include/keys/platform_keyring.h | 12 ++++++++++++
> > > security/integrity/digsig.c | 7 +++++++
> > > 3 files changed, 49 insertions(+)
> > > create mode 100644 include/keys/platform_keyring.h
> > >
> > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > > index 81728717523d..dcef0259e149 100644
> > > --- a/certs/system_keyring.c
> > > +++ b/certs/system_keyring.c
> > > @@ -18,12 +18,14 @@
> > > #include <linux/verification.h>
> > > #include <keys/asymmetric-type.h>
> > > #include <keys/system_keyring.h>
> > > +#include <keys/platform_keyring.h>
> > > #include <crypto/pkcs7.h>
> > >
> > > static struct key *builtin_trusted_keys;
> > > #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> > > static struct key *secondary_trusted_keys;
> > > #endif
> > > +static struct key *platform_keys = NULL;
> > >
> > > extern __initconst const u8 system_certificate_list[];
> > > extern __initconst const unsigned long system_certificate_list_size;
> > > @@ -67,6 +69,12 @@ int restrict_link_by_builtin_and_secondary_trusted(
> > > /* Allow the builtin keyring to be added to the secondary */
> > > return 0;
> > >
> > > + if (type == &key_type_keyring &&
> > > + dest_keyring == secondary_trusted_keys &&
> > > + payload == &platform_keys->payload)
> > > + /* Allow the platform keyring to be added to the secondary */
> > > + return 0;
> > > +
> > > return restrict_link_by_signature(dest_keyring, type, payload,
> > > secondary_trusted_keys);
> > > }
> > > @@ -188,6 +196,28 @@ static __init int load_system_certificate_list(void)
> > > }
> > > late_initcall(load_system_certificate_list);
> > >
> > > +#if defined(CONFIG_INTEGRITY_PLATFORM_KEYRING) && defined(CONFIG_SECONDARY_TRUSTED_KEYRING)
> > > +
> > > +/*
> > > + * Link .platform keyring to .secondary_trusted_key keyring
> > > + */
> > > +static __init int load_platform_certificate_list(void)
> > > +{
> > > + int ret = 0;
> > > + platform_keys = integrity_get_platform_keyring();
> > > + if (!platform_keys) {
> > > + return 0;
> > > + }
> > > + ret = key_link(secondary_trusted_keys, platform_keys);
> > > + if (ret < 0) {
> > > + pr_err("Failed to link platform keyring: %d", ret);
> > > + }
> > > + return 0;
> > > +}
> > > +late_initcall(load_platform_certificate_list);
> > > +
> > > +#endif
> > > +
> > > #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> > >
> > > /**
> > > diff --git a/include/keys/platform_keyring.h b/include/keys/platform_keyring.h
> > > new file mode 100644
> > > index 000000000000..4f92ed6c0b42
> > > --- /dev/null
> > > +++ b/include/keys/platform_keyring.h
> > > @@ -0,0 +1,12 @@
> > > +#ifndef _KEYS_PLATFORM_KEYRING_H
> > > +#define _KEYS_PLATFORM_KEYRING_H
> > > +
> > > +#include <linux/key.h>
> > > +
> > > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > > +
> > > +extern const struct key* __init integrity_get_platform_keyring(void);
> > > +
> > > +#endif /* CONFIG_INTEGRITY_PLATFORM_KEYRING */
> > > +
> > > +#endif /* _KEYS_SYSTEM_KEYRING_H */
> > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > > index f45d6edecf99..397758d4f12d 100644
> > > --- a/security/integrity/digsig.c
> > > +++ b/security/integrity/digsig.c
> > > @@ -176,3 +176,10 @@ int __init integrity_load_cert(const unsigned int id, const char *source,
> > > pr_info("Loading X.509 certificate: %s\n", source);
> > > return integrity_add_key(id, data, len, perm);
> > > }
> > > +
> > > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > > +struct key* __init integrity_get_platform_keyring(void)
> > > +{
> > > + return keyring[INTEGRITY_KEYRING_PLATFORM];
> > > +}
> > > +#endif
> >
>
> Thanks
> Dave
--
Best Regards,
Kairui Song
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys
@ 2019-01-09 2:02 ` Kairui Song
0 siblings, 0 replies; 20+ messages in thread
From: Kairui Song @ 2019-01-09 2:02 UTC (permalink / raw)
To: Dave Young
Cc: Mimi Zohar, linux-kernel, David Howells, dwmw2, jwboyer,
keyrings, jmorris, serge, bauerman, ebiggers, nayna,
linux-security-module, linux-integrity, kexec
Thanks for the explanation Dave, my second thought is to let kexec use
the platform keyring directly, that is let kexec verify the image with
secondary/builtin keyring first then try platform keyring. And better
to make platform keyring independent of integrity subsystem, so kexec
could verify the image and don't depend on integrity. Any thought?
On Wed, Jan 9, 2019 at 9:34 AM Dave Young <dyoung@redhat.com> wrote:
>
> CC kexec list
> On 01/08/19 at 10:18am, Mimi Zohar wrote:
> > [Cc'ing the LSM and integrity mailing lists]
> >
> > Repeating my comment on PATCH 0/1 here with the expanded set of
> > mailing lists.
> >
> > The builtin and secondary keyrings have a signature change of trust
> > rooted in the signed kernel image. Adding the pre-boot keys to the
> > secondary keyring breaks that signature chain of trust.
> >
> > Please do NOT add the pre-boot "platform" keys to the secondary
> > keyring.
>
> If we regard kexec as a bootloader, it sounds natural to use the
> platform key to verify the signature with kexec_file_load syscall.
>
> It will be hard for user to manually sign a kernel and import the key
> then to reuse kexec_file_load.
>
> I think we do not care if platform key can be added to secondary or not,
> any suggestions how can kexec_file to use the platform key?
>
> >
> > Mimi
> >
> >
> > On Tue, 2019-01-08 at 16:12 +0800, Kairui Song wrote:
> > > Currently kexec may need to verify the kerne image, and the kernel image
> > > could be signed with third part keys which are provided by paltform or
> > > firmware (eg. stored in MokListRT EFI variable). And the same time,
> > > kexec_file_load will only verify the image agains .builtin_trusted_keys
> > > or .secondary_trusted_keys according to configuration, but there is no
> > > way for kexec_file_load to verify the image against any third part keys
> > > mentioned above.
> > >
> > > In ea93102f3224 ('integrity: Define a trusted platform keyring') a
> > > .platform keyring is introduced to store the keys provided by platform
> > > or firmware. And with a few following commits including 15ea0e1e3e185
> > > ('efi: Import certificates from UEFI Secure Boot'), now keys required to
> > > verify the image is being imported to .paltform keyring, and later
> > > IMA-appraisal could access the keyring and verify the image.
> > >
> > > This patch links the .platform keyring to .secondary_trusted_keys so
> > > kexec_file_load could also leverage the .platform keyring to verify the
> > > kernel image.
> > >
> > > Signed-off-by: Kairui Song <kasong@redhat.com>
> > > ---
> > > certs/system_keyring.c | 30 ++++++++++++++++++++++++++++++
> > > include/keys/platform_keyring.h | 12 ++++++++++++
> > > security/integrity/digsig.c | 7 +++++++
> > > 3 files changed, 49 insertions(+)
> > > create mode 100644 include/keys/platform_keyring.h
> > >
> > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > > index 81728717523d..dcef0259e149 100644
> > > --- a/certs/system_keyring.c
> > > +++ b/certs/system_keyring.c
> > > @@ -18,12 +18,14 @@
> > > #include <linux/verification.h>
> > > #include <keys/asymmetric-type.h>
> > > #include <keys/system_keyring.h>
> > > +#include <keys/platform_keyring.h>
> > > #include <crypto/pkcs7.h>
> > >
> > > static struct key *builtin_trusted_keys;
> > > #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> > > static struct key *secondary_trusted_keys;
> > > #endif
> > > +static struct key *platform_keys = NULL;
> > >
> > > extern __initconst const u8 system_certificate_list[];
> > > extern __initconst const unsigned long system_certificate_list_size;
> > > @@ -67,6 +69,12 @@ int restrict_link_by_builtin_and_secondary_trusted(
> > > /* Allow the builtin keyring to be added to the secondary */
> > > return 0;
> > >
> > > + if (type == &key_type_keyring &&
> > > + dest_keyring == secondary_trusted_keys &&
> > > + payload == &platform_keys->payload)
> > > + /* Allow the platform keyring to be added to the secondary */
> > > + return 0;
> > > +
> > > return restrict_link_by_signature(dest_keyring, type, payload,
> > > secondary_trusted_keys);
> > > }
> > > @@ -188,6 +196,28 @@ static __init int load_system_certificate_list(void)
> > > }
> > > late_initcall(load_system_certificate_list);
> > >
> > > +#if defined(CONFIG_INTEGRITY_PLATFORM_KEYRING) && defined(CONFIG_SECONDARY_TRUSTED_KEYRING)
> > > +
> > > +/*
> > > + * Link .platform keyring to .secondary_trusted_key keyring
> > > + */
> > > +static __init int load_platform_certificate_list(void)
> > > +{
> > > + int ret = 0;
> > > + platform_keys = integrity_get_platform_keyring();
> > > + if (!platform_keys) {
> > > + return 0;
> > > + }
> > > + ret = key_link(secondary_trusted_keys, platform_keys);
> > > + if (ret < 0) {
> > > + pr_err("Failed to link platform keyring: %d", ret);
> > > + }
> > > + return 0;
> > > +}
> > > +late_initcall(load_platform_certificate_list);
> > > +
> > > +#endif
> > > +
> > > #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
> > >
> > > /**
> > > diff --git a/include/keys/platform_keyring.h b/include/keys/platform_keyring.h
> > > new file mode 100644
> > > index 000000000000..4f92ed6c0b42
> > > --- /dev/null
> > > +++ b/include/keys/platform_keyring.h
> > > @@ -0,0 +1,12 @@
> > > +#ifndef _KEYS_PLATFORM_KEYRING_H
> > > +#define _KEYS_PLATFORM_KEYRING_H
> > > +
> > > +#include <linux/key.h>
> > > +
> > > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > > +
> > > +extern const struct key* __init integrity_get_platform_keyring(void);
> > > +
> > > +#endif /* CONFIG_INTEGRITY_PLATFORM_KEYRING */
> > > +
> > > +#endif /* _KEYS_SYSTEM_KEYRING_H */
> > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > > index f45d6edecf99..397758d4f12d 100644
> > > --- a/security/integrity/digsig.c
> > > +++ b/security/integrity/digsig.c
> > > @@ -176,3 +176,10 @@ int __init integrity_load_cert(const unsigned int id, const char *source,
> > > pr_info("Loading X.509 certificate: %s\n", source);
> > > return integrity_add_key(id, data, len, perm);
> > > }
> > > +
> > > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > > +struct key* __init integrity_get_platform_keyring(void)
> > > +{
> > > + return keyring[INTEGRITY_KEYRING_PLATFORM];
> > > +}
> > > +#endif
> >
>
> Thanks
> Dave
--
Best Regards,
Kairui Song
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys
2019-01-09 1:33 ` Dave Young
(?)
@ 2019-01-09 14:07 ` Mimi Zohar
-1 siblings, 0 replies; 20+ messages in thread
From: Mimi Zohar @ 2019-01-09 14:07 UTC (permalink / raw)
To: Dave Young
Cc: Kairui Song, linux-kernel, dhowells, dwmw2, jwboyer, keyrings,
jmorris, serge, bauerman, ebiggers, nayna, linux-security-module,
linux-integrity, kexec
On Wed, 2019-01-09 at 09:33 +0800, Dave Young wrote:
> CC kexec list
> On 01/08/19 at 10:18am, Mimi Zohar wrote:
> > [Cc'ing the LSM and integrity mailing lists]
> >
> > Repeating my comment on PATCH 0/1 here with the expanded set of
> > mailing lists.
> >
> > The builtin and secondary keyrings have a signature change of trust
> > rooted in the signed kernel image. Adding the pre-boot keys to the
> > secondary keyring breaks that signature chain of trust.
> >
> > Please do NOT add the pre-boot "platform" keys to the secondary
> > keyring.
>
> If we regard kexec as a bootloader, it sounds natural to use the
> platform key to verify the signature with kexec_file_load syscall.
>
> It will be hard for user to manually sign a kernel and import the key
> then to reuse kexec_file_load.
This is really a generic topic, not limited to kexec, which should be
discussed. Let's defer this discussion for now.
>
> I think we do not care if platform key can be added to secondary or not,
> any suggestions how can kexec_file to use the platform key?
I assume the problem is accessing the keyring id.
Instead of defining a function to return the keyring id, as below,
define a function that sets a variable with the keyring id.
platform_keyring_init() would call that function to set the variable.
Similar to builtin_trusted_keys and secondary_trusted_keys, define a
variable named platform_trusted_keys.
[snip]
> > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > > index f45d6edecf99..397758d4f12d 100644
> > > --- a/security/integrity/digsig.c
> > > +++ b/security/integrity/digsig.c
> > > @@ -176,3 +176,10 @@ int __init integrity_load_cert(const unsigned int id, const char *source,
> > > pr_info("Loading X.509 certificate: %s\n", source);
> > > return integrity_add_key(id, data, len, perm);
> > > }
> > > +
> > > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > > +struct key* __init integrity_get_platform_keyring(void)
> > > +{
> > > + return keyring[INTEGRITY_KEYRING_PLATFORM];
> > > +}
> > > +#endif
Mimi
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys
@ 2019-01-09 14:07 ` Mimi Zohar
0 siblings, 0 replies; 20+ messages in thread
From: Mimi Zohar @ 2019-01-09 14:07 UTC (permalink / raw)
To: Dave Young
Cc: jwboyer, Kairui Song, ebiggers, nayna, kexec, linux-kernel,
jmorris, dhowells, linux-security-module, keyrings,
linux-integrity, dwmw2, bauerman, serge
On Wed, 2019-01-09 at 09:33 +0800, Dave Young wrote:
> CC kexec list
> On 01/08/19 at 10:18am, Mimi Zohar wrote:
> > [Cc'ing the LSM and integrity mailing lists]
> >
> > Repeating my comment on PATCH 0/1 here with the expanded set of
> > mailing lists.
> >
> > The builtin and secondary keyrings have a signature change of trust
> > rooted in the signed kernel image. Adding the pre-boot keys to the
> > secondary keyring breaks that signature chain of trust.
> >
> > Please do NOT add the pre-boot "platform" keys to the secondary
> > keyring.
>
> If we regard kexec as a bootloader, it sounds natural to use the
> platform key to verify the signature with kexec_file_load syscall.
>
> It will be hard for user to manually sign a kernel and import the key
> then to reuse kexec_file_load.
This is really a generic topic, not limited to kexec, which should be
discussed. Let's defer this discussion for now.
>
> I think we do not care if platform key can be added to secondary or not,
> any suggestions how can kexec_file to use the platform key?
I assume the problem is accessing the keyring id.
Instead of defining a function to return the keyring id, as below,
define a function that sets a variable with the keyring id.
platform_keyring_init() would call that function to set the variable.
Similar to builtin_trusted_keys and secondary_trusted_keys, define a
variable named platform_trusted_keys.
[snip]
> > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > > index f45d6edecf99..397758d4f12d 100644
> > > --- a/security/integrity/digsig.c
> > > +++ b/security/integrity/digsig.c
> > > @@ -176,3 +176,10 @@ int __init integrity_load_cert(const unsigned int id, const char *source,
> > > pr_info("Loading X.509 certificate: %s\n", source);
> > > return integrity_add_key(id, data, len, perm);
> > > }
> > > +
> > > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > > +struct key* __init integrity_get_platform_keyring(void)
> > > +{
> > > + return keyring[INTEGRITY_KEYRING_PLATFORM];
> > > +}
> > > +#endif
Mimi
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys
@ 2019-01-09 14:07 ` Mimi Zohar
0 siblings, 0 replies; 20+ messages in thread
From: Mimi Zohar @ 2019-01-09 14:07 UTC (permalink / raw)
To: Dave Young
Cc: Kairui Song, linux-kernel, dhowells, dwmw2, jwboyer, keyrings,
jmorris, serge, bauerman, ebiggers, nayna, linux-security-module,
linux-integrity, kexec
On Wed, 2019-01-09 at 09:33 +0800, Dave Young wrote:
> CC kexec list
> On 01/08/19 at 10:18am, Mimi Zohar wrote:
> > [Cc'ing the LSM and integrity mailing lists]
> >
> > Repeating my comment on PATCH 0/1 here with the expanded set of
> > mailing lists.
> >
> > The builtin and secondary keyrings have a signature change of trust
> > rooted in the signed kernel image. Adding the pre-boot keys to the
> > secondary keyring breaks that signature chain of trust.
> >
> > Please do NOT add the pre-boot "platform" keys to the secondary
> > keyring.
>
> If we regard kexec as a bootloader, it sounds natural to use the
> platform key to verify the signature with kexec_file_load syscall.
>
> It will be hard for user to manually sign a kernel and import the key
> then to reuse kexec_file_load.
This is really a generic topic, not limited to kexec, which should be
discussed. Let's defer this discussion for now.
>
> I think we do not care if platform key can be added to secondary or not,
> any suggestions how can kexec_file to use the platform key?
I assume the problem is accessing the keyring id.
Instead of defining a function to return the keyring id, as below,
define a function that sets a variable with the keyring id.
platform_keyring_init() would call that function to set the variable.
Similar to builtin_trusted_keys and secondary_trusted_keys, define a
variable named platform_trusted_keys.
[snip]
> > > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > > index f45d6edecf99..397758d4f12d 100644
> > > --- a/security/integrity/digsig.c
> > > +++ b/security/integrity/digsig.c
> > > @@ -176,3 +176,10 @@ int __init integrity_load_cert(const unsigned int id, const char *source,
> > > pr_info("Loading X.509 certificate: %s\n", source);
> > > return integrity_add_key(id, data, len, perm);
> > > }
> > > +
> > > +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> > > +struct key* __init integrity_get_platform_keyring(void)
> > > +{
> > > + return keyring[INTEGRITY_KEYRING_PLATFORM];
> > > +}
> > > +#endif
Mimi
^ permalink raw reply [flat|nested] 20+ messages in thread