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
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
Acked-by: Dmitry Kasatkin <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Dmitry Kasatkin <[email protected]>
---
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
From: Dmitry Kasatkin <[email protected]>
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 <[email protected]>
Signed-off-by: Mimi Zohar <[email protected]>
---
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
From: Dmitry Kasatkin <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
Cc: David Howells <[email protected]>
Signed-off-by: Mimi Zohar <[email protected]>
---
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
From: Dmitry Kasatkin <[email protected]>
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 <[email protected]>
Signed-off-by: Mimi Zohar <[email protected]>
---
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
Mimi Zohar <[email protected]> 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 <[email protected]>
> Cc: David Howells <[email protected]>
> Signed-off-by: Mimi Zohar <[email protected]>
Acked-by: David Howells <[email protected]>
Mimi Zohar <[email protected]> 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
Mimi Zohar <[email protected]> wrote:
> +static bool builtin_keys;
Could we call this something like "use_builtin_keys_only"?
Looks okay otherwise.
David
Mimi Zohar <[email protected]> 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
Acked-by: David Howells <[email protected]>
On Fri, 2014-06-27 at 14:55 +0100, David Howells wrote:
> Mimi Zohar <[email protected]> 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
On Fri, 2014-06-27 at 14:54 +0100, David Howells wrote:
> Mimi Zohar <[email protected]> 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
On 27/06/14 16:38, David Howells wrote:
> Mimi Zohar <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 27/06/14 20:44, Mimi Zohar wrote:
> On Fri, 2014-06-27 at 14:55 +0100, David Howells wrote:
>> Mimi Zohar <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 <[email protected]> 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
On Mon, 2014-06-30 at 16:14 +0300, Dmitry Kasatkin wrote:
> On 27/06/14 16:38, David Howells wrote:
> > Mimi Zohar <[email protected]> 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
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
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
Mimi Zohar <[email protected]> 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
On Wed, 2014-07-09 at 19:56 +0100, David Howells wrote:
> Mimi Zohar <[email protected]> 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
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 <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Dmitry Kasatkin <[email protected]> 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
On Sun, 2014-07-13 at 22:06 +0100, David Howells wrote:
> Dmitry Kasatkin <[email protected]> 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
Mimi Zohar <[email protected]> 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
On Thu, 2014-07-17 at 20:43 +0100, David Howells wrote:
> Mimi Zohar <[email protected]> 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
Mimi Zohar <[email protected]> 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