diff mbox

[v5,6/6] KEYS: Parse keyring payload for restrict options

Message ID 20160719204613.25495-7-mathew.j.martineau@linux.intel.com
State New, archived
Headers show

Commit Message

Mat Martineau July 19, 2016, 8:46 p.m. UTC
The restrict_link hook was recently added to keyrings so that keys may
be validated prior to linking.  This functionality was only available
using internal kernel APIs.

Configuring keyring restrictions from userspace must work around these
constraints:

 - The restrict_link pointer must be set through keyring_alloc().
 - The add_key system call has a fixed set of parameters.

When creating keyrings, the payload was previously required to be
NULL.  To support restrict_link configuration, the payload is now
parsed for keyring options if a payload is provided.  The expected
option payload format is:

  "restrict_type=<type> restrict_by=<method> restrict_key=<key serial>"

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 Documentation/crypto/asymmetric-keys.txt |  47 ++++++++++++
 Documentation/security/keys.txt          |  14 +++-
 security/keys/keyring.c                  | 128 ++++++++++++++++++++++++++++++-
 3 files changed, 184 insertions(+), 5 deletions(-)

Comments

David Howells July 21, 2016, 3:46 p.m. UTC | #1
Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:

> +Keyrings created from userspace using add_key can be configured to
> +check the signature of the key being linked.

Lines in the documents should wrap at 79 chars.

> +enum {
> +	Opt_err = -1,
> +	Opt_restrict_type,
> +	Opt_restrict_by,
> +	Opt_restrict_key,
> +};

Just put Opt_err at the bottom and don't assign a value.

> + * datablob_parse - parse the keyring options and fill in the required
> + *                  keyring members.

The function name shouldn't be here.

> + *
> + * Returns true if the option string is valid (including the empty case),
> + * otherwise false.

Maybe 0 or -EINVAL instead?

> + */
> +static bool datablob_parse(char *datablob, struct key_preparsed_payload *prep)

keyring_something_or_other() please.

> +	while ((c = strsep(&datablob, " \t"))) {
> +		if (*c == '\0' || *c == ' ' || *c == '\t')

Surely *c can't actually be space or tab?

> +			restrict_type = key_type_lookup(args[0].from);
> ...
> +			if (result < 0)
> +				return false;

You didn't call key_type_put() anywhere.

> +			prep->payload.data[keyring_restrict_key_ref] = key_ref;

I recommend turning the key_ref into a struct key * at this point.

> +	} else if (restrict_type || restrict_by) {
> +		return false;

What about if a restrict_key= is set, but not both type and by?  Isn't that
also an error?

> +	key_ref_t key_ref = prep->payload.data[keyring_restrict_key_ref];
> +
> +	keyring->restrict_link = prep->payload.data[keyring_restrict_link];
> +	keyring->restrict_key = key_ref_to_ptr(key_ref);
> +	key_get(keyring->restrict_key);

Just copy the key pointer in and clear the one you got from
prep->payload.data.  key_put() can handle a NULL pointer.

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
Mat Martineau July 21, 2016, 9:15 p.m. UTC | #2
On Thu, 21 Jul 2016, David Howells wrote:

> Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:

<snipped several comments, will implement all suggested changes>

>> +	} else if (restrict_type || restrict_by) {
>> +		return false;
>
> What about if a restrict_key= is set, but not both type and by?  Isn't that
> also an error?

If only restrict_key= is set, it makes sense to return the error.

>> +	key_ref_t key_ref = prep->payload.data[keyring_restrict_key_ref];
>> +
>> +	keyring->restrict_link = prep->payload.data[keyring_restrict_link];
>> +	keyring->restrict_key = key_ref_to_ptr(key_ref);
>> +	key_get(keyring->restrict_key);
>
> Just copy the key pointer in and clear the one you got from
> prep->payload.data.  key_put() can handle a NULL pointer.

Yes, that streamlines things nicely.


Thanks for the helpful feedback.

--
Mat Martineau
Intel OTC
--
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
diff mbox

Patch

diff --git a/Documentation/crypto/asymmetric-keys.txt b/Documentation/crypto/asymmetric-keys.txt
index 80d45b5..0eb4ddb 100644
--- a/Documentation/crypto/asymmetric-keys.txt
+++ b/Documentation/crypto/asymmetric-keys.txt
@@ -327,3 +327,50 @@  Functions are provided to register and unregister parsers:
 
 Parsers may not have the same name.  The names are otherwise only used for
 displaying in debugging messages.
+
+
+=========================
+KEYRING LINK RESTRICTIONS
+=========================
+
+Keyrings created from userspace using add_key can be configured to
+check the signature of the key being linked.
+
+Several restriction methods are available:
+
+ (1) Restrict using a separate keyring
+
+     - Options used when creating the keyring:
+       - restrict_type=asymmetric
+       - restrict_by=keyring
+       - restrict_key=<keyring serial number>
+
+     Whenever a key link is requested, the keyring identified in
+     restrict_key will be searched for the signing key.
+
+ (2) Restrict using the kernel builtin trusted keyring
+
+     - Options used when creating the keyring:
+       - restrict_type=asymmetric
+       - restrict_by=builtin_trusted
+
+     The kernel builtin trusted keyring will be searched for the
+     signing key.
+
+ (3) Restrict using the kernel builtin and secondary trusted keyrings
+
+     - Options used when creating the keyring:
+       - restrict_type=asymmetric
+       - restrict_by=builtin_and_secondary_trusted
+
+     The kernel builtin and secondary trusted keyrings will be
+     searched for the signing key.
+
+In all of these cases, if the signing key is found the signature of
+the key to be linked will be verified using the signing key.  The
+requested key is added to the keyring only if the signature is
+successfully verified.  -ENOKEY is returned if the parent certificate
+could not be found, or -EKEYREJECTED is returned if the signature
+check fails or the key is blacklisted.  Other errors may be returned
+if the signature check could not be performed.
+
diff --git a/Documentation/security/keys.txt b/Documentation/security/keys.txt
index 45f5ff8e..a152c85c 100644
--- a/Documentation/security/keys.txt
+++ b/Documentation/security/keys.txt
@@ -418,8 +418,18 @@  The main syscalls are:
      the type. The payload is plen in size, and plen can be zero for an empty
      payload.
 
-     A new keyring can be generated by setting type "keyring", the keyring name
-     as the description (or NULL) and setting the payload to NULL.
+     A new keyring can be generated by setting type "keyring" and the
+     keyring name as the description (or NULL).  The payload can be
+     NULL or contain keyring link restriction options using the
+     following format:
+
+	"restrict_type=<type> restrict_by=<method> restrict_key=<key serial>"
+
+     "type" is a registered key type.  The type and method will
+     populate restrict_link in the keyring structure using the
+     lookup_restrict function for the requested type.  restrict_key is
+     optional when creating the keyring, but some restrict methods
+     depend on the presence of a restrict_key value when linking keys.
 
      User defined keys can be created by specifying type "user". It is
      recommended that a user defined key's description by prefixed with a type
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index c91e4e0..0902ba5 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -20,9 +20,18 @@ 
 #include <keys/user-type.h>
 #include <linux/assoc_array_priv.h>
 #include <linux/uaccess.h>
+#include <linux/parser.h>
 #include "internal.h"
 
 /*
+ * Layout of preparse payload
+ */
+enum {
+	keyring_restrict_link,
+	keyring_restrict_key_ref,
+};
+
+/*
  * When plumbing the depths of the key tree, this sets a hard limit
  * set on how deep we're willing to go.
  */
@@ -125,29 +134,141 @@  static void keyring_publish_name(struct key *keyring)
 	}
 }
 
+enum {
+	Opt_err = -1,
+	Opt_restrict_type,
+	Opt_restrict_by,
+	Opt_restrict_key,
+};
+
+static const match_table_t keyring_tokens = {
+	{ Opt_restrict_type,	"restrict_type=%s" },
+	{ Opt_restrict_by,	"restrict_by=%s" },
+	{ Opt_restrict_key,	"restrict_key=%d" },
+	{ Opt_err,		NULL },
+};
+
+/*
+ * datablob_parse - parse the keyring options and fill in the required
+ *                  keyring members.
+ *
+ * Returns true if the option string is valid (including the empty case),
+ * otherwise false.
+ */
+static bool datablob_parse(char *datablob, struct key_preparsed_payload *prep)
+{
+	substring_t args[MAX_OPT_ARGS];
+	char *c;
+	int token;
+	unsigned long token_mask = 0;
+	struct key_type *restrict_type = NULL;
+	char *restrict_by = NULL;
+	restrict_link_func_t restrict_link;
+	key_serial_t serial = 0;
+	int result;
+	key_ref_t key_ref;
+
+	while ((c = strsep(&datablob, " \t"))) {
+		if (*c == '\0' || *c == ' ' || *c == '\t')
+			continue;
+
+		token = match_token(c, keyring_tokens, args);
+		if (test_and_set_bit(token, &token_mask))
+			return false;
+
+		switch (token) {
+		case Opt_restrict_type:
+			restrict_type = key_type_lookup(args[0].from);
+			if (IS_ERR(restrict_type))
+				return false;
+			break;
+		case Opt_restrict_by:
+			restrict_by = args[0].from;
+			break;
+		case Opt_restrict_key:
+			result = kstrtos32(args[0].from, 0, &serial);
+			if (result < 0)
+				return false;
+			break;
+		case Opt_err:
+			return false;
+		}
+	}
+
+	if (restrict_type && restrict_by) {
+		if (!restrict_type->lookup_restrict)
+			return false;
+
+		restrict_link = restrict_type->lookup_restrict(restrict_by);
+		if (IS_ERR(restrict_link))
+			return false;
+
+		if (test_bit(Opt_restrict_key, &token_mask)) {
+			key_ref = lookup_user_key(serial, 0, 0);
+			if (IS_ERR(key_ref))
+				return false;
+
+			prep->payload.data[keyring_restrict_key_ref] = key_ref;
+		}
+
+		prep->payload.data[keyring_restrict_link] = restrict_link;
+	} else if (restrict_type || restrict_by) {
+		return false;
+	}
+
+	return true;
+}
+
 /*
  * Preparse a keyring payload
  */
 static int keyring_preparse(struct key_preparsed_payload *prep)
 {
-	return prep->datalen != 0 ? -EINVAL : 0;
+	char *datablob;
+	size_t datalen = prep->datalen;
+	bool valid;
+
+	if (datalen) {
+		datablob = kmalloc(datalen + 1, GFP_KERNEL);
+		if (!datablob)
+			return -ENOMEM;
+
+		memcpy(datablob, prep->data, datalen);
+		datablob[datalen] = '\0';
+
+		valid = datablob_parse(datablob, prep);
+
+		kfree(datablob);
+
+		if (!valid)
+			return -EINVAL;
+	}
+
+	return 0;
 }
 
 /*
- * Free a preparse of a user defined key payload
+ * Free a preparse of a keyring payload
  */
 static void keyring_free_preparse(struct key_preparsed_payload *prep)
 {
+	key_ref_put(prep->payload.data[keyring_restrict_key_ref]);
 }
 
 /*
  * Initialise a keyring.
  *
- * Returns 0 on success, -EINVAL if given any data.
+ * Returns 0 on success.
  */
 static int keyring_instantiate(struct key *keyring,
 			       struct key_preparsed_payload *prep)
 {
+	key_ref_t key_ref = prep->payload.data[keyring_restrict_key_ref];
+
+	keyring->restrict_link = prep->payload.data[keyring_restrict_link];
+	keyring->restrict_key = key_ref_to_ptr(key_ref);
+	key_get(keyring->restrict_key);
+
 	assoc_array_init(&keyring->keys);
 	/* make the keyring available by name if it has one */
 	keyring_publish_name(keyring);
@@ -394,6 +515,7 @@  static void keyring_destroy(struct key *keyring)
 		write_unlock(&keyring_name_lock);
 	}
 
+	key_put(keyring->restrict_key);
 	assoc_array_destroy(&keyring->keys, &keyring_assoc_array_ops);
 }