[RFC PATCH v5 0/4] ima: extending secure boot certificate chain of trust

[RFC PATCH v5 0/4] ima: extending secure boot certificate chain of trust

[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.

Instead of allowing public keys, with certificates signed by any
key on the system trusted keyring, to be added to a trusted
keyring, the last patch in this patch set further restricts the
certificates to those signed by a particular key on the system keyring.
(This is a total rewrite of the previous version.)

All comments have been addressed aside from the issue of requiring
procfs for loading a key on the ima keyring.

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

 Documentation/kernel-parameters.txt      |  5 ++
 crypto/asymmetric_keys/x509_public_key.c | 86 +++++++++++++++++++++++++++++++-
 include/keys/owner_keyring.h             | 27 ++++++++++
 include/keys/system_keyring.h            | 10 +++-
 init/Kconfig                             | 10 ++++
 kernel/Makefile                          |  1 +
 kernel/owner_keyring.c                   | 85 +++++++++++++++++++++++++++++++
 security/integrity/digsig.c              | 26 ++++++++++
 security/integrity/ima/Kconfig           |  8 +++
 security/integrity/ima/ima_main.c        | 12 ++++-
 security/integrity/integrity.h           |  5 ++
 security/keys/keyctl.c                   |  6 ++-
 12 files changed, 275 insertions(+), 6 deletions(-)
 create mode 100644 include/keys/owner_keyring.h
 create mode 100644 kernel/owner_keyring.c

-- 
1.8.1.4

[RFC PATCH v5 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().

Changelog v5:
- Only prevent userspace from creating a dot prefixed keyring, not
  regular keys  - Dmitry

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..62a9952 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;
 }
@@ -86,6 +84,10 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
 		if (!*description) {
 			kfree(description);
 			description = NULL;
+		} else if ((description[0] == '.') &&  
+			   (strncmp(type, "keyring", 7) == 0)) {
+				ret = -EPERM;
+				goto error2;
 		}
 	}
 
-- 
1.8.1.4

[RFC PATCH v5 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 v5 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 v5:
- Move integrity_init_keyring() to init_ima() - Dmitry
- reset keyring[id] on failure - Dmitry

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_main.c | 12 ++++++++++--
 security/integrity/integrity.h    |  5 +++++
 4 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index b4af4eb..fa4c2fd 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,
@@ -56,3 +62,23 @@ 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]));
+		keyring[id] = NULL;
+	}
+	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_main.c b/security/integrity/ima/ima_main.c
index 09baa33..4c60cc5 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -328,8 +328,16 @@ static int __init init_ima(void)
 
 	hash_setup(CONFIG_IMA_DEFAULT_HASH);
 	error = ima_init();
-	if (!error)
-		ima_initialized = 1;
+	if (error)
+		goto out;
+
+#ifdef CONFIG_IMA_TRUSTED_KEYRING
+	error = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
+	if (error)
+		goto out;
+#endif
+	ima_initialized = 1;
+out:
 	return error;
 }
 
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 v5 4/4] KEYS: define an owner trusted keyring

[Mimi Zohar]

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 a particular key on the system keyring.

When the UEFI secure boot keys are added to the system keyring, the
platform owner will be able to load their key in one of the UEFI DBs
(eg. Machine Owner Key(MOK) list) and select their key, without
having to rebuild the kernel.

This patch defines an owner trusted keyring, a new boot command
line option 'keys_ownerid=', and defines a new function
get_system_or_owner_trusted_keyring().

Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
---
 Documentation/kernel-parameters.txt      |  5 ++
 crypto/asymmetric_keys/x509_public_key.c |  4 +-
 include/keys/owner_keyring.h             | 27 ++++++++++
 init/Kconfig                             | 10 ++++
 kernel/Makefile                          |  1 +
 kernel/owner_keyring.c                   | 85 ++++++++++++++++++++++++++++++++
 6 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 include/keys/owner_keyring.h
 create mode 100644 kernel/owner_keyring.c

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 7116fda..f90d31d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1434,6 +1434,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			use the HighMem zone if it exists, and the Normal
 			zone if it does not.
 
+	keys_ownerid=[KEYS] This parameter identifies a specific key on
+			the system trusted keyring to be added to the
+			owner trusted keyring.
+			format: id:<keyid>
+
 	kgdbdbgp=	[KGDB,HW] kgdb over EHCI usb debug port.
 			Format: <Controller#>[,poll interval]
 			The controller # is the number of the ehci usb debug
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 1af8a30..6af338f 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -19,6 +19,7 @@
 #include <keys/asymmetric-subtype.h>
 #include <keys/asymmetric-parser.h>
 #include <keys/system_keyring.h>
+#include <keys/owner_keyring.h>
 #include <crypto/hash.h>
 #include "asymmetric_keys.h"
 #include "public_key.h"
@@ -237,7 +238,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/owner_keyring.h b/include/keys/owner_keyring.h
new file mode 100644
index 0000000..78dd09d
--- /dev/null
+++ b/include/keys/owner_keyring.h
@@ -0,0 +1,27 @@
+/* 
+ * Copyright (C) 2014 IBM Corporation
+ * Author: Mimi Zohar <zohar@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#ifndef _KEYS_OWNER_KEYRING_H
+#define _KEYS_OWNER_KEYRING_H
+
+#ifdef CONFIG_OWNER_TRUSTED_KEYRING
+
+#include <linux/key.h>
+
+extern struct key *owner_trusted_keyring;
+extern struct key *get_system_or_owner_trusted_keyring(void);
+
+#else
+static inline struct key *get_system_or_owner_trusted_keyring(void)
+{
+	return get_system_trusted_keyring();
+}
+
+#endif
+#endif /* _KEYS_OWNER_KEYRING_H */
diff --git a/init/Kconfig b/init/Kconfig
index 009a797..7876787 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1661,6 +1661,16 @@ config SYSTEM_TRUSTED_KEYRING
 
 	  Keys in this keyring are used by module signature checking.
 
+config OWNER_TRUSTED_KEYRING
+	bool "Verify certificate signatures using a specific system key"
+	depends on SYSTEM_TRUSTED_KEYRING
+	help
+	  Verify a certificate's signature, before adding the key to
+	  a trusted keyring, using a specific key on the system trusted
+	  keyring.  The specific key on the system trusted keyring is
+	  identified using the kernel boot command line option
+	  "keys_ownerid" and is added to the owner_trusted_keyring.
+
 menuconfig MODULES
 	bool "Enable loadable module support"
 	option modules
diff --git a/kernel/Makefile b/kernel/Makefile
index bc010ee..7b44efd 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_UID16) += uid16.o
 obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
 obj-$(CONFIG_MODULES) += module.o
 obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_OWNER_TRUSTED_KEYRING) += owner_keyring.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
 obj-$(CONFIG_KEXEC) += kexec.o
diff --git a/kernel/owner_keyring.c b/kernel/owner_keyring.c
new file mode 100644
index 0000000..a31b865
--- /dev/null
+++ b/kernel/owner_keyring.c
@@ -0,0 +1,85 @@
+/* 
+ * Copyright (C) 2014 IBM Corporation
+ * Author: Mimi Zohar <zohar@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/cred.h>
+#include <linux/err.h>
+#include <keys/asymmetric-type.h>
+#include <keys/system_keyring.h>
+#include "module-internal.h"
+
+struct key *owner_trusted_keyring;
+static int use_owner_trusted_keyring;
+
+static char *owner_keyid;
+static int __init default_owner_keyid_set(char *str)
+{
+	if (!str)		/* default system keyring */
+		return 1;
+
+	if (strncmp(str, "id:", 3) == 0)
+		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
+
+	return 1;
+}
+
+__setup("keys_ownerid=", default_owner_keyid_set);
+
+struct key *get_system_or_owner_trusted_keyring(void)
+{
+	return use_owner_trusted_keyring ? owner_trusted_keyring :
+	    get_system_trusted_keyring();
+}
+
+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");
+
+	set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
+	return 0;
+}
+
+device_initcall(owner_trusted_keyring_init);
+
+void load_owner_identified_key(void)
+{
+	key_ref_t key_ref;
+	int ret;
+
+	if (!owner_keyid)
+		return;
+
+	key_ref = keyring_search(make_key_ref(system_trusted_keyring, 1),
+				 &key_type_asymmetric, owner_keyid);
+	if (IS_ERR(key_ref)) {
+		pr_warn("Request for unknown %s key\n", owner_keyid);
+		goto out;
+	}
+	ret = key_link(owner_trusted_keyring, key_ref_to_ptr(key_ref));
+	pr_info("Loaded owner key %s %s\n", owner_keyid,
+		ret < 0 ? "failed" : "succeeded");
+	key_ref_put(key_ref);
+	if (!ret)
+		use_owner_trusted_keyring = 1;
+out:
+	return;
+}
+
+late_initcall(load_owner_identified_key);
-- 
1.8.1.4

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

[Dmitry Kasatkin]

On 3 June 2014 20:58, 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().
>
> Changelog v5:
> - Only prevent userspace from creating a dot prefixed keyring, not
>   regular keys  - Dmitry
>
> 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..62a9952 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;
>  }
> @@ -86,6 +84,10 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
>                 if (!*description) {
>                         kfree(description);
>                         description = NULL;
> +               } else if ((description[0] == '.') &&
> +                          (strncmp(type, "keyring", 7) == 0)) {
> +                               ret = -EPERM;
> +                               goto error2;
>                 }
>         }
>
> --
> 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

I think it does not another problem...
It is not only prevent creating new keyring with ".abc" name but also
prevent adding new key...

this is wrong...

- Dmitry

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

[Dmitry Kasatkin]

On 3 June 2014 20:58, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 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>

Acked-by: me


> ---
>  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
>
> --
> 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 v5 3/4] ima: define '.ima' as a builtin 'trusted' keyring

[Dmitry Kasatkin]

On 3 June 2014 20:58, 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 v5:
> - Move integrity_init_keyring() to init_ima() - Dmitry
> - reset keyring[id] on failure - Dmitry
>
> 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_main.c | 12 ++++++++++--
>  security/integrity/integrity.h    |  5 +++++
>  4 files changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index b4af4eb..fa4c2fd 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,
> @@ -56,3 +62,23 @@ 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]));
> +               keyring[id] = NULL;
> +       }
> +       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_main.c b/security/integrity/ima/ima_main.c
> index 09baa33..4c60cc5 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -328,8 +328,16 @@ static int __init init_ima(void)
>
>         hash_setup(CONFIG_IMA_DEFAULT_HASH);
>         error = ima_init();
> -       if (!error)
> -               ima_initialized = 1;
> +       if (error)
> +               goto out;
> +
> +#ifdef CONFIG_IMA_TRUSTED_KEYRING
> +       error = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
> +       if (error)
> +               goto out;
> +#endif

integrity_init_keyring() has variation in header file...
Why do you need #ifdef in .c file? You you usually do not like it...

- Dmitry

> +       ima_initialized = 1;
> +out:
>         return error;
>  }
>
> 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 v5 1/4] KEYS: special dot prefixed keyring name bug fix

[Mimi Zohar]

On Sat, 2014-06-07 at 00:48 +0300, Dmitry Kasatkin wrote: 
> On 3 June 2014 20:58, 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().
> >
> > Changelog v5:
> > - Only prevent userspace from creating a dot prefixed keyring, not
> >   regular keys  - Dmitry
> >
> > 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..62a9952 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;
> >  }
> > @@ -86,6 +84,10 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
> >                 if (!*description) {
> >                         kfree(description);
> >                         description = NULL;
> > +               } else if ((description[0] == '.') &&
> > +                          (strncmp(type, "keyring", 7) == 0)) {
> > +                               ret = -EPERM;
> > +                               goto error2;
> >                 }
> >         }

> I think it does not another problem...
> It is not only prevent creating new keyring with ".abc" name but also
> prevent adding new key...
> 
> this is wrong...

Seems to prevent creating a dot prefixed keyring, but permits creating a
dot prefixed key.  Do you have an example?

thanks,

Mimi

Mimi

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

[Mimi Zohar]

On Sat, 2014-06-07 at 00:53 +0300, Dmitry Kasatkin wrote: 
> On 3 June 2014 20:58, 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 v5:
> > - Move integrity_init_keyring() to init_ima() - Dmitry
> > - reset keyring[id] on failure - Dmitry
> >
> > 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_main.c | 12 ++++++++++--
> >  security/integrity/integrity.h    |  5 +++++
> >  4 files changed, 49 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > index b4af4eb..fa4c2fd 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,
> > @@ -56,3 +62,23 @@ 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]));
> > +               keyring[id] = NULL;
> > +       }
> > +       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_main.c b/security/integrity/ima/ima_main.c
> > index 09baa33..4c60cc5 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -328,8 +328,16 @@ static int __init init_ima(void)
> >
> >         hash_setup(CONFIG_IMA_DEFAULT_HASH);
> >         error = ima_init();
> > -       if (!error)
> > -               ima_initialized = 1;
> > +       if (error)
> > +               goto out;
> > +
> > +#ifdef CONFIG_IMA_TRUSTED_KEYRING
> > +       error = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
> > +       if (error)
> > +               goto out;
> > +#endif
> 
> integrity_init_keyring() has variation in header file...
> Why do you need #ifdef in .c file? You you usually do not like it...

Up to now, unsigned certificates could be added to the IMA keyring.  We
can not all of a sudden require all keys being added to the IMA keyring
to be signed.  Although there is a stub integrity_init_keyring()
function definition, it is based on CONFIG_INTEGRITY_SIGNATURE, not
CONFIG_IMA_TRUSTED_KEYRING.

Right, ifdef's don't belong in C code.  One solution would be to define
ima_init_keyring() as a wrapper for integrity_init_keyring() and a stub
function.

Mimi

> 
> > +       ima_initialized = 1;
> > +out:
> >         return error;
> >  }
> >
> > 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
> 
> 
> 

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

[Dmitry Kasatkin]

On 07/06/14 01:00, Mimi Zohar wrote:
> On Sat, 2014-06-07 at 00:48 +0300, Dmitry Kasatkin wrote: 
>> On 3 June 2014 20:58, 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().
>>>
>>> Changelog v5:
>>> - Only prevent userspace from creating a dot prefixed keyring, not
>>>   regular keys  - Dmitry
>>>
>>> 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..62a9952 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;
>>>  }
>>> @@ -86,6 +84,10 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
>>>                 if (!*description) {
>>>                         kfree(description);
>>>                         description = NULL;
>>> +               } else if ((description[0] == '.') &&
>>> +                          (strncmp(type, "keyring", 7) == 0)) {
>>> +                               ret = -EPERM;
>>> +                               goto error2;
>>>                 }
>>>         }
>> I think it does not another problem...
>> It is not only prevent creating new keyring with ".abc" name but also
>> prevent adding new key...
>>
>> this is wrong...
> Seems to prevent creating a dot prefixed keyring, but permits creating a
> dot prefixed key.  Do you have an example?

I think by mistake I was checking old patch in the thread because I have
not noticed strncmp(type, "keyring", 7).
This patch definitely should do the job.

- Dmitry


> thanks,
>
> Mimi
>
> Mimi
>
>
>

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

[Dmitry Kasatkin]

On 09/06/14 10:56, Dmitry Kasatkin wrote:
> On 07/06/14 01:00, Mimi Zohar wrote:
>> On Sat, 2014-06-07 at 00:48 +0300, Dmitry Kasatkin wrote: 
>>> On 3 June 2014 20:58, 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().
>>>>
>>>> Changelog v5:
>>>> - Only prevent userspace from creating a dot prefixed keyring, not
>>>>   regular keys  - Dmitry
>>>>
>>>> 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..62a9952 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;
>>>>  }
>>>> @@ -86,6 +84,10 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
>>>>                 if (!*description) {
>>>>                         kfree(description);
>>>>                         description = NULL;
>>>> +               } else if ((description[0] == '.') &&
>>>> +                          (strncmp(type, "keyring", 7) == 0)) {

BTW. using strcmp is good enough here. string constants are always NULL
terminated.
Comparison will terminate after no more than 7 bytes.


>>>> +                               ret = -EPERM;
>>>> +                               goto error2;
>>>>                 }
>>>>         }
>>> I think it does not another problem...
>>> It is not only prevent creating new keyring with ".abc" name but also
>>> prevent adding new key...
>>>
>>> this is wrong...
>> Seems to prevent creating a dot prefixed keyring, but permits creating a
>> dot prefixed key.  Do you have an example?
> I think by mistake I was checking old patch in the thread because I have
> not noticed strncmp(type, "keyring", 7).
> This patch definitely should do the job.
>
> - Dmitry
>
>
>> thanks,
>>
>> Mimi
>>
>> Mimi
>>
>>
>>

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

[Dmitry Kasatkin]

On 07/06/14 02:27, Mimi Zohar wrote:
> On Sat, 2014-06-07 at 00:53 +0300, Dmitry Kasatkin wrote: 
>> On 3 June 2014 20:58, 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 v5:
>>> - Move integrity_init_keyring() to init_ima() - Dmitry
>>> - reset keyring[id] on failure - Dmitry
>>>
>>> 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_main.c | 12 ++++++++++--
>>>  security/integrity/integrity.h    |  5 +++++
>>>  4 files changed, 49 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
>>> index b4af4eb..fa4c2fd 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,
>>> @@ -56,3 +62,23 @@ 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]));
>>> +               keyring[id] = NULL;
>>> +       }
>>> +       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_main.c b/security/integrity/ima/ima_main.c
>>> index 09baa33..4c60cc5 100644
>>> --- a/security/integrity/ima/ima_main.c
>>> +++ b/security/integrity/ima/ima_main.c
>>> @@ -328,8 +328,16 @@ static int __init init_ima(void)
>>>
>>>         hash_setup(CONFIG_IMA_DEFAULT_HASH);
>>>         error = ima_init();
>>> -       if (!error)
>>> -               ima_initialized = 1;
>>> +       if (error)
>>> +               goto out;
>>> +
>>> +#ifdef CONFIG_IMA_TRUSTED_KEYRING
>>> +       error = integrity_init_keyring(INTEGRITY_KEYRING_IMA);
>>> +       if (error)
>>> +               goto out;
>>> +#endif
>> integrity_init_keyring() has variation in header file...
>> Why do you need #ifdef in .c file? You you usually do not like it...
> Up to now, unsigned certificates could be added to the IMA keyring.  We
> can not all of a sudden require all keys being added to the IMA keyring
> to be signed.  Although there is a stub integrity_init_keyring()
> function definition, it is based on CONFIG_INTEGRITY_SIGNATURE, not
> CONFIG_IMA_TRUSTED_KEYRING.
>
> Right, ifdef's don't belong in C code.  One solution would be to define
> ima_init_keyring() as a wrapper for integrity_init_keyring() and a stub
> function.
>
> Mimi

I think 'ima_init_keyring" could be a good approach.

>From other hand, if you look to kernel/module.c, then you will find ~40
#ifdefs of the source code...

So it may be indeed clear from __init point of view what we initialize
and what not...

- Dmitry


>>> +       ima_initialized = 1;
>>> +out:
>>>         return error;
>>>  }
>>>
>>> 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
>>
>>
>
> --
> 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
>

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

[Dmitry Kasatkin]

On 03/06/14 20:58, Mimi Zohar wrote:
> 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 a particular key on the system keyring.
>
> When the UEFI secure boot keys are added to the system keyring, the
> platform owner will be able to load their key in one of the UEFI DBs
> (eg. Machine Owner Key(MOK) list) and select their key, without
> having to rebuild the kernel.
>
> This patch defines an owner trusted keyring, a new boot command
> line option 'keys_ownerid=', and defines a new function
> get_system_or_owner_trusted_keyring().

Hello,

The functionality of this entire patch can be replaced by only ~2 lines
of code in x509_request_asymmetric_key()

if (keys_ownerid || strcmp(keys_ownerid, id))
     return -EPERM;

Right?

- Dmitry


> Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> ---
>  Documentation/kernel-parameters.txt      |  5 ++
>  crypto/asymmetric_keys/x509_public_key.c |  4 +-
>  include/keys/owner_keyring.h             | 27 ++++++++++
>  init/Kconfig                             | 10 ++++
>  kernel/Makefile                          |  1 +
>  kernel/owner_keyring.c                   | 85 ++++++++++++++++++++++++++++++++
>  6 files changed, 131 insertions(+), 1 deletion(-)
>  create mode 100644 include/keys/owner_keyring.h
>  create mode 100644 kernel/owner_keyring.c
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 7116fda..f90d31d 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1434,6 +1434,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			use the HighMem zone if it exists, and the Normal
>  			zone if it does not.
>  
> +	keys_ownerid=[KEYS] This parameter identifies a specific key on
> +			the system trusted keyring to be added to the
> +			owner trusted keyring.
> +			format: id:<keyid>
> +
>  	kgdbdbgp=	[KGDB,HW] kgdb over EHCI usb debug port.
>  			Format: <Controller#>[,poll interval]
>  			The controller # is the number of the ehci usb debug
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index 1af8a30..6af338f 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -19,6 +19,7 @@
>  #include <keys/asymmetric-subtype.h>
>  #include <keys/asymmetric-parser.h>
>  #include <keys/system_keyring.h>
> +#include <keys/owner_keyring.h>
>  #include <crypto/hash.h>
>  #include "asymmetric_keys.h"
>  #include "public_key.h"
> @@ -237,7 +238,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/owner_keyring.h b/include/keys/owner_keyring.h
> new file mode 100644
> index 0000000..78dd09d
> --- /dev/null
> +++ b/include/keys/owner_keyring.h
> @@ -0,0 +1,27 @@
> +/* 
> + * Copyright (C) 2014 IBM Corporation
> + * Author: Mimi Zohar <zohar@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + */
> +
> +#ifndef _KEYS_OWNER_KEYRING_H
> +#define _KEYS_OWNER_KEYRING_H
> +
> +#ifdef CONFIG_OWNER_TRUSTED_KEYRING
> +
> +#include <linux/key.h>
> +
> +extern struct key *owner_trusted_keyring;
> +extern struct key *get_system_or_owner_trusted_keyring(void);
> +
> +#else
> +static inline struct key *get_system_or_owner_trusted_keyring(void)
> +{
> +	return get_system_trusted_keyring();
> +}
> +
> +#endif
> +#endif /* _KEYS_OWNER_KEYRING_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index 009a797..7876787 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1661,6 +1661,16 @@ config SYSTEM_TRUSTED_KEYRING
>  
>  	  Keys in this keyring are used by module signature checking.
>  
> +config OWNER_TRUSTED_KEYRING
> +	bool "Verify certificate signatures using a specific system key"
> +	depends on SYSTEM_TRUSTED_KEYRING
> +	help
> +	  Verify a certificate's signature, before adding the key to
> +	  a trusted keyring, using a specific key on the system trusted
> +	  keyring.  The specific key on the system trusted keyring is
> +	  identified using the kernel boot command line option
> +	  "keys_ownerid" and is added to the owner_trusted_keyring.
> +
>  menuconfig MODULES
>  	bool "Enable loadable module support"
>  	option modules
> diff --git a/kernel/Makefile b/kernel/Makefile
> index bc010ee..7b44efd 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_UID16) += uid16.o
>  obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
>  obj-$(CONFIG_MODULES) += module.o
>  obj-$(CONFIG_MODULE_SIG) += module_signing.o
> +obj-$(CONFIG_OWNER_TRUSTED_KEYRING) += owner_keyring.o
>  obj-$(CONFIG_KALLSYMS) += kallsyms.o
>  obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
>  obj-$(CONFIG_KEXEC) += kexec.o
> diff --git a/kernel/owner_keyring.c b/kernel/owner_keyring.c
> new file mode 100644
> index 0000000..a31b865
> --- /dev/null
> +++ b/kernel/owner_keyring.c
> @@ -0,0 +1,85 @@
> +/* 
> + * Copyright (C) 2014 IBM Corporation
> + * Author: Mimi Zohar <zohar@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + */
> +
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/cred.h>
> +#include <linux/err.h>
> +#include <keys/asymmetric-type.h>
> +#include <keys/system_keyring.h>
> +#include "module-internal.h"
> +
> +struct key *owner_trusted_keyring;
> +static int use_owner_trusted_keyring;
> +
> +static char *owner_keyid;
> +static int __init default_owner_keyid_set(char *str)
> +{
> +	if (!str)		/* default system keyring */
> +		return 1;
> +
> +	if (strncmp(str, "id:", 3) == 0)
> +		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
> +
> +	return 1;
> +}
> +
> +__setup("keys_ownerid=", default_owner_keyid_set);
> +
> +struct key *get_system_or_owner_trusted_keyring(void)
> +{
> +	return use_owner_trusted_keyring ? owner_trusted_keyring :
> +	    get_system_trusted_keyring();
> +}
> +
> +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");
> +
> +	set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
> +	return 0;
> +}
> +
> +device_initcall(owner_trusted_keyring_init);
> +
> +void load_owner_identified_key(void)
> +{
> +	key_ref_t key_ref;
> +	int ret;
> +
> +	if (!owner_keyid)
> +		return;
> +
> +	key_ref = keyring_search(make_key_ref(system_trusted_keyring, 1),
> +				 &key_type_asymmetric, owner_keyid);
> +	if (IS_ERR(key_ref)) {
> +		pr_warn("Request for unknown %s key\n", owner_keyid);
> +		goto out;
> +	}
> +	ret = key_link(owner_trusted_keyring, key_ref_to_ptr(key_ref));
> +	pr_info("Loaded owner key %s %s\n", owner_keyid,
> +		ret < 0 ? "failed" : "succeeded");
> +	key_ref_put(key_ref);
> +	if (!ret)
> +		use_owner_trusted_keyring = 1;
> +out:
> +	return;
> +}
> +
> +late_initcall(load_owner_identified_key);

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

[Mimi Zohar]

On Mon, 2014-06-09 at 15:13 +0300, Dmitry Kasatkin wrote: 
> On 03/06/14 20:58, Mimi Zohar wrote:
> > 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 a particular key on the system keyring.
> >
> > When the UEFI secure boot keys are added to the system keyring, the
> > platform owner will be able to load their key in one of the UEFI DBs
> > (eg. Machine Owner Key(MOK) list) and select their key, without
> > having to rebuild the kernel.
> >
> > This patch defines an owner trusted keyring, a new boot command
> > line option 'keys_ownerid=', and defines a new function
> > get_system_or_owner_trusted_keyring().
> 
> Hello,
> 
> The functionality of this entire patch can be replaced by only ~2 lines
> of code in x509_request_asymmetric_key()
> 
> if (keys_ownerid || strcmp(keys_ownerid, id))
>      return -EPERM;
> 
> Right?

Are you suggesting only add the one matching key to the system keyring?

The original patch compared the builtin key being loaded onto the system
keyring and, if it matched the requested key, also added the key to the
owner keyring.  This version waits for all the builtin keys to be loaded
onto the system keyring, and in the future the UEFI DB keys, before
adding the matched key to the owner keyring.  In this version, the keys
are already on the system keyring.  So no, your two lines would not
work.

Mimi

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

[Dmitry Kasatkin]

On 09/06/14 15:51, Mimi Zohar wrote:
> On Mon, 2014-06-09 at 15:13 +0300, Dmitry Kasatkin wrote: 
>> On 03/06/14 20:58, Mimi Zohar wrote:
>>> 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 a particular key on the system keyring.
>>>
>>> When the UEFI secure boot keys are added to the system keyring, the
>>> platform owner will be able to load their key in one of the UEFI DBs
>>> (eg. Machine Owner Key(MOK) list) and select their key, without
>>> having to rebuild the kernel.
>>>
>>> This patch defines an owner trusted keyring, a new boot command
>>> line option 'keys_ownerid=', and defines a new function
>>> get_system_or_owner_trusted_keyring().
>> Hello,
>>
>> The functionality of this entire patch can be replaced by only ~2 lines
>> of code in x509_request_asymmetric_key()
>>
>> if (keys_ownerid || strcmp(keys_ownerid, id))
>>      return -EPERM;
>>
>> Right?
> Are you suggesting only add the one matching key to the system keyring?

No. I am not suggesting this.

All built in keys are allocated with KEY_ALLOC_TRUSTED flag and
prep.trusted is set to "true".

So  the following statement has no effect.

#ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
        ret = x509_validate_trust(cert, system_trusted_keyring);
        if (!ret)
            prep->trusted = 1;
#endif

Keys which come from user-space will check for

if (keys_ownerid && strcmp(keys_ownerid, id))
     return -EPERM;


So 2 lines patch works fine..

- Dmitry

> The original patch compared the builtin key being loaded onto the system
> keyring and, if it matched the requested key, also added the key to the
> owner keyring.  This version waits for all the builtin keys to be loaded
> onto the system keyring, and in the future the UEFI DB keys, before
> adding the matched key to the owner keyring.  In this version, the keys
> are already on the system keyring.  So no, your two lines would not
> work.
>
> Mimi
>



>
>
>
> --
> 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
>

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

[Dmitry Kasatkin]

On 07/06/14 00:50, Dmitry Kasatkin wrote:
> On 3 June 2014 20:58, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> 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>
> Acked-by: me
>
>
>> ---
>>  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;

Actually this can be like this

>> +       } else if (!prep->trusted)
>> +               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
>>
>> --
>> 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
>
>

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

[Mimi Zohar]

On Mon, 2014-06-09 at 16:05 +0300, Dmitry Kasatkin wrote: 
> On 09/06/14 15:51, Mimi Zohar wrote:
> > On Mon, 2014-06-09 at 15:13 +0300, Dmitry Kasatkin wrote: 
> >> On 03/06/14 20:58, Mimi Zohar wrote:
> >>> 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 a particular key on the system keyring.
> >>>
> >>> When the UEFI secure boot keys are added to the system keyring, the
> >>> platform owner will be able to load their key in one of the UEFI DBs
> >>> (eg. Machine Owner Key(MOK) list) and select their key, without
> >>> having to rebuild the kernel.
> >>>
> >>> This patch defines an owner trusted keyring, a new boot command
> >>> line option 'keys_ownerid=', and defines a new function
> >>> get_system_or_owner_trusted_keyring().
> >> Hello,
> >>
> >> The functionality of this entire patch can be replaced by only ~2 lines
> >> of code in x509_request_asymmetric_key()
> >>
> >> if (keys_ownerid || strcmp(keys_ownerid, id))
> >>      return -EPERM;
> >>
> >> Right?
> > Are you suggesting only add the one matching key to the system keyring?
> 
> No. I am not suggesting this.
> 
> All built in keys are allocated with KEY_ALLOC_TRUSTED flag and
> prep.trusted is set to "true".
> 
> So  the following statement has no effect.

Ok, so it has no affect on adding builtin keys to the system keyring.

> 
> #ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
>         ret = x509_validate_trust(cert, system_trusted_keyring);
>         if (!ret)
>             prep->trusted = 1;
> #endif

The last patch set changes the test to: 
     ret = x509_validate_trust(cert, get_system_or_owner_trusted_keyring());

> Keys which come from user-space will check for
> 
> if (keys_ownerid && strcmp(keys_ownerid, id))
>      return -EPERM;
> 
> 
> So 2 lines patch works fine..

It works based on the assumption, that you would ever only want a single
key on the 'owner' keyring, which is probably not the case.

Mimi

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

[Mimi Zohar]

On Mon, 2014-06-09 at 16:13 +0300, Dmitry Kasatkin wrote: 
> On 07/06/14 00:50, Dmitry Kasatkin wrote:
> > On 3 June 2014 20:58, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> 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>
> > Acked-by: me
> >
> >
> >> ---
> >>  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;
> 
> Actually this can be like this
> 
> >> +       } else if (!prep->trusted)
> >> +               ret = x509_validate_trust(cert,
> get_system_trusted_keyring());
> >> +               if (!ret)
> >> +                       prep->trusted = 1;
> 
> >>         }

Fine

Mimi

> >>         /* 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
> >>
> >> --
> >> 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
> >
> >
> 
> --
> 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
> 

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

[Dmitry Kasatkin]

On 9 June 2014 16:48, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Mon, 2014-06-09 at 16:05 +0300, Dmitry Kasatkin wrote:
>> On 09/06/14 15:51, Mimi Zohar wrote:
>> > On Mon, 2014-06-09 at 15:13 +0300, Dmitry Kasatkin wrote:
>> >> On 03/06/14 20:58, Mimi Zohar wrote:
>> >>> 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 a particular key on the system keyring.
>> >>>
>> >>> When the UEFI secure boot keys are added to the system keyring, the
>> >>> platform owner will be able to load their key in one of the UEFI DBs
>> >>> (eg. Machine Owner Key(MOK) list) and select their key, without
>> >>> having to rebuild the kernel.
>> >>>
>> >>> This patch defines an owner trusted keyring, a new boot command
>> >>> line option 'keys_ownerid=', and defines a new function
>> >>> get_system_or_owner_trusted_keyring().
>> >> Hello,
>> >>
>> >> The functionality of this entire patch can be replaced by only ~2 lines
>> >> of code in x509_request_asymmetric_key()
>> >>
>> >> if (keys_ownerid || strcmp(keys_ownerid, id))
>> >>      return -EPERM;
>> >>
>> >> Right?
>> > Are you suggesting only add the one matching key to the system keyring?
>>
>> No. I am not suggesting this.
>>
>> All built in keys are allocated with KEY_ALLOC_TRUSTED flag and
>> prep.trusted is set to "true".
>>
>> So  the following statement has no effect.
>
> Ok, so it has no affect on adding builtin keys to the system keyring.
>
>>
>> #ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
>>         ret = x509_validate_trust(cert, system_trusted_keyring);
>>         if (!ret)
>>             prep->trusted = 1;
>> #endif
>
> The last patch set changes the test to:
>      ret = x509_validate_trust(cert, get_system_or_owner_trusted_keyring());
>

It does not really mater. I just copied original code to my response.

>> Keys which come from user-space will check for
>>
>> if (keys_ownerid && strcmp(keys_ownerid, id))
>>      return -EPERM;
>>
>>
>> So 2 lines patch works fine..
>
> It works based on the assumption, that you would ever only want a single
> key on the 'owner' keyring, which is probably not the case.
>

There is no any assumption here. I am discussing functionality of this patch.
That is exactly what this patch does - loads single key on the owners keyring.

There is no need for additional keyring for a single key. That is just
enough to limit verification to the owners key id.

- Dmitry


> Mimi
>
> --
> 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 v5 4/4] KEYS: define an owner trusted keyring

[Dmitry Kasatkin]

On 9 June 2014 16:58, Dmitry Kasatkin <dmitry.kasatkin@gmail.com> wrote:
> On 9 June 2014 16:48, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> On Mon, 2014-06-09 at 16:05 +0300, Dmitry Kasatkin wrote:
>>> On 09/06/14 15:51, Mimi Zohar wrote:
>>> > On Mon, 2014-06-09 at 15:13 +0300, Dmitry Kasatkin wrote:
>>> >> On 03/06/14 20:58, Mimi Zohar wrote:
>>> >>> 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 a particular key on the system keyring.
>>> >>>
>>> >>> When the UEFI secure boot keys are added to the system keyring, the
>>> >>> platform owner will be able to load their key in one of the UEFI DBs
>>> >>> (eg. Machine Owner Key(MOK) list) and select their key, without
>>> >>> having to rebuild the kernel.
>>> >>>
>>> >>> This patch defines an owner trusted keyring, a new boot command
>>> >>> line option 'keys_ownerid=', and defines a new function
>>> >>> get_system_or_owner_trusted_keyring().
>>> >> Hello,
>>> >>
>>> >> The functionality of this entire patch can be replaced by only ~2 lines
>>> >> of code in x509_request_asymmetric_key()
>>> >>
>>> >> if (keys_ownerid || strcmp(keys_ownerid, id))
>>> >>      return -EPERM;
>>> >>
>>> >> Right?
>>> > Are you suggesting only add the one matching key to the system keyring?
>>>
>>> No. I am not suggesting this.
>>>
>>> All built in keys are allocated with KEY_ALLOC_TRUSTED flag and
>>> prep.trusted is set to "true".
>>>
>>> So  the following statement has no effect.
>>
>> Ok, so it has no affect on adding builtin keys to the system keyring.
>>
>>>
>>> #ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
>>>         ret = x509_validate_trust(cert, system_trusted_keyring);
>>>         if (!ret)
>>>             prep->trusted = 1;
>>> #endif
>>
>> The last patch set changes the test to:
>>      ret = x509_validate_trust(cert, get_system_or_owner_trusted_keyring());
>>
>
> It does not really mater. I just copied original code to my response.
>
>>> Keys which come from user-space will check for
>>>
>>> if (keys_ownerid && strcmp(keys_ownerid, id))
>>>      return -EPERM;
>>>
>>>
>>> So 2 lines patch works fine..
>>
>> It works based on the assumption, that you would ever only want a single
>> key on the 'owner' keyring, which is probably not the case.
>>
>
> There is no any assumption here. I am discussing functionality of this patch.
> That is exactly what this patch does - loads single key on the owners keyring.
>
> There is no need for additional keyring for a single key. That is just
> enough to limit verification to the owners key id.
>
> - Dmitry
>
>

There is no reason to have advanced bloated implementation for unsure,
may be never coming use-cases.

It is always very easy to make new patches for the future cases.

- Dmitry

>> Mimi
>>

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

[Dmitry Kasatkin]

On 09/06/14 16:48, Mimi Zohar wrote:
> On Mon, 2014-06-09 at 16:13 +0300, Dmitry Kasatkin wrote: 
>> On 07/06/14 00:50, Dmitry Kasatkin wrote:
>>> On 3 June 2014 20:58, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>>> 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>
>>> Acked-by: me
>>>
>>>
>>>> ---
>>>>  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);

I suspect that key_put(key) is missing here...



>>>> +       }
>>>> +       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;
>> Actually this can be like this
>>
>>>> +       } else if (!prep->trusted)
>>>> +               ret = x509_validate_trust(cert,
>> get_system_trusted_keyring());
>>>> +               if (!ret)
>>>> +                       prep->trusted = 1;
>>>>         }
> Fine
>
> Mimi
>
>>>>         /* 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
>>>>
>>>> --
>>>> 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
>>>
>> --
>> 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
>>
>
> --
> 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
>

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

[Mimi Zohar]

On Mon, 2014-06-09 at 17:06 +0300, Dmitry Kasatkin wrote:
> On 9 June 2014 16:58, Dmitry Kasatkin <dmitry.kasatkin@gmail.com> wrote:
> > On 9 June 2014 16:48, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >> On Mon, 2014-06-09 at 16:05 +0300, Dmitry Kasatkin wrote:
> >>> On 09/06/14 15:51, Mimi Zohar wrote:
> >>> > On Mon, 2014-06-09 at 15:13 +0300, Dmitry Kasatkin wrote:
> >>> >> On 03/06/14 20:58, Mimi Zohar wrote:
> >>> >>> 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 a particular key on the system keyring.
> >>> >>>
> >>> >>> When the UEFI secure boot keys are added to the system keyring, the
> >>> >>> platform owner will be able to load their key in one of the UEFI DBs
> >>> >>> (eg. Machine Owner Key(MOK) list) and select their key, without
> >>> >>> having to rebuild the kernel.
> >>> >>>
> >>> >>> This patch defines an owner trusted keyring, a new boot command
> >>> >>> line option 'keys_ownerid=', and defines a new function
> >>> >>> get_system_or_owner_trusted_keyring().
> >>> >> Hello,
> >>> >>
> >>> >> The functionality of this entire patch can be replaced by only ~2 lines
> >>> >> of code in x509_request_asymmetric_key()
> >>> >>
> >>> >> if (keys_ownerid || strcmp(keys_ownerid, id))
> >>> >>      return -EPERM;
> >>> >>
> >>> >> Right?
> >>> > Are you suggesting only add the one matching key to the system keyring?
> >>>
> >>> No. I am not suggesting this.
> >>>
> >>> All built in keys are allocated with KEY_ALLOC_TRUSTED flag and
> >>> prep.trusted is set to "true".
> >>>
> >>> So  the following statement has no effect.
> >>
> >> Ok, so it has no affect on adding builtin keys to the system keyring.
> >>
> >>>
> >>> #ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
> >>>         ret = x509_validate_trust(cert, system_trusted_keyring);
> >>>         if (!ret)
> >>>             prep->trusted = 1;
> >>> #endif
> >>
> >> The last patch set changes the test to:
> >>      ret = x509_validate_trust(cert, get_system_or_owner_trusted_keyring());
> >>
> >
> > It does not really mater. I just copied original code to my response.
> >
> >>> Keys which come from user-space will check for
> >>>
> >>> if (keys_ownerid && strcmp(keys_ownerid, id))
> >>>      return -EPERM;
> >>>
> >>>
> >>> So 2 lines patch works fine..
> >>
> >> It works based on the assumption, that you would ever only want a single
> >> key on the 'owner' keyring, which is probably not the case.
> >>
> >
> > There is no any assumption here. I am discussing functionality of this patch.
> > That is exactly what this patch does - loads single key on the owners keyring.
> >
> > There is no need for additional keyring for a single key. That is just
> > enough to limit verification to the owners key id.
> >
> 
> There is no reason to have advanced bloated implementation for unsure,
> may be never coming use-cases.
> 
> It is always very easy to make new patches for the future cases.

Normally I would agree with you, but in this case we already have
multiple use case scenarios (eg. builtin keys, transitioning from one
MOK key to another), but are simply waiting for the proposed UEFI key
patches to be upstreamed.  Basically, we need the UEFI key patches
functionality, if not these specific patches, to safely identify the
owner's key(s), without requiring the end user to rebuild the kernel to
include their key(s).

Mimi

[PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

Hi Mimi,

As you asked ofline , here is possible equivalent and simpler alternative
patches not requiring to have additional keyring.

First patch are irrelevant minor fixes.

Also I want to discuss here Fedora UEFI patches as they are the reason for
the these original patchset.

http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch

They provide functionality to specify MokIgnoreDb variable to limit loading of
UEFI keys only from MOK List, while ignoring DB. This is certainly a good
functionality. But once MODULE_SIG_UEFI is enabled, it looks there is no way
to prevent loading keys from UEFI at all. And this might not be a good default
functionality. Someone might want not allow loading of keys from UEFI unless
kernel parameter is specified to allow it without recompiling the kernel
and disabling MODULE_SIG_UEFI.

Josh, why such design decision was made?

Why not to provide kernel parameter to have more fine-tune control over the
functionality? Unconfigured machines will not have MokIgnoreDb and will
allow to load kernel modules signed with certain undesired keys. In fact,
I beleive, it should be default behavior of the kernel. Bootloader can
enable UEFI functionality by specifing it on the kernel command line.

Second patch allows to overcome keys coming from UEFI for key validation by
specifing owner key id and is an alternative for v5 4/4 patch.

It was also a good idea presented in Mimi's v4 4/4 patch to have possibility
to limit key trust valiation by only builtin keys. Third patch as an alternative.
It uses keys->flags to specify origin of the key, but any additional field could
be added as well.

Both key id and origin verification is done in x509_validate_trust().

Thanks,
Dmitry

Dmitry Kasatkin (3):
  KEYS: fix couple of things
  KEYS: validate key trust only with selected owner key
  KEYS: validate key trust only with builtin keys

Mimi Zohar (1):
  KEYS: define an owner trusted keyring

 Documentation/kernel-parameters.txt      |  5 +++++
 crypto/asymmetric_keys/x509_public_key.c | 35 +++++++++++++++++++++++++++++---
 include/linux/key.h                      |  1 +
 kernel/system_keyring.c                  |  1 +
 4 files changed, 39 insertions(+), 3 deletions(-)

-- 
1.9.1

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

[Dmitry Kasatkin]

From: Mimi Zohar <zohar@linux.vnet.ibm.com>

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 a particular key on the system keyring.

When the UEFI secure boot keys are added to the system keyring, the
platform owner will be able to load their key in one of the UEFI DBs
(eg. Machine Owner Key(MOK) list) and select their key, without
having to rebuild the kernel.

This patch defines an owner trusted keyring, a new boot command
line option 'keys_ownerid=', and defines a new function
get_system_or_owner_trusted_keyring().

Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
---
 Documentation/kernel-parameters.txt      |  5 ++
 crypto/asymmetric_keys/x509_public_key.c |  4 +-
 include/keys/owner_keyring.h             | 27 ++++++++++
 init/Kconfig                             | 10 ++++
 kernel/Makefile                          |  1 +
 kernel/owner_keyring.c                   | 85 ++++++++++++++++++++++++++++++++
 6 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 include/keys/owner_keyring.h
 create mode 100644 kernel/owner_keyring.c

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 7116fda..f90d31d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1434,6 +1434,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			use the HighMem zone if it exists, and the Normal
 			zone if it does not.
 
+	keys_ownerid=[KEYS] This parameter identifies a specific key on
+			the system trusted keyring to be added to the
+			owner trusted keyring.
+			format: id:<keyid>
+
 	kgdbdbgp=	[KGDB,HW] kgdb over EHCI usb debug port.
 			Format: <Controller#>[,poll interval]
 			The controller # is the number of the ehci usb debug
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 1af8a30..6af338f 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -19,6 +19,7 @@
 #include <keys/asymmetric-subtype.h>
 #include <keys/asymmetric-parser.h>
 #include <keys/system_keyring.h>
+#include <keys/owner_keyring.h>
 #include <crypto/hash.h>
 #include "asymmetric_keys.h"
 #include "public_key.h"
@@ -237,7 +238,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/owner_keyring.h b/include/keys/owner_keyring.h
new file mode 100644
index 0000000..78dd09d
--- /dev/null
+++ b/include/keys/owner_keyring.h
@@ -0,0 +1,27 @@
+/* 
+ * Copyright (C) 2014 IBM Corporation
+ * Author: Mimi Zohar <zohar@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#ifndef _KEYS_OWNER_KEYRING_H
+#define _KEYS_OWNER_KEYRING_H
+
+#ifdef CONFIG_OWNER_TRUSTED_KEYRING
+
+#include <linux/key.h>
+
+extern struct key *owner_trusted_keyring;
+extern struct key *get_system_or_owner_trusted_keyring(void);
+
+#else
+static inline struct key *get_system_or_owner_trusted_keyring(void)
+{
+	return get_system_trusted_keyring();
+}
+
+#endif
+#endif /* _KEYS_OWNER_KEYRING_H */
diff --git a/init/Kconfig b/init/Kconfig
index 009a797..7876787 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1661,6 +1661,16 @@ config SYSTEM_TRUSTED_KEYRING
 
 	  Keys in this keyring are used by module signature checking.
 
+config OWNER_TRUSTED_KEYRING
+	bool "Verify certificate signatures using a specific system key"
+	depends on SYSTEM_TRUSTED_KEYRING
+	help
+	  Verify a certificate's signature, before adding the key to
+	  a trusted keyring, using a specific key on the system trusted
+	  keyring.  The specific key on the system trusted keyring is
+	  identified using the kernel boot command line option
+	  "keys_ownerid" and is added to the owner_trusted_keyring.
+
 menuconfig MODULES
 	bool "Enable loadable module support"
 	option modules
diff --git a/kernel/Makefile b/kernel/Makefile
index bc010ee..7b44efd 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_UID16) += uid16.o
 obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
 obj-$(CONFIG_MODULES) += module.o
 obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_OWNER_TRUSTED_KEYRING) += owner_keyring.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
 obj-$(CONFIG_KEXEC) += kexec.o
diff --git a/kernel/owner_keyring.c b/kernel/owner_keyring.c
new file mode 100644
index 0000000..a31b865
--- /dev/null
+++ b/kernel/owner_keyring.c
@@ -0,0 +1,85 @@
+/* 
+ * Copyright (C) 2014 IBM Corporation
+ * Author: Mimi Zohar <zohar@us.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ */
+
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/cred.h>
+#include <linux/err.h>
+#include <keys/asymmetric-type.h>
+#include <keys/system_keyring.h>
+#include "module-internal.h"
+
+struct key *owner_trusted_keyring;
+static int use_owner_trusted_keyring;
+
+static char *owner_keyid;
+static int __init default_owner_keyid_set(char *str)
+{
+	if (!str)		/* default system keyring */
+		return 1;
+
+	if (strncmp(str, "id:", 3) == 0)
+		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
+
+	return 1;
+}
+
+__setup("keys_ownerid=", default_owner_keyid_set);
+
+struct key *get_system_or_owner_trusted_keyring(void)
+{
+	return use_owner_trusted_keyring ? owner_trusted_keyring :
+	    get_system_trusted_keyring();
+}
+
+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");
+
+	set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
+	return 0;
+}
+
+device_initcall(owner_trusted_keyring_init);
+
+void load_owner_identified_key(void)
+{
+	key_ref_t key_ref;
+	int ret;
+
+	if (!owner_keyid)
+		return;
+
+	key_ref = keyring_search(make_key_ref(system_trusted_keyring, 1),
+				 &key_type_asymmetric, owner_keyid);
+	if (IS_ERR(key_ref)) {
+		pr_warn("Request for unknown %s key\n", owner_keyid);
+		goto out;
+	}
+	ret = key_link(owner_trusted_keyring, key_ref_to_ptr(key_ref));
+	pr_info("Loaded owner key %s %s\n", owner_keyid,
+		ret < 0 ? "failed" : "succeeded");
+	key_ref_put(key_ref);
+	if (!ret)
+		use_owner_trusted_keyring = 1;
+out:
+	return;
+}
+
+late_initcall(load_owner_identified_key);
-- 
1.9.1

[PATCH 2/4] KEYS: fix couple of things

[Dmitry Kasatkin]

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 crypto/asymmetric_keys/x509_public_key.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 6af338f..962f9b9 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -177,6 +177,7 @@ static int x509_validate_trust(struct x509_certificate *cert,
 	if (!IS_ERR(key))  {
 		pk = key->payload.data;
 		ret = x509_check_signature(pk, cert);
+		key_put(key);
 	}
 	return ret;
 }
@@ -237,7 +238,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 		ret = x509_check_signature(cert->pub, cert); /* self-signed */
 		if (ret < 0)
 			goto error_free_cert;
-	} else {
+	} else if (!prep->trusted) {
 		ret = x509_validate_trust(cert,
 					  get_system_or_owner_trusted_keyring());
 		if (!ret)
-- 
1.9.1

[PATCH 3/4] KEYS: validate key trust only with selected owner key

[Dmitry Kasatkin]

This patch provides kernel parameter to specify owner's key id which
must be used for trust validate of keys. Keys signed with other keys
are not trusted.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 crypto/asymmetric_keys/x509_public_key.c | 27 ++++++++--
 include/keys/owner_keyring.h             | 27 ----------
 init/Kconfig                             | 10 ----
 kernel/Makefile                          |  1 -
 kernel/owner_keyring.c                   | 85 --------------------------------
 5 files changed, 24 insertions(+), 126 deletions(-)
 delete mode 100644 include/keys/owner_keyring.h
 delete mode 100644 kernel/owner_keyring.c

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 962f9b9..d46b790 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -19,12 +19,24 @@
 #include <keys/asymmetric-subtype.h>
 #include <keys/asymmetric-parser.h>
 #include <keys/system_keyring.h>
-#include <keys/owner_keyring.h>
 #include <crypto/hash.h>
 #include "asymmetric_keys.h"
 #include "public_key.h"
 #include "x509_parser.h"
 
+static char *owner_keyid;
+static int __init default_owner_keyid_set(char *str)
+{
+	if (!str)		/* default system keyring */
+		return 1;
+
+	if (strncmp(str, "id:", 3) == 0)
+		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
+
+	return 1;
+}
+__setup("keys_ownerid=", default_owner_keyid_set);
+
 /*
  * Find a key in the given keyring by issuer and authority.
  */
@@ -170,6 +182,16 @@ static int x509_validate_trust(struct x509_certificate *cert,
 	if (!trust_keyring)
 		return -EOPNOTSUPP;
 
+	if (owner_keyid) {
+		/* validate trust only with the owner_keyid if specified */
+		/* partial match of keyid according to the asymmetric_type.c */
+		int idlen = strlen(owner_keyid) - 3; /* - id: */
+		int authlen = strlen(cert->authority);
+		char *auth = cert->authority + authlen - idlen;
+		if (idlen > authlen || strcasecmp(owner_keyid + 3, auth))
+			return -EPERM;
+	}
+
 	key = x509_request_asymmetric_key(trust_keyring,
 					  cert->issuer, strlen(cert->issuer),
 					  cert->authority,
@@ -239,8 +261,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 		if (ret < 0)
 			goto error_free_cert;
 	} else if (!prep->trusted) {
-		ret = x509_validate_trust(cert,
-					  get_system_or_owner_trusted_keyring());
+		ret = x509_validate_trust(cert, get_system_trusted_keyring());
 		if (!ret)
 			prep->trusted = 1;
 	}
diff --git a/include/keys/owner_keyring.h b/include/keys/owner_keyring.h
deleted file mode 100644
index 78dd09d..0000000
--- a/include/keys/owner_keyring.h
+++ /dev/null
@@ -1,27 +0,0 @@
-/* 
- * Copyright (C) 2014 IBM Corporation
- * Author: Mimi Zohar <zohar@us.ibm.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, version 2 of the License.
- */
-
-#ifndef _KEYS_OWNER_KEYRING_H
-#define _KEYS_OWNER_KEYRING_H
-
-#ifdef CONFIG_OWNER_TRUSTED_KEYRING
-
-#include <linux/key.h>
-
-extern struct key *owner_trusted_keyring;
-extern struct key *get_system_or_owner_trusted_keyring(void);
-
-#else
-static inline struct key *get_system_or_owner_trusted_keyring(void)
-{
-	return get_system_trusted_keyring();
-}
-
-#endif
-#endif /* _KEYS_OWNER_KEYRING_H */
diff --git a/init/Kconfig b/init/Kconfig
index 7876787..009a797 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1661,16 +1661,6 @@ config SYSTEM_TRUSTED_KEYRING
 
 	  Keys in this keyring are used by module signature checking.
 
-config OWNER_TRUSTED_KEYRING
-	bool "Verify certificate signatures using a specific system key"
-	depends on SYSTEM_TRUSTED_KEYRING
-	help
-	  Verify a certificate's signature, before adding the key to
-	  a trusted keyring, using a specific key on the system trusted
-	  keyring.  The specific key on the system trusted keyring is
-	  identified using the kernel boot command line option
-	  "keys_ownerid" and is added to the owner_trusted_keyring.
-
 menuconfig MODULES
 	bool "Enable loadable module support"
 	option modules
diff --git a/kernel/Makefile b/kernel/Makefile
index 7b44efd..bc010ee 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -44,7 +44,6 @@ obj-$(CONFIG_UID16) += uid16.o
 obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
 obj-$(CONFIG_MODULES) += module.o
 obj-$(CONFIG_MODULE_SIG) += module_signing.o
-obj-$(CONFIG_OWNER_TRUSTED_KEYRING) += owner_keyring.o
 obj-$(CONFIG_KALLSYMS) += kallsyms.o
 obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
 obj-$(CONFIG_KEXEC) += kexec.o
diff --git a/kernel/owner_keyring.c b/kernel/owner_keyring.c
deleted file mode 100644
index a31b865..0000000
--- a/kernel/owner_keyring.c
+++ /dev/null
@@ -1,85 +0,0 @@
-/* 
- * Copyright (C) 2014 IBM Corporation
- * Author: Mimi Zohar <zohar@us.ibm.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation, version 2 of the License.
- */
-
-#include <linux/export.h>
-#include <linux/kernel.h>
-#include <linux/sched.h>
-#include <linux/cred.h>
-#include <linux/err.h>
-#include <keys/asymmetric-type.h>
-#include <keys/system_keyring.h>
-#include "module-internal.h"
-
-struct key *owner_trusted_keyring;
-static int use_owner_trusted_keyring;
-
-static char *owner_keyid;
-static int __init default_owner_keyid_set(char *str)
-{
-	if (!str)		/* default system keyring */
-		return 1;
-
-	if (strncmp(str, "id:", 3) == 0)
-		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
-
-	return 1;
-}
-
-__setup("keys_ownerid=", default_owner_keyid_set);
-
-struct key *get_system_or_owner_trusted_keyring(void)
-{
-	return use_owner_trusted_keyring ? owner_trusted_keyring :
-	    get_system_trusted_keyring();
-}
-
-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");
-
-	set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
-	return 0;
-}
-
-device_initcall(owner_trusted_keyring_init);
-
-void load_owner_identified_key(void)
-{
-	key_ref_t key_ref;
-	int ret;
-
-	if (!owner_keyid)
-		return;
-
-	key_ref = keyring_search(make_key_ref(system_trusted_keyring, 1),
-				 &key_type_asymmetric, owner_keyid);
-	if (IS_ERR(key_ref)) {
-		pr_warn("Request for unknown %s key\n", owner_keyid);
-		goto out;
-	}
-	ret = key_link(owner_trusted_keyring, key_ref_to_ptr(key_ref));
-	pr_info("Loaded owner key %s %s\n", owner_keyid,
-		ret < 0 ? "failed" : "succeeded");
-	key_ref_put(key_ref);
-	if (!ret)
-		use_owner_trusted_keyring = 1;
-out:
-	return;
-}
-
-late_initcall(load_owner_identified_key);
-- 
1.9.1

[PATCH 4/4] KEYS: validate key trust only with builtin keys

[Dmitry Kasatkin]

This patch allows to specify kernel parameter 'keys_ownerid=builtin'
to allow trust validation only using builtin keys.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 crypto/asymmetric_keys/x509_public_key.c | 9 +++++++--
 include/linux/key.h                      | 1 +
 kernel/system_keyring.c                  | 1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index d46b790..c3805a8 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -24,6 +24,7 @@
 #include "public_key.h"
 #include "x509_parser.h"
 
+static bool builtin_keys;
 static char *owner_keyid;
 static int __init default_owner_keyid_set(char *str)
 {
@@ -32,6 +33,8 @@ static int __init default_owner_keyid_set(char *str)
 
 	if (strncmp(str, "id:", 3) == 0)
 		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
+	else if (strcmp(str, "builtin") == 0)
+		builtin_keys = true;
 
 	return 1;
 }
@@ -197,8 +200,10 @@ static int x509_validate_trust(struct x509_certificate *cert,
 					  cert->authority,
 					  strlen(cert->authority));
 	if (!IS_ERR(key))  {
-		pk = key->payload.data;
-		ret = x509_check_signature(pk, cert);
+		if (!builtin_keys || test_bit(KEY_FLAG_BUILTIN, &key->flags)) {
+			pk = key->payload.data;
+			ret = x509_check_signature(pk, cert);
+		}
 		key_put(key);
 	}
 	return ret;
diff --git a/include/linux/key.h b/include/linux/key.h
index cd0abb8..67c8e7e 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -170,6 +170,7 @@ struct key {
 #define KEY_FLAG_INVALIDATED	7	/* set if key has been invalidated */
 #define KEY_FLAG_TRUSTED	8	/* set if key is trusted */
 #define KEY_FLAG_TRUSTED_ONLY	9	/* set if keyring only accepts links to trusted keys */
+#define KEY_FLAG_BUILTIN	10	/* set if key is builtin */
 
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
diff --git a/kernel/system_keyring.c b/kernel/system_keyring.c
index 52ebc70..875f64e 100644
--- a/kernel/system_keyring.c
+++ b/kernel/system_keyring.c
@@ -89,6 +89,7 @@ static __init int load_system_certificate_list(void)
 			pr_err("Problem loading in-kernel X.509 certificate (%ld)\n",
 			       PTR_ERR(key));
 		} else {
+			set_bit(KEY_FLAG_BUILTIN, &key_ref_to_ptr(key)->flags);
 			pr_notice("Loaded X.509 cert '%s'\n",
 				  key_ref_to_ptr(key)->description);
 			key_ref_put(key);
-- 
1.9.1

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Josh Boyer]

On Tue, Jun 10, 2014 at 11:48:14AM +0300, Dmitry Kasatkin wrote:
> Hi Mimi,
> 
> As you asked ofline , here is possible equivalent and simpler alternative
> patches not requiring to have additional keyring.
> 
> First patch are irrelevant minor fixes.
> 
> Also I want to discuss here Fedora UEFI patches as they are the reason for
> the these original patchset.
> 
> http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
> 
> They provide functionality to specify MokIgnoreDb variable to limit loading of
> UEFI keys only from MOK List, while ignoring DB. This is certainly a good
> functionality. But once MODULE_SIG_UEFI is enabled, it looks there is no way
> to prevent loading keys from UEFI at all. And this might not be a good default
> functionality. Someone might want not allow loading of keys from UEFI unless
> kernel parameter is specified to allow it without recompiling the kernel
> and disabling MODULE_SIG_UEFI.
> 
> Josh, why such design decision was made?

IIRC, it's because kernel parameters can be added programmatically from a
remote user if they gain root access.  Having a kernel parameter to
disable a key piece of secure boot isn't all that great.  We disable
other kernel parameters like acpi_rspd as well.

> Why not to provide kernel parameter to have more fine-tune control over the
> functionality? Unconfigured machines will not have MokIgnoreDb and will
> allow to load kernel modules signed with certain undesired keys. In fact,

Undesired by whom?  If SB is enabled, your machine's firmware already
trusts those keys.

> I beleive, it should be default behavior of the kernel. Bootloader can
> enable UEFI functionality by specifing it on the kernel command line.

If it was enabled via boot params, or done in the early setup code that
might be possible.  I don't think a kernel parameter is the right
solution though.  I've added Matthew on CC.

josh

> Second patch allows to overcome keys coming from UEFI for key validation by
> specifing owner key id and is an alternative for v5 4/4 patch.
> 
> It was also a good idea presented in Mimi's v4 4/4 patch to have possibility
> to limit key trust valiation by only builtin keys. Third patch as an alternative.
> It uses keys->flags to specify origin of the key, but any additional field could
> be added as well.
> 
> Both key id and origin verification is done in x509_validate_trust().
> 
> Thanks,
> Dmitry
> 
> Dmitry Kasatkin (3):
>   KEYS: fix couple of things
>   KEYS: validate key trust only with selected owner key
>   KEYS: validate key trust only with builtin keys
> 
> Mimi Zohar (1):
>   KEYS: define an owner trusted keyring
> 
>  Documentation/kernel-parameters.txt      |  5 +++++
>  crypto/asymmetric_keys/x509_public_key.c | 35 +++++++++++++++++++++++++++++---
>  include/linux/key.h                      |  1 +
>  kernel/system_keyring.c                  |  1 +
>  4 files changed, 39 insertions(+), 3 deletions(-)
> 
> -- 
> 1.9.1
> 

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

[Josh Boyer]

On Tue, Jun 10, 2014 at 11:48:15AM +0300, Dmitry Kasatkin wrote:
> From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> 
> 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 a particular key on the system keyring.
> 
> When the UEFI secure boot keys are added to the system keyring, the
> platform owner will be able to load their key in one of the UEFI DBs
> (eg. Machine Owner Key(MOK) list) and select their key, without
> having to rebuild the kernel.
> 
> This patch defines an owner trusted keyring, a new boot command
> line option 'keys_ownerid=', and defines a new function
> get_system_or_owner_trusted_keyring().
> 
> Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> ---
>  Documentation/kernel-parameters.txt      |  5 ++
>  crypto/asymmetric_keys/x509_public_key.c |  4 +-
>  include/keys/owner_keyring.h             | 27 ++++++++++
>  init/Kconfig                             | 10 ++++
>  kernel/Makefile                          |  1 +
>  kernel/owner_keyring.c                   | 85 ++++++++++++++++++++++++++++++++
>  6 files changed, 131 insertions(+), 1 deletion(-)
>  create mode 100644 include/keys/owner_keyring.h
>  create mode 100644 kernel/owner_keyring.c
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 7116fda..f90d31d 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1434,6 +1434,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			use the HighMem zone if it exists, and the Normal
>  			zone if it does not.
>  
> +	keys_ownerid=[KEYS] This parameter identifies a specific key on
> +			the system trusted keyring to be added to the
> +			owner trusted keyring.
> +			format: id:<keyid>
> +

I'm fairly sure this runs into the same problems I mentioned previously
in the secure boot context.  Namely that a remote attacker could modify
keys_ownerid in the bootloader config file if they gained root access.

josh

>  	kgdbdbgp=	[KGDB,HW] kgdb over EHCI usb debug port.
>  			Format: <Controller#>[,poll interval]
>  			The controller # is the number of the ehci usb debug
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index 1af8a30..6af338f 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -19,6 +19,7 @@
>  #include <keys/asymmetric-subtype.h>
>  #include <keys/asymmetric-parser.h>
>  #include <keys/system_keyring.h>
> +#include <keys/owner_keyring.h>
>  #include <crypto/hash.h>
>  #include "asymmetric_keys.h"
>  #include "public_key.h"
> @@ -237,7 +238,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/owner_keyring.h b/include/keys/owner_keyring.h
> new file mode 100644
> index 0000000..78dd09d
> --- /dev/null
> +++ b/include/keys/owner_keyring.h
> @@ -0,0 +1,27 @@
> +/* 
> + * Copyright (C) 2014 IBM Corporation
> + * Author: Mimi Zohar <zohar@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + */
> +
> +#ifndef _KEYS_OWNER_KEYRING_H
> +#define _KEYS_OWNER_KEYRING_H
> +
> +#ifdef CONFIG_OWNER_TRUSTED_KEYRING
> +
> +#include <linux/key.h>
> +
> +extern struct key *owner_trusted_keyring;
> +extern struct key *get_system_or_owner_trusted_keyring(void);
> +
> +#else
> +static inline struct key *get_system_or_owner_trusted_keyring(void)
> +{
> +	return get_system_trusted_keyring();
> +}
> +
> +#endif
> +#endif /* _KEYS_OWNER_KEYRING_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index 009a797..7876787 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1661,6 +1661,16 @@ config SYSTEM_TRUSTED_KEYRING
>  
>  	  Keys in this keyring are used by module signature checking.
>  
> +config OWNER_TRUSTED_KEYRING
> +	bool "Verify certificate signatures using a specific system key"
> +	depends on SYSTEM_TRUSTED_KEYRING
> +	help
> +	  Verify a certificate's signature, before adding the key to
> +	  a trusted keyring, using a specific key on the system trusted
> +	  keyring.  The specific key on the system trusted keyring is
> +	  identified using the kernel boot command line option
> +	  "keys_ownerid" and is added to the owner_trusted_keyring.
> +
>  menuconfig MODULES
>  	bool "Enable loadable module support"
>  	option modules
> diff --git a/kernel/Makefile b/kernel/Makefile
> index bc010ee..7b44efd 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_UID16) += uid16.o
>  obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
>  obj-$(CONFIG_MODULES) += module.o
>  obj-$(CONFIG_MODULE_SIG) += module_signing.o
> +obj-$(CONFIG_OWNER_TRUSTED_KEYRING) += owner_keyring.o
>  obj-$(CONFIG_KALLSYMS) += kallsyms.o
>  obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
>  obj-$(CONFIG_KEXEC) += kexec.o
> diff --git a/kernel/owner_keyring.c b/kernel/owner_keyring.c
> new file mode 100644
> index 0000000..a31b865
> --- /dev/null
> +++ b/kernel/owner_keyring.c
> @@ -0,0 +1,85 @@
> +/* 
> + * Copyright (C) 2014 IBM Corporation
> + * Author: Mimi Zohar <zohar@us.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, version 2 of the License.
> + */
> +
> +#include <linux/export.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/cred.h>
> +#include <linux/err.h>
> +#include <keys/asymmetric-type.h>
> +#include <keys/system_keyring.h>
> +#include "module-internal.h"
> +
> +struct key *owner_trusted_keyring;
> +static int use_owner_trusted_keyring;
> +
> +static char *owner_keyid;
> +static int __init default_owner_keyid_set(char *str)
> +{
> +	if (!str)		/* default system keyring */
> +		return 1;
> +
> +	if (strncmp(str, "id:", 3) == 0)
> +		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
> +
> +	return 1;
> +}
> +
> +__setup("keys_ownerid=", default_owner_keyid_set);
> +
> +struct key *get_system_or_owner_trusted_keyring(void)
> +{
> +	return use_owner_trusted_keyring ? owner_trusted_keyring :
> +	    get_system_trusted_keyring();
> +}
> +
> +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");
> +
> +	set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
> +	return 0;
> +}
> +
> +device_initcall(owner_trusted_keyring_init);
> +
> +void load_owner_identified_key(void)
> +{
> +	key_ref_t key_ref;
> +	int ret;
> +
> +	if (!owner_keyid)
> +		return;
> +
> +	key_ref = keyring_search(make_key_ref(system_trusted_keyring, 1),
> +				 &key_type_asymmetric, owner_keyid);
> +	if (IS_ERR(key_ref)) {
> +		pr_warn("Request for unknown %s key\n", owner_keyid);
> +		goto out;
> +	}
> +	ret = key_link(owner_trusted_keyring, key_ref_to_ptr(key_ref));
> +	pr_info("Loaded owner key %s %s\n", owner_keyid,
> +		ret < 0 ? "failed" : "succeeded");
> +	key_ref_put(key_ref);
> +	if (!ret)
> +		use_owner_trusted_keyring = 1;
> +out:
> +	return;
> +}
> +
> +late_initcall(load_owner_identified_key);
> -- 
> 1.9.1
> 

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

[Dmitry Kasatkin]

On 10/06/14 15:24, Josh Boyer wrote:
> On Tue, Jun 10, 2014 at 11:48:15AM +0300, Dmitry Kasatkin wrote:
>> From: Mimi Zohar <zohar@linux.vnet.ibm.com>
>>
>> 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 a particular key on the system keyring.
>>
>> When the UEFI secure boot keys are added to the system keyring, the
>> platform owner will be able to load their key in one of the UEFI DBs
>> (eg. Machine Owner Key(MOK) list) and select their key, without
>> having to rebuild the kernel.
>>
>> This patch defines an owner trusted keyring, a new boot command
>> line option 'keys_ownerid=', and defines a new function
>> get_system_or_owner_trusted_keyring().
>>
>> Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
>> ---
>>  Documentation/kernel-parameters.txt      |  5 ++
>>  crypto/asymmetric_keys/x509_public_key.c |  4 +-
>>  include/keys/owner_keyring.h             | 27 ++++++++++
>>  init/Kconfig                             | 10 ++++
>>  kernel/Makefile                          |  1 +
>>  kernel/owner_keyring.c                   | 85 ++++++++++++++++++++++++++++++++
>>  6 files changed, 131 insertions(+), 1 deletion(-)
>>  create mode 100644 include/keys/owner_keyring.h
>>  create mode 100644 kernel/owner_keyring.c
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 7116fda..f90d31d 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -1434,6 +1434,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>>  			use the HighMem zone if it exists, and the Normal
>>  			zone if it does not.
>>  
>> +	keys_ownerid=[KEYS] This parameter identifies a specific key on
>> +			the system trusted keyring to be added to the
>> +			owner trusted keyring.
>> +			format: id:<keyid>
>> +
> I'm fairly sure this runs into the same problems I mentioned previously
> in the secure boot context.  Namely that a remote attacker could modify
> keys_ownerid in the bootloader config file if they gained root access.
>
> josh

This patch is original patch and actually I sent it by mistake providing
HEAD~3 instead of HEAD~2 on git-send-email...
But anyway kernel parameter stays...

ok.  case is "unprotected" boot loader.


Thanks.

>>  	kgdbdbgp=	[KGDB,HW] kgdb over EHCI usb debug port.
>>  			Format: <Controller#>[,poll interval]
>>  			The controller # is the number of the ehci usb debug
>> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
>> index 1af8a30..6af338f 100644
>> --- a/crypto/asymmetric_keys/x509_public_key.c
>> +++ b/crypto/asymmetric_keys/x509_public_key.c
>> @@ -19,6 +19,7 @@
>>  #include <keys/asymmetric-subtype.h>
>>  #include <keys/asymmetric-parser.h>
>>  #include <keys/system_keyring.h>
>> +#include <keys/owner_keyring.h>
>>  #include <crypto/hash.h>
>>  #include "asymmetric_keys.h"
>>  #include "public_key.h"
>> @@ -237,7 +238,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/owner_keyring.h b/include/keys/owner_keyring.h
>> new file mode 100644
>> index 0000000..78dd09d
>> --- /dev/null
>> +++ b/include/keys/owner_keyring.h
>> @@ -0,0 +1,27 @@
>> +/* 
>> + * Copyright (C) 2014 IBM Corporation
>> + * Author: Mimi Zohar <zohar@us.ibm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, version 2 of the License.
>> + */
>> +
>> +#ifndef _KEYS_OWNER_KEYRING_H
>> +#define _KEYS_OWNER_KEYRING_H
>> +
>> +#ifdef CONFIG_OWNER_TRUSTED_KEYRING
>> +
>> +#include <linux/key.h>
>> +
>> +extern struct key *owner_trusted_keyring;
>> +extern struct key *get_system_or_owner_trusted_keyring(void);
>> +
>> +#else
>> +static inline struct key *get_system_or_owner_trusted_keyring(void)
>> +{
>> +	return get_system_trusted_keyring();
>> +}
>> +
>> +#endif
>> +#endif /* _KEYS_OWNER_KEYRING_H */
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 009a797..7876787 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1661,6 +1661,16 @@ config SYSTEM_TRUSTED_KEYRING
>>  
>>  	  Keys in this keyring are used by module signature checking.
>>  
>> +config OWNER_TRUSTED_KEYRING
>> +	bool "Verify certificate signatures using a specific system key"
>> +	depends on SYSTEM_TRUSTED_KEYRING
>> +	help
>> +	  Verify a certificate's signature, before adding the key to
>> +	  a trusted keyring, using a specific key on the system trusted
>> +	  keyring.  The specific key on the system trusted keyring is
>> +	  identified using the kernel boot command line option
>> +	  "keys_ownerid" and is added to the owner_trusted_keyring.
>> +
>>  menuconfig MODULES
>>  	bool "Enable loadable module support"
>>  	option modules
>> diff --git a/kernel/Makefile b/kernel/Makefile
>> index bc010ee..7b44efd 100644
>> --- a/kernel/Makefile
>> +++ b/kernel/Makefile
>> @@ -44,6 +44,7 @@ obj-$(CONFIG_UID16) += uid16.o
>>  obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
>>  obj-$(CONFIG_MODULES) += module.o
>>  obj-$(CONFIG_MODULE_SIG) += module_signing.o
>> +obj-$(CONFIG_OWNER_TRUSTED_KEYRING) += owner_keyring.o
>>  obj-$(CONFIG_KALLSYMS) += kallsyms.o
>>  obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
>>  obj-$(CONFIG_KEXEC) += kexec.o
>> diff --git a/kernel/owner_keyring.c b/kernel/owner_keyring.c
>> new file mode 100644
>> index 0000000..a31b865
>> --- /dev/null
>> +++ b/kernel/owner_keyring.c
>> @@ -0,0 +1,85 @@
>> +/* 
>> + * Copyright (C) 2014 IBM Corporation
>> + * Author: Mimi Zohar <zohar@us.ibm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation, version 2 of the License.
>> + */
>> +
>> +#include <linux/export.h>
>> +#include <linux/kernel.h>
>> +#include <linux/sched.h>
>> +#include <linux/cred.h>
>> +#include <linux/err.h>
>> +#include <keys/asymmetric-type.h>
>> +#include <keys/system_keyring.h>
>> +#include "module-internal.h"
>> +
>> +struct key *owner_trusted_keyring;
>> +static int use_owner_trusted_keyring;
>> +
>> +static char *owner_keyid;
>> +static int __init default_owner_keyid_set(char *str)
>> +{
>> +	if (!str)		/* default system keyring */
>> +		return 1;
>> +
>> +	if (strncmp(str, "id:", 3) == 0)
>> +		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
>> +
>> +	return 1;
>> +}
>> +
>> +__setup("keys_ownerid=", default_owner_keyid_set);
>> +
>> +struct key *get_system_or_owner_trusted_keyring(void)
>> +{
>> +	return use_owner_trusted_keyring ? owner_trusted_keyring :
>> +	    get_system_trusted_keyring();
>> +}
>> +
>> +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");
>> +
>> +	set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
>> +	return 0;
>> +}
>> +
>> +device_initcall(owner_trusted_keyring_init);
>> +
>> +void load_owner_identified_key(void)
>> +{
>> +	key_ref_t key_ref;
>> +	int ret;
>> +
>> +	if (!owner_keyid)
>> +		return;
>> +
>> +	key_ref = keyring_search(make_key_ref(system_trusted_keyring, 1),
>> +				 &key_type_asymmetric, owner_keyid);
>> +	if (IS_ERR(key_ref)) {
>> +		pr_warn("Request for unknown %s key\n", owner_keyid);
>> +		goto out;
>> +	}
>> +	ret = key_link(owner_trusted_keyring, key_ref_to_ptr(key_ref));
>> +	pr_info("Loaded owner key %s %s\n", owner_keyid,
>> +		ret < 0 ? "failed" : "succeeded");
>> +	key_ref_put(key_ref);
>> +	if (!ret)
>> +		use_owner_trusted_keyring = 1;
>> +out:
>> +	return;
>> +}
>> +
>> +late_initcall(load_owner_identified_key);
>> -- 
>> 1.9.1
>>

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Mimi Zohar]

On Tue, 2014-06-10 at 11:48 +0300, Dmitry Kasatkin wrote: 
> Hi Mimi,
> 
> As you asked ofline , here is possible equivalent and simpler alternative
> patches not requiring to have additional keyring.

Please indicate which branch these patches apply to.

thanks,

Mimi

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

On 10/06/14 15:45, Mimi Zohar wrote:
> On Tue, 2014-06-10 at 11:48 +0300, Dmitry Kasatkin wrote: 
>> Hi Mimi,
>>
>> As you asked ofline , here is possible equivalent and simpler alternative
>> patches not requiring to have additional keyring.
> Please indicate which branch these patches apply to.
>
> thanks,
>
> Mimi
>
>

on the top of your tree #next-trusted-keys-experimental, excluding 1/4
patch, which is yours.
I included it by mistake because patches were on the top of that and I
used HEAD~3 instead of HEAD~2 to send them...

- Dmitry

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Mimi Zohar]

On Tue, 2014-06-10 at 08:20 -0400, Josh Boyer wrote: 
> On Tue, Jun 10, 2014 at 11:48:14AM +0300, Dmitry Kasatkin wrote:

> > Also I want to discuss here Fedora UEFI patches as they are the reason for
> > the these original patchset.
> > 
> > http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
> > 
> > They provide functionality to specify MokIgnoreDb variable to limit loading of
> > UEFI keys only from MOK List, while ignoring DB. This is certainly a good
> > functionality. But once MODULE_SIG_UEFI is enabled, it looks there is no way
> > to prevent loading keys from UEFI at all. And this might not be a good default
> > functionality. Someone might want not allow loading of keys from UEFI unless
> > kernel parameter is specified to allow it without recompiling the kernel
> > and disabling MODULE_SIG_UEFI.
> > 
> > Josh, why such design decision was made?
> 
> IIRC, it's because kernel parameters can be added programmatically from a
> remote user if they gain root access.  Having a kernel parameter to
> disable a key piece of secure boot isn't all that great.  We disable
> other kernel parameters like acpi_rspd as well.

In this case, there shouldn't be a problem as the kernel parameters
would further limit the keys usage.

Mimi

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

On 10/06/14 15:20, Josh Boyer wrote:
> On Tue, Jun 10, 2014 at 11:48:14AM +0300, Dmitry Kasatkin wrote:
>> Hi Mimi,
>>
>> As you asked ofline , here is possible equivalent and simpler alternative
>> patches not requiring to have additional keyring.
>>
>> First patch are irrelevant minor fixes.
>>
>> Also I want to discuss here Fedora UEFI patches as they are the reason for
>> the these original patchset.
>>
>> http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
>>
>> They provide functionality to specify MokIgnoreDb variable to limit loading of
>> UEFI keys only from MOK List, while ignoring DB. This is certainly a good
>> functionality. But once MODULE_SIG_UEFI is enabled, it looks there is no way
>> to prevent loading keys from UEFI at all. And this might not be a good default
>> functionality. Someone might want not allow loading of keys from UEFI unless
>> kernel parameter is specified to allow it without recompiling the kernel
>> and disabling MODULE_SIG_UEFI.
>>
>> Josh, why such design decision was made?
> IIRC, it's because kernel parameters can be added programmatically from a
> remote user if they gain root access.  Having a kernel parameter to
> disable a key piece of secure boot isn't all that great.  We disable
> other kernel parameters like acpi_rspd as well.

I see the point, as we have unprotected boot loader configuration.

>> Why not to provide kernel parameter to have more fine-tune control over the
>> functionality? Unconfigured machines will not have MokIgnoreDb and will
>> allow to load kernel modules signed with certain undesired keys. In fact,
> Undesired by whom?  If SB is enabled, your machine's firmware already
> trusts those keys.

It is tricky issue. But yes and no... If I forced to trust MS key to run
SHIM, it does not mean
that I want to trust MS key to run kernel and load modules or use MS key
to valid other keys on system keyring.

Personally I took ownership of my laptop laptop by enrolling my key.
I also re-signed SHIM...

But for convenience I keep MS key to boot from any USB stick, though
booting is password protected...

-> So the only point I trust MS key is when I type my password to boot...

And next when system is running, I do not want MS or Lenovo key would be
used to verify kernel modules or signed files...



>> I beleive, it should be default behavior of the kernel. Bootloader can
>> enable UEFI functionality by specifing it on the kernel command line.
> If it was enabled via boot params, or done in the early setup code that
> might be possible.  I don't think a kernel parameter is the right
> solution though.  I've added Matthew on CC.

Thanks for reply.

> josh
>
>> Second patch allows to overcome keys coming from UEFI for key validation by
>> specifing owner key id and is an alternative for v5 4/4 patch.
>>
>> It was also a good idea presented in Mimi's v4 4/4 patch to have possibility
>> to limit key trust valiation by only builtin keys. Third patch as an alternative.
>> It uses keys->flags to specify origin of the key, but any additional field could
>> be added as well.
>>
>> Both key id and origin verification is done in x509_validate_trust().
>>
>> Thanks,
>> Dmitry
>>
>> Dmitry Kasatkin (3):
>>   KEYS: fix couple of things
>>   KEYS: validate key trust only with selected owner key
>>   KEYS: validate key trust only with builtin keys
>>
>> Mimi Zohar (1):
>>   KEYS: define an owner trusted keyring
>>
>>  Documentation/kernel-parameters.txt      |  5 +++++
>>  crypto/asymmetric_keys/x509_public_key.c | 35 +++++++++++++++++++++++++++++---
>>  include/linux/key.h                      |  1 +
>>  kernel/system_keyring.c                  |  1 +
>>  4 files changed, 39 insertions(+), 3 deletions(-)
>>
>> -- 
>> 1.9.1
>>
> --
> 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
>

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

[Mimi Zohar]

On Tue, 2014-06-10 at 08:24 -0400, Josh Boyer wrote: 
> On Tue, Jun 10, 2014 at 11:48:15AM +0300, Dmitry Kasatkin wrote:
> > From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > 
> > 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 a particular key on the system keyring.
> > 
> > When the UEFI secure boot keys are added to the system keyring, the
> > platform owner will be able to load their key in one of the UEFI DBs
> > (eg. Machine Owner Key(MOK) list) and select their key, without
> > having to rebuild the kernel.
> > 
> > This patch defines an owner trusted keyring, a new boot command
> > line option 'keys_ownerid=', and defines a new function
> > get_system_or_owner_trusted_keyring().
> > 
> > Signed-off-by: Mimi Zohar<zohar@linux.vnet.ibm.com>
> > ---
> >  Documentation/kernel-parameters.txt      |  5 ++
> >  crypto/asymmetric_keys/x509_public_key.c |  4 +-
> >  include/keys/owner_keyring.h             | 27 ++++++++++
> >  init/Kconfig                             | 10 ++++
> >  kernel/Makefile                          |  1 +
> >  kernel/owner_keyring.c                   | 85 ++++++++++++++++++++++++++++++++
> >  6 files changed, 131 insertions(+), 1 deletion(-)
> >  create mode 100644 include/keys/owner_keyring.h
> >  create mode 100644 kernel/owner_keyring.c
> > 
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index 7116fda..f90d31d 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -1434,6 +1434,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> >  			use the HighMem zone if it exists, and the Normal
> >  			zone if it does not.
> >  
> > +	keys_ownerid=[KEYS] This parameter identifies a specific key on
> > +			the system trusted keyring to be added to the
> > +			owner trusted keyring.
> > +			format: id:<keyid>
> > +
> 
> I'm fairly sure this runs into the same problems I mentioned previously
> in the secure boot context.  Namely that a remote attacker could modify
> keys_ownerid in the bootloader config file if they gained root access.

Yes, someone could specify a key not in the UEFI DB or builtin, but it
would not do them much good.  The "keys_ownerid" searches the
UEFI/builtin keys for the keyid.

Mimi

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

On 10/06/14 15:52, Mimi Zohar wrote:
> On Tue, 2014-06-10 at 08:20 -0400, Josh Boyer wrote: 
>> On Tue, Jun 10, 2014 at 11:48:14AM +0300, Dmitry Kasatkin wrote:
>>> Also I want to discuss here Fedora UEFI patches as they are the reason for
>>> the these original patchset.
>>>
>>> http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
>>>
>>> They provide functionality to specify MokIgnoreDb variable to limit loading of
>>> UEFI keys only from MOK List, while ignoring DB. This is certainly a good
>>> functionality. But once MODULE_SIG_UEFI is enabled, it looks there is no way
>>> to prevent loading keys from UEFI at all. And this might not be a good default
>>> functionality. Someone might want not allow loading of keys from UEFI unless
>>> kernel parameter is specified to allow it without recompiling the kernel
>>> and disabling MODULE_SIG_UEFI.
>>>
>>> Josh, why such design decision was made?
>> IIRC, it's because kernel parameters can be added programmatically from a
>> remote user if they gain root access.  Having a kernel parameter to
>> disable a key piece of secure boot isn't all that great.  We disable
>> other kernel parameters like acpi_rspd as well.
> In this case, there shouldn't be a problem as the kernel parameters
> would further limit the keys usage.
>
> Mimi

Josh probably means that it can be removed and restriction is lifted..
And after reboot, all keys come to the keyring..


>

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Josh Boyer]

On Tue, Jun 10, 2014 at 04:21:36PM +0300, Dmitry Kasatkin wrote:
> On 10/06/14 15:52, Mimi Zohar wrote:
> > On Tue, 2014-06-10 at 08:20 -0400, Josh Boyer wrote: 
> >> On Tue, Jun 10, 2014 at 11:48:14AM +0300, Dmitry Kasatkin wrote:
> >>> Also I want to discuss here Fedora UEFI patches as they are the reason for
> >>> the these original patchset.
> >>>
> >>> http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
> >>>
> >>> They provide functionality to specify MokIgnoreDb variable to limit loading of
> >>> UEFI keys only from MOK List, while ignoring DB. This is certainly a good
> >>> functionality. But once MODULE_SIG_UEFI is enabled, it looks there is no way
> >>> to prevent loading keys from UEFI at all. And this might not be a good default
> >>> functionality. Someone might want not allow loading of keys from UEFI unless
> >>> kernel parameter is specified to allow it without recompiling the kernel
> >>> and disabling MODULE_SIG_UEFI.
> >>>
> >>> Josh, why such design decision was made?
> >> IIRC, it's because kernel parameters can be added programmatically from a
> >> remote user if they gain root access.  Having a kernel parameter to
> >> disable a key piece of secure boot isn't all that great.  We disable
> >> other kernel parameters like acpi_rspd as well.
> > In this case, there shouldn't be a problem as the kernel parameters
> > would further limit the keys usage.
> >
> > Mimi
> 
> Josh probably means that it can be removed and restriction is lifted..
> And after reboot, all keys come to the keyring..

Right.  Or if we went with your suggestion of the default being "do not
load UEFI keys", then they could conversely add the parameter instead.
This might not be an immediate threat to the SB attack vector itself
(thought it could be if I thought about it harder), but it's certainly
a change in system behavior that would catch a user unaware.

At any rate, I'm likely not the best person to weigh in on this aspect.
Matthew has certainly done more thinking about these kinds of problems.

josh

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Mimi Zohar]

On Tue, 2014-06-10 at 09:29 -0400, Josh Boyer wrote: 
> On Tue, Jun 10, 2014 at 04:21:36PM +0300, Dmitry Kasatkin wrote:
> > On 10/06/14 15:52, Mimi Zohar wrote:
> > > On Tue, 2014-06-10 at 08:20 -0400, Josh Boyer wrote: 
> > >> On Tue, Jun 10, 2014 at 11:48:14AM +0300, Dmitry Kasatkin wrote:
> > >>> Also I want to discuss here Fedora UEFI patches as they are the reason for
> > >>> the these original patchset.
> > >>>
> > >>> http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
> > >>>
> > >>> They provide functionality to specify MokIgnoreDb variable to limit loading of
> > >>> UEFI keys only from MOK List, while ignoring DB. This is certainly a good
> > >>> functionality. But once MODULE_SIG_UEFI is enabled, it looks there is no way
> > >>> to prevent loading keys from UEFI at all. And this might not be a good default
> > >>> functionality. Someone might want not allow loading of keys from UEFI unless
> > >>> kernel parameter is specified to allow it without recompiling the kernel
> > >>> and disabling MODULE_SIG_UEFI.
> > >>>
> > >>> Josh, why such design decision was made?
> > >> IIRC, it's because kernel parameters can be added programmatically from a
> > >> remote user if they gain root access.  Having a kernel parameter to
> > >> disable a key piece of secure boot isn't all that great.  We disable
> > >> other kernel parameters like acpi_rspd as well.
> > > In this case, there shouldn't be a problem as the kernel parameters
> > > would further limit the keys usage.
> > >
> > > Mimi
> > 
> > Josh probably means that it can be removed and restriction is lifted..
> > And after reboot, all keys come to the keyring..
> 
> Right.  Or if we went with your suggestion of the default being "do not
> load UEFI keys", then they could conversely add the parameter instead.
> This might not be an immediate threat to the SB attack vector itself
> (thought it could be if I thought about it harder), but it's certainly
> a change in system behavior that would catch a user unaware.

Agreed, it could catch the user unaware of the change, but always
allowing all the UEFI keys, is not a better alternative.  Perhaps,
requiring the option, would at least prevent the user from being unaware
of the change.

Mimi

> At any rate, I'm likely not the best person to weigh in on this aspect.
> Matthew has certainly done more thinking about these kinds of problems.
> 
> josh

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Matthew Garrett]

On Tue, Jun 10, 2014 at 03:58:54PM +0300, Dmitry Kasatkin wrote:

> It is tricky issue. But yes and no... If I forced to trust MS key to run
> SHIM, it does not mean
> that I want to trust MS key to run kernel and load modules or use MS key
> to valid other keys on system keyring.

A kernel parameter that refuses to trust the contents of DB would be 
acceptable, but DBX needs to be used unconditionally.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

On 10 June 2014 15:20, Josh Boyer <jwboyer@redhat.com> wrote:
> On Tue, Jun 10, 2014 at 11:48:14AM +0300, Dmitry Kasatkin wrote:
>> Hi Mimi,
>>
>> As you asked ofline , here is possible equivalent and simpler alternative
>> patches not requiring to have additional keyring.
>>
>> First patch are irrelevant minor fixes.
>>
>> Also I want to discuss here Fedora UEFI patches as they are the reason for
>> the these original patchset.
>>
>> http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
>>
>> They provide functionality to specify MokIgnoreDb variable to limit loading of
>> UEFI keys only from MOK List, while ignoring DB. This is certainly a good
>> functionality. But once MODULE_SIG_UEFI is enabled, it looks there is no way
>> to prevent loading keys from UEFI at all. And this might not be a good default
>> functionality. Someone might want not allow loading of keys from UEFI unless
>> kernel parameter is specified to allow it without recompiling the kernel
>> and disabling MODULE_SIG_UEFI.
>>
>> Josh, why such design decision was made?
>
> IIRC, it's because kernel parameters can be added programmatically from a
> remote user if they gain root access.  Having a kernel parameter to
> disable a key piece of secure boot isn't all that great.  We disable
> other kernel parameters like acpi_rspd as well.
>

(repost in plain text)

It just was in my mind. And Mathew uncovered it.

Parameter does not disable security....

Preventing loading keys from uefi except dbx by default actually
improves security.
Adding kernel parameter to read db we make system more vulnerable.

So default should be to read dbx but not db.
Based on kernel parameter to read also db.

- Dmitry

>> Why not to provide kernel parameter to have more fine-tune control over the
>> functionality? Unconfigured machines will not have MokIgnoreDb and will
>> allow to load kernel modules signed with certain undesired keys. In fact,
>
> Undesired by whom?  If SB is enabled, your machine's firmware already
> trusts those keys.
>
>> I beleive, it should be default behavior of the kernel. Bootloader can
>> enable UEFI functionality by specifing it on the kernel command line.
>
> If it was enabled via boot params, or done in the early setup code that
> might be possible.  I don't think a kernel parameter is the right
> solution though.  I've added Matthew on CC.
>
> josh
>
>> Second patch allows to overcome keys coming from UEFI for key validation by
>> specifing owner key id and is an alternative for v5 4/4 patch.
>>
>> It was also a good idea presented in Mimi's v4 4/4 patch to have possibility
>> to limit key trust valiation by only builtin keys. Third patch as an alternative.
>> It uses keys->flags to specify origin of the key, but any additional field could
>> be added as well.
>>
>> Both key id and origin verification is done in x509_validate_trust().
>>
>> Thanks,
>> Dmitry
>>
>> Dmitry Kasatkin (3):
>>   KEYS: fix couple of things
>>   KEYS: validate key trust only with selected owner key
>>   KEYS: validate key trust only with builtin keys
>>
>> Mimi Zohar (1):
>>   KEYS: define an owner trusted keyring
>>
>>  Documentation/kernel-parameters.txt      |  5 +++++
>>  crypto/asymmetric_keys/x509_public_key.c | 35 +++++++++++++++++++++++++++++---
>>  include/linux/key.h                      |  1 +
>>  kernel/system_keyring.c                  |  1 +
>>  4 files changed, 39 insertions(+), 3 deletions(-)
>>
>> --
>> 1.9.1
>>



-- 
Thanks,
Dmitry

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Matthew Garrett]

On Tue, Jun 10, 2014 at 11:34:17PM +0300, Dmitry Kasatkin wrote:

> Preventing loading keys from uefi except dbx by default actually improves
> security. Adding kernel parameter to read db we make system more
> vulnerable.

It only adds security if you're performing a measured boot and remote 
attestation. Otherwise you implicitly trust that key anyway. In almost 
all cases refusing to trust db gives you a false sense of security 
without any real improvement. I don't think it's obvious it should be 
the default.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

On 10 June 2014 23:40, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Jun 10, 2014 at 11:34:17PM +0300, Dmitry Kasatkin wrote:
>
>> Preventing loading keys from uefi except dbx by default actually improves
>> security. Adding kernel parameter to read db we make system more
>> vulnerable.
>
> It only adds security if you're performing a measured boot and remote
> attestation. Otherwise you implicitly trust that key anyway. In almost
> all cases refusing to trust db gives you a false sense of security
> without any real improvement. I don't think it's obvious it should be
> the default.
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org

May be you are right... "in almost all cases"...

It does not mater if one trust DB or not... It's all about
distro/system configuration...

Normal user even will not know what is default behavior and what
kernel parameter disables or enables...
And distro will have it by default or will use kernel parameter... It
does not change anything...

I am just discussing kernel configuration...
Without kind of looking to it  I cannot be sure if UEFI keys will
appear on system keyring or not.
Now I have to be aware how kernel is compiled... If it is compiled
with CONFIG_KEYS_UEFI or so
I need to remember may be to supply addition kernel parameters to
limit key UEFI usage...

It is may be not a big deal...

-- 
Thanks,
Dmitry

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

On 11 June 2014 00:00, Dmitry Kasatkin <dmitry.kasatkin@gmail.com> wrote:
> On 10 June 2014 23:40, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> On Tue, Jun 10, 2014 at 11:34:17PM +0300, Dmitry Kasatkin wrote:
>>
>>> Preventing loading keys from uefi except dbx by default actually improves
>>> security. Adding kernel parameter to read db we make system more
>>> vulnerable.
>>
>> It only adds security if you're performing a measured boot and remote
>> attestation. Otherwise you implicitly trust that key anyway. In almost
>> all cases refusing to trust db gives you a false sense of security
>> without any real improvement. I don't think it's obvious it should be
>> the default.
>>
>> --
>> Matthew Garrett | mjg59@srcf.ucam.org
>
> May be you are right... "in almost all cases"...
>
> It does not mater if one trust DB or not... It's all about
> distro/system configuration...
>
> Normal user even will not know what is default behavior and what
> kernel parameter disables or enables...
> And distro will have it by default or will use kernel parameter... It
> does not change anything...
>
> I am just discussing kernel configuration...
> Without kind of looking to it  I cannot be sure if UEFI keys will
> appear on system keyring or not.
> Now I have to be aware how kernel is compiled... If it is compiled
> with CONFIG_KEYS_UEFI or so
> I need to remember may be to supply addition kernel parameters to
> limit key UEFI usage...
>
> It is may be not a big deal...
>
> --
> Thanks,
> Dmitry


It is probably just a paranoia...
Kconfig MODULE_SIG_UEFI should tell about threat of loading kernel
modules from NSA or Lenovo signed by MS or Lenovo keys..

This hole is opened without warning...

:)


-- 
Thanks,
Dmitry

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Matthew Garrett]

On Wed, Jun 11, 2014 at 12:17:53AM +0300, Dmitry Kasatkin wrote:

> It is probably just a paranoia...
> Kconfig MODULE_SIG_UEFI should tell about threat of loading kernel
> modules from NSA or Lenovo signed by MS or Lenovo keys..
>
> This hole is opened without warning...

It's not typically a hole. If an attacker has root they can just replace 
your bootloader with one signed by a trusted key and then have that 
modify the kernel before booting it.

If you're using a TPM then you can mitigate this, but if you have a TPM 
then you're already performing some extra steps during the boot process. 
Just add a sysfs knob that lets you drop the db keys and incorporate 
that into the TPM management code.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

On 11 June 2014 00:25, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Wed, Jun 11, 2014 at 12:17:53AM +0300, Dmitry Kasatkin wrote:
>
>> It is probably just a paranoia...
>> Kconfig MODULE_SIG_UEFI should tell about threat of loading kernel
>> modules from NSA or Lenovo signed by MS or Lenovo keys..
>>
>> This hole is opened without warning...
>
> It's not typically a hole. If an attacker has root they can just replace
> your bootloader with one signed by a trusted key and then have that
> modify the kernel before booting it.
>
> If you're using a TPM then you can mitigate this, but if you have a TPM
> then you're already performing some extra steps during the boot process.
> Just add a sysfs knob that lets you drop the db keys and incorporate
> that into the TPM management code.
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org

I was expecting this boot loader answer.

Indeed, if system is design to prevent online modification of bootloader then
kernel parameters are protected as well...

My statement is still valid. It is a hole...

To prevent the hole it should be explained that one might follow
certain instructions
to take ownership of your PC. Generate your own keys and remove MS and
Vendor ones...

It is paranoia? May be not.

- Dmitry

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Matthew Garrett]

On Wed, Jun 11, 2014 at 12:34:28AM +0300, Dmitry Kasatkin wrote:

> My statement is still valid. It is a hole...
> 
> To prevent the hole it should be explained that one might follow
> certain instructions
> to take ownership of your PC. Generate your own keys and remove MS and
> Vendor ones...

The hole is that the system trusts keys that you don't trust. The 
appropriate thing to do is to remove that trust from the entire system, 
not just one layer of the system. If people gain the impression that 
they can simply pass a kernel parameter and avoid trusting the vendor 
keys, they'll be upset to discover that it's easily circumvented.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

On 11 June 2014 00:34, Dmitry Kasatkin <dmitry.kasatkin@gmail.com> wrote:
> On 11 June 2014 00:25, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> On Wed, Jun 11, 2014 at 12:17:53AM +0300, Dmitry Kasatkin wrote:
>>
>>> It is probably just a paranoia...
>>> Kconfig MODULE_SIG_UEFI should tell about threat of loading kernel
>>> modules from NSA or Lenovo signed by MS or Lenovo keys..
>>>
>>> This hole is opened without warning...
>>
>> It's not typically a hole. If an attacker has root they can just replace
>> your bootloader with one signed by a trusted key and then have that
>> modify the kernel before booting it.
>>
>> If you're using a TPM then you can mitigate this, but if you have a TPM
>> then you're already performing some extra steps during the boot process.
>> Just add a sysfs knob that lets you drop the db keys and incorporate
>> that into the TPM management code.
>>
>> --
>> Matthew Garrett | mjg59@srcf.ucam.org
>
> I was expecting this boot loader answer.
>
> Indeed, if system is design to prevent online modification of bootloader then
> kernel parameters are protected as well...
>
> My statement is still valid. It is a hole...
>
> To prevent the hole it should be explained that one might follow
> certain instructions
> to take ownership of your PC. Generate your own keys and remove MS and
> Vendor ones...
>
> It is paranoia? May be not.
>
> - Dmitry

I must admit that bootloader replacement is not related to kernel...

It is just paranoia...

- dmitry

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Dmitry Kasatkin]

On 11 June 2014 00:40, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Wed, Jun 11, 2014 at 12:34:28AM +0300, Dmitry Kasatkin wrote:
>
>> My statement is still valid. It is a hole...
>>
>> To prevent the hole it should be explained that one might follow
>> certain instructions
>> to take ownership of your PC. Generate your own keys and remove MS and
>> Vendor ones...
>
> The hole is that the system trusts keys that you don't trust. The
> appropriate thing to do is to remove that trust from the entire system,
> not just one layer of the system. If people gain the impression that
> they can simply pass a kernel parameter and avoid trusting the vendor
> keys, they'll be upset to discover that it's easily circumvented.
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org

Yes. There is no reason to trust anything except your own...
Vendor keys, like Lenovo and OS keys like MS are not trusted.

They need to be completely replaced.

Probably we share the same view but talk about a bit different things..

- Dmitry

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Mimi Zohar]

On Tue, 2014-06-10 at 22:40 +0100, Matthew Garrett wrote: 
> On Wed, Jun 11, 2014 at 12:34:28AM +0300, Dmitry Kasatkin wrote:
> 
> > My statement is still valid. It is a hole...
> > 
> > To prevent the hole it should be explained that one might follow
> > certain instructions
> > to take ownership of your PC. Generate your own keys and remove MS and
> > Vendor ones...
> 
> The hole is that the system trusts keys that you don't trust. The 
> appropriate thing to do is to remove that trust from the entire system, 
> not just one layer of the system. If people gain the impression that 
> they can simply pass a kernel parameter and avoid trusting the vendor 
> keys, they'll be upset to discover that it's easily circumvented.

Assuming I remove all the keys I don't trust, there are still keys that
are trusted while booting, but are not necessary afterwards.  We should
be able to limit the scope of where and when keys are trusted.

Mimi

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Matthew Garrett]

On Tue, Jun 10, 2014 at 09:24:53PM -0400, Mimi Zohar wrote:
> On Tue, 2014-06-10 at 22:40 +0100, Matthew Garrett wrote: 
> > The hole is that the system trusts keys that you don't trust. The 
> > appropriate thing to do is to remove that trust from the entire system, 
> > not just one layer of the system. If people gain the impression that 
> > they can simply pass a kernel parameter and avoid trusting the vendor 
> > keys, they'll be upset to discover that it's easily circumvented.
> 
> Assuming I remove all the keys I don't trust, there are still keys that
> are trusted while booting, but are not necessary afterwards.  We should
> be able to limit the scope of where and when keys are trusted.

Providing a userspace mechanism for selectively dropping keys from the 
kernel seems like a good thing?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Mimi Zohar]

On Wed, 2014-06-11 at 03:22 +0100, Matthew Garrett wrote: 
> On Tue, Jun 10, 2014 at 09:24:53PM -0400, Mimi Zohar wrote:
> > On Tue, 2014-06-10 at 22:40 +0100, Matthew Garrett wrote: 
> > > The hole is that the system trusts keys that you don't trust. The 
> > > appropriate thing to do is to remove that trust from the entire system, 
> > > not just one layer of the system. If people gain the impression that 
> > > they can simply pass a kernel parameter and avoid trusting the vendor 
> > > keys, they'll be upset to discover that it's easily circumvented.
> > 
> > Assuming I remove all the keys I don't trust, there are still keys that
> > are trusted while booting, but are not necessary afterwards.  We should
> > be able to limit the scope of where and when keys are trusted.
> 
> Providing a userspace mechanism for selectively dropping keys from the 
> kernel seems like a good thing?

No, patch "KEYS: verify a certificate is signed by a 'trusted' key" adds
signed public keys.

Mimi

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Matthew Garrett]

On Tue, Jun 10, 2014 at 11:08:15PM -0400, Mimi Zohar wrote:
> On Wed, 2014-06-11 at 03:22 +0100, Matthew Garrett wrote: 
> > Providing a userspace mechanism for selectively dropping keys from the 
> > kernel seems like a good thing?
> 
> No, patch "KEYS: verify a certificate is signed by a 'trusted' key" adds
> signed public keys.

Yes. Wouldn't having a mechanism to allow userspace to drop keys that 
have otherwise been imported be a generally useful solution to the issue 
you have with that?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Mimi Zohar]

On Wed, 2014-06-11 at 04:23 +0100, Matthew Garrett wrote: 
> Yes. Wouldn't having a mechanism to allow userspace to drop keys that 
> have otherwise been imported be a generally useful solution to the issue 
> you have with that?

There are issues removing a key from both the local system(eg. cache,
running apps) and the attestation server (eg. ima-sig template
verification) perspectives.  We would need to address these concerns
before supporting key removal.

Mimi

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Matthew Garrett]

On Wed, Jun 11, 2014 at 08:30:20AM -0400, Mimi Zohar wrote:
> On Wed, 2014-06-11 at 04:23 +0100, Matthew Garrett wrote: 
> > Yes. Wouldn't having a mechanism to allow userspace to drop keys that 
> > have otherwise been imported be a generally useful solution to the issue 
> > you have with that?
> 
> There are issues removing a key from both the local system(eg. cache,
> running apps) and the attestation server (eg. ima-sig template
> verification) perspectives.  We would need to address these concerns
> before supporting key removal.

For your use-case you'd want to do this before running any significant 
quantity of userspace. The firmware keys are going to be hashed into PCR 
7, so they're already part of your attestation.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[Mimi Zohar]

On Tue, 2014-06-10 at 15:49 +0300, Dmitry Kasatkin wrote: 
> On 10/06/14 15:45, Mimi Zohar wrote:
> > On Tue, 2014-06-10 at 11:48 +0300, Dmitry Kasatkin wrote: 
> >> Hi Mimi,
> >>
> >> As you asked ofline , here is possible equivalent and simpler alternative
> >> patches not requiring to have additional keyring.
> > Please indicate which branch these patches apply to.
> >
> > thanks,
> >
> > Mimi
> >
> >
> 
> on the top of your tree #next-trusted-keys-experimental, excluding 1/4
> patch, which is yours.
> I included it by mistake because patches were on the top of that and I
> used HEAD~3 instead of HEAD~2 to send them...

This patch set removes the separate 'owner' trusted keyring.  Please
rebase your patches without the HEAD patch - "f5b4ced KEYS: define an
owner trusted keyring" and re-post.

thanks,

Mimi

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Vivek Goyal]

On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
> This patch provides kernel parameter to specify owner's key id which
> must be used for trust validate of keys. Keys signed with other keys
> are not trusted.
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>

Hi,

I am continuing to work on verifying kernel signature for kexec/kdump. I 
am planning to take david howell's patches for pkcs7 signature
verification and verify bzImage signature.

Part of that process will boil down to verifying a certificate in
pkcs7 x509 cert chain using a key in system_trusted_keyring.

I think the OS vendor key which signs the kernel signing key propagates to
system_trusted_keyring. (shim has that and I am not sure how shim makes
it propogate all they way to system_trusted_keyring).

So I was planning to use same functionality where I look for any key
which can verify the signing cert of kernel. As OS vendor key will be
in system_trusted_keyring, it should work.

Now with this change where you will trust only one selected owner key.
That means you will not even trust the OS vendor key which signs kernel
signing key. I think this will stop working with  keys_ownerid=<....>

As I am doing that work in parallel and I saw these patches, I thought
I will bring it up.

Thanks
Vivek

> ---
>  crypto/asymmetric_keys/x509_public_key.c | 27 ++++++++--
>  include/keys/owner_keyring.h             | 27 ----------
>  init/Kconfig                             | 10 ----
>  kernel/Makefile                          |  1 -
>  kernel/owner_keyring.c                   | 85 --------------------------------
>  5 files changed, 24 insertions(+), 126 deletions(-)
>  delete mode 100644 include/keys/owner_keyring.h
>  delete mode 100644 kernel/owner_keyring.c
> 
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index 962f9b9..d46b790 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -19,12 +19,24 @@
>  #include <keys/asymmetric-subtype.h>
>  #include <keys/asymmetric-parser.h>
>  #include <keys/system_keyring.h>
> -#include <keys/owner_keyring.h>
>  #include <crypto/hash.h>
>  #include "asymmetric_keys.h"
>  #include "public_key.h"
>  #include "x509_parser.h"
>  
> +static char *owner_keyid;
> +static int __init default_owner_keyid_set(char *str)
> +{
> +	if (!str)		/* default system keyring */
> +		return 1;
> +
> +	if (strncmp(str, "id:", 3) == 0)
> +		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
> +
> +	return 1;
> +}
> +__setup("keys_ownerid=", default_owner_keyid_set);
> +
>  /*
>   * Find a key in the given keyring by issuer and authority.
>   */
> @@ -170,6 +182,16 @@ static int x509_validate_trust(struct x509_certificate *cert,
>  	if (!trust_keyring)
>  		return -EOPNOTSUPP;
>  
> +	if (owner_keyid) {
> +		/* validate trust only with the owner_keyid if specified */
> +		/* partial match of keyid according to the asymmetric_type.c */
> +		int idlen = strlen(owner_keyid) - 3; /* - id: */
> +		int authlen = strlen(cert->authority);
> +		char *auth = cert->authority + authlen - idlen;
> +		if (idlen > authlen || strcasecmp(owner_keyid + 3, auth))
> +			return -EPERM;
> +	}
> +
>  	key = x509_request_asymmetric_key(trust_keyring,
>  					  cert->issuer, strlen(cert->issuer),
>  					  cert->authority,
> @@ -239,8 +261,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>  		if (ret < 0)
>  			goto error_free_cert;
>  	} else if (!prep->trusted) {
> -		ret = x509_validate_trust(cert,
> -					  get_system_or_owner_trusted_keyring());
> +		ret = x509_validate_trust(cert, get_system_trusted_keyring());
>  		if (!ret)
>  			prep->trusted = 1;
>  	}
> diff --git a/include/keys/owner_keyring.h b/include/keys/owner_keyring.h
> deleted file mode 100644
> index 78dd09d..0000000
> --- a/include/keys/owner_keyring.h
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -/* 
> - * Copyright (C) 2014 IBM Corporation
> - * Author: Mimi Zohar <zohar@us.ibm.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation, version 2 of the License.
> - */
> -
> -#ifndef _KEYS_OWNER_KEYRING_H
> -#define _KEYS_OWNER_KEYRING_H
> -
> -#ifdef CONFIG_OWNER_TRUSTED_KEYRING
> -
> -#include <linux/key.h>
> -
> -extern struct key *owner_trusted_keyring;
> -extern struct key *get_system_or_owner_trusted_keyring(void);
> -
> -#else
> -static inline struct key *get_system_or_owner_trusted_keyring(void)
> -{
> -	return get_system_trusted_keyring();
> -}
> -
> -#endif
> -#endif /* _KEYS_OWNER_KEYRING_H */
> diff --git a/init/Kconfig b/init/Kconfig
> index 7876787..009a797 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1661,16 +1661,6 @@ config SYSTEM_TRUSTED_KEYRING
>  
>  	  Keys in this keyring are used by module signature checking.
>  
> -config OWNER_TRUSTED_KEYRING
> -	bool "Verify certificate signatures using a specific system key"
> -	depends on SYSTEM_TRUSTED_KEYRING
> -	help
> -	  Verify a certificate's signature, before adding the key to
> -	  a trusted keyring, using a specific key on the system trusted
> -	  keyring.  The specific key on the system trusted keyring is
> -	  identified using the kernel boot command line option
> -	  "keys_ownerid" and is added to the owner_trusted_keyring.
> -
>  menuconfig MODULES
>  	bool "Enable loadable module support"
>  	option modules
> diff --git a/kernel/Makefile b/kernel/Makefile
> index 7b44efd..bc010ee 100644
> --- a/kernel/Makefile
> +++ b/kernel/Makefile
> @@ -44,7 +44,6 @@ obj-$(CONFIG_UID16) += uid16.o
>  obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
>  obj-$(CONFIG_MODULES) += module.o
>  obj-$(CONFIG_MODULE_SIG) += module_signing.o
> -obj-$(CONFIG_OWNER_TRUSTED_KEYRING) += owner_keyring.o
>  obj-$(CONFIG_KALLSYMS) += kallsyms.o
>  obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
>  obj-$(CONFIG_KEXEC) += kexec.o
> diff --git a/kernel/owner_keyring.c b/kernel/owner_keyring.c
> deleted file mode 100644
> index a31b865..0000000
> --- a/kernel/owner_keyring.c
> +++ /dev/null
> @@ -1,85 +0,0 @@
> -/* 
> - * Copyright (C) 2014 IBM Corporation
> - * Author: Mimi Zohar <zohar@us.ibm.com>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation, version 2 of the License.
> - */
> -
> -#include <linux/export.h>
> -#include <linux/kernel.h>
> -#include <linux/sched.h>
> -#include <linux/cred.h>
> -#include <linux/err.h>
> -#include <keys/asymmetric-type.h>
> -#include <keys/system_keyring.h>
> -#include "module-internal.h"
> -
> -struct key *owner_trusted_keyring;
> -static int use_owner_trusted_keyring;
> -
> -static char *owner_keyid;
> -static int __init default_owner_keyid_set(char *str)
> -{
> -	if (!str)		/* default system keyring */
> -		return 1;
> -
> -	if (strncmp(str, "id:", 3) == 0)
> -		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
> -
> -	return 1;
> -}
> -
> -__setup("keys_ownerid=", default_owner_keyid_set);
> -
> -struct key *get_system_or_owner_trusted_keyring(void)
> -{
> -	return use_owner_trusted_keyring ? owner_trusted_keyring :
> -	    get_system_trusted_keyring();
> -}
> -
> -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");
> -
> -	set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
> -	return 0;
> -}
> -
> -device_initcall(owner_trusted_keyring_init);
> -
> -void load_owner_identified_key(void)
> -{
> -	key_ref_t key_ref;
> -	int ret;
> -
> -	if (!owner_keyid)
> -		return;
> -
> -	key_ref = keyring_search(make_key_ref(system_trusted_keyring, 1),
> -				 &key_type_asymmetric, owner_keyid);
> -	if (IS_ERR(key_ref)) {
> -		pr_warn("Request for unknown %s key\n", owner_keyid);
> -		goto out;
> -	}
> -	ret = key_link(owner_trusted_keyring, key_ref_to_ptr(key_ref));
> -	pr_info("Loaded owner key %s %s\n", owner_keyid,
> -		ret < 0 ? "failed" : "succeeded");
> -	key_ref_put(key_ref);
> -	if (!ret)
> -		use_owner_trusted_keyring = 1;
> -out:
> -	return;
> -}
> -
> -late_initcall(load_owner_identified_key);
> -- 
> 1.9.1
> 
> --
> 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

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Mimi Zohar]

On Thu, 2014-06-12 at 12:03 -0400, Vivek Goyal wrote: 
> On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
> > This patch provides kernel parameter to specify owner's key id which
> > must be used for trust validate of keys. Keys signed with other keys
> > are not trusted.
> > 
> > Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> 
> Hi,
> 
> I am continuing to work on verifying kernel signature for kexec/kdump. I 
> am planning to take david howell's patches for pkcs7 signature
> verification and verify bzImage signature.
> 
> Part of that process will boil down to verifying a certificate in
> pkcs7 x509 cert chain using a key in system_trusted_keyring.
> 
> I think the OS vendor key which signs the kernel signing key propagates to
> system_trusted_keyring. (shim has that and I am not sure how shim makes
> it propogate all they way to system_trusted_keyring).

The shim patches are here
http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch

> So I was planning to use same functionality where I look for any key
> which can verify the signing cert of kernel. As OS vendor key will be
> in system_trusted_keyring, it should work.
> 
> Now with this change where you will trust only one selected owner key.
> That means you will not even trust the OS vendor key which signs kernel
> signing key. I think this will stop working with  keys_ownerid=<....>
> 
> As I am doing that work in parallel and I saw these patches, I thought
> I will bring it up.

Right, the current discussion is whether we need an owner trusted
keyring or if just one key was enough.  Thanks for chiming in.

The other option would be to sign the bzImage file creating a
'security.ima' extended attribute and verifying it.  Have you created a
security kexec hook?

thanks,

Mimi

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Vivek Goyal]

On Thu, Jun 12, 2014 at 12:55:26PM -0400, Mimi Zohar wrote:
> On Thu, 2014-06-12 at 12:03 -0400, Vivek Goyal wrote: 
> > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
> > > This patch provides kernel parameter to specify owner's key id which
> > > must be used for trust validate of keys. Keys signed with other keys
> > > are not trusted.
> > > 
> > > Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > 
> > Hi,
> > 
> > I am continuing to work on verifying kernel signature for kexec/kdump. I 
> > am planning to take david howell's patches for pkcs7 signature
> > verification and verify bzImage signature.
> > 
> > Part of that process will boil down to verifying a certificate in
> > pkcs7 x509 cert chain using a key in system_trusted_keyring.
> > 
> > I think the OS vendor key which signs the kernel signing key propagates to
> > system_trusted_keyring. (shim has that and I am not sure how shim makes
> > it propogate all they way to system_trusted_keyring).
> 
> The shim patches are here
> http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
> 
> > So I was planning to use same functionality where I look for any key
> > which can verify the signing cert of kernel. As OS vendor key will be
> > in system_trusted_keyring, it should work.
> > 
> > Now with this change where you will trust only one selected owner key.
> > That means you will not even trust the OS vendor key which signs kernel
> > signing key. I think this will stop working with  keys_ownerid=<....>
> > 
> > As I am doing that work in parallel and I saw these patches, I thought
> > I will bring it up.
> 
> Right, the current discussion is whether we need an owner trusted
> keyring or if just one key was enough.  Thanks for chiming in.
> 
> The other option would be to sign the bzImage file creating a
> 'security.ima' extended attribute and verifying it.  Have you created a
> security kexec hook?

No, I have not created another hook. As bzImage is already signed it is
much simpler to verify same signature instead of carrying another set
of detached signatures and key management etc.

Thanks
Vivek

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Mimi Zohar]

On Thu, 2014-06-12 at 13:00 -0400, Vivek Goyal wrote: 
> On Thu, Jun 12, 2014 at 12:55:26PM -0400, Mimi Zohar wrote:
> > On Thu, 2014-06-12 at 12:03 -0400, Vivek Goyal wrote: 
> > > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
> > > > This patch provides kernel parameter to specify owner's key id which
> > > > must be used for trust validate of keys. Keys signed with other keys
> > > > are not trusted.
> > > > 
> > > > Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > > 
> > > Hi,
> > > 
> > > I am continuing to work on verifying kernel signature for kexec/kdump. I 
> > > am planning to take david howell's patches for pkcs7 signature
> > > verification and verify bzImage signature.
> > > 
> > > Part of that process will boil down to verifying a certificate in
> > > pkcs7 x509 cert chain using a key in system_trusted_keyring.
> > > 
> > > I think the OS vendor key which signs the kernel signing key propagates to
> > > system_trusted_keyring. (shim has that and I am not sure how shim makes
> > > it propogate all they way to system_trusted_keyring).
> > 
> > The shim patches are here
> > http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
> > 
> > > So I was planning to use same functionality where I look for any key
> > > which can verify the signing cert of kernel. As OS vendor key will be
> > > in system_trusted_keyring, it should work.
> > > 
> > > Now with this change where you will trust only one selected owner key.
> > > That means you will not even trust the OS vendor key which signs kernel
> > > signing key. I think this will stop working with  keys_ownerid=<....>
> > > 
> > > As I am doing that work in parallel and I saw these patches, I thought
> > > I will bring it up.
> > 
> > Right, the current discussion is whether we need an owner trusted
> > keyring or if just one key was enough.  Thanks for chiming in.
> > 
> > The other option would be to sign the bzImage file creating a
> > 'security.ima' extended attribute and verifying it.  Have you created a
> > security kexec hook?
> 
> No, I have not created another hook. As bzImage is already signed it is
> much simpler to verify same signature instead of carrying another set
> of detached signatures and key management etc.

Fin (cc'ed) has patches that include the file signatures in the RPM
header and installs them.  There wouldn't be any need for a separate
detached signature.

Mimi

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Vivek Goyal]

On Thu, Jun 12, 2014 at 01:17:03PM -0400, Mimi Zohar wrote:
> On Thu, 2014-06-12 at 13:00 -0400, Vivek Goyal wrote: 
> > On Thu, Jun 12, 2014 at 12:55:26PM -0400, Mimi Zohar wrote:
> > > On Thu, 2014-06-12 at 12:03 -0400, Vivek Goyal wrote: 
> > > > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
> > > > > This patch provides kernel parameter to specify owner's key id which
> > > > > must be used for trust validate of keys. Keys signed with other keys
> > > > > are not trusted.
> > > > > 
> > > > > Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > > > 
> > > > Hi,
> > > > 
> > > > I am continuing to work on verifying kernel signature for kexec/kdump. I 
> > > > am planning to take david howell's patches for pkcs7 signature
> > > > verification and verify bzImage signature.
> > > > 
> > > > Part of that process will boil down to verifying a certificate in
> > > > pkcs7 x509 cert chain using a key in system_trusted_keyring.
> > > > 
> > > > I think the OS vendor key which signs the kernel signing key propagates to
> > > > system_trusted_keyring. (shim has that and I am not sure how shim makes
> > > > it propogate all they way to system_trusted_keyring).
> > > 
> > > The shim patches are here
> > > http://pkgs.fedoraproject.org/cgit/kernel.git/tree/modsign-uefi.patch
> > > 
> > > > So I was planning to use same functionality where I look for any key
> > > > which can verify the signing cert of kernel. As OS vendor key will be
> > > > in system_trusted_keyring, it should work.
> > > > 
> > > > Now with this change where you will trust only one selected owner key.
> > > > That means you will not even trust the OS vendor key which signs kernel
> > > > signing key. I think this will stop working with  keys_ownerid=<....>
> > > > 
> > > > As I am doing that work in parallel and I saw these patches, I thought
> > > > I will bring it up.
> > > 
> > > Right, the current discussion is whether we need an owner trusted
> > > keyring or if just one key was enough.  Thanks for chiming in.
> > > 
> > > The other option would be to sign the bzImage file creating a
> > > 'security.ima' extended attribute and verifying it.  Have you created a
> > > security kexec hook?
> > 
> > No, I have not created another hook. As bzImage is already signed it is
> > much simpler to verify same signature instead of carrying another set
> > of detached signatures and key management etc.
> 
> Fin (cc'ed) has patches that include the file signatures in the RPM
> header and installs them.  There wouldn't be any need for a separate
> detached signature.

Currently distributions are already shipping bzImage signed with PE/COFF
signature. If we go this way, now they will have to sign bzImage twice.
And this time in IMA format and pack signatures in rpm. Managing two
different signatures for same bzImage is just going to be an unnecessary
headache for everybody, IMHO.

I am wondering how about coming up with x509_validate_trust_key() where
you specify that validate trust against a specific key (instead of
keyring). Another function will be x509_validate_trust() which will take
keyring as input and will trust all keys in keyring.

As I am not adding a key, I can just use x509_validate_trust(system_keyring)
for signature verification and in key add path you can continue to
use x509_validate_trust_key() if somebody used command line keys_ownerid=.

Thanks
Vivek

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Dmitry Kasatkin]

On 12/06/14 19:03, Vivek Goyal wrote:
> On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
>> This patch provides kernel parameter to specify owner's key id which
>> must be used for trust validate of keys. Keys signed with other keys
>> are not trusted.
>>
>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> Hi,
>
> I am continuing to work on verifying kernel signature for kexec/kdump. I 
> am planning to take david howell's patches for pkcs7 signature
> verification and verify bzImage signature.
>
> Part of that process will boil down to verifying a certificate in
> pkcs7 x509 cert chain using a key in system_trusted_keyring.
>
> I think the OS vendor key which signs the kernel signing key propagates to
> system_trusted_keyring. (shim has that and I am not sure how shim makes
> it propogate all they way to system_trusted_keyring).
>
> So I was planning to use same functionality where I look for any key
> which can verify the signing cert of kernel. As OS vendor key will be
> in system_trusted_keyring, it should work.
>
> Now with this change where you will trust only one selected owner key.
> That means you will not even trust the OS vendor key which signs kernel
> signing key. I think this will stop working with  keys_ownerid=<....>
>
> As I am doing that work in parallel and I saw these patches, I thought
> I will bring it up.

Hi Vivek,

All keys stays in the keyring. Usage of owner_keyid is limited to
validate the trust of the loaded keys.


Do you really see OS verndor key (Fedora) on the system keyring?
shim is UEFI binary and can add it to the MOK database..

- Dmitry


> Thanks
> Vivek
>
>> ---
>>  crypto/asymmetric_keys/x509_public_key.c | 27 ++++++++--
>>  include/keys/owner_keyring.h             | 27 ----------
>>  init/Kconfig                             | 10 ----
>>  kernel/Makefile                          |  1 -
>>  kernel/owner_keyring.c                   | 85 --------------------------------
>>  5 files changed, 24 insertions(+), 126 deletions(-)
>>  delete mode 100644 include/keys/owner_keyring.h
>>  delete mode 100644 kernel/owner_keyring.c
>>
>> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
>> index 962f9b9..d46b790 100644
>> --- a/crypto/asymmetric_keys/x509_public_key.c
>> +++ b/crypto/asymmetric_keys/x509_public_key.c
>> @@ -19,12 +19,24 @@
>>  #include <keys/asymmetric-subtype.h>
>>  #include <keys/asymmetric-parser.h>
>>  #include <keys/system_keyring.h>
>> -#include <keys/owner_keyring.h>
>>  #include <crypto/hash.h>
>>  #include "asymmetric_keys.h"
>>  #include "public_key.h"
>>  #include "x509_parser.h"
>>  
>> +static char *owner_keyid;
>> +static int __init default_owner_keyid_set(char *str)
>> +{
>> +	if (!str)		/* default system keyring */
>> +		return 1;
>> +
>> +	if (strncmp(str, "id:", 3) == 0)
>> +		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
>> +
>> +	return 1;
>> +}
>> +__setup("keys_ownerid=", default_owner_keyid_set);
>> +
>>  /*
>>   * Find a key in the given keyring by issuer and authority.
>>   */
>> @@ -170,6 +182,16 @@ static int x509_validate_trust(struct x509_certificate *cert,
>>  	if (!trust_keyring)
>>  		return -EOPNOTSUPP;
>>  
>> +	if (owner_keyid) {
>> +		/* validate trust only with the owner_keyid if specified */
>> +		/* partial match of keyid according to the asymmetric_type.c */
>> +		int idlen = strlen(owner_keyid) - 3; /* - id: */
>> +		int authlen = strlen(cert->authority);
>> +		char *auth = cert->authority + authlen - idlen;
>> +		if (idlen > authlen || strcasecmp(owner_keyid + 3, auth))
>> +			return -EPERM;
>> +	}
>> +
>>  	key = x509_request_asymmetric_key(trust_keyring,
>>  					  cert->issuer, strlen(cert->issuer),
>>  					  cert->authority,
>> @@ -239,8 +261,7 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>>  		if (ret < 0)
>>  			goto error_free_cert;
>>  	} else if (!prep->trusted) {
>> -		ret = x509_validate_trust(cert,
>> -					  get_system_or_owner_trusted_keyring());
>> +		ret = x509_validate_trust(cert, get_system_trusted_keyring());
>>  		if (!ret)
>>  			prep->trusted = 1;
>>  	}
>> diff --git a/include/keys/owner_keyring.h b/include/keys/owner_keyring.h
>> deleted file mode 100644
>> index 78dd09d..0000000
>> --- a/include/keys/owner_keyring.h
>> +++ /dev/null
>> @@ -1,27 +0,0 @@
>> -/* 
>> - * Copyright (C) 2014 IBM Corporation
>> - * Author: Mimi Zohar <zohar@us.ibm.com>
>> - *
>> - * This program is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License as published by
>> - * the Free Software Foundation, version 2 of the License.
>> - */
>> -
>> -#ifndef _KEYS_OWNER_KEYRING_H
>> -#define _KEYS_OWNER_KEYRING_H
>> -
>> -#ifdef CONFIG_OWNER_TRUSTED_KEYRING
>> -
>> -#include <linux/key.h>
>> -
>> -extern struct key *owner_trusted_keyring;
>> -extern struct key *get_system_or_owner_trusted_keyring(void);
>> -
>> -#else
>> -static inline struct key *get_system_or_owner_trusted_keyring(void)
>> -{
>> -	return get_system_trusted_keyring();
>> -}
>> -
>> -#endif
>> -#endif /* _KEYS_OWNER_KEYRING_H */
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 7876787..009a797 100644
>> --- a/init/Kconfig
>> +++ b/init/Kconfig
>> @@ -1661,16 +1661,6 @@ config SYSTEM_TRUSTED_KEYRING
>>  
>>  	  Keys in this keyring are used by module signature checking.
>>  
>> -config OWNER_TRUSTED_KEYRING
>> -	bool "Verify certificate signatures using a specific system key"
>> -	depends on SYSTEM_TRUSTED_KEYRING
>> -	help
>> -	  Verify a certificate's signature, before adding the key to
>> -	  a trusted keyring, using a specific key on the system trusted
>> -	  keyring.  The specific key on the system trusted keyring is
>> -	  identified using the kernel boot command line option
>> -	  "keys_ownerid" and is added to the owner_trusted_keyring.
>> -
>>  menuconfig MODULES
>>  	bool "Enable loadable module support"
>>  	option modules
>> diff --git a/kernel/Makefile b/kernel/Makefile
>> index 7b44efd..bc010ee 100644
>> --- a/kernel/Makefile
>> +++ b/kernel/Makefile
>> @@ -44,7 +44,6 @@ obj-$(CONFIG_UID16) += uid16.o
>>  obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
>>  obj-$(CONFIG_MODULES) += module.o
>>  obj-$(CONFIG_MODULE_SIG) += module_signing.o
>> -obj-$(CONFIG_OWNER_TRUSTED_KEYRING) += owner_keyring.o
>>  obj-$(CONFIG_KALLSYMS) += kallsyms.o
>>  obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
>>  obj-$(CONFIG_KEXEC) += kexec.o
>> diff --git a/kernel/owner_keyring.c b/kernel/owner_keyring.c
>> deleted file mode 100644
>> index a31b865..0000000
>> --- a/kernel/owner_keyring.c
>> +++ /dev/null
>> @@ -1,85 +0,0 @@
>> -/* 
>> - * Copyright (C) 2014 IBM Corporation
>> - * Author: Mimi Zohar <zohar@us.ibm.com>
>> - *
>> - * This program is free software; you can redistribute it and/or modify
>> - * it under the terms of the GNU General Public License as published by
>> - * the Free Software Foundation, version 2 of the License.
>> - */
>> -
>> -#include <linux/export.h>
>> -#include <linux/kernel.h>
>> -#include <linux/sched.h>
>> -#include <linux/cred.h>
>> -#include <linux/err.h>
>> -#include <keys/asymmetric-type.h>
>> -#include <keys/system_keyring.h>
>> -#include "module-internal.h"
>> -
>> -struct key *owner_trusted_keyring;
>> -static int use_owner_trusted_keyring;
>> -
>> -static char *owner_keyid;
>> -static int __init default_owner_keyid_set(char *str)
>> -{
>> -	if (!str)		/* default system keyring */
>> -		return 1;
>> -
>> -	if (strncmp(str, "id:", 3) == 0)
>> -		owner_keyid = str;	/* owner local key 'id:xxxxxx' */
>> -
>> -	return 1;
>> -}
>> -
>> -__setup("keys_ownerid=", default_owner_keyid_set);
>> -
>> -struct key *get_system_or_owner_trusted_keyring(void)
>> -{
>> -	return use_owner_trusted_keyring ? owner_trusted_keyring :
>> -	    get_system_trusted_keyring();
>> -}
>> -
>> -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");
>> -
>> -	set_bit(KEY_FLAG_TRUSTED_ONLY, &owner_trusted_keyring->flags);
>> -	return 0;
>> -}
>> -
>> -device_initcall(owner_trusted_keyring_init);
>> -
>> -void load_owner_identified_key(void)
>> -{
>> -	key_ref_t key_ref;
>> -	int ret;
>> -
>> -	if (!owner_keyid)
>> -		return;
>> -
>> -	key_ref = keyring_search(make_key_ref(system_trusted_keyring, 1),
>> -				 &key_type_asymmetric, owner_keyid);
>> -	if (IS_ERR(key_ref)) {
>> -		pr_warn("Request for unknown %s key\n", owner_keyid);
>> -		goto out;
>> -	}
>> -	ret = key_link(owner_trusted_keyring, key_ref_to_ptr(key_ref));
>> -	pr_info("Loaded owner key %s %s\n", owner_keyid,
>> -		ret < 0 ? "failed" : "succeeded");
>> -	key_ref_put(key_ref);
>> -	if (!ret)
>> -		use_owner_trusted_keyring = 1;
>> -out:
>> -	return;
>> -}
>> -
>> -late_initcall(load_owner_identified_key);
>> -- 
>> 1.9.1
>>
>> --
>> 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

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Vivek Goyal]

On Thu, Jun 12, 2014 at 08:23:57PM +0300, Dmitry Kasatkin wrote:
> On 12/06/14 19:03, Vivek Goyal wrote:
> > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
> >> This patch provides kernel parameter to specify owner's key id which
> >> must be used for trust validate of keys. Keys signed with other keys
> >> are not trusted.
> >>
> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > Hi,
> >
> > I am continuing to work on verifying kernel signature for kexec/kdump. I 
> > am planning to take david howell's patches for pkcs7 signature
> > verification and verify bzImage signature.
> >
> > Part of that process will boil down to verifying a certificate in
> > pkcs7 x509 cert chain using a key in system_trusted_keyring.
> >
> > I think the OS vendor key which signs the kernel signing key propagates to
> > system_trusted_keyring. (shim has that and I am not sure how shim makes
> > it propogate all they way to system_trusted_keyring).
> >
> > So I was planning to use same functionality where I look for any key
> > which can verify the signing cert of kernel. As OS vendor key will be
> > in system_trusted_keyring, it should work.
> >
> > Now with this change where you will trust only one selected owner key.
> > That means you will not even trust the OS vendor key which signs kernel
> > signing key. I think this will stop working with  keys_ownerid=<....>
> >
> > As I am doing that work in parallel and I saw these patches, I thought
> > I will bring it up.
> 
> Hi Vivek,
> 
> All keys stays in the keyring. Usage of owner_keyid is limited to
> validate the trust of the loaded keys.

Hi Dmitry,

If owner_keyid scope is just limited to loading extra keys, then it
should be fine as I don't want to load extra keys. I just want to
verify already signed image whose key is supposed to be in
system_trusted_keyring.

> 
> 
> Do you really see OS verndor key (Fedora) on the system keyring?
> shim is UEFI binary and can add it to the MOK database..

I have been told that mechanism to propagate they key in shim which is
used to verify kernel signature is in place and that key should show
up in system_trusted_keyring. I have never verified it though. I will
check it out.

Thanks
Vivek

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Mimi Zohar]

On Thu, 2014-06-12 at 13:32 -0400, Vivek Goyal wrote: 
> On Thu, Jun 12, 2014 at 08:23:57PM +0300, Dmitry Kasatkin wrote:
> > On 12/06/14 19:03, Vivek Goyal wrote:
> > > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
> > >> This patch provides kernel parameter to specify owner's key id which
> > >> must be used for trust validate of keys. Keys signed with other keys
> > >> are not trusted.
> > >>
> > >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > > Hi,
> > >
> > > I am continuing to work on verifying kernel signature for kexec/kdump. I 
> > > am planning to take david howell's patches for pkcs7 signature
> > > verification and verify bzImage signature.
> > >
> > > Part of that process will boil down to verifying a certificate in
> > > pkcs7 x509 cert chain using a key in system_trusted_keyring.
> > >
> > > I think the OS vendor key which signs the kernel signing key propagates to
> > > system_trusted_keyring. (shim has that and I am not sure how shim makes
> > > it propogate all they way to system_trusted_keyring).
> > >
> > > So I was planning to use same functionality where I look for any key
> > > which can verify the signing cert of kernel. As OS vendor key will be
> > > in system_trusted_keyring, it should work.
> > >
> > > Now with this change where you will trust only one selected owner key.
> > > That means you will not even trust the OS vendor key which signs kernel
> > > signing key. I think this will stop working with  keys_ownerid=<....>
> > >
> > > As I am doing that work in parallel and I saw these patches, I thought
> > > I will bring it up.
> > 
> > Hi Vivek,
> > 
> > All keys stays in the keyring. Usage of owner_keyid is limited to
> > validate the trust of the loaded keys.
> 
> Hi Dmitry,
> 
> If owner_keyid scope is just limited to loading extra keys, then it
> should be fine as I don't want to load extra keys. I just want to
> verify already signed image whose key is supposed to be in
> system_trusted_keyring.

Yes, sorry my mistake.

Mimi

> > 
> > 
> > Do you really see OS verndor key (Fedora) on the system keyring?
> > shim is UEFI binary and can add it to the MOK database..
> 
> I have been told that mechanism to propagate they key in shim which is
> used to verify kernel signature is in place and that key should show
> up in system_trusted_keyring. I have never verified it though. I will
> check it out.
> 
> Thanks
> Vivek
> 

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Dmitry Kasatkin]

On 12 June 2014 20:32, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jun 12, 2014 at 08:23:57PM +0300, Dmitry Kasatkin wrote:
>> On 12/06/14 19:03, Vivek Goyal wrote:
>> > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
>> >> This patch provides kernel parameter to specify owner's key id which
>> >> must be used for trust validate of keys. Keys signed with other keys
>> >> are not trusted.
>> >>
>> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> > Hi,
>> >
>> > I am continuing to work on verifying kernel signature for kexec/kdump. I
>> > am planning to take david howell's patches for pkcs7 signature
>> > verification and verify bzImage signature.
>> >
>> > Part of that process will boil down to verifying a certificate in
>> > pkcs7 x509 cert chain using a key in system_trusted_keyring.
>> >
>> > I think the OS vendor key which signs the kernel signing key propagates to
>> > system_trusted_keyring. (shim has that and I am not sure how shim makes
>> > it propogate all they way to system_trusted_keyring).
>> >
>> > So I was planning to use same functionality where I look for any key
>> > which can verify the signing cert of kernel. As OS vendor key will be
>> > in system_trusted_keyring, it should work.
>> >
>> > Now with this change where you will trust only one selected owner key.
>> > That means you will not even trust the OS vendor key which signs kernel
>> > signing key. I think this will stop working with  keys_ownerid=<....>
>> >
>> > As I am doing that work in parallel and I saw these patches, I thought
>> > I will bring it up.
>>
>> Hi Vivek,
>>
>> All keys stays in the keyring. Usage of owner_keyid is limited to
>> validate the trust of the loaded keys.
>
> Hi Dmitry,
>
> If owner_keyid scope is just limited to loading extra keys, then it
> should be fine as I don't want to load extra keys. I just want to
> verify already signed image whose key is supposed to be in
> system_trusted_keyring.
>
>>
>>
>> Do you really see OS verndor key (Fedora) on the system keyring?
>> shim is UEFI binary and can add it to the MOK database..
>
> I have been told that mechanism to propagate they key in shim which is
> used to verify kernel signature is in place and that key should show
> up in system_trusted_keyring. I have never verified it though. I will
> check it out.
>
> Thanks
> Vivek


Hi Vivek,

The easiest way to get OS Vendor ker/certificate is just embed it into
the kernel along with
ephemeral (or not) module signing key... They all appears on .system keyring...

This solution would work also for ARM based HW which does not have
UEFI and MOK...

We also use mini-os with linux kernel as boot loader which in turn
boots other kernels.
It is for R&D purpose but with kexec signing support we could think
about other use-cases..


Thanks,

Dmitry

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Vivek Goyal]

On Thu, Jun 12, 2014 at 09:36:46PM +0300, Dmitry Kasatkin wrote:
> On 12 June 2014 20:32, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Thu, Jun 12, 2014 at 08:23:57PM +0300, Dmitry Kasatkin wrote:
> >> On 12/06/14 19:03, Vivek Goyal wrote:
> >> > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
> >> >> This patch provides kernel parameter to specify owner's key id which
> >> >> must be used for trust validate of keys. Keys signed with other keys
> >> >> are not trusted.
> >> >>
> >> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> >> > Hi,
> >> >
> >> > I am continuing to work on verifying kernel signature for kexec/kdump. I
> >> > am planning to take david howell's patches for pkcs7 signature
> >> > verification and verify bzImage signature.
> >> >
> >> > Part of that process will boil down to verifying a certificate in
> >> > pkcs7 x509 cert chain using a key in system_trusted_keyring.
> >> >
> >> > I think the OS vendor key which signs the kernel signing key propagates to
> >> > system_trusted_keyring. (shim has that and I am not sure how shim makes
> >> > it propogate all they way to system_trusted_keyring).
> >> >
> >> > So I was planning to use same functionality where I look for any key
> >> > which can verify the signing cert of kernel. As OS vendor key will be
> >> > in system_trusted_keyring, it should work.
> >> >
> >> > Now with this change where you will trust only one selected owner key.
> >> > That means you will not even trust the OS vendor key which signs kernel
> >> > signing key. I think this will stop working with  keys_ownerid=<....>
> >> >
> >> > As I am doing that work in parallel and I saw these patches, I thought
> >> > I will bring it up.
> >>
> >> Hi Vivek,
> >>
> >> All keys stays in the keyring. Usage of owner_keyid is limited to
> >> validate the trust of the loaded keys.
> >
> > Hi Dmitry,
> >
> > If owner_keyid scope is just limited to loading extra keys, then it
> > should be fine as I don't want to load extra keys. I just want to
> > verify already signed image whose key is supposed to be in
> > system_trusted_keyring.
> >
> >>
> >>
> >> Do you really see OS verndor key (Fedora) on the system keyring?
> >> shim is UEFI binary and can add it to the MOK database..
> >
> > I have been told that mechanism to propagate they key in shim which is
> > used to verify kernel signature is in place and that key should show
> > up in system_trusted_keyring. I have never verified it though. I will
> > check it out.
> >
> > Thanks
> > Vivek
> 
> 
> Hi Vivek,
> 
> The easiest way to get OS Vendor ker/certificate is just embed it into
> the kernel along with
> ephemeral (or not) module signing key... They all appears on .system keyring...

I think it will be tricky as in current setup, signing happens on a different
server and build server does not have access to keys.

Thanks
Vivek

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Dmitry Kasatkin]

On 12 June 2014 22:01, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jun 12, 2014 at 09:36:46PM +0300, Dmitry Kasatkin wrote:
>> On 12 June 2014 20:32, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Thu, Jun 12, 2014 at 08:23:57PM +0300, Dmitry Kasatkin wrote:
>> >> On 12/06/14 19:03, Vivek Goyal wrote:
>> >> > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
>> >> >> This patch provides kernel parameter to specify owner's key id which
>> >> >> must be used for trust validate of keys. Keys signed with other keys
>> >> >> are not trusted.
>> >> >>
>> >> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> >> > Hi,
>> >> >
>> >> > I am continuing to work on verifying kernel signature for kexec/kdump. I
>> >> > am planning to take david howell's patches for pkcs7 signature
>> >> > verification and verify bzImage signature.
>> >> >
>> >> > Part of that process will boil down to verifying a certificate in
>> >> > pkcs7 x509 cert chain using a key in system_trusted_keyring.
>> >> >
>> >> > I think the OS vendor key which signs the kernel signing key propagates to
>> >> > system_trusted_keyring. (shim has that and I am not sure how shim makes
>> >> > it propogate all they way to system_trusted_keyring).
>> >> >
>> >> > So I was planning to use same functionality where I look for any key
>> >> > which can verify the signing cert of kernel. As OS vendor key will be
>> >> > in system_trusted_keyring, it should work.
>> >> >
>> >> > Now with this change where you will trust only one selected owner key.
>> >> > That means you will not even trust the OS vendor key which signs kernel
>> >> > signing key. I think this will stop working with  keys_ownerid=<....>
>> >> >
>> >> > As I am doing that work in parallel and I saw these patches, I thought
>> >> > I will bring it up.
>> >>
>> >> Hi Vivek,
>> >>
>> >> All keys stays in the keyring. Usage of owner_keyid is limited to
>> >> validate the trust of the loaded keys.
>> >
>> > Hi Dmitry,
>> >
>> > If owner_keyid scope is just limited to loading extra keys, then it
>> > should be fine as I don't want to load extra keys. I just want to
>> > verify already signed image whose key is supposed to be in
>> > system_trusted_keyring.
>> >
>> >>
>> >>
>> >> Do you really see OS verndor key (Fedora) on the system keyring?
>> >> shim is UEFI binary and can add it to the MOK database..
>> >
>> > I have been told that mechanism to propagate they key in shim which is
>> > used to verify kernel signature is in place and that key should show
>> > up in system_trusted_keyring. I have never verified it though. I will
>> > check it out.
>> >
>> > Thanks
>> > Vivek
>>
>>
>> Hi Vivek,
>>
>> The easiest way to get OS Vendor ker/certificate is just embed it into
>> the kernel along with
>> ephemeral (or not) module signing key... They all appears on .system keyring...
>
> I think it will be tricky as in current setup, signing happens on a different
> server and build server does not have access to keys.
>
> Thanks
> Vivek

It should not be tricky... Signing is done with private key...
Security measure has to be considered.

Certificate is public and is not a secret.. It can be on any build
machine in the world...

-- 
Thanks,
Dmitry

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Vivek Goyal]

On Thu, Jun 12, 2014 at 03:01:54PM -0400, Vivek Goyal wrote:
> On Thu, Jun 12, 2014 at 09:36:46PM +0300, Dmitry Kasatkin wrote:
> > On 12 June 2014 20:32, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > On Thu, Jun 12, 2014 at 08:23:57PM +0300, Dmitry Kasatkin wrote:
> > >> On 12/06/14 19:03, Vivek Goyal wrote:
> > >> > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
> > >> >> This patch provides kernel parameter to specify owner's key id which
> > >> >> must be used for trust validate of keys. Keys signed with other keys
> > >> >> are not trusted.
> > >> >>
> > >> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
> > >> > Hi,
> > >> >
> > >> > I am continuing to work on verifying kernel signature for kexec/kdump. I
> > >> > am planning to take david howell's patches for pkcs7 signature
> > >> > verification and verify bzImage signature.
> > >> >
> > >> > Part of that process will boil down to verifying a certificate in
> > >> > pkcs7 x509 cert chain using a key in system_trusted_keyring.
> > >> >
> > >> > I think the OS vendor key which signs the kernel signing key propagates to
> > >> > system_trusted_keyring. (shim has that and I am not sure how shim makes
> > >> > it propogate all they way to system_trusted_keyring).
> > >> >
> > >> > So I was planning to use same functionality where I look for any key
> > >> > which can verify the signing cert of kernel. As OS vendor key will be
> > >> > in system_trusted_keyring, it should work.
> > >> >
> > >> > Now with this change where you will trust only one selected owner key.
> > >> > That means you will not even trust the OS vendor key which signs kernel
> > >> > signing key. I think this will stop working with  keys_ownerid=<....>
> > >> >
> > >> > As I am doing that work in parallel and I saw these patches, I thought
> > >> > I will bring it up.
> > >>
> > >> Hi Vivek,
> > >>
> > >> All keys stays in the keyring. Usage of owner_keyid is limited to
> > >> validate the trust of the loaded keys.
> > >
> > > Hi Dmitry,
> > >
> > > If owner_keyid scope is just limited to loading extra keys, then it
> > > should be fine as I don't want to load extra keys. I just want to
> > > verify already signed image whose key is supposed to be in
> > > system_trusted_keyring.
> > >
> > >>
> > >>
> > >> Do you really see OS verndor key (Fedora) on the system keyring?
> > >> shim is UEFI binary and can add it to the MOK database..
> > >
> > > I have been told that mechanism to propagate they key in shim which is
> > > used to verify kernel signature is in place and that key should show
> > > up in system_trusted_keyring. I have never verified it though. I will
> > > check it out.
> > >
> > > Thanks
> > > Vivek
> > 
> > 
> > Hi Vivek,
> > 
> > The easiest way to get OS Vendor ker/certificate is just embed it into
> > the kernel along with
> > ephemeral (or not) module signing key... They all appears on .system keyring...
> 
> I think it will be tricky as in current setup, signing happens on a different
> server and build server does not have access to keys.

Apart from this, all the builds might not signed. And at build time it is
might not known if a particular build will be signed with final keys or not. 
One might decide to sign kernel based on testing results post build. So
embedding the kernel signing key in the kernel during build is little
tricky from process point of view.

Thanks
Vivek

Re: [PATCH 3/4] KEYS: validate key trust only with selected owner key

[Dmitry Kasatkin]

On 12 June 2014 22:05, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Thu, Jun 12, 2014 at 03:01:54PM -0400, Vivek Goyal wrote:
>> On Thu, Jun 12, 2014 at 09:36:46PM +0300, Dmitry Kasatkin wrote:
>> > On 12 June 2014 20:32, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > > On Thu, Jun 12, 2014 at 08:23:57PM +0300, Dmitry Kasatkin wrote:
>> > >> On 12/06/14 19:03, Vivek Goyal wrote:
>> > >> > On Tue, Jun 10, 2014 at 11:48:17AM +0300, Dmitry Kasatkin wrote:
>> > >> >> This patch provides kernel parameter to specify owner's key id which
>> > >> >> must be used for trust validate of keys. Keys signed with other keys
>> > >> >> are not trusted.
>> > >> >>
>> > >> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
>> > >> > Hi,
>> > >> >
>> > >> > I am continuing to work on verifying kernel signature for kexec/kdump. I
>> > >> > am planning to take david howell's patches for pkcs7 signature
>> > >> > verification and verify bzImage signature.
>> > >> >
>> > >> > Part of that process will boil down to verifying a certificate in
>> > >> > pkcs7 x509 cert chain using a key in system_trusted_keyring.
>> > >> >
>> > >> > I think the OS vendor key which signs the kernel signing key propagates to
>> > >> > system_trusted_keyring. (shim has that and I am not sure how shim makes
>> > >> > it propogate all they way to system_trusted_keyring).
>> > >> >
>> > >> > So I was planning to use same functionality where I look for any key
>> > >> > which can verify the signing cert of kernel. As OS vendor key will be
>> > >> > in system_trusted_keyring, it should work.
>> > >> >
>> > >> > Now with this change where you will trust only one selected owner key.
>> > >> > That means you will not even trust the OS vendor key which signs kernel
>> > >> > signing key. I think this will stop working with  keys_ownerid=<....>
>> > >> >
>> > >> > As I am doing that work in parallel and I saw these patches, I thought
>> > >> > I will bring it up.
>> > >>
>> > >> Hi Vivek,
>> > >>
>> > >> All keys stays in the keyring. Usage of owner_keyid is limited to
>> > >> validate the trust of the loaded keys.
>> > >
>> > > Hi Dmitry,
>> > >
>> > > If owner_keyid scope is just limited to loading extra keys, then it
>> > > should be fine as I don't want to load extra keys. I just want to
>> > > verify already signed image whose key is supposed to be in
>> > > system_trusted_keyring.
>> > >
>> > >>
>> > >>
>> > >> Do you really see OS verndor key (Fedora) on the system keyring?
>> > >> shim is UEFI binary and can add it to the MOK database..
>> > >
>> > > I have been told that mechanism to propagate they key in shim which is
>> > > used to verify kernel signature is in place and that key should show
>> > > up in system_trusted_keyring. I have never verified it though. I will
>> > > check it out.
>> > >
>> > > Thanks
>> > > Vivek
>> >
>> >
>> > Hi Vivek,
>> >
>> > The easiest way to get OS Vendor ker/certificate is just embed it into
>> > the kernel along with
>> > ephemeral (or not) module signing key... They all appears on .system keyring...
>>
>> I think it will be tricky as in current setup, signing happens on a different
>> server and build server does not have access to keys.
>
> Apart from this, all the builds might not signed. And at build time it is
> might not known if a particular build will be signed with final keys or not.
> One might decide to sign kernel based on testing results post build. So
> embedding the kernel signing key in the kernel during build is little
> tricky from process point of view.
>
> Thanks
> Vivek


Please note, kernel signing key is not embedded to the kernel..
It is certificate/public key.

bootloader has embedded OS vendor certificate.
It is "well known" and public.

Distro might have multiple kernels installed and all of them must boot.
Kernel updates will come signed with the same key...
So all kernels probably signed with the same distro private key or
multiple public keys need to be embedded into the bootloader.

So kernel signing key is not ephemeral but managed and stored
somewhere secretly.

There is no problem to distribute public key...

-- 
Thanks,
Dmitry

Re: [PATCH 0/4] KEYS: validate key trust with owner and builtin keys only

[David Howells]

Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> Yes. Wouldn't having a mechanism to allow userspace to drop keys that 
> have otherwise been imported be a generally useful solution to the issue 
> you have with that?

"keyctl invalidate" could be a way to drop keys.

David