* [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-21 10:00 ` Ondrej Mosnacek
0 siblings, 0 replies; 26+ messages in thread
From: Ondrej Mosnacek @ 2019-05-21 10:00 UTC (permalink / raw)
To: linux-crypto, Herbert Xu
Cc: keyrings, David Howells, Stephan Mueller, Milan Broz,
Ondrej Kozina, Daniel Zatovic
This patch adds new socket options to AF_ALG that allow setting key from
kernel keyring. For simplicity, each keyring key type (logon, user,
trusted, encrypted) has its own socket option name and the value is just
the key description string that identifies the key to be used. The key
description doesn't need to be NULL-terminated, but bytes after the
first zero byte are ignored.
Note that this patch also adds three socket option names that are
already defined and used in libkcapi [1], but have never been added to
the kernel...
Tested via libkcapi with keyring patches [2] applied (user and logon key
types only).
[1] https://github.com/smuellerDD/libkcapi
[2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
crypto/af_alg.c | 150 +++++++++++++++++++++++++++++++++++-
include/uapi/linux/if_alg.h | 7 ++
2 files changed, 156 insertions(+), 1 deletion(-)
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index edca0998b2a4..fd6699e4dc3d 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -12,11 +12,15 @@
*
*/
-#include <linux/atomic.h>
#include <crypto/if_alg.h>
+#include <keys/user-type.h>
+#include <keys/trusted-type.h>
+#include <keys/encrypted-type.h>
+#include <linux/atomic.h>
#include <linux/crypto.h>
#include <linux/init.h>
#include <linux/kernel.h>
+#include <linux/key.h>
#include <linux/list.h>
#include <linux/module.h>
#include <linux/net.h>
@@ -230,6 +234,108 @@ out:
return err;
}
+#ifdef CONFIG_KEYS
+struct alg_keyring_type {
+ struct key_type *key_type;
+ void (*key_from_payload)(void *payload, void **key, unsigned int *len);
+};
+
+static void alg_key_from_payload_user(void *payload, void **key,
+ unsigned int *len)
+{
+ struct user_key_payload *ukp = payload;
+
+ *key = ukp->data;
+ *len = ukp->datalen;
+}
+
+static const struct alg_keyring_type alg_keyring_type_logon = {
+ .key_type = &key_type_logon,
+ .key_from_payload = &alg_key_from_payload_user,
+};
+static const struct alg_keyring_type alg_keyring_type_user = {
+ .key_type = &key_type_user,
+ .key_from_payload = &alg_key_from_payload_user,
+};
+
+#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
+static void alg_key_from_payload_trusted(void *payload, void **key,
+ unsigned int *len)
+{
+ struct trusted_key_payload *tkp = payload;
+
+ *key = tkp->key;
+ *len = tkp->key_len;
+}
+
+static const struct alg_keyring_type alg_keyring_type_trusted = {
+ .key_type = &key_type_trusted,
+ .key_from_payload = &alg_key_from_payload_trusted,
+};
+#endif
+
+#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
+static void alg_key_from_payload_encrypted(void *payload, void **key,
+ unsigned int *len)
+{
+ struct encrypted_key_payload *ekp = payload;
+
+ *key = ekp->decrypted_data;
+ *len = ekp->decrypted_datalen;
+}
+
+static const struct alg_keyring_type alg_keyring_type_encrypted = {
+ .key_type = &key_type_encrypted,
+ .key_from_payload = &alg_key_from_payload_encrypted,
+};
+#endif
+
+static int alg_setkey_keyring(struct sock *sk,
+ const struct alg_keyring_type *type,
+ char __user *udesc, unsigned int desclen)
+{
+ struct alg_sock *ask = alg_sk(sk);
+ struct key *key;
+ char *desc;
+ void *payload;
+ void *key_data;
+ unsigned int key_len;
+ int err;
+
+ desc = sock_kmalloc(sk, desclen + 1, GFP_KERNEL);
+ if (!desc)
+ return -ENOMEM;
+
+ if (copy_from_user(desc, udesc, desclen)) {
+ sock_kzfree_s(sk, desc, desclen + 1);
+ return -EFAULT;
+ }
+ desc[desclen] = '\0';
+
+ key = request_key(type->key_type, desc, NULL);
+ sock_kzfree_s(sk, desc, desclen + 1);
+ if (IS_ERR(key))
+ return PTR_ERR(key);
+
+ down_read(&key->sem);
+
+ payload = dereference_key_locked(key);
+ if (!payload) {
+ err = -EKEYREVOKED;
+ goto out;
+ }
+
+ type->key_from_payload(payload, &key_data, &key_len);
+
+ err = ask->type->setkey(ask->private, key_data, key_len);
+
+out:
+ up_read(&key->sem);
+ key_put(key);
+ return err;
+}
+#endif /* CONFIG_KEYS */
+
static int alg_setsockopt(struct socket *sock, int level, int optname,
char __user *optval, unsigned int optlen)
{
@@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
goto unlock;
err = alg_setkey(sk, optval, optlen);
+#ifdef CONFIG_KEYS
+ break;
+ case ALG_SET_KEY_KEYRING_LOGON:
+ if (sock->state = SS_CONNECTED)
+ goto unlock;
+ if (!type->setkey)
+ goto unlock;
+
+ err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
+ optval, optlen);
+ break;
+ case ALG_SET_KEY_KEYRING_USER:
+ if (sock->state = SS_CONNECTED)
+ goto unlock;
+ if (!type->setkey)
+ goto unlock;
+
+ err = alg_setkey_keyring(sk, &alg_keyring_type_user,
+ optval, optlen);
+#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
+ break;
+ case ALG_SET_KEY_KEYRING_TRUSTED:
+ if (sock->state = SS_CONNECTED)
+ goto unlock;
+ if (!type->setkey)
+ goto unlock;
+
+ err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
+ optval, optlen);
+#endif
+#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
+ break;
+ case ALG_SET_KEY_KEYRING_ENCRYPTED:
+ if (sock->state = SS_CONNECTED)
+ goto unlock;
+ if (!type->setkey)
+ goto unlock;
+
+ err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
+ optval, optlen);
+#endif
+#endif /* CONFIG_KEYS */
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 bc2bcdec377b..f2d777901f00 100644
--- a/include/uapi/linux/if_alg.h
+++ b/include/uapi/linux/if_alg.h
@@ -35,6 +35,13 @@ struct af_alg_iv {
#define ALG_SET_OP 3
#define ALG_SET_AEAD_ASSOCLEN 4
#define ALG_SET_AEAD_AUTHSIZE 5
+#define ALG_SET_PUBKEY 6 /* reserved for future use */
+#define ALG_SET_DH_PARAMETERS 7 /* reserved for future use */
+#define ALG_SET_ECDH_CURVE 8 /* reserved for future use */
+#define ALG_SET_KEY_KEYRING_LOGON 9
+#define ALG_SET_KEY_KEYRING_USER 10
+#define ALG_SET_KEY_KEYRING_TRUSTED 11
+#define ALG_SET_KEY_KEYRING_ENCRYPTED 12
/* Operations */
#define ALG_OP_DECRYPT 0
--
2.20.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-21 10:00 ` Ondrej Mosnacek
@ 2019-05-21 10:47 ` Marcel Holtmann
-1 siblings, 0 replies; 26+ messages in thread
From: Marcel Holtmann @ 2019-05-21 10:47 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
Hi Ondrej,
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.
Regards
Marcel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-21 10:47 ` Marcel Holtmann
0 siblings, 0 replies; 26+ messages in thread
From: Marcel Holtmann @ 2019-05-21 10:47 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
Hi Ondrej,
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.
Regards
Marcel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-21 10:47 ` Marcel Holtmann
@ 2019-05-21 11:02 ` Ondrej Mosnacek
-1 siblings, 0 replies; 26+ messages in thread
From: Ondrej Mosnacek @ 2019-05-21 11:02 UTC (permalink / raw)
To: Marcel Holtmann
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
Hi Marcel,
On Tue, May 21, 2019 at 12:48 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Ondrej,
>
> > This patch adds new socket options to AF_ALG that allow setting key from
> > kernel keyring. For simplicity, each keyring key type (logon, user,
> > trusted, encrypted) has its own socket option name and the value is just
> > the key description string that identifies the key to be used. The key
> > description doesn't need to be NULL-terminated, but bytes after the
> > first zero byte are ignored.
>
> why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.
I was basing this on the approach taken by dm-crypt/cryptsetup, which
is actually the main target consumer for this feature (cryptsetup
needs to be able to encrypt/decrypt data using a keyring key (possibly
unreadable by userspace) without having to create a temporary dm-crypt
mapping, which requires CAP_SYSADMIN). I'm not sure why they didn't
just use key IDs there... @Milan/Ondrej, what was you motivation for
using key descriptions rather than key IDs?
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-21 11:02 ` Ondrej Mosnacek
0 siblings, 0 replies; 26+ messages in thread
From: Ondrej Mosnacek @ 2019-05-21 11:02 UTC (permalink / raw)
To: Marcel Holtmann
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
Hi Marcel,
On Tue, May 21, 2019 at 12:48 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Ondrej,
>
> > This patch adds new socket options to AF_ALG that allow setting key from
> > kernel keyring. For simplicity, each keyring key type (logon, user,
> > trusted, encrypted) has its own socket option name and the value is just
> > the key description string that identifies the key to be used. The key
> > description doesn't need to be NULL-terminated, but bytes after the
> > first zero byte are ignored.
>
> why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.
I was basing this on the approach taken by dm-crypt/cryptsetup, which
is actually the main target consumer for this feature (cryptsetup
needs to be able to encrypt/decrypt data using a keyring key (possibly
unreadable by userspace) without having to create a temporary dm-crypt
mapping, which requires CAP_SYSADMIN). I'm not sure why they didn't
just use key IDs there... @Milan/Ondrej, what was you motivation for
using key descriptions rather than key IDs?
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-21 11:02 ` Ondrej Mosnacek
@ 2019-05-21 11:30 ` Ondrej Kozina
-1 siblings, 0 replies; 26+ messages in thread
From: Ondrej Kozina @ 2019-05-21 11:30 UTC (permalink / raw)
To: Ondrej Mosnacek, Marcel Holtmann
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Daniel Zatovic
On 5/21/19 1:02 PM, Ondrej Mosnacek wrote:
> Hi Marcel,
>
> On Tue, May 21, 2019 at 12:48 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Ondrej,
>>
>>> This patch adds new socket options to AF_ALG that allow setting key from
>>> kernel keyring. For simplicity, each keyring key type (logon, user,
>>> trusted, encrypted) has its own socket option name and the value is just
>>> the key description string that identifies the key to be used. The key
>>> description doesn't need to be NULL-terminated, but bytes after the
>>> first zero byte are ignored.
>>
>> why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.
>
> I was basing this on the approach taken by dm-crypt/cryptsetup, which
> is actually the main target consumer for this feature (cryptsetup
> needs to be able to encrypt/decrypt data using a keyring key (possibly
> unreadable by userspace) without having to create a temporary dm-crypt
> mapping, which requires CAP_SYSADMIN). I'm not sure why they didn't
> just use key IDs there... @Milan/Ondrej, what was you motivation for
> using key descriptions rather than key IDs?
>
We decided to use key descriptions so that we could identify more
clearly devices managed by cryptsetup (thus 'cryptsetup:' prefix in our
descriptions put in dm-crypt table). I understood numeric ids were too
generic for this purpose. Also cryptsetup uses by default thread keyring
to upload keys in kernel. Such keys are obsolete the moment cryptsetup
process finishes.
I don't think it's any problem to go with IDs for your patch. IIUC, as
long as process has permission to access key metadata it can obtain also
current key id so it's not a big problem for as to adapt when we use kcapi.
Regards
O.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-21 11:30 ` Ondrej Kozina
0 siblings, 0 replies; 26+ messages in thread
From: Ondrej Kozina @ 2019-05-21 11:30 UTC (permalink / raw)
To: Ondrej Mosnacek, Marcel Holtmann
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Daniel Zatovic
On 5/21/19 1:02 PM, Ondrej Mosnacek wrote:
> Hi Marcel,
>
> On Tue, May 21, 2019 at 12:48 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Ondrej,
>>
>>> This patch adds new socket options to AF_ALG that allow setting key from
>>> kernel keyring. For simplicity, each keyring key type (logon, user,
>>> trusted, encrypted) has its own socket option name and the value is just
>>> the key description string that identifies the key to be used. The key
>>> description doesn't need to be NULL-terminated, but bytes after the
>>> first zero byte are ignored.
>>
>> why use the description instead the actual key id? I wonder if a single socket option and a struct providing the key type and key id might be more useful.
>
> I was basing this on the approach taken by dm-crypt/cryptsetup, which
> is actually the main target consumer for this feature (cryptsetup
> needs to be able to encrypt/decrypt data using a keyring key (possibly
> unreadable by userspace) without having to create a temporary dm-crypt
> mapping, which requires CAP_SYSADMIN). I'm not sure why they didn't
> just use key IDs there... @Milan/Ondrej, what was you motivation for
> using key descriptions rather than key IDs?
>
We decided to use key descriptions so that we could identify more
clearly devices managed by cryptsetup (thus 'cryptsetup:' prefix in our
descriptions put in dm-crypt table). I understood numeric ids were too
generic for this purpose. Also cryptsetup uses by default thread keyring
to upload keys in kernel. Such keys are obsolete the moment cryptsetup
process finishes.
I don't think it's any problem to go with IDs for your patch. IIUC, as
long as process has permission to access key metadata it can obtain also
current key id so it's not a big problem for as to adapt when we use kcapi.
Regards
O.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-21 10:00 ` Ondrej Mosnacek
@ 2019-05-21 19:15 ` Stephan Müller
-1 siblings, 0 replies; 26+ messages in thread
From: Stephan Müller @ 2019-05-21 19:15 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, Herbert Xu, keyrings, David Howells, Milan Broz,
Ondrej Kozina, Daniel Zatovic
Am Dienstag, 21. Mai 2019, 12:00:34 CEST schrieb Ondrej Mosnacek:
Hi Ondrej,
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
>
> Note that this patch also adds three socket option names that are
> already defined and used in libkcapi [1], but have never been added to
> the kernel...
>
> Tested via libkcapi with keyring patches [2] applied (user and logon key
> types only).
>
> [1] https://github.com/smuellerDD/libkcapi
> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Thank you!
Reviewed-by: Stephan Müller <smueller@chronox.de>
If the patch goes in, I will merge the libkcapi patch set and create a new
release.
Ciao
Stephan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-21 19:15 ` Stephan Müller
0 siblings, 0 replies; 26+ messages in thread
From: Stephan Müller @ 2019-05-21 19:15 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, Herbert Xu, keyrings, David Howells, Milan Broz,
Ondrej Kozina, Daniel Zatovic
Am Dienstag, 21. Mai 2019, 12:00:34 CEST schrieb Ondrej Mosnacek:
Hi Ondrej,
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
>
> Note that this patch also adds three socket option names that are
> already defined and used in libkcapi [1], but have never been added to
> the kernel...
>
> Tested via libkcapi with keyring patches [2] applied (user and logon key
> types only).
>
> [1] https://github.com/smuellerDD/libkcapi
> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Thank you!
Reviewed-by: Stephan Müller <smueller@chronox.de>
If the patch goes in, I will merge the libkcapi patch set and create a new
release.
Ciao
Stephan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-21 10:00 ` Ondrej Mosnacek
@ 2019-05-21 21:18 ` David Howells
-1 siblings, 0 replies; 26+ messages in thread
From: David Howells @ 2019-05-21 21:18 UTC (permalink / raw)
To: Marcel Holtmann
Cc: dhowells, Ondrej Mosnacek, linux-crypto, Herbert Xu, keyrings,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
Marcel Holtmann <marcel@holtmann.org> wrote:
> why use the description instead the actual key id? I wonder if a single
> socket option and a struct providing the key type and key id might be more
> useful.
If the key becomes invalid in some way, you can call request_key() again if
you have the description to get a new key. This is how afs and af_rxrpc do
things on the client side.
David
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-21 21:18 ` David Howells
0 siblings, 0 replies; 26+ messages in thread
From: David Howells @ 2019-05-21 21:18 UTC (permalink / raw)
To: Marcel Holtmann
Cc: dhowells, Ondrej Mosnacek, linux-crypto, Herbert Xu, keyrings,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
Marcel Holtmann <marcel@holtmann.org> wrote:
> why use the description instead the actual key id? I wonder if a single
> socket option and a struct providing the key type and key id might be more
> useful.
If the key becomes invalid in some way, you can call request_key() again if
you have the description to get a new key. This is how afs and af_rxrpc do
things on the client side.
David
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-21 10:00 ` Ondrej Mosnacek
@ 2019-05-25 3:10 ` Eric Biggers
-1 siblings, 0 replies; 26+ messages in thread
From: Eric Biggers @ 2019-05-25 3:10 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
>
> Note that this patch also adds three socket option names that are
> already defined and used in libkcapi [1], but have never been added to
> the kernel...
>
> Tested via libkcapi with keyring patches [2] applied (user and logon key
> types only).
>
> [1] https://github.com/smuellerDD/libkcapi
> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
The "interesting" thing about this is that given a key to which you have only
Search permission, you can request plaintext-ciphertext pairs with it using any
algorithm from the kernel's crypto API. So if there are any really broken
algorithms and they happen to take the correct length key, you can effectively
read the key. That's true even if you don't have Read permission on the key
and/or the key is of the "logon" type which doesn't have a ->read() method.
Since this is already also true for dm-crypt and maybe some other features in
the kernel, and there will be a new API for fscrypt that doesn't rely on "logon"
keys with Search access thus avoiding this problem and many others
(https://patchwork.kernel.org/cover/10951999/), I don't really care much whether
this patch is applied. But I wonder whether this is something you've actually
considered, and what security properties you think you are achieving by using
the Linux keyrings.
- Eric
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-25 3:10 ` Eric Biggers
0 siblings, 0 replies; 26+ messages in thread
From: Eric Biggers @ 2019-05-25 3:10 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
>
> Note that this patch also adds three socket option names that are
> already defined and used in libkcapi [1], but have never been added to
> the kernel...
>
> Tested via libkcapi with keyring patches [2] applied (user and logon key
> types only).
>
> [1] https://github.com/smuellerDD/libkcapi
> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
The "interesting" thing about this is that given a key to which you have only
Search permission, you can request plaintext-ciphertext pairs with it using any
algorithm from the kernel's crypto API. So if there are any really broken
algorithms and they happen to take the correct length key, you can effectively
read the key. That's true even if you don't have Read permission on the key
and/or the key is of the "logon" type which doesn't have a ->read() method.
Since this is already also true for dm-crypt and maybe some other features in
the kernel, and there will be a new API for fscrypt that doesn't rely on "logon"
keys with Search access thus avoiding this problem and many others
(https://patchwork.kernel.org/cover/10951999/), I don't really care much whether
this patch is applied. But I wonder whether this is something you've actually
considered, and what security properties you think you are achieving by using
the Linux keyrings.
- Eric
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-25 3:10 ` Eric Biggers
@ 2019-05-25 7:04 ` Milan Broz
-1 siblings, 0 replies; 26+ messages in thread
From: Milan Broz @ 2019-05-25 7:04 UTC (permalink / raw)
To: Eric Biggers, Ondrej Mosnacek
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
On 25/05/2019 05:10, Eric Biggers wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
>> This patch adds new socket options to AF_ALG that allow setting key from
>> kernel keyring. For simplicity, each keyring key type (logon, user,
>> trusted, encrypted) has its own socket option name and the value is just
>> the key description string that identifies the key to be used. The key
>> description doesn't need to be NULL-terminated, but bytes after the
>> first zero byte are ignored.
>>
>> Note that this patch also adds three socket option names that are
>> already defined and used in libkcapi [1], but have never been added to
>> the kernel...
>>
>> Tested via libkcapi with keyring patches [2] applied (user and logon key
>> types only).
>>
>> [1] https://github.com/smuellerDD/libkcapi
>> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> The "interesting" thing about this is that given a key to which you have only
> Search permission, you can request plaintext-ciphertext pairs with it using any
> algorithm from the kernel's crypto API. So if there are any really broken
> algorithms and they happen to take the correct length key, you can effectively
> read the key. That's true even if you don't have Read permission on the key
> and/or the key is of the "logon" type which doesn't have a ->read() method.
Yes, also if non-root user has access to some key in keyring, he can effectively
use it from another context (for example simulate dmcrypt and decrypt an image
of another user). But this is about policy of storing keys in keyrings.
> Since this is already also true for dm-crypt and maybe some other features in
> the kernel, and there will be a new API for fscrypt that doesn't rely on "logon"
> keys with Search access thus avoiding this problem and many others
> (https://patchwork.kernel.org/cover/10951999/), I don't really care much whether
> this patch is applied. But I wonder whether this is something you've actually
> considered, and what security properties you think you are achieving by using
> the Linux keyrings.
We use what kernel provides now. If there is a better way, we can switch to it.
The reason for using keyring for dmcrypt was to avoid sending key in plain
in every device-mapper ioctl structure. We use process keyring here, so
the key in keyring actually disappears after the setup utility finished its job.
The patch for crypto API is needed mainly for the "trusted" key type, that we
would like to support in dmcrypt as well. For integration in LUKS we need
to somehow validate that the key for particular dm-crypt device is valid.
If we cannot calculate a key digest directly (as LUKS does), we need to provide
some operation with the key from userspace (like HMAC) and compare known output.
This was the main reason for this patch. But I can imagine a lot of other
use cases where key in keyring actually simplifies things.
Milan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-25 7:04 ` Milan Broz
0 siblings, 0 replies; 26+ messages in thread
From: Milan Broz @ 2019-05-25 7:04 UTC (permalink / raw)
To: Eric Biggers, Ondrej Mosnacek
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
On 25/05/2019 05:10, Eric Biggers wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
>> This patch adds new socket options to AF_ALG that allow setting key from
>> kernel keyring. For simplicity, each keyring key type (logon, user,
>> trusted, encrypted) has its own socket option name and the value is just
>> the key description string that identifies the key to be used. The key
>> description doesn't need to be NULL-terminated, but bytes after the
>> first zero byte are ignored.
>>
>> Note that this patch also adds three socket option names that are
>> already defined and used in libkcapi [1], but have never been added to
>> the kernel...
>>
>> Tested via libkcapi with keyring patches [2] applied (user and logon key
>> types only).
>>
>> [1] https://github.com/smuellerDD/libkcapi
>> [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
>>
>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> The "interesting" thing about this is that given a key to which you have only
> Search permission, you can request plaintext-ciphertext pairs with it using any
> algorithm from the kernel's crypto API. So if there are any really broken
> algorithms and they happen to take the correct length key, you can effectively
> read the key. That's true even if you don't have Read permission on the key
> and/or the key is of the "logon" type which doesn't have a ->read() method.
Yes, also if non-root user has access to some key in keyring, he can effectively
use it from another context (for example simulate dmcrypt and decrypt an image
of another user). But this is about policy of storing keys in keyrings.
> Since this is already also true for dm-crypt and maybe some other features in
> the kernel, and there will be a new API for fscrypt that doesn't rely on "logon"
> keys with Search access thus avoiding this problem and many others
> (https://patchwork.kernel.org/cover/10951999/), I don't really care much whether
> this patch is applied. But I wonder whether this is something you've actually
> considered, and what security properties you think you are achieving by using
> the Linux keyrings.
We use what kernel provides now. If there is a better way, we can switch to it.
The reason for using keyring for dmcrypt was to avoid sending key in plain
in every device-mapper ioctl structure. We use process keyring here, so
the key in keyring actually disappears after the setup utility finished its job.
The patch for crypto API is needed mainly for the "trusted" key type, that we
would like to support in dmcrypt as well. For integration in LUKS we need
to somehow validate that the key for particular dm-crypt device is valid.
If we cannot calculate a key digest directly (as LUKS does), we need to provide
some operation with the key from userspace (like HMAC) and compare known output.
This was the main reason for this patch. But I can imagine a lot of other
use cases where key in keyring actually simplifies things.
Milan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-25 3:10 ` Eric Biggers
@ 2019-05-29 13:54 ` Ondrej Mosnacek
-1 siblings, 0 replies; 26+ messages in thread
From: Ondrej Mosnacek @ 2019-05-29 13:54 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
On Sat, May 25, 2019 at 5:10 AM Eric Biggers <ebiggers@kernel.org> wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> > This patch adds new socket options to AF_ALG that allow setting key from
> > kernel keyring. For simplicity, each keyring key type (logon, user,
> > trusted, encrypted) has its own socket option name and the value is just
> > the key description string that identifies the key to be used. The key
> > description doesn't need to be NULL-terminated, but bytes after the
> > first zero byte are ignored.
> >
> > Note that this patch also adds three socket option names that are
> > already defined and used in libkcapi [1], but have never been added to
> > the kernel...
> >
> > Tested via libkcapi with keyring patches [2] applied (user and logon key
> > types only).
> >
> > [1] https://github.com/smuellerDD/libkcapi
> > [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> The "interesting" thing about this is that given a key to which you have only
> Search permission, you can request plaintext-ciphertext pairs with it using any
> algorithm from the kernel's crypto API. So if there are any really broken
> algorithms and they happen to take the correct length key, you can effectively
> read the key. That's true even if you don't have Read permission on the key
> and/or the key is of the "logon" type which doesn't have a ->read() method.
Well, initially I was looking for a "KEY_NEED_USE" permission that
would allow using the key for encryption/decryption, but not to
actually read it. But then I was told by some people that the
KEY_NEED_SEARCH permission already serves exactly this purpose (i.e.
that when you can find the key, it means you can use it). I would
imagine that any practical use case for trusted keys would involve
encrypting/decrypting some data with the key (maybe not flowing
directly from/to userspace, but what use is a key with which you can
encrypt only some "internal" data...?), so I'm not sure where we want
to draw the boundary of what is safe to do with (userspace-unreadable
but findable) keyring keys... Maybe the keyring API needs some way to
control the intended usage of each key (something a bit like the "key
usage" in X.509 certificates [1]) - so you can e.g. mark some key to
be used for XYZ, but not for AF_ALG or dm-crypt...
Either way, I agree that this functionality opens up a potential
security hole (in that it makes it much more likely that a
vulnerability in the crypto drivers or crypto algorithms themselves
can reveal the value of a key that is not supposed to be readable by
userspace). However, I'm not sure how to mitigate this without some
new "KEY_NEED_PROCESS_ARBITRARY_DATA" permission or something... For
now I can at least add a Kconfig option to enable/disable keyring
support in AF_ALG so that people/distros who want both keyring and
AF_ALG enabled, but do not want to expose keyring keys via AF_ALG, can
just disable it.
BTW, I'm still undecided if I should convert this patch to use key IDs
rather than descriptions, but I tend to prefer to stay with the
current approach (mainly because it would be a lot of effort to
rewrite everything :)
[1] https://tools.ietf.org/html/rfc5280#section-4.2.1.3
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-29 13:54 ` Ondrej Mosnacek
0 siblings, 0 replies; 26+ messages in thread
From: Ondrej Mosnacek @ 2019-05-29 13:54 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-crypto, Herbert Xu, keyrings, David Howells,
Stephan Mueller, Milan Broz, Ondrej Kozina, Daniel Zatovic
On Sat, May 25, 2019 at 5:10 AM Eric Biggers <ebiggers@kernel.org> wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> > This patch adds new socket options to AF_ALG that allow setting key from
> > kernel keyring. For simplicity, each keyring key type (logon, user,
> > trusted, encrypted) has its own socket option name and the value is just
> > the key description string that identifies the key to be used. The key
> > description doesn't need to be NULL-terminated, but bytes after the
> > first zero byte are ignored.
> >
> > Note that this patch also adds three socket option names that are
> > already defined and used in libkcapi [1], but have never been added to
> > the kernel...
> >
> > Tested via libkcapi with keyring patches [2] applied (user and logon key
> > types only).
> >
> > [1] https://github.com/smuellerDD/libkcapi
> > [2] https://github.com/WOnder93/libkcapi/compare/f283458...1fb501c
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> The "interesting" thing about this is that given a key to which you have only
> Search permission, you can request plaintext-ciphertext pairs with it using any
> algorithm from the kernel's crypto API. So if there are any really broken
> algorithms and they happen to take the correct length key, you can effectively
> read the key. That's true even if you don't have Read permission on the key
> and/or the key is of the "logon" type which doesn't have a ->read() method.
Well, initially I was looking for a "KEY_NEED_USE" permission that
would allow using the key for encryption/decryption, but not to
actually read it. But then I was told by some people that the
KEY_NEED_SEARCH permission already serves exactly this purpose (i.e.
that when you can find the key, it means you can use it). I would
imagine that any practical use case for trusted keys would involve
encrypting/decrypting some data with the key (maybe not flowing
directly from/to userspace, but what use is a key with which you can
encrypt only some "internal" data...?), so I'm not sure where we want
to draw the boundary of what is safe to do with (userspace-unreadable
but findable) keyring keys... Maybe the keyring API needs some way to
control the intended usage of each key (something a bit like the "key
usage" in X.509 certificates [1]) - so you can e.g. mark some key to
be used for XYZ, but not for AF_ALG or dm-crypt...
Either way, I agree that this functionality opens up a potential
security hole (in that it makes it much more likely that a
vulnerability in the crypto drivers or crypto algorithms themselves
can reveal the value of a key that is not supposed to be readable by
userspace). However, I'm not sure how to mitigate this without some
new "KEY_NEED_PROCESS_ARBITRARY_DATA" permission or something... For
now I can at least add a Kconfig option to enable/disable keyring
support in AF_ALG so that people/distros who want both keyring and
AF_ALG enabled, but do not want to expose keyring keys via AF_ALG, can
just disable it.
BTW, I'm still undecided if I should convert this patch to use key IDs
rather than descriptions, but I tend to prefer to stay with the
current approach (mainly because it would be a lot of effort to
rewrite everything :)
[1] https://tools.ietf.org/html/rfc5280#section-4.2.1.3
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-21 10:00 ` Ondrej Mosnacek
@ 2019-05-30 7:14 ` Herbert Xu
-1 siblings, 0 replies; 26+ messages in thread
From: Herbert Xu @ 2019-05-30 7:14 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, keyrings, David Howells, Stephan Mueller,
Milan Broz, Ondrej Kozina, Daniel Zatovic
On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
>
> @@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
> goto unlock;
>
> err = alg_setkey(sk, optval, optlen);
> +#ifdef CONFIG_KEYS
> + break;
> + case ALG_SET_KEY_KEYRING_LOGON:
> + if (sock->state == SS_CONNECTED)
> + goto unlock;
> + if (!type->setkey)
> + goto unlock;
> +
> + err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
> + optval, optlen);
> + break;
> + case ALG_SET_KEY_KEYRING_USER:
> + if (sock->state == SS_CONNECTED)
> + goto unlock;
> + if (!type->setkey)
> + goto unlock;
> +
> + err = alg_setkey_keyring(sk, &alg_keyring_type_user,
> + optval, optlen);
> +#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
> + break;
> + case ALG_SET_KEY_KEYRING_TRUSTED:
> + if (sock->state == SS_CONNECTED)
> + goto unlock;
> + if (!type->setkey)
> + goto unlock;
> +
> + err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
> + optval, optlen);
> +#endif
> +#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
> + break;
> + case ALG_SET_KEY_KEYRING_ENCRYPTED:
> + if (sock->state == SS_CONNECTED)
> + goto unlock;
> + if (!type->setkey)
> + goto unlock;
> +
> + err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
> + optval, optlen);
> +#endif
> +#endif /* CONFIG_KEYS */
> break;
What's with the funky placement of "break" outside of the ifdefs?
> diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> index bc2bcdec377b..f2d777901f00 100644
> --- a/include/uapi/linux/if_alg.h
> +++ b/include/uapi/linux/if_alg.h
> @@ -35,6 +35,13 @@ struct af_alg_iv {
> #define ALG_SET_OP 3
> #define ALG_SET_AEAD_ASSOCLEN 4
> #define ALG_SET_AEAD_AUTHSIZE 5
> +#define ALG_SET_PUBKEY 6 /* reserved for future use */
> +#define ALG_SET_DH_PARAMETERS 7 /* reserved for future use */
> +#define ALG_SET_ECDH_CURVE 8 /* reserved for future use */
Why do you need to reserve these values?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-30 7:14 ` Herbert Xu
0 siblings, 0 replies; 26+ messages in thread
From: Herbert Xu @ 2019-05-30 7:14 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, keyrings, David Howells, Stephan Mueller,
Milan Broz, Ondrej Kozina, Daniel Zatovic
On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
>
> @@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
> goto unlock;
>
> err = alg_setkey(sk, optval, optlen);
> +#ifdef CONFIG_KEYS
> + break;
> + case ALG_SET_KEY_KEYRING_LOGON:
> + if (sock->state = SS_CONNECTED)
> + goto unlock;
> + if (!type->setkey)
> + goto unlock;
> +
> + err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
> + optval, optlen);
> + break;
> + case ALG_SET_KEY_KEYRING_USER:
> + if (sock->state = SS_CONNECTED)
> + goto unlock;
> + if (!type->setkey)
> + goto unlock;
> +
> + err = alg_setkey_keyring(sk, &alg_keyring_type_user,
> + optval, optlen);
> +#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
> + break;
> + case ALG_SET_KEY_KEYRING_TRUSTED:
> + if (sock->state = SS_CONNECTED)
> + goto unlock;
> + if (!type->setkey)
> + goto unlock;
> +
> + err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
> + optval, optlen);
> +#endif
> +#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
> + break;
> + case ALG_SET_KEY_KEYRING_ENCRYPTED:
> + if (sock->state = SS_CONNECTED)
> + goto unlock;
> + if (!type->setkey)
> + goto unlock;
> +
> + err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
> + optval, optlen);
> +#endif
> +#endif /* CONFIG_KEYS */
> break;
What's with the funky placement of "break" outside of the ifdefs?
> diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> index bc2bcdec377b..f2d777901f00 100644
> --- a/include/uapi/linux/if_alg.h
> +++ b/include/uapi/linux/if_alg.h
> @@ -35,6 +35,13 @@ struct af_alg_iv {
> #define ALG_SET_OP 3
> #define ALG_SET_AEAD_ASSOCLEN 4
> #define ALG_SET_AEAD_AUTHSIZE 5
> +#define ALG_SET_PUBKEY 6 /* reserved for future use */
> +#define ALG_SET_DH_PARAMETERS 7 /* reserved for future use */
> +#define ALG_SET_ECDH_CURVE 8 /* reserved for future use */
Why do you need to reserve these values?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-30 7:14 ` Herbert Xu
@ 2019-05-30 11:35 ` Ondrej Mosnacek
-1 siblings, 0 replies; 26+ messages in thread
From: Ondrej Mosnacek @ 2019-05-30 11:35 UTC (permalink / raw)
To: Herbert Xu
Cc: linux-crypto, keyrings, David Howells, Stephan Mueller,
Milan Broz, Ondrej Kozina, Daniel Zatovic
On Thu, May 30, 2019 at 10:12 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> >
> > @@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
> > goto unlock;
> >
> > err = alg_setkey(sk, optval, optlen);
> > +#ifdef CONFIG_KEYS
> > + break;
> > + case ALG_SET_KEY_KEYRING_LOGON:
> > + if (sock->state == SS_CONNECTED)
> > + goto unlock;
> > + if (!type->setkey)
> > + goto unlock;
> > +
> > + err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
> > + optval, optlen);
> > + break;
> > + case ALG_SET_KEY_KEYRING_USER:
> > + if (sock->state == SS_CONNECTED)
> > + goto unlock;
> > + if (!type->setkey)
> > + goto unlock;
> > +
> > + err = alg_setkey_keyring(sk, &alg_keyring_type_user,
> > + optval, optlen);
> > +#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
> > + break;
> > + case ALG_SET_KEY_KEYRING_TRUSTED:
> > + if (sock->state == SS_CONNECTED)
> > + goto unlock;
> > + if (!type->setkey)
> > + goto unlock;
> > +
> > + err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
> > + optval, optlen);
> > +#endif
> > +#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
> > + break;
> > + case ALG_SET_KEY_KEYRING_ENCRYPTED:
> > + if (sock->state == SS_CONNECTED)
> > + goto unlock;
> > + if (!type->setkey)
> > + goto unlock;
> > +
> > + err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
> > + optval, optlen);
> > +#endif
> > +#endif /* CONFIG_KEYS */
> > break;
>
> What's with the funky placement of "break" outside of the ifdefs?
I swear that checkpatch.pl was complaining when I did it the normal
way, but now I tried it again and it isn't complaining anymore...
Perhaps in the meantime I rebased onto a more recent tree where this
checkpatch.pl quirk has been fixed... I'll remove the funk in future
revisions.
>
> > diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> > index bc2bcdec377b..f2d777901f00 100644
> > --- a/include/uapi/linux/if_alg.h
> > +++ b/include/uapi/linux/if_alg.h
> > @@ -35,6 +35,13 @@ struct af_alg_iv {
> > #define ALG_SET_OP 3
> > #define ALG_SET_AEAD_ASSOCLEN 4
> > #define ALG_SET_AEAD_AUTHSIZE 5
> > +#define ALG_SET_PUBKEY 6 /* reserved for future use */
> > +#define ALG_SET_DH_PARAMETERS 7 /* reserved for future use */
> > +#define ALG_SET_ECDH_CURVE 8 /* reserved for future use */
>
> Why do you need to reserve these values?
Because libkcapi already assumes these values [1] and has code that
uses them. Reserving will allow existing builds of libkcapi to work
correctly once the functionality actually lands in the kernel (if that
ever happens). Of course, it is libkcapi's fault to assume values for
these symbols (in released code) before they are commited in the
kernel, but it seemed easier to just add them along with this patch
rather than creating a confusing situation.
[1] https://github.com/smuellerDD/libkcapi/blob/master/lib/internal.h#L54
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-30 11:35 ` Ondrej Mosnacek
0 siblings, 0 replies; 26+ messages in thread
From: Ondrej Mosnacek @ 2019-05-30 11:35 UTC (permalink / raw)
To: Herbert Xu
Cc: linux-crypto, keyrings, David Howells, Stephan Mueller,
Milan Broz, Ondrej Kozina, Daniel Zatovic
On Thu, May 30, 2019 at 10:12 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, May 21, 2019 at 12:00:34PM +0200, Ondrej Mosnacek wrote:
> >
> > @@ -256,6 +362,48 @@ static int alg_setsockopt(struct socket *sock, int level, int optname,
> > goto unlock;
> >
> > err = alg_setkey(sk, optval, optlen);
> > +#ifdef CONFIG_KEYS
> > + break;
> > + case ALG_SET_KEY_KEYRING_LOGON:
> > + if (sock->state = SS_CONNECTED)
> > + goto unlock;
> > + if (!type->setkey)
> > + goto unlock;
> > +
> > + err = alg_setkey_keyring(sk, &alg_keyring_type_logon,
> > + optval, optlen);
> > + break;
> > + case ALG_SET_KEY_KEYRING_USER:
> > + if (sock->state = SS_CONNECTED)
> > + goto unlock;
> > + if (!type->setkey)
> > + goto unlock;
> > +
> > + err = alg_setkey_keyring(sk, &alg_keyring_type_user,
> > + optval, optlen);
> > +#if IS_REACHABLE(CONFIG_TRUSTED_KEYS)
> > + break;
> > + case ALG_SET_KEY_KEYRING_TRUSTED:
> > + if (sock->state = SS_CONNECTED)
> > + goto unlock;
> > + if (!type->setkey)
> > + goto unlock;
> > +
> > + err = alg_setkey_keyring(sk, &alg_keyring_type_trusted,
> > + optval, optlen);
> > +#endif
> > +#if IS_REACHABLE(CONFIG_ENCRYPTED_KEYS)
> > + break;
> > + case ALG_SET_KEY_KEYRING_ENCRYPTED:
> > + if (sock->state = SS_CONNECTED)
> > + goto unlock;
> > + if (!type->setkey)
> > + goto unlock;
> > +
> > + err = alg_setkey_keyring(sk, &alg_keyring_type_encrypted,
> > + optval, optlen);
> > +#endif
> > +#endif /* CONFIG_KEYS */
> > break;
>
> What's with the funky placement of "break" outside of the ifdefs?
I swear that checkpatch.pl was complaining when I did it the normal
way, but now I tried it again and it isn't complaining anymore...
Perhaps in the meantime I rebased onto a more recent tree where this
checkpatch.pl quirk has been fixed... I'll remove the funk in future
revisions.
>
> > diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> > index bc2bcdec377b..f2d777901f00 100644
> > --- a/include/uapi/linux/if_alg.h
> > +++ b/include/uapi/linux/if_alg.h
> > @@ -35,6 +35,13 @@ struct af_alg_iv {
> > #define ALG_SET_OP 3
> > #define ALG_SET_AEAD_ASSOCLEN 4
> > #define ALG_SET_AEAD_AUTHSIZE 5
> > +#define ALG_SET_PUBKEY 6 /* reserved for future use */
> > +#define ALG_SET_DH_PARAMETERS 7 /* reserved for future use */
> > +#define ALG_SET_ECDH_CURVE 8 /* reserved for future use */
>
> Why do you need to reserve these values?
Because libkcapi already assumes these values [1] and has code that
uses them. Reserving will allow existing builds of libkcapi to work
correctly once the functionality actually lands in the kernel (if that
ever happens). Of course, it is libkcapi's fault to assume values for
these symbols (in released code) before they are commited in the
kernel, but it seemed easier to just add them along with this patch
rather than creating a confusing situation.
[1] https://github.com/smuellerDD/libkcapi/blob/master/lib/internal.h#L54
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-30 11:35 ` Ondrej Mosnacek
@ 2019-05-30 13:22 ` Herbert Xu
-1 siblings, 0 replies; 26+ messages in thread
From: Herbert Xu @ 2019-05-30 13:22 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, keyrings, David Howells, Stephan Mueller,
Milan Broz, Ondrej Kozina, Daniel Zatovic
On Thu, May 30, 2019 at 01:35:06PM +0200, Ondrej Mosnacek wrote:
>
> Because libkcapi already assumes these values [1] and has code that
> uses them. Reserving will allow existing builds of libkcapi to work
> correctly once the functionality actually lands in the kernel (if that
> ever happens). Of course, it is libkcapi's fault to assume values for
> these symbols (in released code) before they are commited in the
> kernel, but it seemed easier to just add them along with this patch
> rather than creating a confusing situation.
>
> [1] https://github.com/smuellerDD/libkcapi/blob/master/lib/internal.h#L54
OK but please just leave a gap (or a comment) rather than actually
adding these reserved symbols to the kernel.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-30 13:22 ` Herbert Xu
0 siblings, 0 replies; 26+ messages in thread
From: Herbert Xu @ 2019-05-30 13:22 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: linux-crypto, keyrings, David Howells, Stephan Mueller,
Milan Broz, Ondrej Kozina, Daniel Zatovic
On Thu, May 30, 2019 at 01:35:06PM +0200, Ondrej Mosnacek wrote:
>
> Because libkcapi already assumes these values [1] and has code that
> uses them. Reserving will allow existing builds of libkcapi to work
> correctly once the functionality actually lands in the kernel (if that
> ever happens). Of course, it is libkcapi's fault to assume values for
> these symbols (in released code) before they are commited in the
> kernel, but it seemed easier to just add them along with this patch
> rather than creating a confusing situation.
>
> [1] https://github.com/smuellerDD/libkcapi/blob/master/lib/internal.h#L54
OK but please just leave a gap (or a comment) rather than actually
adding these reserved symbols to the kernel.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] crypto: af_alg - implement keyring support
2019-05-21 10:00 ` Ondrej Mosnacek
@ 2019-05-30 7:39 ` Milan Broz
-1 siblings, 0 replies; 26+ messages in thread
From: Milan Broz @ 2019-05-30 7:39 UTC (permalink / raw)
To: Ondrej Mosnacek, linux-crypto, Herbert Xu
Cc: keyrings, David Howells, Stephan Mueller, Ondrej Kozina, Daniel Zatovic
Hi,
On 21/05/2019 12:00, Ondrej Mosnacek wrote:
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
There is one nasty problem with this approach (we hit the same in dmcrypt now).
These lines cause hard module dependence on trusted.ko and encrypted.ko
modules (key_type_* are exported symbols):
+static const struct alg_keyring_type alg_keyring_type_trusted = {
+ .key_type = &key_type_trusted,...
...
+ .key_type = &key_type_encrypted,
I do not think this is what we actually want - the dependence should be dynamic,
the modules should be loaded per-request...
I asked David Howells, and seems kernel keyring does not have such
interface yet. (There is an internal lookup function already though.)
So until such a lookup interface is present, and the patch is ported to it,
I think this patch adds module dependency that should not be there.
Milan
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH] crypto: af_alg - implement keyring support
@ 2019-05-30 7:39 ` Milan Broz
0 siblings, 0 replies; 26+ messages in thread
From: Milan Broz @ 2019-05-30 7:39 UTC (permalink / raw)
To: Ondrej Mosnacek, linux-crypto, Herbert Xu
Cc: keyrings, David Howells, Stephan Mueller, Ondrej Kozina, Daniel Zatovic
Hi,
On 21/05/2019 12:00, Ondrej Mosnacek wrote:
> This patch adds new socket options to AF_ALG that allow setting key from
> kernel keyring. For simplicity, each keyring key type (logon, user,
> trusted, encrypted) has its own socket option name and the value is just
> the key description string that identifies the key to be used. The key
> description doesn't need to be NULL-terminated, but bytes after the
> first zero byte are ignored.
There is one nasty problem with this approach (we hit the same in dmcrypt now).
These lines cause hard module dependence on trusted.ko and encrypted.ko
modules (key_type_* are exported symbols):
+static const struct alg_keyring_type alg_keyring_type_trusted = {
+ .key_type = &key_type_trusted,...
...
+ .key_type = &key_type_encrypted,
I do not think this is what we actually want - the dependence should be dynamic,
the modules should be loaded per-request...
I asked David Howells, and seems kernel keyring does not have such
interface yet. (There is an internal lookup function already though.)
So until such a lookup interface is present, and the patch is ported to it,
I think this patch adds module dependency that should not be there.
Milan
^ permalink raw reply [flat|nested] 26+ messages in thread