[PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[Tadeusz Struk]

From: Tadeusz Struk <tadeusz.struk@intel.com>

Created on top of patchset from Stephan Mueller <smueller@chronox.de>
https://patchwork.kernel.org/patch/7877921/
https://patchwork.kernel.org/patch/7877971/
https://patchwork.kernel.org/patch/7877961/

This patch adds support for asymmetric key type to AF_ALG.
It will work as follows: A new PF_ALG socket options will be
added on top of existing ALG_SET_KEY and ALG_SET_PUBKEY, namely
ALG_SET_PUBKEY_ID and ALG_SET_KEY_ID for setting public and
private keys respectively. When these new options will be used
the user instead of providing the key material, will provide a
key id and the key itself will be obtained from kernel keyring
subsystem. The user will use the standard tools (keyctl tool
or the keyctl syscall) for key instantiation and to obtain the
key id. The key id can also be obtained by reading the
/proc/keys file.

When a key will be found, the request_key() function will
return a requested key. Next the asymmetric key subtype will be
used to obtain the public_key, which can be either a public key
or a private key from the cryptographic point of view, and the
key payload will be passed to the akcipher pf_alg subtype.
Pf_alg code will then call crypto API functions, either the
crypto_akcipher_set_priv_key or the crypto_akcipher_set_pub_key,
depending on the used option. Subsequently the asymmetric key
will be freed and return code returned back to the user.

Currently the interface will be restricted only to asymmetric
ciphers, but it can be extended later to work with symmetric
ciphers if required.

The assumption is that access rights for a given user will be
verified by the key subsystem so the pf_alg interface can call
the request_key() without checking if the user has appropriate
rights (Please verify this assumption).

Changes in v2:
Separate logic for setkey and setkey_id into two separate
functions as proposed by Stephan.

Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 crypto/af_alg.c             |   49 +++++++++++++++++++++++++++++++++++++++----
 include/uapi/linux/if_alg.h |    2 ++
 2 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 767a134..e96e8c9 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -22,6 +22,8 @@
 #include <linux/net.h>
 #include <linux/rwsem.h>
 #include <linux/security.h>
+#include <crypto/public_key.h>
+#include <keys/asymmetric-type.h>
 
 struct alg_type_list {
 	const struct af_alg_type *type;
@@ -172,8 +174,38 @@ static int alg_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	return 0;
 }
 
+static int alg_setkey_id(void *private, const u8 *key, unsigned int keylen,
+			 int (*setkey)(void *private, const u8 *key,
+				       unsigned int keylen))
+{
+	struct key *keyring;
+	struct public_key *pkey;
+	char key_name[12];
+	u32 keyid = *((u32 *)key);
+	int err;
+
+	sprintf(key_name, "id:%08x", keyid);
+	keyring = request_key(&key_type_asymmetric, key_name, NULL);
+
+	err = -ENOKEY;
+	if (IS_ERR(keyring))
+		goto out;
+
+	pkey = keyring->payload.data[asym_crypto];
+	if (!pkey) {
+		key_put(keyring);
+		goto out;
+	}
+
+	err = setkey(private, pkey->key, pkey->keylen);
+	key_put(keyring);
+
+out:
+	return err;
+}
+
 static int alg_setkey(struct sock *sk, char __user *ukey,
-		      unsigned int keylen,
+		      unsigned int keylen, bool key_id,
 		      int (*setkey)(void *private, const u8 *key,
 				    unsigned int keylen))
 {
@@ -192,7 +224,8 @@ static int alg_setkey(struct sock *sk, char __user *ukey,
 	if (copy_from_user(key, ukey, keylen))
 		goto out;
 
-	err = setkey(ask->private, key, keylen);
+	err = key_id ? alg_setkey_id(ask->private, key, keylen, setkey) :
+		       setkey(ask->private, key, keylen);
 
 out:
 	sock_kzfree_s(sk, key, keylen);
@@ -207,6 +240,8 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
 	struct alg_sock *ask = alg_sk(sk);
 	const struct af_alg_type *type;
 	int err = -ENOPROTOOPT;
+	bool key_id = ((optname == ALG_SET_PUBKEY_ID) ||
+		       (optname == ALG_SET_KEY_ID));
 
 	lock_sock(sk);
 	type = ask->type;
@@ -216,16 +251,22 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
 
 	switch (optname) {
 	case ALG_SET_KEY:
+	case ALG_SET_KEY_ID:
 		if (sock->state == SS_CONNECTED)
 			goto unlock;
 
-		err = alg_setkey(sk, optval, optlen, type->setkey);
+		/* ALG_SET_KEY_ID is only for akcipher */
+		if (!strcmp(type->name, "akcipher") && key_id)
+			goto unlock;
+
+		err = alg_setkey(sk, optval, optlen, key_id, type->setkey);
 		break;
 	case ALG_SET_PUBKEY:
+	case ALG_SET_PUBKEY_ID:
 		if (sock->state == SS_CONNECTED)
 			goto unlock;
 
-		err = alg_setkey(sk, optval, optlen, type->setpubkey);
+		err = alg_setkey(sk, optval, optlen, key_id, type->setpubkey);
 		break;
 	case ALG_SET_AEAD_AUTHSIZE:
 		if (sock->state == SS_CONNECTED)
diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
index 02e6162..0379766 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,8 @@ struct af_alg_iv {
 #define ALG_SET_AEAD_ASSOCLEN		4
 #define ALG_SET_AEAD_AUTHSIZE		5
 #define ALG_SET_PUBKEY			6
+#define ALG_SET_PUBKEY_ID		7
+#define ALG_SET_KEY_ID			8
 
 /* Operations */
 #define ALG_OP_DECRYPT			0

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[Tadeusz Struk]

Hi David, David and Marcel,
On 12/26/2015 07:50 AM, Tadeusz Struk wrote:
> From: Tadeusz Struk <tadeusz.struk-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Created on top of patchset from Stephan Mueller <smueller-T9tCv8IpfcWELgA04lAiVw@public.gmane.org>
> https://patchwork.kernel.org/patch/7877921/
> https://patchwork.kernel.org/patch/7877971/
> https://patchwork.kernel.org/patch/7877961/
> 
> This patch adds support for asymmetric key type to AF_ALG.
> It will work as follows: A new PF_ALG socket options will be
> added on top of existing ALG_SET_KEY and ALG_SET_PUBKEY, namely
> ALG_SET_PUBKEY_ID and ALG_SET_KEY_ID for setting public and
> private keys respectively. When these new options will be used
> the user instead of providing the key material, will provide a
> key id and the key itself will be obtained from kernel keyring
> subsystem. The user will use the standard tools (keyctl tool
> or the keyctl syscall) for key instantiation and to obtain the
> key id. The key id can also be obtained by reading the
> /proc/keys file.
> 
> When a key will be found, the request_key() function will
> return a requested key. Next the asymmetric key subtype will be
> used to obtain the public_key, which can be either a public key
> or a private key from the cryptographic point of view, and the
> key payload will be passed to the akcipher pf_alg subtype.
> Pf_alg code will then call crypto API functions, either the
> crypto_akcipher_set_priv_key or the crypto_akcipher_set_pub_key,
> depending on the used option. Subsequently the asymmetric key
> will be freed and return code returned back to the user.
> 
> Currently the interface will be restricted only to asymmetric
> ciphers, but it can be extended later to work with symmetric
> ciphers if required.
> 
> The assumption is that access rights for a given user will be
> verified by the key subsystem so the pf_alg interface can call
> the request_key() without checking if the user has appropriate
> rights (Please verify this assumption).
> 
> Changes in v2:
> Separate logic for setkey and setkey_id into two separate
> functions as proposed by Stephan.
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  crypto/af_alg.c             |   49 +++++++++++++++++++++++++++++++++++++++----
>  include/uapi/linux/if_alg.h |    2 ++
>  2 files changed, 47 insertions(+), 4 deletions(-)

Do you have any comments on this?
Based on your feedback after Stephan sent the v2 algif_akcipher patches
the conclusion was that having both setkey and setkey_id will be acceptable.
This is exactly what this patch does, so will the algif_akcipher patches
with this one on top work for you?
Thanks,
-- 
TS

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

On Sat, 2015-12-26 at 07:50 -0800, Tadeusz Struk wrote:
> +static int alg_setkey_id(void *private, const u8 *key, unsigned int keylen,
> +                        int (*setkey)(void *private, const u8 *key,
> +                                      unsigned int keylen))
> +{
> +       struct key *keyring;
> +       struct public_key *pkey;
> +       char key_name[12];
> +       u32 keyid = *((u32 *)key);
> +       int err;
> +
> +       sprintf(key_name, "id:%08x", keyid);
> +       keyring = request_key(&key_type_asymmetric, key_name, NULL);
> +
> +       err = -ENOKEY;
> +       if (IS_ERR(keyring))
> +               goto out;
> +
> +       pkey = keyring->payload.data[asym_crypto];
> +       if (!pkey) {
> +               key_put(keyring);
> +               goto out;
> +       }
> +
> +       err = setkey(private, pkey->key, pkey->keylen);
> +       key_put(keyring);
> +
> +out:
> +       return err;
> +}

This seems entirely wrong to me. You cannot just assume that the
private key data are available in software form to the kernel and can
be extracted.

Please ensure you test this with a key in a TPM, or in a SGX software
enclave or something like that.

David, is there a way to do that test purely in software without
needing hardware support? We know that the data might not actually be
present in all cases... is there an easy test for that case?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[David Howells]

Tadeusz Struk <tstruk@gmail.com> wrote:

> +	pkey = keyring->payload.data[asym_crypto];
> +	if (!pkey) {
> +		key_put(keyring);
> +		goto out;
> +	}
> +
> +	err = setkey(private, pkey->key, pkey->keylen);
> +	key_put(keyring);

Note that you may not assume that there's data there that you can use in this
manner.  The key might be a pointer to some hardware device such as a TPM.  I
have a TPM asymmetric subtype in progress.

I think this really needs to be driven from a keyctl() because you need to let
the asymmetric subtype decide how it wants to handle this.  I would suggest
adding KEYCTL_{ASYM_GETINFO,SIGN,VERIFY,ENCRYPT,DECRYPT} - the problem is how
to pass sufficient arguments, how to decrypt the private key and what metadata
needs to be passed vs what is inline with the data.

David

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[Tadeusz Struk]

Hi David,
On 01/13/2016 05:31 AM, David Howells wrote:
>> > +	pkey = keyring->payload.data[asym_crypto];
>> > +	if (!pkey) {
>> > +		key_put(keyring);
>> > +		goto out;
>> > +	}
>> > +
>> > +	err = setkey(private, pkey->key, pkey->keylen);
>> > +	key_put(keyring);
> Note that you may not assume that there's data there that you can use in this
> manner.  The key might be a pointer to some hardware device such as a TPM.  I
> have a TPM asymmetric subtype in progress.

So is there anything in place that can be used to tell what the key actually is?
The security/integrity/digsig_asymmetric.c is using this api in a similar way, so
if it is incorrect digsig_asymmetric shouldn't work neither.

> 
> I think this really needs to be driven from a keyctl() because you need to let
> the asymmetric subtype decide how it wants to handle this.  I would suggest
> adding KEYCTL_{ASYM_GETINFO,SIGN,VERIFY,ENCRYPT,DECRYPT} - the problem is how
> to pass sufficient arguments, how to decrypt the private key and what metadata
> needs to be passed vs what is inline with the data.

I agree, ideally keyctl should do the job for all the cases and request_key()
should just return a key data.
Thanks,
-- 
TS

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Wed, 2016-01-13 at 06:05 -0800, Tadeusz Struk wrote:
> 
> I agree, ideally keyctl should do the job for all the cases and
> request_key() should just return a key data.

No, you can NOT RELY ON HAVING THE KEY DATA. It might be in hardware.
You might have something which will perform sign/verify/encrypt/decrypt
operations *with* the key at your request, but which can never just
*give* you the key.

Any crypto API which relies on *having* the key is fundamentally wrong.

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[Tadeusz Struk]

On 01/13/2016 07:06 AM, David Woodhouse wrote:
> On Wed, 2016-01-13 at 06:05 -0800, Tadeusz Struk wrote:
>>
>> I agree, ideally keyctl should do the job for all the cases and
>> request_key() should just return a key data.
> 
> No, you can NOT RELY ON HAVING THE KEY DATA. It might be in hardware.

Ok, I get it now.

> You might have something which will perform sign/verify/encrypt/decrypt
> operations *with* the key at your request, but which can never just
> *give* you the key.
> 
> Any crypto API which relies on *having* the key is fundamentally wrong.
> 

All the crypto APIs out there rely on this.

I think the coupling of an algorithm to its key is the problem here.
Usually an algorithm should be able to work with any (valid) key.

The solution to this can be implemented on the crypto API. 
If the TMP driver would register its supported algorithms on the crypto API
and in the setkey function it would check if a key is a real key or this
"something" (probably a ptr to TMP dev instance?) then in the first
case it would fallback to an implementation that takes a key data.
In the second case it can do its thing whatever it is.

This will make it transparent to the users of both the request_key()
and the crypto API.
Thanks,
-- 
TS

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[David Howells]

Tadeusz Struk <tadeusz.struk@intel.com> wrote:

> I think the coupling of an algorithm to its key is the problem here.
> Usually an algorithm should be able to work with any (valid) key.

A key stored in hardware and used through that hardware won't necessarily
support all crypto operations - it may be restricted to just signing or just
encrypting for example.

> The solution to this can be implemented on the crypto API. 
> If the TMP driver would register its supported algorithms on the crypto API
> and in the setkey function it would check if a key is a real key or this
> "something" (probably a ptr to TMP dev instance?) then in the first
> case it would fallback to an implementation that takes a key data.
> In the second case it can do its thing whatever it is.

>From what Herbert has said, he won't go for that since the TPM keys are
restricted in the operations one can use the key for.

I think the crypto API is what we use when the key data is available to us in
the kernel - but it should be driven through the asymmetric key API.  You ask
the asymmetric key API to verify a key, say, then that will switch to the TPM
driver or to the software public key implementation.  The latter will then
load the appropriate crypto layer akcipher algorithm (eg. RSA) and use that -
where the akcipher implementation will either be a software one or will get
offloaded to some hardware that can do it - but that doesn't itself securely
hold the key.

David

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[David Howells]

David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> David, is there a way to do that test purely in software without
> needing hardware support? We know that the data might not actually be
> present in all cases... is there an easy test for that case?

I have written a user TPM driver that talks to a userspace TPM implementation
out of the backend.  It's been pushed to the TPM driver guy but I'm not sure
what became of it.  I'll chase it up.

David

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

On Wed, 2016-01-13 at 13:36 +0000, David Howells wrote:
> David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > David, is there a way to do that test purely in software without
> > needing hardware support? We know that the data might not actually be
> > present in all cases... is there an easy test for that case?
> 
> I have written a user TPM driver that talks to a userspace TPM implementation
> out of the backend.  It's been pushed to the TPM driver guy but I'm not sure
> what became of it.  I'll chase it up.

I was thinking of something a lot simpler — like a test hack with a key
type that just puts a *pointer* to the key data in the 'payload', to
ensure that nobody is violating the rules about directly touching the
payload (which should be private to the implementation).

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[Tadeusz Struk]

Hello David,
On 01/13/2016 04:27 AM, David Woodhouse wrote:
>> +static int alg_setkey_id(void *private, const u8 *key, unsigned int keylen,
>> > +                        int (*setkey)(void *private, const u8 *key,
>> > +                                      unsigned int keylen))
>> > +{
>> > +       struct key *keyring;
>> > +       struct public_key *pkey;
>> > +       char key_name[12];
>> > +       u32 keyid = *((u32 *)key);
>> > +       int err;
>> > +
>> > +       sprintf(key_name, "id:%08x", keyid);
>> > +       keyring = request_key(&key_type_asymmetric, key_name, NULL);
>> > +
>> > +       err = -ENOKEY;
>> > +       if (IS_ERR(keyring))
>> > +               goto out;
>> > +
>> > +       pkey = keyring->payload.data[asym_crypto];
>> > +       if (!pkey) {
>> > +               key_put(keyring);
>> > +               goto out;
>> > +       }
>> > +
>> > +       err = setkey(private, pkey->key, pkey->keylen);
>> > +       key_put(keyring);
>> > +
>> > +out:
>> > +       return err;
>> > +}
> This seems entirely wrong to me. You cannot just assume that the
> private key data are available in software form to the kernel and can
> be extracted.

Thanks for your feedback.
Yes, I'm aware that there might be something missing, and it might not
work in all the cases, but as far as the user interface is concerned
do you think having the ALG_SET_KEY, ALG_SET_PUBKEY along with
ALG_SET_PUBKEY_ID and ALG_SET_KEY_ID should be enough for all cases.
That's the main point of this patch - to get the user interface right.
The worst what can happen now if the keys need to be explicitly
extracted from a TPM HW is that the user will get ENOKEY error.
>From the other hand shouldn't that happen under the hood in request_key()
It doesn't look right to me that the pf_alg module has to directly
interact with the TPM.

> 
> Please ensure you test this with a key in a TPM, or in a SGX software
> enclave or something like that.

I don't have any HW for this. I only tested it with the keys instantiated
using the keyctl tool.

Thanks,
-- 
TS