[RFC PATCH v4 0/4] ima: extending secure boot certificate chain

[RFC PATCH v4 0/4] ima: extending secure boot certificate chain

[Mimi Zohar]

The original patches extended the secure boot signature chain of trust
to IMA-appraisal, by allowing only certificates signed by a 'trusted'
key on the system_trusted_keyring to be added to the IMA keyring.

There were a number of issues with the original patch set, including
kbuild issues, which have been resolved, and with the special dot
prefixed keyrings.  The function key_get_type_from_user(), verfies
the '_type', not the '_description', is dot prefixed.  The previous
version added an additional test, but left the existing check in
key_get_type_from_user().  This patch set removes the unnecessary
dot check.

Previous versions attempted to verify a certificate is signed by a
trusted key, but not necessarily by any key on the system_trusted_keyring.
The first attempt, permitted any key on the targeted trusted keyring
to verify a certificate. This introduced concerns of transitive trust.
The subsequent attempt defined a separate keyring, associated with
each targeted trusted keyring.  This patch set defines a single new
owner_trusted_keyring.

thanks,

Mimi

Mimi Zohar (4):
  KEYS: special dot prefixed keyring name bug fix
  KEYS: verify a certificate is signed by a 'trusted' key
  ima: define '.ima' as a builtin 'trusted' keyring
  KEYS: define an owner trusted keyring

 crypto/asymmetric_keys/x509_public_key.c | 85 +++++++++++++++++++++++++++++++-
 include/keys/system_keyring.h            | 13 +++++
 include/linux/key.h                      |  4 ++
 kernel/system_keyring.c                  | 85 ++++++++++++++++++++++++++++++++
 security/integrity/digsig.c              | 26 +++++++++-
 security/integrity/ima/Kconfig           |  8 +++
 security/integrity/ima/ima_appraise.c    | 11 +++++
 security/integrity/integrity.h           |  5 ++
 security/keys/key.c                      | 20 ++++++++
 security/keys/keyctl.c                   |  6 ++-
 10 files changed, 259 insertions(+), 4 deletions(-)

-- 
1.8.1.4

[RFC PATCH v4 1/4] KEYS: special dot prefixed keyring name bug fix

[Mimi Zohar]

Dot prefixed keyring names are supposed to be reserved for the
kernel, but add_key() calls key_get_type_from_user(), which
incorrectly verifies the 'type' field, not the 'description' field.
This patch verifies the 'description' field isn't dot prefixed,
when creating a new keyring, and removes the dot prefix test in
key_get_type_from_user().

Reported-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/keys/keyctl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index cd5bd0c..9e9a762 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -37,8 +37,6 @@ static int key_get_type_from_user(char *type,
 		return ret;
 	if (ret == 0 || ret >= len)
 		return -EINVAL;
-	if (type[0] == '.')
-		return -EPERM;
 	type[len - 1] = '\0';
 	return 0;
 }
@@ -87,6 +85,10 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
 			kfree(description);
 			description = NULL;
 		}
+		if (description[0] == '.') {
+			ret = -EPERM;
+			goto error2;
+		}
 	}
 
 	/* pull the payload in if one was supplied */
-- 
1.8.1.4

[RFC PATCH v4 2/4] KEYS: verify a certificate is signed by a 'trusted' key

[Mimi Zohar]

Only public keys, with certificates signed by an existing
'trusted' key on the system trusted keyring, should be added
to a trusted keyring.  This patch adds support for verifying
a certificate's signature.

This is derived from David Howells pkcs7_request_asymmetric_key() patch.

Changelog:
- define get_system_trusted_keyring() to fix kbuild issues

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
 crypto/asymmetric_keys/x509_public_key.c | 84 +++++++++++++++++++++++++++++++-
 include/keys/system_keyring.h            | 10 +++-
 2 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 382ef0d..1af8a30 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -18,12 +18,60 @@
 #include <linux/asn1_decoder.h>
 #include <keys/asymmetric-subtype.h>
 #include <keys/asymmetric-parser.h>
+#include <keys/system_keyring.h>
 #include <crypto/hash.h>
 #include "asymmetric_keys.h"
 #include "public_key.h"
 #include "x509_parser.h"
 
 /*
+ * Find a key in the given keyring by issuer and authority.
+ */
+static struct key *x509_request_asymmetric_key(
+	struct key *keyring,
+	const char *signer, size_t signer_len,
+	const char *authority, size_t auth_len)
+{
+	key_ref_t key;
+	char *id;
+
+	/* Construct an identifier. */
+	id = kmalloc(signer_len + 2 + auth_len + 1, GFP_KERNEL);
+	if (!id)
+		return ERR_PTR(-ENOMEM);
+
+	memcpy(id, signer, signer_len);
+	id[signer_len + 0] = ':';
+	id[signer_len + 1] = ' ';
+	memcpy(id + signer_len + 2, authority, auth_len);
+	id[signer_len + 2 + auth_len] = 0;
+
+	pr_debug("Look up: \"%s\"\n", id);
+
+	key = keyring_search(make_key_ref(keyring, 1),
+			     &key_type_asymmetric, id);
+	if (IS_ERR(key))
+		pr_debug("Request for module key '%s' err %ld\n",
+			 id, PTR_ERR(key));
+	kfree(id);
+
+	if (IS_ERR(key)) {
+		switch (PTR_ERR(key)) {
+			/* Hide some search errors */
+		case -EACCES:
+		case -ENOTDIR:
+		case -EAGAIN:
+			return ERR_PTR(-ENOKEY);
+		default:
+			return ERR_CAST(key);
+		}
+	}
+
+	pr_devel("<==%s() = 0 [%x]\n", __func__, key_serial(key_ref_to_ptr(key)));
+	return key_ref_to_ptr(key);
+}
+
+/*
  * Set up the signature parameters in an X.509 certificate.  This involves
  * digesting the signed data and extracting the signature.
  */
@@ -103,6 +151,36 @@ int x509_check_signature(const struct public_key *pub,
 EXPORT_SYMBOL_GPL(x509_check_signature);
 
 /*
+ * Check the new certificate against the ones in the trust keyring.  If one of
+ * those is the signing key and validates the new certificate, then mark the
+ * new certificate as being trusted.
+ *
+ * Return 0 if the new certificate was successfully validated, 1 if we couldn't
+ * find a matching parent certificate in the trusted list and an error if there
+ * is a matching certificate but the signature check fails.
+ */
+static int x509_validate_trust(struct x509_certificate *cert,
+			       struct key *trust_keyring)
+{
+	const struct public_key *pk;
+	struct key *key;
+	int ret = 1;
+
+	if (!trust_keyring)
+		return -EOPNOTSUPP;
+
+	key = x509_request_asymmetric_key(trust_keyring,
+					  cert->issuer, strlen(cert->issuer),
+					  cert->authority,
+					  strlen(cert->authority));
+	if (!IS_ERR(key))  {
+		pk = key->payload.data;
+		ret = x509_check_signature(pk, cert);
+	}
+	return ret;
+}
+
+/*
  * Attempt to parse a data blob for a key as an X509 certificate.
  */
 static int x509_key_preparse(struct key_preparsed_payload *prep)
@@ -155,9 +233,13 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 	/* Check the signature on the key if it appears to be self-signed */
 	if (!cert->authority ||
 	    strcmp(cert->fingerprint, cert->authority) == 0) {
-		ret = x509_check_signature(cert->pub, cert);
+		ret = x509_check_signature(cert->pub, cert); /* self-signed */
 		if (ret < 0)
 			goto error_free_cert;
+	} else {
+		ret = x509_validate_trust(cert, get_system_trusted_keyring());
+		if (!ret)
+			prep->trusted = 1;
 	}
 
 	/* Propose a description */
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 8dabc39..72665eb 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -17,7 +17,15 @@
 #include <linux/key.h>
 
 extern struct key *system_trusted_keyring;
-
+static inline struct key *get_system_trusted_keyring(void)
+{
+	return system_trusted_keyring;
+}
+#else
+static inline struct key *get_system_trusted_keyring(void)
+{
+	return NULL;
+}
 #endif
 
 #endif /* _KEYS_SYSTEM_KEYRING_H */
-- 
1.8.1.4

[RFC PATCH v4 3/4] ima: define '.ima' as a builtin 'trusted' keyring

[Mimi Zohar]

Require all keys added to the IMA keyring be signed by an
existing trusted key on the system trusted keyring.

Changelog v1:
- don't link IMA trusted keyring to user keyring

Changelog:
- define stub integrity_init_keyring() function (reported-by Fengguang Wu)
- differentiate between regular and trusted keyring names.
- replace printk with pr_info (D. Kasatkin)
- only make the IMA keyring a trusted keyring (reported-by D. Kastatkin)
- define stub integrity_init_keyring() definition based on
  CONFIG_INTEGRITY_SIGNATURE, not CONFIG_INTEGRITY_ASYMMETRIC_KEYS.
  (reported-by Jim Davis)

Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
---
 security/integrity/digsig.c           | 26 +++++++++++++++++++++++++-
 security/integrity/ima/Kconfig        |  8 ++++++++
 security/integrity/ima/ima_appraise.c | 11 +++++++++++
 security/integrity/integrity.h        |  5 +++++
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index b4af4eb..7da5f9c 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -13,7 +13,9 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/err.h>
+#include <linux/sched.h>
 #include <linux/rbtree.h>
+#include <linux/cred.h>
 #include <linux/key-type.h>
 #include <linux/digsig.h>
 
@@ -24,7 +26,11 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX];
 static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
 	"_evm",
 	"_module",
+#ifndef CONFIG_IMA_TRUSTED_KEYRING
 	"_ima",
+#else
+	".ima",
+#endif
 };
 
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
@@ -35,7 +41,7 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 
 	if (!keyring[id]) {
 		keyring[id] =
-			request_key(&key_type_keyring, keyring_name[id], NULL);
+		    request_key(&key_type_keyring, keyring_name[id], NULL);
 		if (IS_ERR(keyring[id])) {
 			int err = PTR_ERR(keyring[id]);
 			pr_err("no %s keyring: %d\n", keyring_name[id], err);
@@ -56,3 +62,21 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 
 	return -EOPNOTSUPP;
 }
+
+int integrity_init_keyring(const unsigned int id)
+{
+	const struct cred *cred = current_cred();
+
+	keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
+				    KGIDT_INIT(0), cred,
+				    ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
+				      KEY_USR_VIEW | KEY_USR_READ |
+				      KEY_USR_WRITE | KEY_USR_SEARCH),
+				    KEY_ALLOC_NOT_IN_QUOTA, NULL);
+	if (!IS_ERR(keyring[id]))
+		set_bit(KEY_FLAG_TRUSTED_ONLY, &keyring[id]->flags);
+	else
+		pr_info("Can't allocate %s keyring (%ld)\n",
+			keyring_name[id], PTR_ERR(keyring[id]));
+	return 0;
+}
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 81a2797..dad8d4c 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -123,3 +123,11 @@ config IMA_APPRAISE
 	  For more information on integrity appraisal refer to:
 	  <http://linux-ima.sourceforge.net>
 	  If unsure, say N.
+
+config IMA_TRUSTED_KEYRING
+	bool "Require all keys on the _ima keyring be signed"
+	depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
+	default y
+	help
+	   This option requires that all keys added to the _ima
+	   keyring be signed by a key on the system trusted keyring.
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index d3113d4..003ff46 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -385,3 +385,14 @@ int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
 	}
 	return result;
 }
+
+#ifdef CONFIG_IMA_TRUSTED_KEYRING
+static int __init init_ima_keyring(void)
+{
+	int ret;
+
+	ret = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
+	return 0;
+}
+late_initcall(init_ima_keyring);
+#endif
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 33c0a70..09c440d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -124,6 +124,7 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
 int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 			    const char *digest, int digestlen);
 
+int integrity_init_keyring(const unsigned int id);
 #else
 
 static inline int integrity_digsig_verify(const unsigned int id,
@@ -133,6 +134,10 @@ static inline int integrity_digsig_verify(const unsigned int id,
 	return -EOPNOTSUPP;
 }
 
+static inline int integrity_init_keyring(const unsigned int id)
+{
+	return 0;
+}
 #endif /* CONFIG_INTEGRITY_SIGNATURE */
 
 #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
-- 
1.8.1.4

[RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Mimi Zohar]

(UEFI) secure boot provides a signature chain of trust rooted in
hardware. The signature chain of trust includes the Machine Owner
Keys(MOKs), which cannot be modified without physical presence.

Instead of allowing public keys, with certificates signed by any
key on the system trusted keyring, to be added to a trusted
keyring, this patch further restricts the certificates to those
signed by the machine owner's key or chosen key.

This patch defines an owner trusted keyring, defines a new boot
command line option 'keyring=' to designate the machine owner's
chosen key, and renames the function get_system_trusted_keyring()
to get_system_or_owner_trusted_keyring().

This patch permits the machine owner to safely identify their own
or chosen key, without requiring it to be builtin the kernel.

Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
---
 crypto/asymmetric_keys/x509_public_key.c |  3 +-
 include/keys/system_keyring.h            | 15 ++++--
 include/linux/key.h                      |  4 ++
 kernel/system_keyring.c                  | 85 ++++++++++++++++++++++++++++++++
 security/keys/key.c                      | 20 ++++++++
 5 files changed, 121 insertions(+), 6 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 1af8a30..6d52790 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -237,7 +237,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 		if (ret < 0)
 			goto error_free_cert;
 	} else {
-		ret = x509_validate_trust(cert, get_system_trusted_keyring());
+		ret = x509_validate_trust(cert,
+					  get_system_or_owner_trusted_keyring());
 		if (!ret)
 			prep->trusted = 1;
 	}
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 72665eb..f665c33 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -17,15 +17,20 @@
 #include <linux/key.h>
 
 extern struct key *system_trusted_keyring;
-static inline struct key *get_system_trusted_keyring(void)
-{
-	return system_trusted_keyring;
-}
+extern struct key *owner_trusted_keyring;
+
+extern struct key *get_system_or_owner_trusted_keyring(void);
+extern void load_owner_identified_uefi_key(key_ref_t key);
+
 #else
-static inline struct key *get_system_trusted_keyring(void)
+static inline struct key *get_system_or_owner_trusted_keyring(void)
 {
 	return NULL;
 }
+
+static void load_owner_identified_uefi_key(key_ref_t key)
+{
+}
 #endif
 
 #endif /* _KEYS_SYSTEM_KEYRING_H */
diff --git a/include/linux/key.h b/include/linux/key.h
index cd0abb8..861843a 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -267,6 +267,10 @@ extern int wait_for_key_construction(struct key *key, bool intr);
 
 extern int key_validate(const struct key *key);
 
+extern int key_match(key_ref_t key,
+		     const char *type,
+		     const char *description);
+
 extern key_ref_t key_create_or_update(key_ref_t keyring,
 				      const char *type,
 				      const char *description,
diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
index 52ebc70..e9b14ac 100644
--- a/kernel/system_keyring.c
+++ b/kernel/system_keyring.c
@@ -19,11 +19,75 @@
 #include "module-internal.h"
 
 struct key *system_trusted_keyring;
+struct key *owner_trusted_keyring;
 EXPORT_SYMBOL_GPL(system_trusted_keyring);
 
 extern __initconst const u8 system_certificate_list[];
 extern __initconst const unsigned long system_certificate_list_size;
 
+static int use_owner_trusted_keyring;
+
+static char *owner_keyid;
+static int builtin_keyring;
+static int __init default_keyring_set(char *str)
+{
+	if (!str)	/* default: builtin */
+		return 1;
+
+	if (strcmp(str, "system") == 0)  /* use system keyring */
+		;
+	else if (strcmp(str, "builtin") == 0)  /* only builtin keys */
+		builtin_keyring = 1;	
+	else 
+		owner_keyid = str;   /* owner local key 'id:xxxxxx' */
+	return 1;
+}
+__setup("keyring=", default_keyring_set);
+
+/*
+ * Load the owner identified key on the 'owner' trusted keyring.
+ */
+void load_owner_identified_uefi_key(key_ref_t key)
+{
+	if (!owner_keyid || use_owner_trusted_keyring)
+		return;
+
+	if (!key_match(key, "asymmetric", owner_keyid))
+		return;
+
+	if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
+		set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
+		use_owner_trusted_keyring = 1;
+		pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
+				  key_ref_to_ptr(key)->description);
+	}
+}
+
+static void load_owner_identified_builtin_key(key_ref_t key)
+{
+	if (!owner_keyid && !builtin_keyring)
+		return;
+
+	if (!builtin_keyring && !key_match(key, "asymmetric", owner_keyid))
+		return;
+
+	if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
+		set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
+		use_owner_trusted_keyring = 1;
+		pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
+				  key_ref_to_ptr(key)->description);
+	}
+}
+
+/*
+ * Use the owner_trusted_keyring if available
+ */
+struct key *get_system_or_owner_trusted_keyring(void)
+{
+	return use_owner_trusted_keyring ? owner_trusted_keyring :
+		system_trusted_keyring;
+}
+
 /*
  * Load the compiled-in keys
  */
@@ -50,6 +114,25 @@ static __init int system_trusted_keyring_init(void)
 device_initcall(system_trusted_keyring_init);
 
 /*
+ * Load the owner trusted key
+ */
+static __init int owner_trusted_keyring_init(void)
+{
+	pr_notice("Initialize the owner trusted keyring\n");
+
+	owner_trusted_keyring =
+		keyring_alloc(".owner_keyring",
+			      KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
+			      ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
+			      KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
+			      KEY_ALLOC_NOT_IN_QUOTA, NULL);
+	if (IS_ERR(owner_trusted_keyring))
+		panic("Can't allocate owner trusted keyring\n");
+	return 0;
+}
+device_initcall(owner_trusted_keyring_init);
+
+/*
  * Load the compiled-in list of X.509 certificates.
  */
 static __init int load_system_certificate_list(void)
@@ -91,6 +174,8 @@ static __init int load_system_certificate_list(void)
 		} else {
 			pr_notice("Loaded X.509 cert '%s'\n",
 				  key_ref_to_ptr(key)->description);
+
+			load_owner_identified_builtin_key(key);
 			key_ref_put(key);
 		}
 		p += plen;
diff --git a/security/keys/key.c b/security/keys/key.c
index 2048a11..b448ab1 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -701,6 +701,26 @@ void key_type_put(struct key_type *ktype)
 	up_read(&key_types_sem);
 }
 
+/* 
+ * Use the key type's match function to compare the key's id.
+ */
+int key_match(key_ref_t key, const char *type, const char *description)
+{
+	struct keyring_index_key index_key;
+	int ret = 0;
+
+	index_key.type = key_type_lookup(type);
+	if (IS_ERR(index_key.type))
+		goto out;
+
+	if (index_key.type->match &&
+	    index_key.type->match(key_ref_to_ptr(key), description))
+		ret = 1;	
+	key_type_put(index_key.type);
+out:
+	return ret;
+}
+
 /*
  * Attempt to update an existing key.
  *
-- 
1.8.1.4

Re: [RFC PATCH v4 3/4] ima: define '.ima' as a builtin 'trusted' keyring

[Dmitry Kasatkin]

On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Require all keys added to the IMA keyring be signed by an
> existing trusted key on the system trusted keyring.
>
> Changelog v1:
> - don't link IMA trusted keyring to user keyring
>
> Changelog:
> - define stub integrity_init_keyring() function (reported-by Fengguang Wu)
> - differentiate between regular and trusted keyring names.
> - replace printk with pr_info (D. Kasatkin)
> - only make the IMA keyring a trusted keyring (reported-by D. Kastatkin)
> - define stub integrity_init_keyring() definition based on
>   CONFIG_INTEGRITY_SIGNATURE, not CONFIG_INTEGRITY_ASYMMETRIC_KEYS.
>   (reported-by Jim Davis)
>
> Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> ---
>  security/integrity/digsig.c           | 26 +++++++++++++++++++++++++-
>  security/integrity/ima/Kconfig        |  8 ++++++++
>  security/integrity/ima/ima_appraise.c | 11 +++++++++++
>  security/integrity/integrity.h        |  5 +++++
>  4 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index b4af4eb..7da5f9c 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -13,7 +13,9 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
>  #include <linux/err.h>
> +#include <linux/sched.h>
>  #include <linux/rbtree.h>
> +#include <linux/cred.h>
>  #include <linux/key-type.h>
>  #include <linux/digsig.h>
>
> @@ -24,7 +26,11 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX];
>  static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
>         "_evm",
>         "_module",
> +#ifndef CONFIG_IMA_TRUSTED_KEYRING
>         "_ima",
> +#else
> +       ".ima",
> +#endif
>  };
>
>  int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> @@ -35,7 +41,7 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>
>         if (!keyring[id]) {
>                 keyring[id] =
> -                       request_key(&key_type_keyring, keyring_name[id], NULL);
> +                   request_key(&key_type_keyring, keyring_name[id], NULL);
>                 if (IS_ERR(keyring[id])) {
>                         int err = PTR_ERR(keyring[id]);
>                         pr_err("no %s keyring: %d\n", keyring_name[id], err);
> @@ -56,3 +62,21 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>
>         return -EOPNOTSUPP;
>  }
> +
> +int integrity_init_keyring(const unsigned int id)
> +{
> +       const struct cred *cred = current_cred();
> +
> +       keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
> +                                   KGIDT_INIT(0), cred,
> +                                   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> +                                     KEY_USR_VIEW | KEY_USR_READ |
> +                                     KEY_USR_WRITE | KEY_USR_SEARCH),
> +                                   KEY_ALLOC_NOT_IN_QUOTA, NULL);

Last parameter "destination" is NULL. It makes keyring "unsearchable"
from user space.
It prevents loading trusted keys from user-space, e.g. initramfs...

Should it be "cred->user->uid_keyring"??



> +       if (!IS_ERR(keyring[id]))
> +               set_bit(KEY_FLAG_TRUSTED_ONLY, &keyring[id]->flags);
> +       else
> +               pr_info("Can't allocate %s keyring (%ld)\n",
> +                       keyring_name[id], PTR_ERR(keyring[id]));

keyring[id] should be set "back" to NULL. Otherwise bad value might be
used in other places.


> +       return 0;
> +}
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 81a2797..dad8d4c 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -123,3 +123,11 @@ config IMA_APPRAISE
>           For more information on integrity appraisal refer to:
>           <http://linux-ima.sourceforge.net>
>           If unsure, say N.
> +
> +config IMA_TRUSTED_KEYRING
> +       bool "Require all keys on the _ima keyring be signed"
> +       depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
> +       default y
> +       help
> +          This option requires that all keys added to the _ima
> +          keyring be signed by a key on the system trusted keyring.
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index d3113d4..003ff46 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -385,3 +385,14 @@ int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
>         }
>         return result;
>  }
> +
> +#ifdef CONFIG_IMA_TRUSTED_KEYRING
> +static int __init init_ima_keyring(void)
> +{
> +       int ret;
> +
> +       ret = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
> +       return 0;
> +}
> +late_initcall(init_ima_keyring);


late_initcall(init_ima_keyring) ordering competes with late_initcall(init_ima);
but we want keyring to be initialized before IMA might use it.

In the case when we would load keys from ima kernel initialization
code, order is important.

we already have init_ima() and ima_init calls().
Why not call integrity_init_keyring() from there?

Indeed, we have one late_initcall(init_evm) for EVM, and one
late_initcall(init_ima) for IMA.

It's enough...

- Dmitry


> +#endif
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 33c0a70..09c440d 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -124,6 +124,7 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>  int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>                             const char *digest, int digestlen);
>
> +int integrity_init_keyring(const unsigned int id);
>  #else
>
>  static inline int integrity_digsig_verify(const unsigned int id,
> @@ -133,6 +134,10 @@ static inline int integrity_digsig_verify(const unsigned int id,
>         return -EOPNOTSUPP;
>  }
>
> +static inline int integrity_init_keyring(const unsigned int id)
> +{
> +       return 0;
> +}
>  #endif /* CONFIG_INTEGRITY_SIGNATURE */
>
>  #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> --
> 1.8.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,
Dmitry

Re: [RFC PATCH v4 3/4] ima: define '.ima' as a builtin 'trusted' keyring

[Mimi Zohar]

On Wed, 2014-05-28 at 21:55 +0300, Dmitry Kasatkin wrote: 
> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > Require all keys added to the IMA keyring be signed by an
> > existing trusted key on the system trusted keyring.
> >
> > Changelog v1:
> > - don't link IMA trusted keyring to user keyring
> >
> > Changelog:
> > - define stub integrity_init_keyring() function (reported-by Fengguang Wu)
> > - differentiate between regular and trusted keyring names.
> > - replace printk with pr_info (D. Kasatkin)
> > - only make the IMA keyring a trusted keyring (reported-by D. Kastatkin)
> > - define stub integrity_init_keyring() definition based on
> >   CONFIG_INTEGRITY_SIGNATURE, not CONFIG_INTEGRITY_ASYMMETRIC_KEYS.
> >   (reported-by Jim Davis)
> >
> > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> > ---
> >  security/integrity/digsig.c           | 26 +++++++++++++++++++++++++-
> >  security/integrity/ima/Kconfig        |  8 ++++++++
> >  security/integrity/ima/ima_appraise.c | 11 +++++++++++
> >  security/integrity/integrity.h        |  5 +++++
> >  4 files changed, 49 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > index b4af4eb..7da5f9c 100644
> > --- a/security/integrity/digsig.c
> > +++ b/security/integrity/digsig.c
> > @@ -13,7 +13,9 @@
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> >  #include <linux/err.h>
> > +#include <linux/sched.h>
> >  #include <linux/rbtree.h>
> > +#include <linux/cred.h>
> >  #include <linux/key-type.h>
> >  #include <linux/digsig.h>
> >
> > @@ -24,7 +26,11 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX];
> >  static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
> >         "_evm",
> >         "_module",
> > +#ifndef CONFIG_IMA_TRUSTED_KEYRING
> >         "_ima",
> > +#else
> > +       ".ima",
> > +#endif
> >  };
> >
> >  int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> > @@ -35,7 +41,7 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> >
> >         if (!keyring[id]) {
> >                 keyring[id] =
> > -                       request_key(&key_type_keyring, keyring_name[id], NULL);
> > +                   request_key(&key_type_keyring, keyring_name[id], NULL);
> >                 if (IS_ERR(keyring[id])) {
> >                         int err = PTR_ERR(keyring[id]);
> >                         pr_err("no %s keyring: %d\n", keyring_name[id], err);
> > @@ -56,3 +62,21 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> >
> >         return -EOPNOTSUPP;
> >  }
> > +
> > +int integrity_init_keyring(const unsigned int id)
> > +{
> > +       const struct cred *cred = current_cred();
> > +
> > +       keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
> > +                                   KGIDT_INIT(0), cred,
> > +                                   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > +                                     KEY_USR_VIEW | KEY_USR_READ |
> > +                                     KEY_USR_WRITE | KEY_USR_SEARCH),
> > +                                   KEY_ALLOC_NOT_IN_QUOTA, NULL);
> 
> Last parameter "destination" is NULL. It makes keyring "unsearchable"
> from user space.
> It prevents loading trusted keys from user-space, e.g. initramfs...
> 
> Should it be "cred->user->uid_keyring"??

David extended keyctl with the '%keyring' option.  For example,
"keyctl show %keyring:.ima" returns the .ima keyring id with a list of
all the keys.

> 
> 
> > +       if (!IS_ERR(keyring[id]))
> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &keyring[id]->flags);
> > +       else
> > +               pr_info("Can't allocate %s keyring (%ld)\n",
> > +                       keyring_name[id], PTR_ERR(keyring[id]));
> 
> keyring[id] should be set "back" to NULL. Otherwise bad value might be
> used in other places.

Good catch, thanks.

> 
> > +       return 0;
> > +}
> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> > index 81a2797..dad8d4c 100644
> > --- a/security/integrity/ima/Kconfig
> > +++ b/security/integrity/ima/Kconfig
> > @@ -123,3 +123,11 @@ config IMA_APPRAISE
> >           For more information on integrity appraisal refer to:
> >           <http://linux-ima.sourceforge.net>
> >           If unsure, say N.
> > +
> > +config IMA_TRUSTED_KEYRING
> > +       bool "Require all keys on the _ima keyring be signed"
> > +       depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
> > +       default y
> > +       help
> > +          This option requires that all keys added to the _ima
> > +          keyring be signed by a key on the system trusted keyring.
> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > index d3113d4..003ff46 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -385,3 +385,14 @@ int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
> >         }
> >         return result;
> >  }
> > +
> > +#ifdef CONFIG_IMA_TRUSTED_KEYRING
> > +static int __init init_ima_keyring(void)
> > +{
> > +       int ret;
> > +
> > +       ret = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
> > +       return 0;
> > +}
> > +late_initcall(init_ima_keyring);
> 
> 
> late_initcall(init_ima_keyring) ordering competes with late_initcall(init_ima);
> but we want keyring to be initialized before IMA might use it.

> In the case when we would load keys from ima kernel initialization
> code, order is important.
> 
> we already have init_ima() and ima_init calls().
> Why not call integrity_init_keyring() from there?
> 
> Indeed, we have one late_initcall(init_evm) for EVM, and one
> late_initcall(init_ima) for IMA.
> 
> It's enough...

Right, there's no reason to have an additional call.

thanks,

Mimi

Re: [RFC PATCH v4 1/4] KEYS: special dot prefixed keyring name bug fix

[Dmitry Kasatkin]

On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Dot prefixed keyring names are supposed to be reserved for the
> kernel, but add_key() calls key_get_type_from_user(), which
> incorrectly verifies the 'type' field, not the 'description' field.
> This patch verifies the 'description' field isn't dot prefixed,
> when creating a new keyring, and removes the dot prefix test in
> key_get_type_from_user().
>
> Reported-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  security/keys/keyctl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index cd5bd0c..9e9a762 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -37,8 +37,6 @@ static int key_get_type_from_user(char *type,
>                 return ret;
>         if (ret == 0 || ret >= len)
>                 return -EINVAL;
> -       if (type[0] == '.')
> -               return -EPERM;
>         type[len - 1] = '\0';
>         return 0;
>  }
> @@ -87,6 +85,10 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
>                         kfree(description);
>                         description = NULL;
>                 }
> +               if (description[0] == '.') {
> +                       ret = -EPERM;
> +                       goto error2;
> +               }

1. 3 lines above "discription = NULL" will cause kernel oops...
It happens when using empty description... like:

cat x509_ima.der | keyctl padd asymmetric "" keyid

2. It prevents adding trusted keys to ".ima" from user space...
This is NOT what we want... right?


- Dmitry


>         }
>
>         /* pull the payload in if one was supplied */
> --
> 1.8.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,
Dmitry

Re: [RFC PATCH v4 3/4] ima: define '.ima' as a builtin 'trusted' keyring

[Dmitry Kasatkin]

On 28 May 2014 22:26, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Wed, 2014-05-28 at 21:55 +0300, Dmitry Kasatkin wrote:
>> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> > Require all keys added to the IMA keyring be signed by an
>> > existing trusted key on the system trusted keyring.
>> >
>> > Changelog v1:
>> > - don't link IMA trusted keyring to user keyring
>> >
>> > Changelog:
>> > - define stub integrity_init_keyring() function (reported-by Fengguang Wu)
>> > - differentiate between regular and trusted keyring names.
>> > - replace printk with pr_info (D. Kasatkin)
>> > - only make the IMA keyring a trusted keyring (reported-by D. Kastatkin)
>> > - define stub integrity_init_keyring() definition based on
>> >   CONFIG_INTEGRITY_SIGNATURE, not CONFIG_INTEGRITY_ASYMMETRIC_KEYS.
>> >   (reported-by Jim Davis)
>> >
>> > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
>> > ---
>> >  security/integrity/digsig.c           | 26 +++++++++++++++++++++++++-
>> >  security/integrity/ima/Kconfig        |  8 ++++++++
>> >  security/integrity/ima/ima_appraise.c | 11 +++++++++++
>> >  security/integrity/integrity.h        |  5 +++++
>> >  4 files changed, 49 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
>> > index b4af4eb..7da5f9c 100644
>> > --- a/security/integrity/digsig.c
>> > +++ b/security/integrity/digsig.c
>> > @@ -13,7 +13,9 @@
>> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> >
>> >  #include <linux/err.h>
>> > +#include <linux/sched.h>
>> >  #include <linux/rbtree.h>
>> > +#include <linux/cred.h>
>> >  #include <linux/key-type.h>
>> >  #include <linux/digsig.h>
>> >
>> > @@ -24,7 +26,11 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX];
>> >  static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
>> >         "_evm",
>> >         "_module",
>> > +#ifndef CONFIG_IMA_TRUSTED_KEYRING
>> >         "_ima",
>> > +#else
>> > +       ".ima",
>> > +#endif
>> >  };
>> >
>> >  int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>> > @@ -35,7 +41,7 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>> >
>> >         if (!keyring[id]) {
>> >                 keyring[id] =
>> > -                       request_key(&key_type_keyring, keyring_name[id], NULL);
>> > +                   request_key(&key_type_keyring, keyring_name[id], NULL);
>> >                 if (IS_ERR(keyring[id])) {
>> >                         int err = PTR_ERR(keyring[id]);
>> >                         pr_err("no %s keyring: %d\n", keyring_name[id], err);
>> > @@ -56,3 +62,21 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>> >
>> >         return -EOPNOTSUPP;
>> >  }
>> > +
>> > +int integrity_init_keyring(const unsigned int id)
>> > +{
>> > +       const struct cred *cred = current_cred();
>> > +
>> > +       keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
>> > +                                   KGIDT_INIT(0), cred,
>> > +                                   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>> > +                                     KEY_USR_VIEW | KEY_USR_READ |
>> > +                                     KEY_USR_WRITE | KEY_USR_SEARCH),
>> > +                                   KEY_ALLOC_NOT_IN_QUOTA, NULL);
>>
>> Last parameter "destination" is NULL. It makes keyring "unsearchable"
>> from user space.
>> It prevents loading trusted keys from user-space, e.g. initramfs...
>>
>> Should it be "cred->user->uid_keyring"??
>
> David extended keyctl with the '%keyring' option.  For example,
> "keyctl show %keyring:.ima" returns the .ima keyring id with a list of
> all the keys.
>

That is not kernel feature, but keyctl feature as I can see.
It will not find keyring from user space..

keyutils.c 3.5.7 has this kind of thing
f = fopen("/proc/keys", "r");

But it would require CONFIG_PROC_KEYS to be enabled.

May be David may comment...

- Dmitry


>>
>>
>> > +       if (!IS_ERR(keyring[id]))
>> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &keyring[id]->flags);
>> > +       else
>> > +               pr_info("Can't allocate %s keyring (%ld)\n",
>> > +                       keyring_name[id], PTR_ERR(keyring[id]));
>>
>> keyring[id] should be set "back" to NULL. Otherwise bad value might be
>> used in other places.
>
> Good catch, thanks.
>
>>
>> > +       return 0;
>> > +}
>> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
>> > index 81a2797..dad8d4c 100644
>> > --- a/security/integrity/ima/Kconfig
>> > +++ b/security/integrity/ima/Kconfig
>> > @@ -123,3 +123,11 @@ config IMA_APPRAISE
>> >           For more information on integrity appraisal refer to:
>> >           <http://linux-ima.sourceforge.net>
>> >           If unsure, say N.
>> > +
>> > +config IMA_TRUSTED_KEYRING
>> > +       bool "Require all keys on the _ima keyring be signed"
>> > +       depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
>> > +       default y
>> > +       help
>> > +          This option requires that all keys added to the _ima
>> > +          keyring be signed by a key on the system trusted keyring.
>> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> > index d3113d4..003ff46 100644
>> > --- a/security/integrity/ima/ima_appraise.c
>> > +++ b/security/integrity/ima/ima_appraise.c
>> > @@ -385,3 +385,14 @@ int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
>> >         }
>> >         return result;
>> >  }
>> > +
>> > +#ifdef CONFIG_IMA_TRUSTED_KEYRING
>> > +static int __init init_ima_keyring(void)
>> > +{
>> > +       int ret;
>> > +
>> > +       ret = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
>> > +       return 0;
>> > +}
>> > +late_initcall(init_ima_keyring);
>>
>>
>> late_initcall(init_ima_keyring) ordering competes with late_initcall(init_ima);
>> but we want keyring to be initialized before IMA might use it.
>
>> In the case when we would load keys from ima kernel initialization
>> code, order is important.
>>
>> we already have init_ima() and ima_init calls().
>> Why not call integrity_init_keyring() from there?
>>
>> Indeed, we have one late_initcall(init_evm) for EVM, and one
>> late_initcall(init_ima) for IMA.
>>
>> It's enough...
>
> Right, there's no reason to have an additional call.
>
> thanks,
>
> Mimi
>



-- 
Thanks,
Dmitry

Re: [RFC PATCH v4 1/4] KEYS: special dot prefixed keyring name bug fix

[Mimi Zohar]

On Fri, 2014-05-30 at 18:58 +0300, Dmitry Kasatkin wrote: 
> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > Dot prefixed keyring names are supposed to be reserved for the
> > kernel, but add_key() calls key_get_type_from_user(), which
> > incorrectly verifies the 'type' field, not the 'description' field.
> > This patch verifies the 'description' field isn't dot prefixed,
> > when creating a new keyring, and removes the dot prefix test in
> > key_get_type_from_user().
> >
> > Reported-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > Cc: David Howells <dhowells@redhat.com>
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > ---
> >  security/keys/keyctl.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > index cd5bd0c..9e9a762 100644
> > --- a/security/keys/keyctl.c
> > +++ b/security/keys/keyctl.c
> > @@ -37,8 +37,6 @@ static int key_get_type_from_user(char *type,
> >                 return ret;
> >         if (ret == 0 || ret >= len)
> >                 return -EINVAL;
> > -       if (type[0] == '.')
> > -               return -EPERM;
> >         type[len - 1] = '\0';
> >         return 0;
> >  }
> > @@ -87,6 +85,10 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
> >                         kfree(description);
> >                         description = NULL;
> >                 }
> > +               if (description[0] == '.') {
> > +                       ret = -EPERM;
> > +                       goto error2;
> > +               }
> 
> 1. 3 lines above "discription = NULL" will cause kernel oops...
> It happens when using empty description... like:
> 
> cat x509_ima.der | keyctl padd asymmetric "" keyid

Right, that should be 'else if'. 

> 2. It prevents adding trusted keys to ".ima" from user space...
> This is NOT what we want... right?

It prevents creating a dot prefixed keyring.

Mimi

Re: [RFC PATCH v4 1/4] KEYS: special dot prefixed keyring name bug fix

[Mimi Zohar]

On Fri, 2014-05-30 at 21:24 +0300, Dmitry Kasatkin wrote: 
> On 30 May 2014 20:58, "Mimi Zohar" <zohar@linux.vnet.ibm.com> wrote:
> >
> > On Fri, 2014-05-30 at 18:58 +0300, Dmitry Kasatkin wrote:
> > > On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > > Dot prefixed keyring names are supposed to be reserved for the
> > > > kernel, but add_key() calls key_get_type_from_user(), which
> > > > incorrectly verifies the 'type' field, not the 'description' field.
> > > > This patch verifies the 'description' field isn't dot prefixed,
> > > > when creating a new keyring, and removes the dot prefix test in
> > > > key_get_type_from_user().
> > > >
> > > > Reported-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > > > Cc: David Howells <dhowells@redhat.com>
> > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > > > ---
> > > >  security/keys/keyctl.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > > > index cd5bd0c..9e9a762 100644
> > > > --- a/security/keys/keyctl.c
> > > > +++ b/security/keys/keyctl.c
> > > > @@ -37,8 +37,6 @@ static int key_get_type_from_user(char *type,
> > > >                 return ret;
> > > >         if (ret == 0 || ret >= len)
> > > >                 return -EINVAL;
> > > > -       if (type[0] == '.')
> > > > -               return -EPERM;
> > > >         type[len - 1] = '\0';
> > > >         return 0;
> > > >  }
> > > > @@ -87,6 +85,10 @@ SYSCALL_DEFINE5(add_key, const char __user *,
> _type,
> > > >                         kfree(description);
> > > >                         description = NULL;
> > > >                 }
> > > > +               if (description[0] == '.') {
> > > > +                       ret = -EPERM;
> > > > +                       goto error2;
> > > > +               }
> > >
> > > 1. 3 lines above "discription = NULL" will cause kernel oops...
> > > It happens when using empty description... like:
> > >
> > > cat x509_ima.der | keyctl padd asymmetric "" keyid
> >
> > Right, that should be 'else if'.
> >
> > > 2. It prevents adding trusted keys to ".ima" from user space...
> > > This is NOT what we want... right?
> >
> > It prevents creating a dot prefixed keyring.
> >
> 
> May be. But it prevents also adding the key....
> It needs to distinguish key adding and keyring adding then...

Perhaps, but assuming you created a keyring on @u, you would still need
to look up the keyid and use it.  The same is true here.  Instead of
using "keyctl search @u keyring _ima", you would use "keyctl describe %
keyring:.ima".  The first field is the keyring id.

Mimi

Re: [RFC PATCH v4 1/4] KEYS: special dot prefixed keyring name bug fix

[Dmitry Kasatkin]

On 30 May 2014 22:12, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Fri, 2014-05-30 at 21:24 +0300, Dmitry Kasatkin wrote:
>> On 30 May 2014 20:58, "Mimi Zohar" <zohar@linux.vnet.ibm.com> wrote:
>> >
>> > On Fri, 2014-05-30 at 18:58 +0300, Dmitry Kasatkin wrote:
>> > > On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> > > > Dot prefixed keyring names are supposed to be reserved for the
>> > > > kernel, but add_key() calls key_get_type_from_user(), which
>> > > > incorrectly verifies the 'type' field, not the 'description' field.
>> > > > This patch verifies the 'description' field isn't dot prefixed,
>> > > > when creating a new keyring, and removes the dot prefix test in
>> > > > key_get_type_from_user().
>> > > >
>> > > > Reported-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> > > > Cc: David Howells <dhowells@redhat.com>
>> > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
>> > > > ---
>> > > >  security/keys/keyctl.c | 6 ++++--
>> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
>> > > > index cd5bd0c..9e9a762 100644
>> > > > --- a/security/keys/keyctl.c
>> > > > +++ b/security/keys/keyctl.c
>> > > > @@ -37,8 +37,6 @@ static int key_get_type_from_user(char *type,
>> > > >                 return ret;
>> > > >         if (ret == 0 || ret >= len)
>> > > >                 return -EINVAL;
>> > > > -       if (type[0] == '.')
>> > > > -               return -EPERM;
>> > > >         type[len - 1] = '\0';
>> > > >         return 0;
>> > > >  }
>> > > > @@ -87,6 +85,10 @@ SYSCALL_DEFINE5(add_key, const char __user *,
>> _type,
>> > > >                         kfree(description);
>> > > >                         description = NULL;
>> > > >                 }
>> > > > +               if (description[0] == '.') {
>> > > > +                       ret = -EPERM;
>> > > > +                       goto error2;
>> > > > +               }
>> > >
>> > > 1. 3 lines above "discription = NULL" will cause kernel oops...
>> > > It happens when using empty description... like:
>> > >
>> > > cat x509_ima.der | keyctl padd asymmetric "" keyid
>> >
>> > Right, that should be 'else if'.
>> >
>> > > 2. It prevents adding trusted keys to ".ima" from user space...
>> > > This is NOT what we want... right?
>> >
>> > It prevents creating a dot prefixed keyring.
>> >
>>
>> May be. But it prevents also adding the key....
>> It needs to distinguish key adding and keyring adding then...
>
> Perhaps, but assuming you created a keyring on @u, you would still need
> to look up the keyid and use it.  The same is true here.  Instead of
> using "keyctl search @u keyring _ima", you would use "keyctl describe %
> keyring:.ima".  The first field is the keyring id.
>
> Mimi
>

Not perhaps, but for sure.
In the case of adding the key, "description" is not a keyring name...
it is a key name.
In the case of adding keyring, "description" is a keyring name...

- Dmitry

-- 
Thanks,
Dmitry

Re: [RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Dmitry Kasatkin]

On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> (UEFI) secure boot provides a signature chain of trust rooted in
> hardware. The signature chain of trust includes the Machine Owner
> Keys(MOKs), which cannot be modified without physical presence.
>
> Instead of allowing public keys, with certificates signed by any
> key on the system trusted keyring, to be added to a trusted
> keyring, this patch further restricts the certificates to those
> signed by the machine owner's key or chosen key.
>
> This patch defines an owner trusted keyring, defines a new boot
> command line option 'keyring=' to designate the machine owner's
> chosen key, and renames the function get_system_trusted_keyring()
> to get_system_or_owner_trusted_keyring().
>
> This patch permits the machine owner to safely identify their own
> or chosen key, without requiring it to be builtin the kernel.
>
> Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> ---
>  crypto/asymmetric_keys/x509_public_key.c |  3 +-
>  include/keys/system_keyring.h            | 15 ++++--
>  include/linux/key.h                      |  4 ++
>  kernel/system_keyring.c                  | 85 ++++++++++++++++++++++++++++++++
>  security/keys/key.c                      | 20 ++++++++
>  5 files changed, 121 insertions(+), 6 deletions(-)
>
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index 1af8a30..6d52790 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -237,7 +237,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>                 if (ret < 0)
>                         goto error_free_cert;
>         } else {
> -               ret = x509_validate_trust(cert, get_system_trusted_keyring());
> +               ret = x509_validate_trust(cert,
> +                                         get_system_or_owner_trusted_keyring());
>                 if (!ret)
>                         prep->trusted = 1;
>         }
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 72665eb..f665c33 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -17,15 +17,20 @@
>  #include <linux/key.h>
>
>  extern struct key *system_trusted_keyring;
> -static inline struct key *get_system_trusted_keyring(void)
> -{
> -       return system_trusted_keyring;
> -}
> +extern struct key *owner_trusted_keyring;
> +
> +extern struct key *get_system_or_owner_trusted_keyring(void);
> +extern void load_owner_identified_uefi_key(key_ref_t key);
> +
>  #else
> -static inline struct key *get_system_trusted_keyring(void)
> +static inline struct key *get_system_or_owner_trusted_keyring(void)
>  {
>         return NULL;
>  }
> +
> +static void load_owner_identified_uefi_key(key_ref_t key)
> +{
> +}
>  #endif
>
>  #endif /* _KEYS_SYSTEM_KEYRING_H */
> diff --git a/include/linux/key.h b/include/linux/key.h
> index cd0abb8..861843a 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -267,6 +267,10 @@ extern int wait_for_key_construction(struct key *key, bool intr);
>
>  extern int key_validate(const struct key *key);
>
> +extern int key_match(key_ref_t key,
> +                    const char *type,
> +                    const char *description);
> +
>  extern key_ref_t key_create_or_update(key_ref_t keyring,
>                                       const char *type,
>                                       const char *description,
> diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
> index 52ebc70..e9b14ac 100644
> --- a/kernel/system_keyring.c
> +++ b/kernel/system_keyring.c
> @@ -19,11 +19,75 @@
>  #include "module-internal.h"
>
>  struct key *system_trusted_keyring;
> +struct key *owner_trusted_keyring;
>  EXPORT_SYMBOL_GPL(system_trusted_keyring);
>
>  extern __initconst const u8 system_certificate_list[];
>  extern __initconst const unsigned long system_certificate_list_size;
>
> +static int use_owner_trusted_keyring;
> +
> +static char *owner_keyid;
> +static int builtin_keyring;
> +static int __init default_keyring_set(char *str)
> +{
> +       if (!str)       /* default: builtin */
> +               return 1;
> +
> +       if (strcmp(str, "system") == 0)  /* use system keyring */
> +               ;
> +       else if (strcmp(str, "builtin") == 0)  /* only builtin keys */
> +               builtin_keyring = 1;
> +       else
> +               owner_keyid = str;   /* owner local key 'id:xxxxxx' */
> +       return 1;
> +}
> +__setup("keyring=", default_keyring_set);
> +
> +/*
> + * Load the owner identified key on the 'owner' trusted keyring.
> + */
> +void load_owner_identified_uefi_key(key_ref_t key)
> +{
> +       if (!owner_keyid || use_owner_trusted_keyring)
> +               return;
> +
> +       if (!key_match(key, "asymmetric", owner_keyid))
> +               return;
> +
> +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
> +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
> +               use_owner_trusted_keyring = 1;

This is a bit strange...
Linking any key forces to use owner trusted keyring...

> +               pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
> +                                 key_ref_to_ptr(key)->description);
> +       }
> +}
> +
> +static void load_owner_identified_builtin_key(key_ref_t key)
> +{
> +       if (!owner_keyid && !builtin_keyring)
> +               return;
> +

It looks like builtin_keyring is useless...
Is it like linking all .system keys to owners keyring???

Why not just to return .system keyring in get_system_or_owner_trusted_keyring()
based on owner_keyid?



> +       if (!builtin_keyring && !key_match(key, "asymmetric", owner_keyid))
> +               return;
> +
> +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
> +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
> +               use_owner_trusted_keyring = 1;

The same...
Linking any key forces to use owner trusted keyring...


> +               pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
> +                                 key_ref_to_ptr(key)->description);
> +       }
> +}
> +
> +/*
> + * Use the owner_trusted_keyring if available
> + */
> +struct key *get_system_or_owner_trusted_keyring(void)
> +{
> +       return use_owner_trusted_keyring ? owner_trusted_keyring :
> +               system_trusted_keyring;
> +}
> +
>  /*
>   * Load the compiled-in keys
>   */
> @@ -50,6 +114,25 @@ static __init int system_trusted_keyring_init(void)
>  device_initcall(system_trusted_keyring_init);
>
>  /*
> + * Load the owner trusted key
> + */
> +static __init int owner_trusted_keyring_init(void)
> +{
> +       pr_notice("Initialize the owner trusted keyring\n");
> +
> +       owner_trusted_keyring =
> +               keyring_alloc(".owner_keyring",
> +                             KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
> +                             ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> +                             KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
> +                             KEY_ALLOC_NOT_IN_QUOTA, NULL);
> +       if (IS_ERR(owner_trusted_keyring))
> +               panic("Can't allocate owner trusted keyring\n");
> +       return 0;
> +}
> +device_initcall(owner_trusted_keyring_init);
> +
> +/*
>   * Load the compiled-in list of X.509 certificates.
>   */
>  static __init int load_system_certificate_list(void)
> @@ -91,6 +174,8 @@ static __init int load_system_certificate_list(void)
>                 } else {
>                         pr_notice("Loaded X.509 cert '%s'\n",
>                                   key_ref_to_ptr(key)->description);
> +
> +                       load_owner_identified_builtin_key(key);
>                         key_ref_put(key);
>                 }
>                 p += plen;
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 2048a11..b448ab1 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -701,6 +701,26 @@ void key_type_put(struct key_type *ktype)
>         up_read(&key_types_sem);
>  }
>
> +/*
> + * Use the key type's match function to compare the key's id.
> + */
> +int key_match(key_ref_t key, const char *type, const char *description)
> +{
> +       struct keyring_index_key index_key;
> +       int ret = 0;
> +
> +       index_key.type = key_type_lookup(type);
> +       if (IS_ERR(index_key.type))
> +               goto out;
> +
> +       if (index_key.type->match &&
> +           index_key.type->match(key_ref_to_ptr(key), description))
> +               ret = 1;
> +       key_type_put(index_key.type);
> +out:
> +       return ret;
> +}
> +
>  /*
>   * Attempt to update an existing key.
>   *
> --
> 1.8.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,
Dmitry

Re: [RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Mimi Zohar]

On Sat, 2014-05-31 at 01:37 +0300, Dmitry Kasatkin wrote: 
> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > (UEFI) secure boot provides a signature chain of trust rooted in
> > hardware. The signature chain of trust includes the Machine Owner
> > Keys(MOKs), which cannot be modified without physical presence.
> >
> > Instead of allowing public keys, with certificates signed by any
> > key on the system trusted keyring, to be added to a trusted
> > keyring, this patch further restricts the certificates to those
> > signed by the machine owner's key or chosen key.
> >
> > This patch defines an owner trusted keyring, defines a new boot
> > command line option 'keyring=' to designate the machine owner's
> > chosen key, and renames the function get_system_trusted_keyring()
> > to get_system_or_owner_trusted_keyring().
> >
> > This patch permits the machine owner to safely identify their own
> > or chosen key, without requiring it to be builtin the kernel.
> >
> > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> > ---
> >  crypto/asymmetric_keys/x509_public_key.c |  3 +-
> >  include/keys/system_keyring.h            | 15 ++++--
> >  include/linux/key.h                      |  4 ++
> >  kernel/system_keyring.c                  | 85 ++++++++++++++++++++++++++++++++
> >  security/keys/key.c                      | 20 ++++++++
> >  5 files changed, 121 insertions(+), 6 deletions(-)
> >
> > diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> > index 1af8a30..6d52790 100644
> > --- a/crypto/asymmetric_keys/x509_public_key.c
> > +++ b/crypto/asymmetric_keys/x509_public_key.c
> > @@ -237,7 +237,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> >                 if (ret < 0)
> >                         goto error_free_cert;
> >         } else {
> > -               ret = x509_validate_trust(cert, get_system_trusted_keyring());
> > +               ret = x509_validate_trust(cert,
> > +                                         get_system_or_owner_trusted_keyring());
> >                 if (!ret)
> >                         prep->trusted = 1;
> >         }
> > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> > index 72665eb..f665c33 100644
> > --- a/include/keys/system_keyring.h
> > +++ b/include/keys/system_keyring.h
> > @@ -17,15 +17,20 @@
> >  #include <linux/key.h>
> >
> >  extern struct key *system_trusted_keyring;
> > -static inline struct key *get_system_trusted_keyring(void)
> > -{
> > -       return system_trusted_keyring;
> > -}
> > +extern struct key *owner_trusted_keyring;
> > +
> > +extern struct key *get_system_or_owner_trusted_keyring(void);
> > +extern void load_owner_identified_uefi_key(key_ref_t key);
> > +
> >  #else
> > -static inline struct key *get_system_trusted_keyring(void)
> > +static inline struct key *get_system_or_owner_trusted_keyring(void)
> >  {
> >         return NULL;
> >  }
> > +
> > +static void load_owner_identified_uefi_key(key_ref_t key)
> > +{
> > +}
> >  #endif
> >
> >  #endif /* _KEYS_SYSTEM_KEYRING_H */
> > diff --git a/include/linux/key.h b/include/linux/key.h
> > index cd0abb8..861843a 100644
> > --- a/include/linux/key.h
> > +++ b/include/linux/key.h
> > @@ -267,6 +267,10 @@ extern int wait_for_key_construction(struct key *key, bool intr);
> >
> >  extern int key_validate(const struct key *key);
> >
> > +extern int key_match(key_ref_t key,
> > +                    const char *type,
> > +                    const char *description);
> > +
> >  extern key_ref_t key_create_or_update(key_ref_t keyring,
> >                                       const char *type,
> >                                       const char *description,
> > diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
> > index 52ebc70..e9b14ac 100644
> > --- a/kernel/system_keyring.c
> > +++ b/kernel/system_keyring.c
> > @@ -19,11 +19,75 @@
> >  #include "module-internal.h"
> >
> >  struct key *system_trusted_keyring;
> > +struct key *owner_trusted_keyring;
> >  EXPORT_SYMBOL_GPL(system_trusted_keyring);
> >
> >  extern __initconst const u8 system_certificate_list[];
> >  extern __initconst const unsigned long system_certificate_list_size;
> >
> > +static int use_owner_trusted_keyring;
> > +
> > +static char *owner_keyid;
> > +static int builtin_keyring;
> > +static int __init default_keyring_set(char *str)
> > +{
> > +       if (!str)       /* default: builtin */
> > +               return 1;
> > +
> > +       if (strcmp(str, "system") == 0)  /* use system keyring */
> > +               ;
> > +       else if (strcmp(str, "builtin") == 0)  /* only builtin keys */
> > +               builtin_keyring = 1;
> > +       else
> > +               owner_keyid = str;   /* owner local key 'id:xxxxxx' */
> > +       return 1;
> > +}
> > +__setup("keyring=", default_keyring_set);
> > +
> > +/*
> > + * Load the owner identified key on the 'owner' trusted keyring.
> > + */
> > +void load_owner_identified_uefi_key(key_ref_t key)
> > +{
> > +       if (!owner_keyid || use_owner_trusted_keyring)
> > +               return;
> > +
> > +       if (!key_match(key, "asymmetric", owner_keyid))
> > +               return;
> > +
> > +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
> > +               use_owner_trusted_keyring = 1;
> 
> This is a bit strange...
> Linking any key forces to use owner trusted keyring...

Wouldn't it be stranger to identify a specific key on the system keyring
and add it to the owner keyring, but then not use it?   I don't
understand your concern.  What is the problem?

> > +               pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
> > +                                 key_ref_to_ptr(key)->description);
> > +       }
> > +}
> > +
> > +static void load_owner_identified_builtin_key(key_ref_t key)
> > +{
> > +       if (!owner_keyid && !builtin_keyring)
> > +               return;
> > +
> 
> It looks like builtin_keyring is useless...
> Is it like linking all .system keys to owners keyring???
> 
> Why not just to return .system keyring in get_system_or_owner_trusted_keyring()
> based on owner_keyid?

Before David and Josh's proposed UEFI secure boot patches, only the
builtin keys are on the system keyring. After those patches, the UEFI
secure boot keys, including the MOK keys, are there as well.

I think splitting this patch up into before and after adding the secure
boot keys to the system keyring, would help clarify this patch.

Mimi

> 
> 
> > +       if (!builtin_keyring && !key_match(key, "asymmetric", owner_keyid))
> > +               return;
> > +
> > +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
> > +               use_owner_trusted_keyring = 1;
> 
> The same...
> Linking any key forces to use owner trusted keyring...
> 
> 
> > +               pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
> > +                                 key_ref_to_ptr(key)->description);
> > +       }
> > +}
> > +
> > +/*
> > + * Use the owner_trusted_keyring if available
> > + */
> > +struct key *get_system_or_owner_trusted_keyring(void)
> > +{
> > +       return use_owner_trusted_keyring ? owner_trusted_keyring :
> > +               system_trusted_keyring;
> > +}
> > +
> >  /*
> >   * Load the compiled-in keys
> >   */
> > @@ -50,6 +114,25 @@ static __init int system_trusted_keyring_init(void)
> >  device_initcall(system_trusted_keyring_init);
> >
> >  /*
> > + * Load the owner trusted key
> > + */
> > +static __init int owner_trusted_keyring_init(void)
> > +{
> > +       pr_notice("Initialize the owner trusted keyring\n");
> > +
> > +       owner_trusted_keyring =
> > +               keyring_alloc(".owner_keyring",
> > +                             KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
> > +                             ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > +                             KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
> > +                             KEY_ALLOC_NOT_IN_QUOTA, NULL);
> > +       if (IS_ERR(owner_trusted_keyring))
> > +               panic("Can't allocate owner trusted keyring\n");
> > +       return 0;
> > +}
> > +device_initcall(owner_trusted_keyring_init);
> > +
> > +/*
> >   * Load the compiled-in list of X.509 certificates.
> >   */
> >  static __init int load_system_certificate_list(void)
> > @@ -91,6 +174,8 @@ static __init int load_system_certificate_list(void)
> >                 } else {
> >                         pr_notice("Loaded X.509 cert '%s'\n",
> >                                   key_ref_to_ptr(key)->description);
> > +
> > +                       load_owner_identified_builtin_key(key);
> >                         key_ref_put(key);
> >                 }
> >                 p += plen;
> > diff --git a/security/keys/key.c b/security/keys/key.c
> > index 2048a11..b448ab1 100644
> > --- a/security/keys/key.c
> > +++ b/security/keys/key.c
> > @@ -701,6 +701,26 @@ void key_type_put(struct key_type *ktype)
> >         up_read(&key_types_sem);
> >  }
> >
> > +/*
> > + * Use the key type's match function to compare the key's id.
> > + */
> > +int key_match(key_ref_t key, const char *type, const char *description)
> > +{
> > +       struct keyring_index_key index_key;
> > +       int ret = 0;
> > +
> > +       index_key.type = key_type_lookup(type);
> > +       if (IS_ERR(index_key.type))
> > +               goto out;
> > +
> > +       if (index_key.type->match &&
> > +           index_key.type->match(key_ref_to_ptr(key), description))
> > +               ret = 1;
> > +       key_type_put(index_key.type);
> > +out:
> > +       return ret;
> > +}
> > +
> >  /*
> >   * Attempt to update an existing key.
> >   *

Re: [RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Dmitry Kasatkin]

On 1 June 2014 05:14, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Sat, 2014-05-31 at 01:37 +0300, Dmitry Kasatkin wrote:
>> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> > (UEFI) secure boot provides a signature chain of trust rooted in
>> > hardware. The signature chain of trust includes the Machine Owner
>> > Keys(MOKs), which cannot be modified without physical presence.
>> >
>> > Instead of allowing public keys, with certificates signed by any
>> > key on the system trusted keyring, to be added to a trusted
>> > keyring, this patch further restricts the certificates to those
>> > signed by the machine owner's key or chosen key.
>> >
>> > This patch defines an owner trusted keyring, defines a new boot
>> > command line option 'keyring=' to designate the machine owner's
>> > chosen key, and renames the function get_system_trusted_keyring()
>> > to get_system_or_owner_trusted_keyring().
>> >
>> > This patch permits the machine owner to safely identify their own
>> > or chosen key, without requiring it to be builtin the kernel.
>> >
>> > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
>> > ---
>> >  crypto/asymmetric_keys/x509_public_key.c |  3 +-
>> >  include/keys/system_keyring.h            | 15 ++++--
>> >  include/linux/key.h                      |  4 ++
>> >  kernel/system_keyring.c                  | 85 ++++++++++++++++++++++++++++++++
>> >  security/keys/key.c                      | 20 ++++++++
>> >  5 files changed, 121 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
>> > index 1af8a30..6d52790 100644
>> > --- a/crypto/asymmetric_keys/x509_public_key.c
>> > +++ b/crypto/asymmetric_keys/x509_public_key.c
>> > @@ -237,7 +237,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>> >                 if (ret < 0)
>> >                         goto error_free_cert;
>> >         } else {
>> > -               ret = x509_validate_trust(cert, get_system_trusted_keyring());
>> > +               ret = x509_validate_trust(cert,
>> > +                                         get_system_or_owner_trusted_keyring());
>> >                 if (!ret)
>> >                         prep->trusted = 1;
>> >         }
>> > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
>> > index 72665eb..f665c33 100644
>> > --- a/include/keys/system_keyring.h
>> > +++ b/include/keys/system_keyring.h
>> > @@ -17,15 +17,20 @@
>> >  #include <linux/key.h>
>> >
>> >  extern struct key *system_trusted_keyring;
>> > -static inline struct key *get_system_trusted_keyring(void)
>> > -{
>> > -       return system_trusted_keyring;
>> > -}
>> > +extern struct key *owner_trusted_keyring;
>> > +
>> > +extern struct key *get_system_or_owner_trusted_keyring(void);
>> > +extern void load_owner_identified_uefi_key(key_ref_t key);
>> > +
>> >  #else
>> > -static inline struct key *get_system_trusted_keyring(void)
>> > +static inline struct key *get_system_or_owner_trusted_keyring(void)
>> >  {
>> >         return NULL;
>> >  }
>> > +
>> > +static void load_owner_identified_uefi_key(key_ref_t key)
>> > +{
>> > +}
>> >  #endif
>> >
>> >  #endif /* _KEYS_SYSTEM_KEYRING_H */
>> > diff --git a/include/linux/key.h b/include/linux/key.h
>> > index cd0abb8..861843a 100644
>> > --- a/include/linux/key.h
>> > +++ b/include/linux/key.h
>> > @@ -267,6 +267,10 @@ extern int wait_for_key_construction(struct key *key, bool intr);
>> >
>> >  extern int key_validate(const struct key *key);
>> >
>> > +extern int key_match(key_ref_t key,
>> > +                    const char *type,
>> > +                    const char *description);
>> > +
>> >  extern key_ref_t key_create_or_update(key_ref_t keyring,
>> >                                       const char *type,
>> >                                       const char *description,
>> > diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
>> > index 52ebc70..e9b14ac 100644
>> > --- a/kernel/system_keyring.c
>> > +++ b/kernel/system_keyring.c
>> > @@ -19,11 +19,75 @@
>> >  #include "module-internal.h"
>> >
>> >  struct key *system_trusted_keyring;
>> > +struct key *owner_trusted_keyring;
>> >  EXPORT_SYMBOL_GPL(system_trusted_keyring);
>> >
>> >  extern __initconst const u8 system_certificate_list[];
>> >  extern __initconst const unsigned long system_certificate_list_size;
>> >
>> > +static int use_owner_trusted_keyring;
>> > +
>> > +static char *owner_keyid;
>> > +static int builtin_keyring;
>> > +static int __init default_keyring_set(char *str)
>> > +{
>> > +       if (!str)       /* default: builtin */
>> > +               return 1;
>> > +
>> > +       if (strcmp(str, "system") == 0)  /* use system keyring */
>> > +               ;
>> > +       else if (strcmp(str, "builtin") == 0)  /* only builtin keys */
>> > +               builtin_keyring = 1;
>> > +       else
>> > +               owner_keyid = str;   /* owner local key 'id:xxxxxx' */
>> > +       return 1;
>> > +}
>> > +__setup("keyring=", default_keyring_set);
>> > +
>> > +/*
>> > + * Load the owner identified key on the 'owner' trusted keyring.
>> > + */
>> > +void load_owner_identified_uefi_key(key_ref_t key)
>> > +{
>> > +       if (!owner_keyid || use_owner_trusted_keyring)
>> > +               return;
>> > +
>> > +       if (!key_match(key, "asymmetric", owner_keyid))
>> > +               return;
>> > +
>> > +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
>> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
>> > +               use_owner_trusted_keyring = 1;
>>
>> This is a bit strange...
>> Linking any key forces to use owner trusted keyring...
>
> Wouldn't it be stranger to identify a specific key on the system keyring
> and add it to the owner keyring, but then not use it?   I don't
> understand your concern.  What is the problem?

This patch technically specifies to use all keys from the system
keyring or the one specified
by the owner_keyid. Why the owner keyring is needed then at all?
owner_keyid can identify the key to use from the system keyring....


>
>> > +               pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
>> > +                                 key_ref_to_ptr(key)->description);
>> > +       }
>> > +}
>> > +
>> > +static void load_owner_identified_builtin_key(key_ref_t key)
>> > +{
>> > +       if (!owner_keyid && !builtin_keyring)
>> > +               return;
>> > +
>>
>> It looks like builtin_keyring is useless...
>> Is it like linking all .system keys to owners keyring???
>>
>> Why not just to return .system keyring in get_system_or_owner_trusted_keyring()
>> based on owner_keyid?
>
> Before David and Josh's proposed UEFI secure boot patches, only the
> builtin keys are on the system keyring. After those patches, the UEFI
> secure boot keys, including the MOK keys, are there as well.
>
> I think splitting this patch up into before and after adding the secure
> boot keys to the system keyring, would help clarify this patch.
>

> Mimi
>
>>
>>
>> > +       if (!builtin_keyring && !key_match(key, "asymmetric", owner_keyid))
>> > +               return;
>> > +
>> > +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
>> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
>> > +               use_owner_trusted_keyring = 1;
>>
>> The same...
>> Linking any key forces to use owner trusted keyring...
>>
>>
>> > +               pr_notice("Loaded X.509 cert '%s' on .owner_keyring\n",
>> > +                                 key_ref_to_ptr(key)->description);
>> > +       }
>> > +}
>> > +
>> > +/*
>> > + * Use the owner_trusted_keyring if available
>> > + */
>> > +struct key *get_system_or_owner_trusted_keyring(void)
>> > +{
>> > +       return use_owner_trusted_keyring ? owner_trusted_keyring :
>> > +               system_trusted_keyring;
>> > +}
>> > +
>> >  /*
>> >   * Load the compiled-in keys
>> >   */
>> > @@ -50,6 +114,25 @@ static __init int system_trusted_keyring_init(void)
>> >  device_initcall(system_trusted_keyring_init);
>> >
>> >  /*
>> > + * Load the owner trusted key
>> > + */
>> > +static __init int owner_trusted_keyring_init(void)
>> > +{
>> > +       pr_notice("Initialize the owner trusted keyring\n");
>> > +
>> > +       owner_trusted_keyring =
>> > +               keyring_alloc(".owner_keyring",
>> > +                             KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
>> > +                             ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>> > +                             KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
>> > +                             KEY_ALLOC_NOT_IN_QUOTA, NULL);
>> > +       if (IS_ERR(owner_trusted_keyring))
>> > +               panic("Can't allocate owner trusted keyring\n");
>> > +       return 0;
>> > +}
>> > +device_initcall(owner_trusted_keyring_init);
>> > +
>> > +/*
>> >   * Load the compiled-in list of X.509 certificates.
>> >   */
>> >  static __init int load_system_certificate_list(void)
>> > @@ -91,6 +174,8 @@ static __init int load_system_certificate_list(void)
>> >                 } else {
>> >                         pr_notice("Loaded X.509 cert '%s'\n",
>> >                                   key_ref_to_ptr(key)->description);
>> > +
>> > +                       load_owner_identified_builtin_key(key);
>> >                         key_ref_put(key);
>> >                 }
>> >                 p += plen;
>> > diff --git a/security/keys/key.c b/security/keys/key.c
>> > index 2048a11..b448ab1 100644
>> > --- a/security/keys/key.c
>> > +++ b/security/keys/key.c
>> > @@ -701,6 +701,26 @@ void key_type_put(struct key_type *ktype)
>> >         up_read(&key_types_sem);
>> >  }
>> >
>> > +/*
>> > + * Use the key type's match function to compare the key's id.
>> > + */
>> > +int key_match(key_ref_t key, const char *type, const char *description)
>> > +{
>> > +       struct keyring_index_key index_key;
>> > +       int ret = 0;
>> > +
>> > +       index_key.type = key_type_lookup(type);
>> > +       if (IS_ERR(index_key.type))
>> > +               goto out;
>> > +
>> > +       if (index_key.type->match &&
>> > +           index_key.type->match(key_ref_to_ptr(key), description))
>> > +               ret = 1;
>> > +       key_type_put(index_key.type);
>> > +out:
>> > +       return ret;
>> > +}
>> > +
>> >  /*
>> >   * Attempt to update an existing key.
>> >   *
>
>



-- 
Thanks,
Dmitry

Re: [RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Mimi Zohar]

On Mon, 2014-06-02 at 13:48 +0300, Dmitry Kasatkin wrote: 
> On 1 June 2014 05:14, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Sat, 2014-05-31 at 01:37 +0300, Dmitry Kasatkin wrote:
> >> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> > (UEFI) secure boot provides a signature chain of trust rooted in
> >> > hardware. The signature chain of trust includes the Machine Owner
> >> > Keys(MOKs), which cannot be modified without physical presence.
> >> >
> >> > Instead of allowing public keys, with certificates signed by any
> >> > key on the system trusted keyring, to be added to a trusted
> >> > keyring, this patch further restricts the certificates to those
> >> > signed by the machine owner's key or chosen key.
> >> >
> >> > This patch defines an owner trusted keyring, defines a new boot
> >> > command line option 'keyring=' to designate the machine owner's
> >> > chosen key, and renames the function get_system_trusted_keyring()
> >> > to get_system_or_owner_trusted_keyring().
> >> >
> >> > This patch permits the machine owner to safely identify their own
> >> > or chosen key, without requiring it to be builtin the kernel.
> >> >
> >> > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> >> > ---
> >> >  crypto/asymmetric_keys/x509_public_key.c |  3 +-
> >> >  include/keys/system_keyring.h            | 15 ++++--
> >> >  include/linux/key.h                      |  4 ++
> >> >  kernel/system_keyring.c                  | 85 ++++++++++++++++++++++++++++++++
> >> >  security/keys/key.c                      | 20 ++++++++
> >> >  5 files changed, 121 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> >> > index 1af8a30..6d52790 100644
> >> > --- a/crypto/asymmetric_keys/x509_public_key.c
> >> > +++ b/crypto/asymmetric_keys/x509_public_key.c
> >> > @@ -237,7 +237,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> >> >                 if (ret < 0)
> >> >                         goto error_free_cert;
> >> >         } else {
> >> > -               ret = x509_validate_trust(cert, get_system_trusted_keyring());
> >> > +               ret = x509_validate_trust(cert,
> >> > +                                         get_system_or_owner_trusted_keyring());
> >> >                 if (!ret)
> >> >                         prep->trusted = 1;
> >> >         }
> >> > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> >> > index 72665eb..f665c33 100644
> >> > --- a/include/keys/system_keyring.h
> >> > +++ b/include/keys/system_keyring.h
> >> > @@ -17,15 +17,20 @@
> >> >  #include <linux/key.h>
> >> >
> >> >  extern struct key *system_trusted_keyring;
> >> > -static inline struct key *get_system_trusted_keyring(void)
> >> > -{
> >> > -       return system_trusted_keyring;
> >> > -}
> >> > +extern struct key *owner_trusted_keyring;
> >> > +
> >> > +extern struct key *get_system_or_owner_trusted_keyring(void);
> >> > +extern void load_owner_identified_uefi_key(key_ref_t key);
> >> > +
> >> >  #else
> >> > -static inline struct key *get_system_trusted_keyring(void)
> >> > +static inline struct key *get_system_or_owner_trusted_keyring(void)
> >> >  {
> >> >         return NULL;
> >> >  }
> >> > +
> >> > +static void load_owner_identified_uefi_key(key_ref_t key)
> >> > +{
> >> > +}
> >> >  #endif
> >> >
> >> >  #endif /* _KEYS_SYSTEM_KEYRING_H */
> >> > diff --git a/include/linux/key.h b/include/linux/key.h
> >> > index cd0abb8..861843a 100644
> >> > --- a/include/linux/key.h
> >> > +++ b/include/linux/key.h
> >> > @@ -267,6 +267,10 @@ extern int wait_for_key_construction(struct key *key, bool intr);
> >> >
> >> >  extern int key_validate(const struct key *key);
> >> >
> >> > +extern int key_match(key_ref_t key,
> >> > +                    const char *type,
> >> > +                    const char *description);
> >> > +
> >> >  extern key_ref_t key_create_or_update(key_ref_t keyring,
> >> >                                       const char *type,
> >> >                                       const char *description,
> >> > diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
> >> > index 52ebc70..e9b14ac 100644
> >> > --- a/kernel/system_keyring.c
> >> > +++ b/kernel/system_keyring.c
> >> > @@ -19,11 +19,75 @@
> >> >  #include "module-internal.h"
> >> >
> >> >  struct key *system_trusted_keyring;
> >> > +struct key *owner_trusted_keyring;
> >> >  EXPORT_SYMBOL_GPL(system_trusted_keyring);
> >> >
> >> >  extern __initconst const u8 system_certificate_list[];
> >> >  extern __initconst const unsigned long system_certificate_list_size;
> >> >
> >> > +static int use_owner_trusted_keyring;
> >> > +
> >> > +static char *owner_keyid;
> >> > +static int builtin_keyring;
> >> > +static int __init default_keyring_set(char *str)
> >> > +{
> >> > +       if (!str)       /* default: builtin */
> >> > +               return 1;
> >> > +
> >> > +       if (strcmp(str, "system") == 0)  /* use system keyring */
> >> > +               ;
> >> > +       else if (strcmp(str, "builtin") == 0)  /* only builtin keys */
> >> > +               builtin_keyring = 1;
> >> > +       else
> >> > +               owner_keyid = str;   /* owner local key 'id:xxxxxx' */
> >> > +       return 1;
> >> > +}
> >> > +__setup("keyring=", default_keyring_set);
> >> > +
> >> > +/*
> >> > + * Load the owner identified key on the 'owner' trusted keyring.
> >> > + */
> >> > +void load_owner_identified_uefi_key(key_ref_t key)
> >> > +{
> >> > +       if (!owner_keyid || use_owner_trusted_keyring)
> >> > +               return;
> >> > +
> >> > +       if (!key_match(key, "asymmetric", owner_keyid))
> >> > +               return;
> >> > +
> >> > +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
> >> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
> >> > +               use_owner_trusted_keyring = 1;
> >>
> >> This is a bit strange...
> >> Linking any key forces to use owner trusted keyring...
> >
> > Wouldn't it be stranger to identify a specific key on the system keyring
> > and add it to the owner keyring, but then not use it?   I don't
> > understand your concern.  What is the problem?
> 
> This patch technically specifies to use all keys from the system
> keyring or the one specified
> by the owner_keyid. Why the owner keyring is needed then at all?
> owner_keyid can identify the key to use from the system keyring....

Currently only the builtin keys are on the system keyring, but once
David and Josh's UEFI patches are upstreamed, the UEFI keys will also be
on the system keyring.  At that point, we would want to differentiate
between the builtin keys and the remaining keys on the system keyring.
Splitting this patch definitely helps clarify what's happening and, more
importantly, why. 

Mimi

Re: [RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Dmitry Kasatkin]

On 2 June 2014 14:33, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Mon, 2014-06-02 at 13:48 +0300, Dmitry Kasatkin wrote:
>> On 1 June 2014 05:14, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> > On Sat, 2014-05-31 at 01:37 +0300, Dmitry Kasatkin wrote:
>> >> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> >> > (UEFI) secure boot provides a signature chain of trust rooted in
>> >> > hardware. The signature chain of trust includes the Machine Owner
>> >> > Keys(MOKs), which cannot be modified without physical presence.
>> >> >
>> >> > Instead of allowing public keys, with certificates signed by any
>> >> > key on the system trusted keyring, to be added to a trusted
>> >> > keyring, this patch further restricts the certificates to those
>> >> > signed by the machine owner's key or chosen key.
>> >> >
>> >> > This patch defines an owner trusted keyring, defines a new boot
>> >> > command line option 'keyring=' to designate the machine owner's
>> >> > chosen key, and renames the function get_system_trusted_keyring()
>> >> > to get_system_or_owner_trusted_keyring().
>> >> >
>> >> > This patch permits the machine owner to safely identify their own
>> >> > or chosen key, without requiring it to be builtin the kernel.
>> >> >
>> >> > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
>> >> > ---
>> >> >  crypto/asymmetric_keys/x509_public_key.c |  3 +-
>> >> >  include/keys/system_keyring.h            | 15 ++++--
>> >> >  include/linux/key.h                      |  4 ++
>> >> >  kernel/system_keyring.c                  | 85 ++++++++++++++++++++++++++++++++
>> >> >  security/keys/key.c                      | 20 ++++++++
>> >> >  5 files changed, 121 insertions(+), 6 deletions(-)
>> >> >
>> >> > diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
>> >> > index 1af8a30..6d52790 100644
>> >> > --- a/crypto/asymmetric_keys/x509_public_key.c
>> >> > +++ b/crypto/asymmetric_keys/x509_public_key.c
>> >> > @@ -237,7 +237,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>> >> >                 if (ret < 0)
>> >> >                         goto error_free_cert;
>> >> >         } else {
>> >> > -               ret = x509_validate_trust(cert, get_system_trusted_keyring());
>> >> > +               ret = x509_validate_trust(cert,
>> >> > +                                         get_system_or_owner_trusted_keyring());
>> >> >                 if (!ret)
>> >> >                         prep->trusted = 1;
>> >> >         }
>> >> > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
>> >> > index 72665eb..f665c33 100644
>> >> > --- a/include/keys/system_keyring.h
>> >> > +++ b/include/keys/system_keyring.h
>> >> > @@ -17,15 +17,20 @@
>> >> >  #include <linux/key.h>
>> >> >
>> >> >  extern struct key *system_trusted_keyring;
>> >> > -static inline struct key *get_system_trusted_keyring(void)
>> >> > -{
>> >> > -       return system_trusted_keyring;
>> >> > -}
>> >> > +extern struct key *owner_trusted_keyring;
>> >> > +
>> >> > +extern struct key *get_system_or_owner_trusted_keyring(void);
>> >> > +extern void load_owner_identified_uefi_key(key_ref_t key);
>> >> > +
>> >> >  #else
>> >> > -static inline struct key *get_system_trusted_keyring(void)
>> >> > +static inline struct key *get_system_or_owner_trusted_keyring(void)
>> >> >  {
>> >> >         return NULL;
>> >> >  }
>> >> > +
>> >> > +static void load_owner_identified_uefi_key(key_ref_t key)
>> >> > +{
>> >> > +}
>> >> >  #endif
>> >> >
>> >> >  #endif /* _KEYS_SYSTEM_KEYRING_H */
>> >> > diff --git a/include/linux/key.h b/include/linux/key.h
>> >> > index cd0abb8..861843a 100644
>> >> > --- a/include/linux/key.h
>> >> > +++ b/include/linux/key.h
>> >> > @@ -267,6 +267,10 @@ extern int wait_for_key_construction(struct key *key, bool intr);
>> >> >
>> >> >  extern int key_validate(const struct key *key);
>> >> >
>> >> > +extern int key_match(key_ref_t key,
>> >> > +                    const char *type,
>> >> > +                    const char *description);
>> >> > +
>> >> >  extern key_ref_t key_create_or_update(key_ref_t keyring,
>> >> >                                       const char *type,
>> >> >                                       const char *description,
>> >> > diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
>> >> > index 52ebc70..e9b14ac 100644
>> >> > --- a/kernel/system_keyring.c
>> >> > +++ b/kernel/system_keyring.c
>> >> > @@ -19,11 +19,75 @@
>> >> >  #include "module-internal.h"
>> >> >
>> >> >  struct key *system_trusted_keyring;
>> >> > +struct key *owner_trusted_keyring;
>> >> >  EXPORT_SYMBOL_GPL(system_trusted_keyring);
>> >> >
>> >> >  extern __initconst const u8 system_certificate_list[];
>> >> >  extern __initconst const unsigned long system_certificate_list_size;
>> >> >
>> >> > +static int use_owner_trusted_keyring;
>> >> > +
>> >> > +static char *owner_keyid;
>> >> > +static int builtin_keyring;
>> >> > +static int __init default_keyring_set(char *str)
>> >> > +{
>> >> > +       if (!str)       /* default: builtin */
>> >> > +               return 1;
>> >> > +
>> >> > +       if (strcmp(str, "system") == 0)  /* use system keyring */
>> >> > +               ;
>> >> > +       else if (strcmp(str, "builtin") == 0)  /* only builtin keys */
>> >> > +               builtin_keyring = 1;
>> >> > +       else
>> >> > +               owner_keyid = str;   /* owner local key 'id:xxxxxx' */
>> >> > +       return 1;
>> >> > +}
>> >> > +__setup("keyring=", default_keyring_set);
>> >> > +
>> >> > +/*
>> >> > + * Load the owner identified key on the 'owner' trusted keyring.
>> >> > + */
>> >> > +void load_owner_identified_uefi_key(key_ref_t key)
>> >> > +{
>> >> > +       if (!owner_keyid || use_owner_trusted_keyring)
>> >> > +               return;
>> >> > +
>> >> > +       if (!key_match(key, "asymmetric", owner_keyid))
>> >> > +               return;
>> >> > +
>> >> > +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
>> >> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
>> >> > +               use_owner_trusted_keyring = 1;
>> >>
>> >> This is a bit strange...
>> >> Linking any key forces to use owner trusted keyring...
>> >
>> > Wouldn't it be stranger to identify a specific key on the system keyring
>> > and add it to the owner keyring, but then not use it?   I don't
>> > understand your concern.  What is the problem?
>>
>> This patch technically specifies to use all keys from the system
>> keyring or the one specified
>> by the owner_keyid. Why the owner keyring is needed then at all?
>> owner_keyid can identify the key to use from the system keyring....
>
> Currently only the builtin keys are on the system keyring, but once
> David and Josh's UEFI patches are upstreamed, the UEFI keys will also be
> on the system keyring.  At that point, we would want to differentiate
> between the builtin keys and the remaining keys on the system keyring.
> Splitting this patch definitely helps clarify what's happening and, more
> importantly, why.
>
> Mimi
>

Ok. May be would should focus on patches for existing functionality.
If something new comes from other side, it can be addressed by new
another set of patches.

- Dmitry

Re: [RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Mimi Zohar]

On Mon, 2014-06-02 at 14:40 +0300, Dmitry Kasatkin wrote: 
> On 2 June 2014 14:33, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Mon, 2014-06-02 at 13:48 +0300, Dmitry Kasatkin wrote:

> > Currently only the builtin keys are on the system keyring, but once
> > David and Josh's UEFI patches are upstreamed, the UEFI keys will also be
> > on the system keyring.  At that point, we would want to differentiate
> > between the builtin keys and the remaining keys on the system keyring.
> > Splitting this patch definitely helps clarify what's happening and, more
> > importantly, why.
> >
> > Mimi
> >
> 
> Ok. May be would should focus on patches for existing functionality.
> If something new comes from other side, it can be addressed by new
> another set of patches.

True, making the IMA keyring a trusted keyring is important by itself,
but the real benefit is the ability for the platform owner to create and
use their own key without having to rebuild the kernel.  The platform
owner then controls which keys are to be trusted.

Mimi

Re: [RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Josh Boyer]

On Mon, Jun 02, 2014 at 02:40:28PM +0300, Dmitry Kasatkin wrote:
> On 2 June 2014 14:33, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Mon, 2014-06-02 at 13:48 +0300, Dmitry Kasatkin wrote:
> >> On 1 June 2014 05:14, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> > On Sat, 2014-05-31 at 01:37 +0300, Dmitry Kasatkin wrote:
> >> >> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> >> > (UEFI) secure boot provides a signature chain of trust rooted in
> >> >> > hardware. The signature chain of trust includes the Machine Owner
> >> >> > Keys(MOKs), which cannot be modified without physical presence.
> >> >> >
> >> >> > Instead of allowing public keys, with certificates signed by any
> >> >> > key on the system trusted keyring, to be added to a trusted
> >> >> > keyring, this patch further restricts the certificates to those
> >> >> > signed by the machine owner's key or chosen key.
> >> >> >
> >> >> > This patch defines an owner trusted keyring, defines a new boot
> >> >> > command line option 'keyring=' to designate the machine owner's
> >> >> > chosen key, and renames the function get_system_trusted_keyring()
> >> >> > to get_system_or_owner_trusted_keyring().
> >> >> >
> >> >> > This patch permits the machine owner to safely identify their own
> >> >> > or chosen key, without requiring it to be builtin the kernel.
> >> >> >
> >> >> > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> >> >> > ---
> >> >> >  crypto/asymmetric_keys/x509_public_key.c |  3 +-
> >> >> >  include/keys/system_keyring.h            | 15 ++++--
> >> >> >  include/linux/key.h                      |  4 ++
> >> >> >  kernel/system_keyring.c                  | 85 ++++++++++++++++++++++++++++++++
> >> >> >  security/keys/key.c                      | 20 ++++++++
> >> >> >  5 files changed, 121 insertions(+), 6 deletions(-)
> >> >> >
> >> >> > diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> >> >> > index 1af8a30..6d52790 100644
> >> >> > --- a/crypto/asymmetric_keys/x509_public_key.c
> >> >> > +++ b/crypto/asymmetric_keys/x509_public_key.c
> >> >> > @@ -237,7 +237,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
> >> >> >                 if (ret < 0)
> >> >> >                         goto error_free_cert;
> >> >> >         } else {
> >> >> > -               ret = x509_validate_trust(cert, get_system_trusted_keyring());
> >> >> > +               ret = x509_validate_trust(cert,
> >> >> > +                                         get_system_or_owner_trusted_keyring());
> >> >> >                 if (!ret)
> >> >> >                         prep->trusted = 1;
> >> >> >         }
> >> >> > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> >> >> > index 72665eb..f665c33 100644
> >> >> > --- a/include/keys/system_keyring.h
> >> >> > +++ b/include/keys/system_keyring.h
> >> >> > @@ -17,15 +17,20 @@
> >> >> >  #include <linux/key.h>
> >> >> >
> >> >> >  extern struct key *system_trusted_keyring;
> >> >> > -static inline struct key *get_system_trusted_keyring(void)
> >> >> > -{
> >> >> > -       return system_trusted_keyring;
> >> >> > -}
> >> >> > +extern struct key *owner_trusted_keyring;
> >> >> > +
> >> >> > +extern struct key *get_system_or_owner_trusted_keyring(void);
> >> >> > +extern void load_owner_identified_uefi_key(key_ref_t key);
> >> >> > +
> >> >> >  #else
> >> >> > -static inline struct key *get_system_trusted_keyring(void)
> >> >> > +static inline struct key *get_system_or_owner_trusted_keyring(void)
> >> >> >  {
> >> >> >         return NULL;
> >> >> >  }
> >> >> > +
> >> >> > +static void load_owner_identified_uefi_key(key_ref_t key)
> >> >> > +{
> >> >> > +}
> >> >> >  #endif
> >> >> >
> >> >> >  #endif /* _KEYS_SYSTEM_KEYRING_H */
> >> >> > diff --git a/include/linux/key.h b/include/linux/key.h
> >> >> > index cd0abb8..861843a 100644
> >> >> > --- a/include/linux/key.h
> >> >> > +++ b/include/linux/key.h
> >> >> > @@ -267,6 +267,10 @@ extern int wait_for_key_construction(struct key *key, bool intr);
> >> >> >
> >> >> >  extern int key_validate(const struct key *key);
> >> >> >
> >> >> > +extern int key_match(key_ref_t key,
> >> >> > +                    const char *type,
> >> >> > +                    const char *description);
> >> >> > +
> >> >> >  extern key_ref_t key_create_or_update(key_ref_t keyring,
> >> >> >                                       const char *type,
> >> >> >                                       const char *description,
> >> >> > diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
> >> >> > index 52ebc70..e9b14ac 100644
> >> >> > --- a/kernel/system_keyring.c
> >> >> > +++ b/kernel/system_keyring.c
> >> >> > @@ -19,11 +19,75 @@
> >> >> >  #include "module-internal.h"
> >> >> >
> >> >> >  struct key *system_trusted_keyring;
> >> >> > +struct key *owner_trusted_keyring;
> >> >> >  EXPORT_SYMBOL_GPL(system_trusted_keyring);
> >> >> >
> >> >> >  extern __initconst const u8 system_certificate_list[];
> >> >> >  extern __initconst const unsigned long system_certificate_list_size;
> >> >> >
> >> >> > +static int use_owner_trusted_keyring;
> >> >> > +
> >> >> > +static char *owner_keyid;
> >> >> > +static int builtin_keyring;
> >> >> > +static int __init default_keyring_set(char *str)
> >> >> > +{
> >> >> > +       if (!str)       /* default: builtin */
> >> >> > +               return 1;
> >> >> > +
> >> >> > +       if (strcmp(str, "system") == 0)  /* use system keyring */
> >> >> > +               ;
> >> >> > +       else if (strcmp(str, "builtin") == 0)  /* only builtin keys */
> >> >> > +               builtin_keyring = 1;
> >> >> > +       else
> >> >> > +               owner_keyid = str;   /* owner local key 'id:xxxxxx' */
> >> >> > +       return 1;
> >> >> > +}
> >> >> > +__setup("keyring=", default_keyring_set);
> >> >> > +
> >> >> > +/*
> >> >> > + * Load the owner identified key on the 'owner' trusted keyring.
> >> >> > + */
> >> >> > +void load_owner_identified_uefi_key(key_ref_t key)
> >> >> > +{
> >> >> > +       if (!owner_keyid || use_owner_trusted_keyring)
> >> >> > +               return;
> >> >> > +
> >> >> > +       if (!key_match(key, "asymmetric", owner_keyid))
> >> >> > +               return;
> >> >> > +
> >> >> > +       if (key_link(owner_trusted_keyring, key_ref_to_ptr(key)) == 0) {
> >> >> > +               set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
> >> >> > +               use_owner_trusted_keyring = 1;
> >> >>
> >> >> This is a bit strange...
> >> >> Linking any key forces to use owner trusted keyring...
> >> >
> >> > Wouldn't it be stranger to identify a specific key on the system keyring
> >> > and add it to the owner keyring, but then not use it?   I don't
> >> > understand your concern.  What is the problem?
> >>
> >> This patch technically specifies to use all keys from the system
> >> keyring or the one specified
> >> by the owner_keyid. Why the owner keyring is needed then at all?
> >> owner_keyid can identify the key to use from the system keyring....
> >
> > Currently only the builtin keys are on the system keyring, but once
> > David and Josh's UEFI patches are upstreamed, the UEFI keys will also be
> > on the system keyring.  At that point, we would want to differentiate
> > between the builtin keys and the remaining keys on the system keyring.
> > Splitting this patch definitely helps clarify what's happening and, more
> > importantly, why.
> >
> > Mimi
> >
> 
> Ok. May be would should focus on patches for existing functionality.
> If something new comes from other side, it can be addressed by new
> another set of patches.

I agree.  I appreciate taking the UEFI key patches into account, but
they're held up and won't be hitting mainline in the next release or
two.  Focus on the changes you need to make against mainline.

josh

Re: [RFC PATCH v4 4/4] KEYS: define an owner trusted keyring

[Mimi Zohar]

On Mon, 2014-06-02 at 07:55 -0400, Josh Boyer wrote: 
> On Mon, Jun 02, 2014 at 02:40:28PM +0300, Dmitry Kasatkin wrote:
> > On 2 June 2014 14:33, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > > On Mon, 2014-06-02 at 13:48 +0300, Dmitry Kasatkin wrote:
> > >> On 1 June 2014 05:14, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> > > Currently only the builtin keys are on the system keyring, but once
> > > David and Josh's UEFI patches are upstreamed, the UEFI keys will also be
> > > on the system keyring.  At that point, we would want to differentiate
> > > between the builtin keys and the remaining keys on the system keyring.
> > > Splitting this patch definitely helps clarify what's happening and, more
> > > importantly, why.
> > >
> > > Mimi
> > >
> > 
> > Ok. May be would should focus on patches for existing functionality.
> > If something new comes from other side, it can be addressed by new
> > another set of patches.
> 
> I agree.  I appreciate taking the UEFI key patches into account, but
> they're held up and won't be hitting mainline in the next release or
> two.  Focus on the changes you need to make against mainline.

Ok.  I've rewritten the patch.  Instead of loading the key on the owner
keyring at the same time as loading it on the system keyring, it waits
until all the keys are on the system keyring, before adding the
specified key to the owner keyring.  We loose the ability to specify all
the builtin keys, but at least for now, this isn't important.  For now,
we can identify a single builtin key, and when the UEFI patches are
upstreamed, we should be able to identify a UEFI key, as well.

thanks,

Mimi

Re: [RFC PATCH v4 3/4] ima: define '.ima' as a builtin 'trusted' keyring

[Mimi Zohar]

On Fri, 2014-05-30 at 19:05 +0300, Dmitry Kasatkin wrote:
> On 28 May 2014 22:26, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Wed, 2014-05-28 at 21:55 +0300, Dmitry Kasatkin wrote:
> >> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> > Require all keys added to the IMA keyring be signed by an
> >> > existing trusted key on the system trusted keyring.
> >> >
> >> > Changelog v1:
> >> > - don't link IMA trusted keyring to user keyring
> >> >
> >> > Changelog:
> >> > - define stub integrity_init_keyring() function (reported-by Fengguang Wu)
> >> > - differentiate between regular and trusted keyring names.
> >> > - replace printk with pr_info (D. Kasatkin)
> >> > - only make the IMA keyring a trusted keyring (reported-by D. Kastatkin)
> >> > - define stub integrity_init_keyring() definition based on
> >> >   CONFIG_INTEGRITY_SIGNATURE, not CONFIG_INTEGRITY_ASYMMETRIC_KEYS.
> >> >   (reported-by Jim Davis)
> >> >
> >> > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> >> > ---
> >> >  security/integrity/digsig.c           | 26 +++++++++++++++++++++++++-
> >> >  security/integrity/ima/Kconfig        |  8 ++++++++
> >> >  security/integrity/ima/ima_appraise.c | 11 +++++++++++
> >> >  security/integrity/integrity.h        |  5 +++++
> >> >  4 files changed, 49 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> >> > index b4af4eb..7da5f9c 100644
> >> > --- a/security/integrity/digsig.c
> >> > +++ b/security/integrity/digsig.c
> >> > @@ -13,7 +13,9 @@
> >> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >> >
> >> >  #include <linux/err.h>
> >> > +#include <linux/sched.h>
> >> >  #include <linux/rbtree.h>
> >> > +#include <linux/cred.h>
> >> >  #include <linux/key-type.h>
> >> >  #include <linux/digsig.h>
> >> >
> >> > @@ -24,7 +26,11 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX];
> >> >  static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
> >> >         "_evm",
> >> >         "_module",
> >> > +#ifndef CONFIG_IMA_TRUSTED_KEYRING
> >> >         "_ima",
> >> > +#else
> >> > +       ".ima",
> >> > +#endif
> >> >  };
> >> >
> >> >  int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> >> > @@ -35,7 +41,7 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> >> >
> >> >         if (!keyring[id]) {
> >> >                 keyring[id] =
> >> > -                       request_key(&key_type_keyring, keyring_name[id], NULL);
> >> > +                   request_key(&key_type_keyring, keyring_name[id], NULL);
> >> >                 if (IS_ERR(keyring[id])) {
> >> >                         int err = PTR_ERR(keyring[id]);
> >> >                         pr_err("no %s keyring: %d\n", keyring_name[id], err);
> >> > @@ -56,3 +62,21 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
> >> >
> >> >         return -EOPNOTSUPP;
> >> >  }
> >> > +
> >> > +int integrity_init_keyring(const unsigned int id)
> >> > +{
> >> > +       const struct cred *cred = current_cred();
> >> > +
> >> > +       keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
> >> > +                                   KGIDT_INIT(0), cred,
> >> > +                                   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> >> > +                                     KEY_USR_VIEW | KEY_USR_READ |
> >> > +                                     KEY_USR_WRITE | KEY_USR_SEARCH),
> >> > +                                   KEY_ALLOC_NOT_IN_QUOTA, NULL);
> >>
> >> Last parameter "destination" is NULL. It makes keyring "unsearchable"
> >> from user space.
> >> It prevents loading trusted keys from user-space, e.g. initramfs...
> >>
> >> Should it be "cred->user->uid_keyring"??
> >
> > David extended keyctl with the '%keyring' option.  For example,
> > "keyctl show %keyring:.ima" returns the .ima keyring id with a list of
> > all the keys.
> >
> 
> That is not kernel feature, but keyctl feature as I can see.
> It will not find keyring from user space..
> 
> keyutils.c 3.5.7 has this kind of thing
> f = fopen("/proc/keys", "r");
> 
> But it would require CONFIG_PROC_KEYS to be enabled.
> 
> May be David may comment...

David commented on an prior patch set, which defined a new id for the
system trusted keyring. For hjs comments, refer to
http://marc.info/?l=linux-security-module&m=137829415530503&w=2

thanks,

Mimi

Re: [RFC PATCH v4 3/4] ima: define '.ima' as a builtin 'trusted' keyring

[Dmitry Kasatkin]

On 09/06/14 14:06, Mimi Zohar wrote:
> On Fri, 2014-05-30 at 19:05 +0300, Dmitry Kasatkin wrote:
>> On 28 May 2014 22:26, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>> On Wed, 2014-05-28 at 21:55 +0300, Dmitry Kasatkin wrote:
>>>> On 28 May 2014 18:09, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>>>> Require all keys added to the IMA keyring be signed by an
>>>>> existing trusted key on the system trusted keyring.
>>>>>
>>>>> Changelog v1:
>>>>> - don't link IMA trusted keyring to user keyring
>>>>>
>>>>> Changelog:
>>>>> - define stub integrity_init_keyring() function (reported-by Fengguang Wu)
>>>>> - differentiate between regular and trusted keyring names.
>>>>> - replace printk with pr_info (D. Kasatkin)
>>>>> - only make the IMA keyring a trusted keyring (reported-by D. Kastatkin)
>>>>> - define stub integrity_init_keyring() definition based on
>>>>>   CONFIG_INTEGRITY_SIGNATURE, not CONFIG_INTEGRITY_ASYMMETRIC_KEYS.
>>>>>   (reported-by Jim Davis)
>>>>>
>>>>> Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
>>>>> ---
>>>>>  security/integrity/digsig.c           | 26 +++++++++++++++++++++++++-
>>>>>  security/integrity/ima/Kconfig        |  8 ++++++++
>>>>>  security/integrity/ima/ima_appraise.c | 11 +++++++++++
>>>>>  security/integrity/integrity.h        |  5 +++++
>>>>>  4 files changed, 49 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
>>>>> index b4af4eb..7da5f9c 100644
>>>>> --- a/security/integrity/digsig.c
>>>>> +++ b/security/integrity/digsig.c
>>>>> @@ -13,7 +13,9 @@
>>>>>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>>>>
>>>>>  #include <linux/err.h>
>>>>> +#include <linux/sched.h>
>>>>>  #include <linux/rbtree.h>
>>>>> +#include <linux/cred.h>
>>>>>  #include <linux/key-type.h>
>>>>>  #include <linux/digsig.h>
>>>>>
>>>>> @@ -24,7 +26,11 @@ static struct key *keyring[INTEGRITY_KEYRING_MAX];
>>>>>  static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
>>>>>         "_evm",
>>>>>         "_module",
>>>>> +#ifndef CONFIG_IMA_TRUSTED_KEYRING
>>>>>         "_ima",
>>>>> +#else
>>>>> +       ".ima",
>>>>> +#endif
>>>>>  };
>>>>>
>>>>>  int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>>>>> @@ -35,7 +41,7 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>>>>>
>>>>>         if (!keyring[id]) {
>>>>>                 keyring[id] =
>>>>> -                       request_key(&key_type_keyring, keyring_name[id], NULL);
>>>>> +                   request_key(&key_type_keyring, keyring_name[id], NULL);
>>>>>                 if (IS_ERR(keyring[id])) {
>>>>>                         int err = PTR_ERR(keyring[id]);
>>>>>                         pr_err("no %s keyring: %d\n", keyring_name[id], err);
>>>>> @@ -56,3 +62,21 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>>>>>
>>>>>         return -EOPNOTSUPP;
>>>>>  }
>>>>> +
>>>>> +int integrity_init_keyring(const unsigned int id)
>>>>> +{
>>>>> +       const struct cred *cred = current_cred();
>>>>> +
>>>>> +       keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
>>>>> +                                   KGIDT_INIT(0), cred,
>>>>> +                                   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>>>>> +                                     KEY_USR_VIEW | KEY_USR_READ |
>>>>> +                                     KEY_USR_WRITE | KEY_USR_SEARCH),
>>>>> +                                   KEY_ALLOC_NOT_IN_QUOTA, NULL);
>>>> Last parameter "destination" is NULL. It makes keyring "unsearchable"
>>>> from user space.
>>>> It prevents loading trusted keys from user-space, e.g. initramfs...
>>>>
>>>> Should it be "cred->user->uid_keyring"??
>>> David extended keyctl with the '%keyring' option.  For example,
>>> "keyctl show %keyring:.ima" returns the .ima keyring id with a list of
>>> all the keys.
>>>
>> That is not kernel feature, but keyctl feature as I can see.
>> It will not find keyring from user space..
>>
>> keyutils.c 3.5.7 has this kind of thing
>> f = fopen("/proc/keys", "r");
>>
>> But it would require CONFIG_PROC_KEYS to be enabled.
>>
>> May be David may comment...
> David commented on an prior patch set, which defined a new id for the
> system trusted keyring. For hjs comments, refer to
> http://marc.info/?l=linux-security-module&m=137829415530503&w=2
>
> thanks,
>
> Mimi

Fine for me if such API is fine for David.

I just checked one again. They option to enable /proc/keys is called
CONFIG_KEYS_DEBUG_PROC_KEYS

It is a bit weired that in order to be able to load keys to trusted
keyring it is necessary to enable *_DEBUG_* option.

David stated: (1) Make /proc/keys always present if CONFIG_KEYS=y.

It is not there yet...

Should than CONFIG_IMA_TRUSTED_KEYRING "select
CONFIG_KEYS_DEBUG_PROC_KEYS" by David suggestion?

- Dmitry

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