[PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[PATCH v6 0/6] 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, this
patch set further restricts the certificates to those signed by a
particular key, or the builtin keys, on the system keyring.

Other than the "KEYS: validate certificate trust only with builtin keys"
patch, which is included in this patch set for completeness, but can be
deferred until the UEFI key patches are upstreamed, these patches are
ready to be upstreamed.  David, how do you want to go forward with
this patchset.  Did you want to take them?

thanks,

Mimi

Dmitry Kasatkin (3):
  KEYS: make partial key id matching as a dedicated function
  KEYS: validate certificate trust only with selected owner key
  KEYS: validate certificate trust only with builtin keys

Mimi Zohar (3):
  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

 Documentation/kernel-parameters.txt      |   5 ++
 crypto/asymmetric_keys/asymmetric_keys.h |   2 +
 crypto/asymmetric_keys/asymmetric_type.c |  51 +++++++++------
 crypto/asymmetric_keys/x509_public_key.c | 109 ++++++++++++++++++++++++++++++-
 include/keys/system_keyring.h            |  10 ++-
 include/linux/key.h                      |   1 +
 kernel/system_keyring.c                  |   1 +
 security/integrity/digsig.c              |  28 ++++++++
 security/integrity/ima/Kconfig           |  10 +++
 security/integrity/ima/ima.h             |  12 ++++
 security/integrity/ima/ima_main.c        |  10 ++-
 security/integrity/integrity.h           |   5 ++
 security/keys/keyctl.c                   |   6 +-
 13 files changed, 225 insertions(+), 25 deletions(-)

-- 
1.8.1.4

[PATCH v6 1/6] 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 v6:
- whitespace and other cleanup

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..8a8c233 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

Re: [PATCH v6 1/6] KEYS: special dot prefixed keyring name bug fix

[David Howells]

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 v6:
> - whitespace and other cleanup
> 
> 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>

Acked-by: David Howells <dhowells@redhat.com>

[PATCH v6 2/6] 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 v6:
- on error free key - Dmitry
- validate trust only for not already trusted keys - Dmitry
- formatting cleanup

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: Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
---
 crypto/asymmetric_keys/x509_public_key.c | 87 +++++++++++++++++++++++++++++++-
 include/keys/system_keyring.h            | 10 +++-
 2 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 382ef0d..436fbd8 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -18,12 +18,62 @@
 #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 +153,37 @@ 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);
+		key_put(key);
+	}
+	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 +236,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 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

[PATCH v6 3/6] KEYS: make partial key id matching as a dedicated function

[Mimi Zohar]

From: Dmitry Kasatkin <d.kasatkin@samsung.com>

To avoid code duplication this patch refactors asymmetric_key_match(),
making partial ID string match a separate function.

This patch also implicitly fixes a bug in the code.  asymmetric_key_match()
allows to match the key by its subtype. But subtype matching could be
undone if asymmetric_key_id(key) would return NULL. This patch first
checks for matching spec and then for its value.

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 crypto/asymmetric_keys/asymmetric_keys.h |  2 ++
 crypto/asymmetric_keys/asymmetric_type.c | 50 ++++++++++++++++++++------------
 2 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/crypto/asymmetric_keys/asymmetric_keys.h b/crypto/asymmetric_keys/asymmetric_keys.h
index 515b634..a63c551 100644
--- a/crypto/asymmetric_keys/asymmetric_keys.h
+++ b/crypto/asymmetric_keys/asymmetric_keys.h
@@ -9,6 +9,8 @@
  * 2 of the Licence, or (at your option) any later version.
  */
 
+int asymmetric_keyid_match(const char *kid, const char *id);
+
 static inline const char *asymmetric_key_id(const struct key *key)
 {
 	return key->type_data.p[1];
diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index b77eb53..1fd1d30 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -23,6 +23,34 @@ static LIST_HEAD(asymmetric_key_parsers);
 static DECLARE_RWSEM(asymmetric_key_parsers_sem);
 
 /*
+ * Match asymmetric key id with partial match
+ * @id:		key id to match in a form "id:<id>"
+ */
+int asymmetric_keyid_match(const char *kid, const char *id)
+{
+	size_t idlen, kidlen;
+
+	if (!kid || !id)
+		return 0;
+
+	/* make it possible to use id as in the request: "id:<id>" */
+	if (strncmp(id, "id:", 3) == 0)
+		id += 3;
+
+	/* Anything after here requires a partial match on the ID string */
+	idlen = strlen(id);
+	kidlen = strlen(kid);
+	if (idlen > kidlen)
+		return 0;
+
+	kid += kidlen - idlen;
+	if (strcasecmp(id, kid) != 0)
+		return 0;
+
+	return 1;
+}
+
+/*
  * Match asymmetric keys on (part of) their name
  * We have some shorthand methods for matching keys.  We allow:
  *
@@ -34,9 +62,8 @@ static int asymmetric_key_match(const struct key *key, const void *description)
 {
 	const struct asymmetric_key_subtype *subtype = asymmetric_key_subtype(key);
 	const char *spec = description;
-	const char *id, *kid;
+	const char *id;
 	ptrdiff_t speclen;
-	size_t idlen, kidlen;
 
 	if (!subtype || !spec || !*spec)
 		return 0;
@@ -55,23 +82,8 @@ static int asymmetric_key_match(const struct key *key, const void *description)
 	speclen = id - spec;
 	id++;
 
-	/* Anything after here requires a partial match on the ID string */
-	kid = asymmetric_key_id(key);
-	if (!kid)
-		return 0;
-
-	idlen = strlen(id);
-	kidlen = strlen(kid);
-	if (idlen > kidlen)
-		return 0;
-
-	kid += kidlen - idlen;
-	if (strcasecmp(id, kid) != 0)
-		return 0;
-
-	if (speclen == 2 &&
-	    memcmp(spec, "id", 2) == 0)
-		return 1;
+	if (speclen == 2 && memcmp(spec, "id", 2) == 0)
+		return asymmetric_keyid_match(asymmetric_key_id(key), id);
 
 	if (speclen == subtype->name_len &&
 	    memcmp(spec, subtype->name, speclen) == 0)
-- 
1.8.1.4

Re: [PATCH v6 3/6] KEYS: make partial key id matching as a dedicated function

[David Howells]

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

> +	if (strncmp(id, "id:", 3) == 0)

Use memcmp() here.

> -	kid += kidlen - idlen;
> -	if (strcasecmp(id, kid) != 0)
> -		return 0;

This test is no longer applied in the "<subtype>:..." case.

David

Re: [PATCH v6 3/6] KEYS: make partial key id matching as a dedicated function

[Dmitry Kasatkin]

On 27/06/14 16:38, David Howells wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
>> +	if (strncmp(id, "id:", 3) == 0)

>> Use memcmp() here.

'id' function parameter comes from "keys_ownerid" kernel parameter.
User can supply anything shorter than "id:".
Though comparing 3 bytes should not produce any memory access errors,
memcmp can access beyond the length of the string.
I think 'strcnmp' is more appropriate here...


>> -	kid += kidlen - idlen;
>> -	if (strcasecmp(id, kid) != 0)
>> -		return 0;
> This test is no longer applied in the "<subtype>:..." case.

I did not get fully what you comment here or ask to do..
But yes, with this patch, it is no longer the case.

Thanks,
Dmitry

> David
> --
> 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 v6 3/6] KEYS: make partial key id matching as a dedicated function

[Mimi Zohar]

On Mon, 2014-06-30 at 16:14 +0300, Dmitry Kasatkin wrote: 
> On 27/06/14 16:38, David Howells wrote:
> > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >
> >> +	if (strncmp(id, "id:", 3) == 0)
> 
> >> Use memcmp() here.
> 
> 'id' function parameter comes from "keys_ownerid" kernel parameter.
> User can supply anything shorter than "id:".
> Though comparing 3 bytes should not produce any memory access errors,
> memcmp can access beyond the length of the string.
> I think 'strcnmp' is more appropriate here...
> 
> 
> >> -	kid += kidlen - idlen;
> >> -	if (strcasecmp(id, kid) != 0)
> >> -		return 0;
> > This test is no longer applied in the "<subtype>:..." case.
> 
> I did not get fully what you comment here or ask to do..
> But yes, with this patch, it is no longer the case.

Other than this comment, all of the other comments have been addressed.
The updated patches are available from
linux-integrity/next-trusted-keys.

thanks,

Mimi

[PATCH v6 4/6] KEYS: validate certificate trust only with selected owner key

[Mimi Zohar]

From: Dmitry Kasatkin <d.kasatkin@samsung.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.

This patch defines a new kernel parameter 'keys_ownerid' to identify
the owner's key which must be used for trust validation of certificates.

Simplified Mimi's "KEYS: define an owner trusted keyring" patch.

Changelog:
- support for builtin x509 public keys only
- export "asymmetric_keyid_match"
- remove ifndefs MODULE

Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 Documentation/kernel-parameters.txt      |  5 +++++
 crypto/asymmetric_keys/asymmetric_type.c |  1 +
 crypto/asymmetric_keys/x509_public_key.c | 20 ++++++++++++++++++++
 3 files changed, 26 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 6eaa9cd..b0cc47d 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1492,6 +1492,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(s) on
+			the system trusted keyring to be used for certificate
+			trust validation.
+			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/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index 1fd1d30..c948df5 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -49,6 +49,7 @@ int asymmetric_keyid_match(const char *kid, const char *id)
 
 	return 1;
 }
+EXPORT_SYMBOL_GPL(asymmetric_keyid_match);
 
 /*
  * Match asymmetric keys on (part of) their name
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 436fbd8..44850f2 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -24,6 +24,22 @@
 #include "public_key.h"
 #include "x509_parser.h"
 
+static char *owner_keyid;
+
+#ifndef MODULE
+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);
+#endif
+
 /*
  * Find a key in the given keyring by issuer and authority.
  */
@@ -171,6 +187,10 @@ static int x509_validate_trust(struct x509_certificate *cert,
 	if (!trust_keyring)
 		return -EOPNOTSUPP;
 
+	if (owner_keyid &&
+	    !asymmetric_keyid_match(cert->authority, owner_keyid))
+		return -EPERM;
+
 	key = x509_request_asymmetric_key(trust_keyring,
 					  cert->issuer, strlen(cert->issuer),
 					  cert->authority,
-- 
1.8.1.4

Re: [PATCH v6 4/6] KEYS: validate certificate trust only with selected owner key

[David Howells]

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

> This patch defines a new kernel parameter 'keys_ownerid' to identify
> the owner's key which must be used for trust validation of certificates.

"ca_keys" or "only_ca" instead, maybe?

David

Re: [PATCH v6 4/6] KEYS: validate certificate trust only with selected owner key

[Mimi Zohar]

On Fri, 2014-06-27 at 14:55 +0100, David Howells wrote: 
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > This patch defines a new kernel parameter 'keys_ownerid' to identify
> > the owner's key which must be used for trust validation of certificates.
> 
> "ca_keys" or "only_ca" instead, maybe?

Neither of these names reflect the concept of the machine owner or a
local key.  The initial patches named it 'owner_keyid'.  If kernel
parameters don't need to be prefixed with the subsystem, we could revert
the name change or call it localca_keyid.

Mimi

Re: [PATCH v6 4/6] KEYS: validate certificate trust only with selected owner key

[Dmitry Kasatkin]

On 27/06/14 20:44, Mimi Zohar wrote:
> On Fri, 2014-06-27 at 14:55 +0100, David Howells wrote: 
>> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>
>>> This patch defines a new kernel parameter 'keys_ownerid' to identify
>>> the owner's key which must be used for trust validation of certificates.
>> "ca_keys" or "only_ca" instead, maybe?
> Neither of these names reflect the concept of the machine owner or a
> local key.  The initial patches named it 'owner_keyid'.  If kernel
> parameters don't need to be prefixed with the subsystem, we could revert
> the name change or call it localca_keyid.
>
> Mimi

I neither against any of proposals.

But considering that we use those keys to verify other keys, they become
ca keys.
So from that point of view I think 'ca_keys' reflects functionality
quite ok.

localca_ prefix is may be not very relevant as builtin keys may
comesfrom kernel vendor (RH, Ubuntu)
and is not really local...

so let's decide on 'ca_keys'?

Thanks,
Dmitry

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

Re: [PATCH v6 4/6] KEYS: validate certificate trust only with selected owner key

[Mimi Zohar]

On Mon, 2014-06-30 at 16:47 +0300, Dmitry Kasatkin wrote: 
> On 27/06/14 20:44, Mimi Zohar wrote:
> > On Fri, 2014-06-27 at 14:55 +0100, David Howells wrote: 
> >> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >>
> >>> This patch defines a new kernel parameter 'keys_ownerid' to identify
> >>> the owner's key which must be used for trust validation of certificates.
> >> "ca_keys" or "only_ca" instead, maybe?
> > Neither of these names reflect the concept of the machine owner or a
> > local key.  The initial patches named it 'owner_keyid'.  If kernel
> > parameters don't need to be prefixed with the subsystem, we could revert
> > the name change or call it localca_keyid.
> >
> > Mimi
> 
> I neither against any of proposals.
> 
> But considering that we use those keys to verify other keys, they become
> ca keys.
> So from that point of view I think 'ca_keys' reflects functionality
> quite ok.
> 
> localca_ prefix is may be not very relevant as builtin keys may
> comesfrom kernel vendor (RH, Ubuntu)
> and is not really local...

Ok.

> so let's decide on 'ca_keys'?

Ok.  This change isn't limited to just the kernel boot parameter name,
but needs to be reflected in the patch description and variable/function
names.

thanks,

Mimi

[PATCH v6 5/6] KEYS: validate certificate trust only with builtin keys

[Mimi Zohar]

From: Dmitry Kasatkin <d.kasatkin@samsung.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 only by
builtin keys on the system keyring.

This patch defines a new option 'builtin' for the kernel parameter
'keys_ownerid' to allow trust validation using builtin keys.

Simplified Mimi's "KEYS: define an owner trusted keyring" patch

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

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index b0cc47d..a0c155c 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1495,7 +1495,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	keys_ownerid=[KEYS] This parameter identifies a specific key(s) on
 			the system trusted keyring to be used for certificate
 			trust validation.
-			format: id:<keyid>
+			format: { id:<keyid> | builtin }
 
 	kgdbdbgp=	[KGDB,HW] kgdb over EHCI usb debug port.
 			Format: <Controller#>[,poll interval]
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 44850f2..541329d 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;
 
 #ifndef MODULE
@@ -34,6 +35,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;
 }
@@ -180,7 +183,6 @@ EXPORT_SYMBOL_GPL(x509_check_signature);
 static int x509_validate_trust(struct x509_certificate *cert,
 			       struct key *trust_keyring)
 {
-	const struct public_key *pk;
 	struct key *key;
 	int ret = 1;
 
@@ -196,8 +198,8 @@ 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))
+			ret = x509_check_signature(key->payload.data, cert);
 		key_put(key);
 	}
 	return ret;
diff --git a/include/linux/key.h b/include/linux/key.h
index 017b082..65316f7 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.8.1.4

Re: [PATCH v6 5/6] KEYS: validate certificate trust only with builtin keys

[David Howells]

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

> +static bool builtin_keys;

Could we call this something like "use_builtin_keys_only"?

Looks okay otherwise.

David

Re: [PATCH v6 5/6] KEYS: validate certificate trust only with builtin keys

[Mimi Zohar]

On Fri, 2014-06-27 at 14:54 +0100, David Howells wrote: 
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > +static bool builtin_keys;
> 
> Could we call this something like "use_builtin_keys_only"?
> 
> Looks okay otherwise.

The current patches support either a single key or the builtin keys, not
both.  In case this behavior changes, I'd prefer using
'use_builtin_keys'.

thanks,

Mimi

[PATCH v6 6/6] 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 v6:
- remove ifdef CONFIG_IMA_TRUSTED_KEYRING in C code - Dmitry
- update Kconfig dependency and help
- select KEYS_DEBUG_PROC_KEYS - Dmitry

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>
Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com>
---
 security/integrity/digsig.c       | 28 ++++++++++++++++++++++++++++
 security/integrity/ima/Kconfig    | 10 ++++++++++
 security/integrity/ima/ima.h      | 12 ++++++++++++
 security/integrity/ima/ima_main.c | 10 ++++++++--
 security/integrity/integrity.h    |  5 +++++
 5 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index b4af4eb..8d4fbff 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,25 @@ 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();
+	int err = 0;
+
+	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 {
+		err = PTR_ERR(keyring[id]);
+		pr_info("Can't allocate %s keyring (%d)\n",
+			keyring_name[id], err);
+		keyring[id] = NULL;
+	}
+	return err;
+}
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 81a2797..08758fb 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -123,3 +123,13 @@ 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
+	depends on INTEGRITY_ASYMMETRIC_KEYS
+	select KEYS_DEBUG_PROC_KEYS
+	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.h b/security/integrity/ima/ima.h
index f79fa8b..c42056e 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -249,4 +249,16 @@ static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
 	return -EINVAL;
 }
 #endif /* CONFIG_IMA_LSM_RULES */
+
+#ifdef CONFIG_IMA_TRUSTED_KEYRING
+static inline int ima_init_keyring(const unsigned int id)
+{
+	return integrity_init_keyring(id);
+}
+#else
+static inline int ima_init_keyring(const unsigned int id)
+{
+	return 0;
+}
+#endif /* CONFIG_IMA_TRUSTED_KEYRING */
 #endif
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f474c60..0d69643 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -325,8 +325,14 @@ static int __init init_ima(void)
 
 	hash_setup(CONFIG_IMA_DEFAULT_HASH);
 	error = ima_init();
-	if (!error)
-		ima_initialized = 1;
+	if (error)
+		goto out;
+
+	error = ima_init_keyring(INTEGRITY_KEYRING_IMA);
+	if (error)
+		goto out;
+	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

Re: [PATCH v6 6/6] ima: define '.ima' as a builtin 'trusted' keyring

[David Howells]

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[David Howells]

Okay, I've merged a number of patchsets together that I want to push for the
next merge window:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-next

I'd like to add yours.  Shall I pull:

	linux-integrity/next-trusted-keys

which is here, I presume:

	http://git.kernel.org/cgit/linux/kernel/git/zohar/linux-integrity.git/

David

Re: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[Mimi Zohar]

On Wed, 2014-07-09 at 16:31 +0100, David Howells wrote: 
> Okay, I've merged a number of patchsets together that I want to push for the
> next merge window:
> 
> 	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-next
> 
> I'd like to add yours.  Shall I pull:
> 
> 	linux-integrity/next-trusted-keys
> 
> which is here, I presume:
> 
> 	http://git.kernel.org/cgit/linux/kernel/git/zohar/linux-integrity.git/
> 

Yes, that's fine.  My concern, however, is that the trusted keyring
patches are independent of the other patches being upstreamed and should
be upstreamed regardless of the other patches.

Mimi

Re: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[David Howells]

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

> Yes, that's fine.  My concern, however, is that the trusted keyring
> patches are independent of the other patches being upstreamed and should
> be upstreamed regardless of the other patches.

There is overlap in the X.509 certificate request function that you took from
my pkcs#7 patches.

David

Re: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[Mimi Zohar]

On Wed, 2014-07-09 at 19:56 +0100, David Howells wrote: 
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > Yes, that's fine.  My concern, however, is that the trusted keyring
> > patches are independent of the other patches being upstreamed and should
> > be upstreamed regardless of the other patches.
> 
> There is overlap in the X.509 certificate request function that you took from
> my pkcs#7 patches.

Right, x509_request_asymmetric_key() is the same as
pkcs7_request_asymmetric_key().

Mimi

Re: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[Dmitry Kasatkin]

Hi David,

If patches from integrity/next-trusted-keys goes via your tree, then I
suggest that you re-base your patches on the top of our
patchset, because it is unclear how long review of PE, PKCS7 patches
will take and if they will be pulled...

I would do it with different pull requests.

- Dmitry


On 10/07/14 00:29, Mimi Zohar wrote:
> On Wed, 2014-07-09 at 19:56 +0100, David Howells wrote: 
>> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>
>>> Yes, that's fine.  My concern, however, is that the trusted keyring
>>> patches are independent of the other patches being upstreamed and should
>>> be upstreamed regardless of the other patches.
>> There is overlap in the X.509 certificate request function that you took from
>> my pkcs#7 patches.
> Right, x509_request_asymmetric_key() is the same as
> pkcs7_request_asymmetric_key().
>
> 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: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[David Howells]

Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:

> If patches from integrity/next-trusted-keys goes via your tree, then I
> suggest that you re-base your patches on the top of our
> patchset, because it is unclear how long review of PE, PKCS7 patches
> will take and if they will be pulled...

I'd rather not do that since other things are now depending on my patches.

What's probably better is just to clean up the duplication later.  That way,
there need be no dependency between the two sets.

David

Re: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[Mimi Zohar]

On Sun, 2014-07-13 at 22:06 +0100, David Howells wrote: 
> Dmitry Kasatkin <d.kasatkin@samsung.com> wrote:
> 
> > If patches from integrity/next-trusted-keys goes via your tree, then I
> > suggest that you re-base your patches on the top of our
> > patchset, because it is unclear how long review of PE, PKCS7 patches
> > will take and if they will be pulled...
> 
> I'd rather not do that since other things are now depending on my patches.
> 
> What's probably better is just to clean up the duplication later.  That way,
> there need be no dependency between the two sets.

We're already at -rc5.  Any pull requests for the next open window need
to submitted soon.  If/when are you planning on sending James the pull
request?  (I'm kind of waiting for the KEY patches to make it in, before
sending the linux-integrity pull request.)

thanks,

Mimi

Re: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[David Howells]

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

> We're already at -rc5.  Any pull requests for the next open window need
> to submitted soon.  If/when are you planning on sending James the pull
> request?  (I'm kind of waiting for the KEY patches to make it in, before
> sending the linux-integrity pull request.)

Just merging it all together now on top of the same base as yours.  Is
next-with-keys the branch I should be merging?

David

Re: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[Mimi Zohar]

On Thu, 2014-07-17 at 20:43 +0100, David Howells wrote: 
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > We're already at -rc5.  Any pull requests for the next open window need
> > to submitted soon.  If/when are you planning on sending James the pull
> > request?  (I'm kind of waiting for the KEY patches to make it in, before
> > sending the linux-integrity pull request.)
> 
> Just merging it all together now on top of the same base as yours.  Is
> next-with-keys the branch I should be merging?

Yes, but you should be rebasing on top of James' #next. He just pulled
the #next-without-keys.

Mimi

Re: [PATCH v6 0/6] ima: extending secure boot certificate chain of trust

[David Howells]

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

> Yes, but you should be rebasing on top of James' #next. He just pulled
> the #next-without-keys.

If I merge your branch in, GIT should just take care of that.

David