Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
On Tue, May 12, 2020 at 4:03 PM David Howells wrote: > > Jason A. Donenfeld wrote: > > > So long as that ->update function: > > 1. Deletes the old on-disk data. > > 2. Deletes the old key from the inode. > > 3. Generates a new key using get_random_bytes. > > 4. Stores that new key in the inode. > > 5. Encrypts the updated data afresh with the new key. > > 6. Puts the updated data onto disk, > > > > then this is fine with me, and feel free to have my Acked-by if you > > want. But if it doesn't do that -- i.e. if it tries to reuse the old > > key or similar -- then this isn't fine. But it sounds like from what > > you've described that things are actually fine, in which case, I guess > > it makes sense to apply your patch ontop of mine and commit these. > > Yep. It calls big_key_destroy(), which clears away the old stuff just as when > a key is being destroyed, then generic_key_instantiate() just as when a key is > being set up. > > The key ID and the key metadata (ownership, perms, expiry) are maintained, but > the payload is just completely replaced. Okay, in that case, take my: Acked-by: Jason A. Donenfeld And then perhaps you can take both my patch and your addendum into keys-next. Jason
Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
Jason A. Donenfeld wrote: > So long as that ->update function: > 1. Deletes the old on-disk data. > 2. Deletes the old key from the inode. > 3. Generates a new key using get_random_bytes. > 4. Stores that new key in the inode. > 5. Encrypts the updated data afresh with the new key. > 6. Puts the updated data onto disk, > > then this is fine with me, and feel free to have my Acked-by if you > want. But if it doesn't do that -- i.e. if it tries to reuse the old > key or similar -- then this isn't fine. But it sounds like from what > you've described that things are actually fine, in which case, I guess > it makes sense to apply your patch ontop of mine and commit these. Yep. It calls big_key_destroy(), which clears away the old stuff just as when a key is being destroyed, then generic_key_instantiate() just as when a key is being set up. The key ID and the key metadata (ownership, perms, expiry) are maintained, but the payload is just completely replaced. David
Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
Hi David, So long as that ->update function: 1. Deletes the old on-disk data. 2. Deletes the old key from the inode. 3. Generates a new key using get_random_bytes. 4. Stores that new key in the inode. 5. Encrypts the updated data afresh with the new key. 6. Puts the updated data onto disk, then this is fine with me, and feel free to have my Acked-by if you want. But if it doesn't do that -- i.e. if it tries to reuse the old key or similar -- then this isn't fine. But it sounds like from what you've described that things are actually fine, in which case, I guess it makes sense to apply your patch ontop of mine and commit these. Jason
Re: [PATCH v3] security/keys: rewrite big_key crypto to use library interface
Jason A. Donenfeld wrote:
> - /* no ->update(); don't add it without changing big_key_crypt() nonce */
> + /* no ->update(); don't add it without changing chacha20poly1305's nonce
Note that ->update() doesn't have to modify the contents of the key, but can
just rather replace them entirely. See attached. The actual work is done in
big_key_preparse(); all big_key_update() has to do is apply it and dispose of
the old payload.
David
---
commit 724e76c185d517f35ead4f72f9958850c9218f4d
Author: David Howells
Date: Tue May 12 14:03:53 2020 +0100
keys: Implement update for the big_key type
Implement the ->update op for the big_key type.
Signed-off-by: David Howells
diff --git a/include/keys/big_key-type.h b/include/keys/big_key-type.h
index 3fee04f81439..988d90d77f53 100644
--- a/include/keys/big_key-type.h
+++ b/include/keys/big_key-type.h
@@ -18,5 +18,6 @@ extern void big_key_revoke(struct key *key);
extern void big_key_destroy(struct key *key);
extern void big_key_describe(const struct key *big_key, struct seq_file *m);
extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
+extern int big_key_update(struct key *key, struct key_preparsed_payload *prep);
#endif /* _KEYS_BIG_KEY_TYPE_H */
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index d43f3daab2b8..dd708e8f13c0 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -47,7 +47,7 @@ struct key_type key_type_big_key = {
.destroy= big_key_destroy,
.describe = big_key_describe,
.read = big_key_read,
- /* no ->update(); don't add it without changing chacha20poly1305's
nonce */
+ .update = big_key_update,
};
/*
@@ -191,6 +191,23 @@ void big_key_destroy(struct key *key)
key->payload.data[big_key_data] = NULL;
}
+/*
+ * Update a big key
+ */
+int big_key_update(struct key *key, struct key_preparsed_payload *prep)
+{
+ int ret;
+
+ ret = key_payload_reserve(key, prep->datalen);
+ if (ret < 0)
+ return ret;
+
+ if (key_is_positive(key))
+ big_key_destroy(key);
+
+ return generic_key_instantiate(key, prep);
+}
+
/*
* describe the big_key key
*/
[PATCH v3] security/keys: rewrite big_key crypto to use library interface
A while back, I noticed that the crypto and crypto API usage in big_keys
were entirely broken in multiple ways, so I rewrote it. Now, I'm
rewriting it again, but this time using the simpler ChaCha20Poly1305
library function. This makes the file considerably more simple; the
diffstat alone should justify this commit. It also should be faster,
since it no longer requires a mutex around the "aead api object" (nor
allocations), allowing us to encrypt multiple items in parallel. We also
benefit from being able to pass any type of pointer, so we can get rid
of the ridiculously complex custom page allocator that big_key really
doesn't need.
Cc: David Howells
Cc: Andy Lutomirski
Cc: Greg KH
Cc: Linus Torvalds
Cc: kernel-harden...@lists.openwall.com
Reviewed-by: Eric Biggers
Signed-off-by: Jason A. Donenfeld
---
Changes v2->v3:
- [Eric] Unify kernel_read/write handling in big_key_preparse and
big_key_read.
- [Eric] Update commit message.
Changes v1->v2:
- [Eric] Return -EBADMSG instead of -EINVAL if the authtag fails.
- [Eric] Select CONFIG_CRYPTO, since it's required by the LIB selection.
- [Eric] Zero out buffers that formerly contained either plaintext or
ciphertext keys.
- [Jason] If kernel_read() fails, return that error value, instead of
relying on the subsequent decryption to fail.
Note v1:
I finally got around to updating this patch from the mailing list posts
back in 2017-2018, using the library interface that we eventually
merged in 2019. I haven't retested this for functionality, but nothing
much has changed, so I suspect things should still be good to go.
security/keys/Kconfig | 3 +-
security/keys/big_key.c | 240 ++--
2 files changed, 35 insertions(+), 208 deletions(-)
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 47c041563d41..7da6c1b496f9 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -61,8 +61,7 @@ config BIG_KEYS
depends on KEYS
depends on TMPFS
select CRYPTO
- select CRYPTO_AES
- select CRYPTO_GCM
+ select CRYPTO_LIB_CHACHA20POLY1305
help
This option provides support for holding large keys within the kernel
(for example Kerberos ticket caches). The data may be stored out to
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 82008f900930..d43f3daab2b8 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -1,7 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/* Large capacity key type
*
- * Copyright (C) 2017 Jason A. Donenfeld . All Rights
Reserved.
+ * Copyright (C) 2017-2020 Jason A. Donenfeld . All Rights
Reserved.
* Copyright (C) 2013 Red Hat, Inc. All Rights Reserved.
* Written by David Howells (dhowe...@redhat.com)
*/
@@ -12,20 +12,10 @@
#include
#include
#include
-#include
#include
-#include
#include
#include
-#include
-#include
-
-struct big_key_buf {
- unsigned intnr_pages;
- void*virt;
- struct scatterlist *sg;
- struct page *pages[];
-};
+#include
/*
* Layout of key payload words.
@@ -37,14 +27,6 @@ enum {
big_key_len,
};
-/*
- * Crypto operation with big_key data
- */
-enum big_key_op {
- BIG_KEY_ENC,
- BIG_KEY_DEC,
-};
-
/*
* If the data is under this limit, there's no point creating a shm file to
* hold it as the permanently resident metadata for the shmem fs will be at
@@ -52,16 +34,6 @@ enum big_key_op {
*/
#define BIG_KEY_FILE_THRESHOLD (sizeof(struct inode) + sizeof(struct dentry))
-/*
- * Key size for big_key data encryption
- */
-#define ENC_KEY_SIZE 32
-
-/*
- * Authentication tag length
- */
-#define ENC_AUTHTAG_SIZE 16
-
/*
* big_key defined keys take an arbitrary string as the description and an
* arbitrary blob of data as the payload
@@ -75,136 +47,20 @@ struct key_type key_type_big_key = {
.destroy= big_key_destroy,
.describe = big_key_describe,
.read = big_key_read,
- /* no ->update(); don't add it without changing big_key_crypt() nonce */
+ /* no ->update(); don't add it without changing chacha20poly1305's
nonce */
};
-/*
- * Crypto names for big_key data authenticated encryption
- */
-static const char big_key_alg_name[] = "gcm(aes)";
-#define BIG_KEY_IV_SIZEGCM_AES_IV_SIZE
-
-/*
- * Crypto algorithms for big_key data authenticated encryption
- */
-static struct crypto_aead *big_key_aead;
-
-/*
- * Since changing the key affects the entire object, we need a mutex.
- */
-static DEFINE_MUTEX(big_key_aead_lock);
-
-/*
- * Encrypt/decrypt big_key data
- */
-static int big_key_crypt(enum big_key_op op, struct big_key_buf *buf, size_t
datalen, u8 *key)
-{
- int ret;
- struct aead_request *aead_req;
- /* We always use a zero nonce. The reason we can get away with this is
-* because we're
