linux-kernel - Re: [PATCH] Security: Keys: Added derived keytype

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <31460.1459526175@warthog.procyon.org.uk>
Date:	Fri, 01 Apr 2016 16:56:15 +0100
From:	David Howells <dhowells@...hat.com>
To:	Kirill Marinushkin <k.marinushkin@...il.com>
Cc:	dhowells@...hat.com, linux-kernel@...r.kernel.org,
	keyrings@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH] Security: Keys: Added derived keytype

Kirill Marinushkin <k.marinushkin@...il.com> wrote:

> For details see
> Documentation/security/keys-derived.txt

Please include at least a summary in the patch description, not just a pointer
to the documentation file.

> +			Derived Keys
> +
> +Derived is a keytype of the kernel keyring facility.
> +The key secret is derived from the secret value given by user.

I'm not keen on the type name "derived" as it's totally non-obvious.  How
about "secret", "shared-secret" or "salted" or something like that.

> +		i=,  iterations=	- number of itaretions,

"iterations"

> +#ifndef INCLUDE_KEYS_DERIVED_TYPE_H_
> +#define INCLUDE_KEYS_DERIVED_TYPE_H_

I would drop the initial "INCLUDE_" from that.

> +extern int derived_instantiate(struct key *key,
> +		struct key_preparsed_payload *prep);
> +extern int derived_update(struct key *key,
> +		struct key_preparsed_payload *prep);
> +extern long derived_read(const struct key *key,
> +		char __user *buffer, size_t buflen);
> +extern void derived_revoke(struct key *key);
> +extern void derived_destroy(struct key *key);

Is there a reason you're exporting all the methods?

> +struct derived_key_payload {

Should this struct go in your type header?

> +	struct rcu_head rcu;	/* RCU destructor */
> +	char *alg_name;			/* null-terminated digest algorithm name */
> +	char *rng_name;			/* null-terminated random generator algorithm name */
> +	u64 iter;				/* number of iterations */

Isn't the max value for this 0x000FFFFF?  If so, why is it u64?

> +	unsigned int saltlen;	/* length of salt */
> +	unsigned char *salt;	/* salt */
> +	unsigned int datalen;	/* length of derived data */
> +	unsigned char *data;	/* derived data */

Reorder these to put saltlen and datalen next to each other, thereby
eliminating two holes in the struct on a 64-bit machine.


> +static int gen_random(const char *rnd_name, u8 *buf, unsigned int len)

Prefix with "derived_" please.

> +		case OPT_FORMAT_RAND:
> +			if (kstrtouint(v[i].b->data, 0, &tempu)
> +					|| tempu == 0
> +					|| tempu > RAND_MAX_SIZE) {
> +				pr_err(PREFIX "invalid random size");
> +				return -EINVAL;
> +			}
> +			v[i].b->data = kmalloc(tempu, GFP_KERNEL);
> +			if (!v[i].b->data) {
> +				pr_err(PREFIX "random data alloc failed");
> +				return -ENOMEM;
> +			}
> +			*v[i].b->lenp = tempu;
> +			ret = gen_random(payload->rng_name, v[i].b->data, *v[i].b->lenp);

I would move the kmalloc() inside the gen_random() function.

> +static void free_payload_content(struct derived_key_payload *payload)
> +{
> +	if (payload->alg_name)
> +		kzfree(payload->alg_name);
> +	if (payload->rng_name)
> +		kzfree(payload->rng_name);
> +	if (payload->data)
> +		kzfree(payload->data);
> +	if (payload->salt)
> +		kzfree(payload->salt);
> +}

kzfree() can handle a NULL pointer.  You've got more instances of this.

Your functions should all be prefixed with "derived_".

> +	sdesc = kzalloc(sizeof(struct shash_desc) + crypto_shash_descsize(sh), GFP_KERNEL);

Do you need some wrappers on this to get the alignment correct?

> +	if (!sdesc) {
> +		pr_err(PREFIX "sdesc alloc failed");

Don't print an error here.

> +		ret = -ENOMEM;
> +		goto out;
> +	}

You should stick a label about four lines below "out:" and go there instead.
Then you can get rid of the conditionalisation in the following:

	+	if (!IS_ERR(sh))
	+		crypto_free_shash(sh);

> +	payload = kzalloc(sizeof(*payload), GFP_KERNEL);
> +	if (!payload) {
> +		pr_err(PREFIX "payload alloc failed");
> +		return -ENOMEM;
> +	}
> +
> +	/* fill payload */
> +	ret = fill_payload(payload, prep);

Move the kzalloc() call into fill_payload().

> +int derived_update(struct key *key, struct key_preparsed_payload *prep)
> +{
> +	int ret  = -EINVAL;
> +	struct derived_key_payload *payload =
> +			(struct derived_key_payload *)key->payload.data;
> +
> +	/* free current payload */
> +	free_payload_content(payload);
> +	memset(payload, 0x00, sizeof(*payload));
> +
> +	ret = fill_payload(payload, prep);
> +	if (!ret)
> +		ret = reserve_derived_payload(key, payload);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(derived_update);

This is *not* RCU safe.

You should implement the ->preparse() method and do the argument parsing and
creation and filling in of struct derived_key_payload there.  Take a look at
user_preparse().  I would start by renaming fill_payload() to
derived_preparse() - it's almost exactly what you want.

You will also need to implement ->free_preparse().

You can then get rid of reserve_derived_payload() and just put the quota
amount into prep->quotalen and the payload into prep->payload.data[0].

derived_instantiate() can then be replaced with generic_key_instantiate.

Since derived_preparse() would be called prior to derived_update(), the latter
can just replace where prep->payload.data[0] points using
rcu_assign_keypointer() and then call_rcu() on the old payload.

David