| Message ID | 20180110124418.24385-1-git@andred.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi André, On Wed, Jan 10, 2018 at 12:44:16PM +0000, André Draszik wrote: > This is heavily based on commit 79a73d188726 > ("encrypted-keys: add ecryptfs format support"). > > The 'encrypted' key type defines its own payload format which contains a > symmetric key randomly generated that cannot be used directly by the > fscrypt subsystem, because it instead expects an fscrypt_key structure. > > This patch introduces the new format 'fscrypt' that allows to store an > fscrypt_key structure inside the encrypted key payload containing > a randomly generated symmetric key, as the same for the format 'default' > and 'ecryptfs'. > > More details about the usage of encrypted keys with the fscrypt > subsystem can be found in the file 'Documentation/security/keys/fscrypt.rst'. > I don't think a new encrypted-key format is needed. fscrypt really only needs the raw key. The fact that fscrypt uses 'struct fscrypt_key' for the key payloads is a mistake, given that a raw byte array would work just as well. In particular, the 'size' field is redundant, since a 'struct key' knows the size of its payload; and the 'mode' field is meaningless and therefore is ignored. Also since there are no reserved fields the only way we would ever be able to add anything new to 'struct fscrypt_key' is by doing a hack where we put an invalid value in the 'size' field, which would be ugly. Also I have proposed an fscrypt ioctl to add keys to a filesystem-level keyring, and it doesn't use 'struct fscrypt_key' at all: https://marc.info/?l=linux-fsdevel&m=150879505206393 So I think you should just use the "default" encrypted-key format, where the payload is just the raw key. fscrypt can very easily be updated to work with such keys. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Eric for the review! On Wed, 2018-01-10 at 20:00 -0800, Eric Biggers wrote: > Hi André, > > On Wed, Jan 10, 2018 at 12:44:16PM +0000, André Draszik wrote: > > This is heavily based on commit 79a73d188726 > > ("encrypted-keys: add ecryptfs format support"). > > > > The 'encrypted' key type defines its own payload format which contains a > > symmetric key randomly generated that cannot be used directly by the > > fscrypt subsystem, because it instead expects an fscrypt_key structure. > > > > This patch introduces the new format 'fscrypt' that allows to store an > > fscrypt_key structure inside the encrypted key payload containing > > a randomly generated symmetric key, as the same for the format 'default' > > and 'ecryptfs'. > > > > More details about the usage of encrypted keys with the fscrypt > > subsystem can be found in the file > > 'Documentation/security/keys/fscrypt.rst'. > > > > I don't think a new encrypted-key format is needed. fscrypt really only > needs > the raw key. This was actually my original approach, but I thought it might potentially be useful to have a new encrypted-key type to be able to do verification of parameters (e.g. key size) early. Additionally, because the type is then encoded in the blob stored in the file system (keyctl pipe), it'd be easy to spot incompatibilities in case fscrypt internal data structures change, whereas without one can only rely on the name of the key (key description), should such a change ever happen. > Also I have proposed an fscrypt ioctl to add keys to a filesystem-level > keyring, > and it doesn't use 'struct fscrypt_key' at all: > https://marc.info/?l=linux-fsdevel&m=150879505206393 > > So I think you should just use the "default" encrypted-key format, where > the > payload is just the raw key. fscrypt can very easily be updated to work > with > such keys. I've done that in v2 - I am generating the fscrypt_key temporarily on the fly but haven't gotten rid of fscrypt_key altogether... Is that what you had in mind? I also haven't based it on your work mentioned above, would that be preferred? Cheers, Andre' -- To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi André, On Wed, Jan 17, 2018 at 02:29:29PM +0000, André Draszik wrote: > Thanks Eric for the review! > > On Wed, 2018-01-10 at 20:00 -0800, Eric Biggers wrote: > > Hi André, > > > > On Wed, Jan 10, 2018 at 12:44:16PM +0000, André Draszik wrote: > > > This is heavily based on commit 79a73d188726 > > > ("encrypted-keys: add ecryptfs format support"). > > > > > > The 'encrypted' key type defines its own payload format which contains a > > > symmetric key randomly generated that cannot be used directly by the > > > fscrypt subsystem, because it instead expects an fscrypt_key structure. > > > > > > This patch introduces the new format 'fscrypt' that allows to store an > > > fscrypt_key structure inside the encrypted key payload containing > > > a randomly generated symmetric key, as the same for the format 'default' > > > and 'ecryptfs'. > > > > > > More details about the usage of encrypted keys with the fscrypt > > > subsystem can be found in the file > > > 'Documentation/security/keys/fscrypt.rst'. > > > > > > > I don't think a new encrypted-key format is needed. fscrypt really only > > needs > > the raw key. > > This was actually my original approach, but I thought it might potentially > be useful to have a new encrypted-key type to be able to do verification of > parameters (e.g. key size) early. > Additionally, because the type is then encoded in the blob stored in the > file system (keyctl pipe), it'd be easy to spot incompatibilities in case > fscrypt internal data structures change, whereas without one can only rely > on the name of the key (key description), should such a change ever happen. 'struct fscrypt_key' isn't really useful because it doesn't have any reserved fields. So if we wanted to change the format we'd have to change the key description anyway, by setting some flag in the fscrypt_policy and the fscrypt_context that indicates a different key description should be used. And fscrypt does verify the key's size before it uses it already. Sure, it might to nice to verify it at add_key() time, but I don't think it's necessary. > > > Also I have proposed an fscrypt ioctl to add keys to a filesystem-level > > keyring, > > and it doesn't use 'struct fscrypt_key' at all: > > https://marc.info/?l=linux-fsdevel&m=150879505206393 > > > > So I think you should just use the "default" encrypted-key format, where > > the > > payload is just the raw key. fscrypt can very easily be updated to work > > with > > such keys. > > I've done that in v2 - I am generating the fscrypt_key temporarily on the > fly but haven't gotten rid of fscrypt_key altogether... Is that what you had > in mind? That would work, but we don't actually need the fscrypt_key structure at all. Just declare 'const u8 *raw' and 'u32 size' local variables and set them appropriately, then pass those into derive_key_aes() (changing its prototype) rather than the fscrypt_key. > > I also haven't based it on your work mentioned above, would that be > preferred? Not really, that patch series ("fscrypt: filesystem-level keyring and v2 policy support") is still in RFC status, so we don't know which would be merged first. I'm still looking for people with the interest and expertise to review it :-) That being said, we could choose to support the "encrypted" key type using only FS_IOC_ADD_ENCRYPTION_KEY, rather than the current mechanism of process-subscribed keyrings. (With FS_IOC_ADD_ENCRYPTION_KEY, we'd need to use a flag in 'struct fscrypt_add_key_args' to indicate that the key should be found by searching the current process's keyrings for an "encrypted" key with the given description, rather than taking the key directly from the ioctl argument.) But it wouldn't be a huge deal to support it both ways. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/keys/encrypted-keys/Makefile b/security/keys/encrypted-keys/Makefile index 7a44dce6f69d..586702ce9622 100644 --- a/security/keys/encrypted-keys/Makefile +++ b/security/keys/encrypted-keys/Makefile @@ -5,7 +5,7 @@ obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys.o -encrypted-keys-y := encrypted.o ecryptfs_format.o +encrypted-keys-y := encrypted.o ecryptfs_format.o fscrypt_format.o masterkey-$(CONFIG_TRUSTED_KEYS) := masterkey_trusted.o masterkey-$(CONFIG_TRUSTED_KEYS)-$(CONFIG_ENCRYPTED_KEYS) := masterkey_trusted.o encrypted-keys-y += $(masterkey-y) $(masterkey-m-m) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index d92cbf9687c3..b570a930583a 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -37,6 +37,7 @@ #include "encrypted.h" #include "ecryptfs_format.h" +#include "fscrypt_format.h" static const char KEY_TRUSTED_PREFIX[] = "trusted:"; static const char KEY_USER_PREFIX[] = "user:"; @@ -45,6 +46,7 @@ static const char hmac_alg[] = "hmac(sha256)"; static const char blkcipher_alg[] = "cbc(aes)"; static const char key_format_default[] = "default"; static const char key_format_ecryptfs[] = "ecryptfs"; +static const char key_format_fscrypt[] = "fscrypt"; static unsigned int ivsize; static int blksize; @@ -62,12 +64,13 @@ enum { }; enum { - Opt_error = -1, Opt_default, Opt_ecryptfs + Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_fscrypt }; static const match_table_t key_format_tokens = { {Opt_default, "default"}, {Opt_ecryptfs, "ecryptfs"}, + {Opt_fscrypt, "fscrypt"}, {Opt_error, NULL} }; @@ -185,7 +188,7 @@ static int datablob_parse(char *datablob, const char **format, } key_cmd = match_token(keyword, key_tokens, args); - /* Get optional format: default | ecryptfs */ + /* Get optional format: default | ecryptfs | fscrypt */ p = strsep(&datablob, " \t"); if (!p) { pr_err("encrypted_key: insufficient parameters specified\n"); @@ -194,6 +197,7 @@ static int datablob_parse(char *datablob, const char **format, key_format = match_token(p, key_format_tokens, args); switch (key_format) { + case Opt_fscrypt: case Opt_ecryptfs: case Opt_default: *format = p; @@ -634,6 +638,11 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key, } decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES; payload_datalen = sizeof(struct ecryptfs_auth_tok); + } else if (format && !strcmp(format, key_format_fscrypt)) { + ret = fscrypt_encrypted_key_reserve_payload(decrypted_datalen, + &payload_datalen); + if (ret < 0) + return ERR_PTR(ret); } encrypted_datalen = roundup(decrypted_datalen, blksize); @@ -734,6 +743,8 @@ static void __ekey_init(struct encrypted_key_payload *epayload, if (!strcmp(format, key_format_ecryptfs)) epayload->decrypted_data = ecryptfs_get_auth_tok_key((struct ecryptfs_auth_tok *)epayload->payload_data); + else if (!strcmp(format, key_format_fscrypt)) + fscrypt__ekey_init(epayload); memcpy(epayload->format, format, format_len); } @@ -762,6 +773,10 @@ static int encrypted_init(struct encrypted_key_payload *epayload, ecryptfs_fill_auth_tok((struct ecryptfs_auth_tok *)epayload->payload_data, key_desc); + } else if (format && !strcmp(format, key_format_fscrypt)) { + ret = fscrypt_valid_desc(key_desc); + if (ret < 0) + return ret; } __ekey_init(epayload, format, master_desc, datalen); diff --git a/security/keys/encrypted-keys/fscrypt_format.c b/security/keys/encrypted-keys/fscrypt_format.c new file mode 100644 index 000000000000..7620c0fa3ff9 --- /dev/null +++ b/security/keys/encrypted-keys/fscrypt_format.c @@ -0,0 +1,70 @@ +/* + * fscrypt_format.c: helper functions for the encrypted key type + * + * Copyright (C) 2006 International Business Machines Corp. + * Copyright (C) 2010 Politecnico di Torino, Italy + * TORSEC group -- http://security.polito.it + * + * Authors: + * André Draszik <git@andred.net> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, version 2 of the License. + */ + +#include <linux/ctype.h> +#define __FS_HAS_ENCRYPTION 0 +#include <linux/fscrypt.h> +#include <keys/encrypted-type.h> +#include <crypto/aes.h> +#include "fscrypt_format.h" + + +#define FS_KEY_DESCRIPTOR_HEX_SIZE (FS_KEY_DESCRIPTOR_SIZE*2) + +int fscrypt_encrypted_key_reserve_payload(unsigned short decrypted_datalen, + unsigned short *payload_datalen) +{ + if (decrypted_datalen < AES_BLOCK_SIZE /* FS_AES_128_CBC_KEY_SIZE */ + || decrypted_datalen > FS_MAX_KEY_SIZE + || decrypted_datalen % AES_BLOCK_SIZE != 0) { + pr_err("encrypted_key: fscrypt keylen must be a multiple of %d up to %d bytes\n", + AES_BLOCK_SIZE, FS_MAX_KEY_SIZE); + return -EINVAL; + } + *payload_datalen = sizeof(struct fscrypt_key); + return 0; +} + +void fscrypt__ekey_init(struct encrypted_key_payload *epayload) +{ + struct fscrypt_key *fk = (struct fscrypt_key *)epayload->payload_data; + + epayload->decrypted_data = fk->raw; + + fk->mode = 0; + fk->size = epayload->decrypted_datalen; +} + +int fscrypt_valid_desc(const char *desc) +{ + int i; + + if (strlen(desc) != (FS_KEY_DESC_PREFIX_SIZE + + FS_KEY_DESCRIPTOR_HEX_SIZE)) + goto error; + if (memcmp(desc, FS_KEY_DESC_PREFIX, FS_KEY_DESC_PREFIX_SIZE)) + goto error; + desc += FS_KEY_DESC_PREFIX_SIZE; + for (i = 0; i < FS_KEY_DESCRIPTOR_HEX_SIZE; i++) + if (!isxdigit(desc[i])) + goto error; + + return 0; + +error: + pr_err("encrypted_key: key description must be 'fscrypt:<policy>'\n"); + return -EINVAL; +} + diff --git a/security/keys/encrypted-keys/fscrypt_format.h b/security/keys/encrypted-keys/fscrypt_format.h new file mode 100644 index 000000000000..c6d7da1a2113 --- /dev/null +++ b/security/keys/encrypted-keys/fscrypt_format.h @@ -0,0 +1,20 @@ +/* + * fscrypt_format.h: helper functions for the encrypted key type + * + * Copyright (C) 2006 International Business Machines Corp. + * Copyright (C) 2010 Politecnico di Torino, Italy + * TORSEC group -- http://security.polito.it + * + * Authors: + * André Draszik <git@andred.net> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, version 2 of the License. + */ +#pragma once + +int fscrypt_encrypted_key_reserve_payload(unsigned short decrypted_datalen, + unsigned short *payload_datalen); +void fscrypt__ekey_init(struct encrypted_key_payload *epayload); +int fscrypt_valid_desc(const char *desc);
This is heavily based on commit 79a73d188726 ("encrypted-keys: add ecryptfs format support"). The 'encrypted' key type defines its own payload format which contains a symmetric key randomly generated that cannot be used directly by the fscrypt subsystem, because it instead expects an fscrypt_key structure. This patch introduces the new format 'fscrypt' that allows to store an fscrypt_key structure inside the encrypted key payload containing a randomly generated symmetric key, as the same for the format 'default' and 'ecryptfs'. More details about the usage of encrypted keys with the fscrypt subsystem can be found in the file 'Documentation/security/keys/fscrypt.rst'. Signed-off-by: André Draszik <git@andred.net> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com> Cc: David Howells <dhowells@redhat.com> Cc: James Morris <james.l.morris@oracle.com> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: "Theodore Y. Ts'o" <tytso@mit.edu> Cc: Jaegeuk Kim <jaegeuk@kernel.org> Cc: Kees Cook <keescook@chromium.org> Cc: linux-integrity@vger.kernel.org Cc: keyrings@vger.kernel.org Cc: linux-security-module@vger.kernel.org Cc: linux-fscrypt@vger.kernel.org --- security/keys/encrypted-keys/Makefile | 2 +- security/keys/encrypted-keys/encrypted.c | 19 +++++++- security/keys/encrypted-keys/fscrypt_format.c | 70 +++++++++++++++++++++++++++ security/keys/encrypted-keys/fscrypt_format.h | 20 ++++++++ 4 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 security/keys/encrypted-keys/fscrypt_format.c create mode 100644 security/keys/encrypted-keys/fscrypt_format.h