Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #3]

[Mimi Zohar]

Hi David,

On Wed, 2016-03-09 at 11:19 +0000, 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 <dhowells@xxxxxxxxxx>
> ---
> 
>  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 blacklist, please change the
IMA_MOK_KEYRING to IMA_BLACKLIST_KEYRING.

> -	bool "Create IMA machine owner keys (MOK) and blacklist keyrings"
> +	bool "Create IMA machine owner blacklist keyrings"

Both the .ima_mok and .ima_blacklist keyrings should have been annotated
as experimental.   Please change this to "Create IMA machine owner
blacklist keyrings (EXPERIMENTAL)".

>  	depends on SYSTEM_TRUSTED_KEYRING
>  	depends on IMA_TRUSTED_KEYRING
>  	default n
>  	help
> -	   This option creates IMA MOK and blacklist keyrings.  IMA MOK is an
> -	   intermediate keyring that sits between .system and .ima keyrings,
> -	   effectively forming a simple CA hierarchy.  To successfully import a
> -	   key into .ima_mok it must be signed by a key which CA is in .system
> -	   keyring.  On turn any key that needs to go in .ima keyring must be
> -	   signed by CA in either .system or .ima_mok keyrings. IMA MOK is empty
> -	   at kernel boot.
> -
> -	   IMA blacklist keyring contains all revoked IMA keys.  It is consulted
> -	   before any other keyring.  If the search is successful the requested
> -	   operation is rejected and error is returned to the caller.
> +	   This option creates IMA blacklist keyring.  This contains all
> +	   revoked IMA keys.  It is consulted before any other keyring.  If the
> +	   search is successful the requested operation is rejected and error
> +	   is returned to the caller.
> +
> +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.

if IMA is enabled, I'm not sure what IMA_NO_ADD_TO_IMA_KEYRINGS means.

> +	help
> +	  This option selects whether keys may be added to the ima keyrings
> +	  using add_key() or KEYCTL_LINK, and, if so, under what restrictions
> +	  the key being added will be placed.
> +
> +config IMA_KEYRINGS_COMPILE_LOAD_ONLY
> +	bool "No runtime key addition"
> +	help
> +	  No keys may be added to the IMA keyrings by userspace in the running
> +	  kernel.  Keys may still be added by including X.509 certificates in
> +	  the kernel image at compile time.
> +
> +	  Attempts to add to the ima keyrings will be rejected with EPERM.
> +

This could be useful for namespacing IMA. 

> +config IMA_KEYRINGS_ADD_IF_SIGNED_BY_BUILTIN
> +	bool "Keys may be added at runtime if validly signed by a built-in CA cert"
> +	depends on SYSTEM_TRUSTED_KEYRING
> +	select INTEGRITY_TRUSTED_KEYRING
> +	help
> +	  keys may be added to the IMA keyrings by userspace in the running
> +	  kernel if the keys to be added are validly signed by a CA cert in the
> +	  system built-in trusted keyring.
> +
> +config IMA_KEYRINGS_ADD_IF_SIGNED_BY_BUILTIN_OR_SECONDARY
> +	bool "Keys may be added at runtime if validly signed by a built-in or secondary CA cert"

Please add "(EXPERIMENTAL)" to this option as well.

> +	depends on SYSTEM_TRUSTED_KEYRING
> +	depends on SECONDARY_TRUSTED_KEYRING
> +	select INTEGRITY_TRUSTED_KEYRING
> +	help
> +	  keys may be added to the IMA keyrings by userspace in the running
> +	  kernel if the keys to be added are validly signed by a CA cert in the
> +	  system built-in or secondary trusted keyrings.

> +	  Intermediate keys between those the kernel has compiled in and the
> +	  IMA keys to be added may be added to the system secondary keyring,
> +	  provided they are validly signed by a key already resident in the
> +	  built-in or secondary trusted keyrings.

A reminder should be included here indicating that not only IMA specific
keys are on the secondary keyring.

Thanks!

Mimi

> +endchoice
> 
>  config IMA_LOAD_X509
>  	bool "Load X509 certificate onto the '.ima' trusted keyring"
> diff --git a/security/integrity/ima/ima_mok.c b/security/integrity/ima/ima_mok.c
> index 2988726d30d6..1480f68a7fd8 100644
> --- a/security/integrity/ima/ima_mok.c
> +++ b/security/integrity/ima/ima_mok.c
> @@ -20,23 +20,14 @@
>  #include <keys/system_keyring.h>
> 
> 
> -struct key *ima_mok_keyring;
>  struct key *ima_blacklist_keyring;
> 
>  /*
> - * Allocate the IMA MOK and blacklist keyrings
> + * Allocate the IMA blacklist keyring
>   */
>  __init int ima_mok_init(void)
>  {
> -	pr_notice("Allocating IMA MOK and blacklist keyrings.\n");
> -
> -	ima_mok_keyring = keyring_alloc(".ima_mok",
> -			      KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
> -			      (KEY_POS_ALL & ~KEY_POS_SETATTR) |
> -			      KEY_USR_VIEW | KEY_USR_READ |
> -			      KEY_USR_WRITE | KEY_USR_SEARCH,
> -			      KEY_ALLOC_NOT_IN_QUOTA,
> -			      restrict_link_by_builtin_trusted, NULL);
> +	pr_notice("Allocating IMA blacklist keyrings.\n");
> 
>  	ima_blacklist_keyring = keyring_alloc(".ima_blacklist",
>  				KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
> @@ -46,7 +37,7 @@ __init int ima_mok_init(void)
>  				KEY_ALLOC_NOT_IN_QUOTA,
>  				restrict_link_by_builtin_trusted, NULL);
> 
> -	if (IS_ERR(ima_mok_keyring) || IS_ERR(ima_blacklist_keyring))
> +	if (IS_ERR(ima_blacklist_keyring))
>  		panic("Can't allocate IMA MOK or blacklist keyrings.");
> 
>  	set_bit(KEY_FLAG_KEEP, &ima_blacklist_keyring->flags);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>