Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
On Wed, 2016-04-06 at 17:13 +0100, David Howells wrote: > Mimi Zohar wrote: > > > FYI, restrict_link_by_ima_mok() allows keys to be added to the IMA > > keyring signed by a key on the .ima_mok keyring, but > > restrict_link_by_builtin_and_secondary_trusted() results in "errno: > > Required key not available (126)". > > Is that fixed by fixing restrict_link_by_builtin_and_secondary_trusted() to > check the right keyring? Yes
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
Mimi Zohar wrote: > FYI, restrict_link_by_ima_mok() allows keys to be added to the IMA > keyring signed by a key on the .ima_mok keyring, but > restrict_link_by_builtin_and_secondary_trusted() results in "errno: > Required key not available (126)". Is that fixed by fixing restrict_link_by_builtin_and_secondary_trusted() to check the right keyring? David
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
On Wed, 2016-03-09 at 11:19 +, David Howells wrote:
> -#ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
> -/*
> - * Restrict the addition of keys into the IMA keyring.
> - *
> - * Any key that needs to go in .ima keyring must be signed by CA in
> - * either .system or .ima_mok keyrings.
> - */
> -static int restrict_link_by_ima_mok(struct key *keyring,
> - const struct key_type *type,
> - const union key_payload *payload)
> -{
> - int ret;
> -
> - ret = restrict_link_by_builtin_trusted(keyring, type, payload);
> - if (ret != -ENOKEY)
> - return ret;
> -
> - return restrict_link_by_signature(get_ima_mok_keyring(),
> - type, payload);
> -}
> +#if defined(CONFIG_IMA_KEYRINGS_ADD_IF_SIGNED_BY_BUILTIN)
> +#define restrict_link_to_ima restrict_link_by_builtin_trusted
> +#elif defined(CONFIG_IMA_KEYRINGS_ADD_IF_SIGNED_BY_BUILTIN_OR_SECONDARY)
> +#define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted
FYI, restrict_link_by_ima_mok() allows keys to be added to the IMA
keyring signed by a key on the .ima_mok keyring, but
restrict_link_by_builtin_and_secondary_trusted() results in "errno:
Required key not available (126)".
Mimi
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
On Fri, 2016-04-01 at 15:06 +0100, David Howells wrote: > David Howells wrote: > > > The three choice options I implemented don't exactly provide new features. > > Firstly: > > > > config IMA_LOAD_X509 > > > > allow keys to be loaded in at compile time, > > Ah - I think I'm labouring under a slight misapprehension here. IMA_LOAD_X509 > doesn't load keys at compile time, but rather the kernel loads a file by name > when booting, right? Right, all certificates must be signed by a key on the builtin (or secondary keyring) before being added to the IMA keyring. Similarly, dracut (modules/98integrity/ and systemd (ima-setup.c) have support for loading signed certificates on the IMA keyring. Mimi
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
On Fri, 2016-04-01 at 15:33 +0100, David Howells wrote: > Mimi Zohar wrote: > > > The only place where "KEY_ALLOC_BYPASS_RESTRICTION" is specified is in > > load_system_certificate_list(), when adding keys to > > the .builtin_trusted_keys keyring. There is no other set of keys > > builtin and added to the IMA keyring. > > Are the keys loaded by integrity_load_x509() required to be validly signed by > the builtin/secondary keys? Or is that unnecessary given that they are loaded > and thus protected through integrity_read_file()? Loading keys on the IMA keyring is safe, because the certificates must be signed by a key on the builtin keyring or the secondary keyring, if it is Kconfig enabled. Mimi
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
Mimi Zohar wrote: > The only place where "KEY_ALLOC_BYPASS_RESTRICTION" is specified is in > load_system_certificate_list(), when adding keys to > the .builtin_trusted_keys keyring. There is no other set of keys > builtin and added to the IMA keyring. Are the keys loaded by integrity_load_x509() required to be validly signed by the builtin/secondary keys? Or is that unnecessary given that they are loaded and thus protected through integrity_read_file()? David
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
David Howells wrote: > The three choice options I implemented don't exactly provide new features. > Firstly: > > config IMA_LOAD_X509 > > allow keys to be loaded in at compile time, Ah - I think I'm labouring under a slight misapprehension here. IMA_LOAD_X509 doesn't load keys at compile time, but rather the kernel loads a file by name when booting, right? David
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
On Thu, 2016-03-31 at 11:55 -0400, Mimi Zohar wrote: > On Thu, 2016-03-31 at 16:18 +0100, David Howells wrote: > > Mimi Zohar wrote: > > > > > You said you didn't want this option above (to quote: In this patch, the > > > > choice should be between checking just the builtin trusted keys or both > > > > the > > > > builtin trusted and secondary keys.) > > > > > > Does the ability of adding builtin X.509 certificates directly to the > > > IMA keyring already exist or is it something that still needs to be > > > done? Assuming the latter, this option would be added with the ability > > > of adding X.509 certificates directly to the IMA keyring. > > > > I don't know what you mean by "directly" here. Even without this patch, you > > can already add X.509 certs to the .ima keyring, both at compile time > > I'm must be missing something obvious here. I need to review the > patches again. The builtin keys are loaded onto > the .builtin_trusted_keys keyring, not the IMA keyring.is the The only place where "KEY_ALLOC_BYPASS_RESTRICTION" is specified is in load_system_certificate_list(), when adding keys to the .builtin_trusted_keys keyring. There is no other set of keys builtin and added to the IMA keyring. Mimi
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
On Thu, 2016-03-31 at 16:18 +0100, David Howells wrote: > Mimi Zohar wrote: > > > You said you didn't want this option above (to quote: In this patch, the > > > choice should be between checking just the builtin trusted keys or both > > > the > > > builtin trusted and secondary keys.) > > > > Does the ability of adding builtin X.509 certificates directly to the > > IMA keyring already exist or is it something that still needs to be > > done? Assuming the latter, this option would be added with the ability > > of adding X.509 certificates directly to the IMA keyring. > > I don't know what you mean by "directly" here. Even without this patch, you > can already add X.509 certs to the .ima keyring, both at compile time I'm must be missing something obvious here. I need to review the patches again. The builtin keys are loaded onto the .builtin_trusted_keys keyring, not the IMA keyring. Mimi > and at > runtime. This patch doesn't take that away - except by explicitly setting the > option to prevent runtime addition. > > David >
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
Mimi Zohar wrote: > > The third option I've added is that you can't add to .ima at all. You only > > get what's included at build time. You don't want that option? > > Adding keys directly to the IMA keyring is a new feature. There's enough > changes as it is, that this patch should be limited to replicating existing > usage, not adding new features. The three choice options I implemented don't exactly provide new features. Firstly: config IMA_LOAD_X509 allow keys to be loaded in at compile time, secondly, config INTEGRITY_SIGNATURE allows keys to be inserted whilst the kernel is running if those keys are "trusted", and, thirdly, config IMA_MOK_KEYRING modifies what it means for a key to be "trusted" by allowing intermediate levels of CA certificates to be chained. So, of the three choice options I'm providing, config IMA_KEYRINGS_ADD_IF_SIGNED_BY_BUILTIN is equivalent to setting INTEGRITY_SIGNATURE plus IMA_LOAD_X509 if you set it; then config IMA_KEYRINGS_ADD_IF_SIGNED_BY_BUILTIN_OR_SECONDARY is the equivalent of turning on IMA_MOK_KEYRING as well. Fair enough, you can argue that: config IMA_KEYRINGS_COMPILE_LOAD_ONLY allowing you to disable addition of keys whilst the kernel is running is a 'new feature'. I disagree, I think it should be there for completeness. > > > > +config IMA_KEYRINGS_COMPILE_LOAD_ONLY > > > > + bool "No runtime key addition" > > > ... > > > This could be useful for namespacing IMA. > > > > You said you didn't want this option above (to quote: In this patch, the > > choice should be between checking just the builtin trusted keys or both the > > builtin trusted and secondary keys.) > > Does the ability of adding builtin X.509 certificates directly to the > IMA keyring already exist or is it something that still needs to be > done? Assuming the latter, this option would be added with the ability > of adding X.509 certificates directly to the IMA keyring. I don't know what you mean by "directly" here. Even without this patch, you can already add X.509 certs to the .ima keyring, both at compile time and at runtime. This patch doesn't take that away - except by explicitly setting the option to prevent runtime addition. David
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
On Wed, 2016-03-30 at 17:19 +0100, David Howells wrote: > Mimi Zohar wrote: > > > > +choice > > > + prompt "Allow keys to be added to the ima keyrings by userspace?" > > > + depends on IMA_APPRAISE > > > + depends on INTEGRITY_ASYMMETRIC_KEYS > > > + default IMA_NO_ADD_TO_IMA_KEYRINGS > > > > In this patch, the choice should be between checking just the builtin > > trusted keys or both the builtin trusted and secondary keys. > > The third option I've added is that you can't add to .ima at all. You only > get what's included at build time. You don't want that option? Adding keys directly to the IMA keyring is a new feature. There's enough changes as it is, that this patch should be limited to replicating existing usage, not adding new features. > > if IMA is enabled, I'm not sure what IMA_NO_ADD_TO_IMA_KEYRINGS means. > > Oops. that should be IMA_KEYRINGS_COMPILE_LOAD_ONLY. The default should be to validate keys against the builtin trusted keys, like it is currently. > "IMA_NO_ADD_TO_IMA_KEYRINGS" seemed to be phrased too clunkily, but the > Kconfig parser warn you about the undefined symbol. > > > > +config IMA_KEYRINGS_COMPILE_LOAD_ONLY > > > + bool "No runtime key addition" > > ... > > This could be useful for namespacing IMA. > > You said you didn't want this option above (to quote: In this patch, the > choice should be between checking just the builtin trusted keys or both the > builtin trusted and secondary keys.) Does the ability of adding builtin X.509 certificates directly to the IMA keyring already exist or is it something that still needs to be done? Assuming the latter, this option would be added with the ability of adding X.509 certificates directly to the IMA keyring. Mimi
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
Mimi Zohar wrote: > > +choice > > + prompt "Allow keys to be added to the ima keyrings by userspace?" > > + depends on IMA_APPRAISE > > + depends on INTEGRITY_ASYMMETRIC_KEYS > > + default IMA_NO_ADD_TO_IMA_KEYRINGS > > In this patch, the choice should be between checking just the builtin > trusted keys or both the builtin trusted and secondary keys. The third option I've added is that you can't add to .ima at all. You only get what's included at build time. You don't want that option? > if IMA is enabled, I'm not sure what IMA_NO_ADD_TO_IMA_KEYRINGS means. Oops. that should be IMA_KEYRINGS_COMPILE_LOAD_ONLY. "IMA_NO_ADD_TO_IMA_KEYRINGS" seemed to be phrased too clunkily, but the Kconfig parser warn you about the undefined symbol. > > +config IMA_KEYRINGS_COMPILE_LOAD_ONLY > > + bool "No runtime key addition" > ... > This could be useful for namespacing IMA. You said you didn't want this option above (to quote: In this patch, the choice should be between checking just the builtin trusted keys or both the builtin trusted and secondary keys.) David
Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]
Hi David,
On Wed, 2016-03-09 at 11:19 +, David Howells wrote:
> Provide a config option (IMA_PERMIT_ADD_TO_IMA_KEYRINGS) that, when
> enabled, allows keys to be added to the IMA keyrings by userspace - with
> the restriction that each must be signed by a key in the system trusted
> keyrings.
>
> EPERM will be returned if this option is disabled, ENOKEY will be returned if
> no authoritative key can be found and EKEYREJECTED will be returned if the
> signature doesn't match. Other errors such as ENOPKG may also be returned.
>
> If this new option is enabled, the builtin system keyring is searched, as is
> the secondary system keyring if that is also enabled. Intermediate keys
> between the builtin system keyring and the key being added can be added to
> the secondary keyring (which replaces .ima_mok) to form a trust chain -
> provided they are also validly signed by a key in one of the trusted keyrings.
>
> The .ima_mok keyring is removed.
This patch adds new Kconfig options, when it should be limited to
removing the .ima_mok keyring. Lets change the title to "IMA: use the
secondary_trusted_keys instead of .ima_mok" and limit the new Kconfig
option to using the secondary_trusted_keys.
> Signed-off-by: David Howells
> ---
>
> include/keys/system_keyring.h|9 --
> security/integrity/digsig.c | 33
> security/integrity/ima/Kconfig | 62
> +++---
> security/integrity/ima/ima_mok.c | 15 ++---
> 4 files changed, 60 insertions(+), 59 deletions(-)
>
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 614424029de7..87eeea4b816c 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -34,22 +34,13 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
> #endif
>
> #ifdef CONFIG_IMA_MOK_KEYRING
> -extern struct key *ima_mok_keyring;
> extern struct key *ima_blacklist_keyring;
>
> -static inline struct key *get_ima_mok_keyring(void)
> -{
> - return ima_mok_keyring;
> -}
> static inline struct key *get_ima_blacklist_keyring(void)
> {
> return ima_blacklist_keyring;
> }
> #else
> -static inline struct key *get_ima_mok_keyring(void)
> -{
> - return NULL;
> -}
> static inline struct key *get_ima_blacklist_keyring(void)
> {
> return NULL;
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 98ee4c752cf5..ef2f911d5c76 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -42,32 +42,12 @@ static bool init_keyring __initdata = true;
> static bool init_keyring __initdata;
> #endif
>
> -#ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
> -/*
> - * Restrict the addition of keys into the IMA keyring.
> - *
> - * Any key that needs to go in .ima keyring must be signed by CA in
> - * either .system or .ima_mok keyrings.
> - */
> -static int restrict_link_by_ima_mok(struct key *keyring,
> - const struct key_type *type,
> - const union key_payload *payload)
> -{
> - int ret;
> -
> - ret = restrict_link_by_builtin_trusted(keyring, type, payload);
> - if (ret != -ENOKEY)
> - return ret;
> -
> - return restrict_link_by_signature(get_ima_mok_keyring(),
> - type, payload);
> -}
> +#if defined(CONFIG_IMA_KEYRINGS_ADD_IF_SIGNED_BY_BUILTIN)
> +#define restrict_link_to_ima restrict_link_by_builtin_trusted
> +#elif defined(CONFIG_IMA_KEYRINGS_ADD_IF_SIGNED_BY_BUILTIN_OR_SECONDARY)
> +#define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted
> #else
> -/*
> - * If there's no system trusted keyring, then keys cannot be loaded into
> - * .ima_mok and added keys cannot be marked trusted.
> - */
> -#define restrict_link_by_ima_mok restrict_link_reject
> +#define restrict_link_to_ima restrict_link_reject
> #endif
>
> int integrity_digsig_verify(const unsigned int id, const char *sig, int
> siglen,
> @@ -114,7 +94,8 @@ int __init integrity_init_keyring(const unsigned int id)
>KEY_USR_VIEW | KEY_USR_READ |
>KEY_USR_WRITE | KEY_USR_SEARCH),
> KEY_ALLOC_NOT_IN_QUOTA,
> - restrict_link_by_ima_mok, NULL);
> + restrict_link_to_ima,
> + NULL);
> if (IS_ERR(keyring[id])) {
> err = PTR_ERR(keyring[id]);
> pr_info("Can't allocate %s keyring (%d)\n",
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index e54a8a8dae94..90a65fba6f39 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -156,22 +156,60 @@ config IMA_TRUSTED_KEYRING
> This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING
>
> config IMA_MOK_KEYRING
As we're left with only the IMA bl
