| Message ID | 20180118131359.8365-1-git@andred.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 18, 2018 at 01:13:59PM +0000, André Draszik wrote: > fscrypt uses a master key for each directory policy from > which all further keys for that policy are derived, and > at the moment such a master key has to be inserted into > a kernel keyring as a 'logon' key by user-space. > > While 'logon' keys have the nice property of not being > readable by user-space (keyctl dump etc.), the fscrypt There is no 'keyctl dump' command. Probably you meant 'keyctl read'. > master key still needs to be generated initially, in a > secure way, and it needs to be saved somewhere for > subsequent reboots, and hence also needs securing itself. > > Usage of the 'logon' key means that it is up to user-space > to do all that, opening up the possibility of creating > cryptographically non-sound master keys, or not storing > the master key securely across reboots. > > One approach for securing the master key would be to do > that using a TPM and while one could manually do so e.g. > using tpm-tools, that still leaves creation and actual > correct usage of tpm-tools up to user-space, though. As an > aside, tpm-tools needs the tcsd daemon running, which makes > it awkward to use from within an initramfs, and while other > libraries for interfacing with a TPM do exist, there > appears to be a better way: > > The kernel already has a concept of trusted as well as > encrypted keys. Trusted keys are TPM backed keys, which can > be sealed to PCRs and also easily be re-sealed against new > future PCRs, and encrypted keys are keys that are encrypted > using another key, e.g. a trusted key. All are generated > automatically by the kernel using the RNG and never leave > kernel memory space (bar any kernel or hardware bugs). Saying "the RNG" is unclear, since "encrypted" keys are generated using the kernel's RNG whereas "trusted" keys are generated using the TPM's RNG. > > So it seems reasonable to allow the fscrypt subsystem to > work with encrypted keys as well. This is what this change > here does. > > We can utilise keys that never ever exist in user-space, > not even at initial creation time, as well as simplifying > usage / configuration. Something very very similar exists > for eCrypts already. typo: eCrypts => eCryptfs [...] > diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst > index 776ddc655f79..852ac2900b66 100644 > --- a/Documentation/filesystems/fscrypt.rst > +++ b/Documentation/filesystems/fscrypt.rst > @@ -368,11 +368,19 @@ Adding keys > To provide a master key, userspace must add it to an appropriate > keyring using the add_key() system call (see: > ``Documentation/security/keys/core.rst``). The key type must be > -"logon"; keys of this type are kept in kernel memory and cannot be > -read back by userspace. The key description must be "fscrypt:" > -followed by the 16-character lower case hex representation of the > -``master_key_descriptor`` that was set in the encryption policy. The > -key payload must conform to the following structure:: > +either "logon" or "encrypted"; "logon" keys are kept in kernel > +memory and cannot be read back by userspace while "encrypted" > +keys can be rooted in a "trusted" key and thus are protected by > +a TPM and cannot be read by userspace in unencrypted form. Note > +that while an "encrypted" key can also be rooted in a "user" key, > +any "encrypted" key rooted in a "user" key can effectively be > +retrieved in the clear, hence only rooting the key in a "trusted" > +key has any useful security properties! > + > +The key description must be "fscrypt:" followed by the 16-character > +lower case hex representation of the ``master_key_descriptor`` that > +was set in the encryption policy. For a "logon" key, key payload > +must conform to the following structure:: > > #define FS_MAX_KEY_SIZE 64 > > @@ -386,6 +394,17 @@ key payload must conform to the following structure:: > ``raw`` with ``size`` indicating its size in bytes. That is, the > bytes ``raw[0..size-1]`` (inclusive) are the actual key. > > +When using an "encrypted" key, only the actual ``raw`` key from above > +fscrypt_key structure is needed:: > + > + keyctl add encrypted "fscrypt:``master_key_descriptor``" "new default trusted:``master-key-name`` ``size``" ``ring`` > + keyctl add encrypted "fscrypt:``master_key_descriptor``" "load ``hex_blob``" ``ring`` > + > +Where:: > + > + master-key-name:= name of the trusted key this fscrypt master key > + shall be rooted in > + We need to be very careful with the wording. For example it's implied that encrypted keys cannot be read by userspace because they are protected by a TPM, but that's not true. They can be *wrapped* by a key which in turn can be TPM-sealed, but once unwrapped they are present in the clear in kernel memory, and it's only the policy of the kernel code that they can't be read back in the clear. Also the "logon" and "encrypted" key types should probably be in their own subsections, so that there is space to explain them properly. I've tried reorganizing the section a bit and this is what I came up with: Adding keys ----------- To provide a master key, userspace must add it to an appropriate keyring using the add_key() system call (see: ``Documentation/security/keys/core.rst``). The key description must be "fscrypt:" followed by the 16-character lower case hex representation of the ``master_key_descriptor`` that was set in the encryption policy. The "fscrypt:" prefix can alternatively be replaced with a filesystem-specific prefix such as "ext4:". However, the filesystem-specific prefixes are deprecated and should not be used in new programs. The key type must be "logon" or "encrypted", as described below. Support for the "logon" key type will always be available when any filesystem has fscrypt support enabled, whereas support for the "encrypted" key type additionally requires CONFIG_ENCRYPTED_KEYS=y. Logon key type ~~~~~~~~~~~~~~ With the "logon" type, userspace is responsible for generating or unwrapping the master key, then passing it to the kernel in the following format: #define FS_MAX_KEY_SIZE 64 struct fscrypt_key { u32 mode; u8 raw[FS_MAX_KEY_SIZE]; u32 size; }; ``mode`` is ignored; just set it to 0. The actual key is provided in ``raw`` with ``size`` indicating its size in bytes. That is, the bytes ``raw[0..size-1]`` (inclusive) are the actual key. The "logon" key will then be stored in kernel memory for the filesystem to use. There is no API for userspace to read it back. Encrypted key type ~~~~~~~~~~~~~~~~~~ With the "encrypted" key type, the key is initially randomly generated by the kernel. Then, userspace can read it back as a blob wrapped by another key. In particular, it can be wrapped by a "trusted" key, which can be sealed to a TPM in a certain state, with only the TPM-sealed blob being made available to userspace. Then, userspace can later re-instantiate the "encrypted" key by first re-instantiating the needed "trusted" key, then providing the encrypted-key blob to be unwrapped by the kernel. For example: # create a new encrypted-key keyctl add encrypted "fscrypt:``master_key_descriptor``" "new default trusted:``master-key-desc`` ``size``" ``ring`` # unwrap an existing encrypted-key blob keyctl add encrypted "fscrypt:``master_key_descriptor``" "load ``hex_blob``" ``ring`` Where:: master-key-desc:= description of the trusted key this fscrypt master key shall be wrapped by size := desired size of the fscrypt master key in bytes ring := keyring to add the key to The "encrypted" key must be instantiated using the "default" format, since its decrypted payload is treated as the raw master key; that is, the ``fscrypt_key`` structure is not used. Note that just like "logon" keys, "encrypted" keys are actually present in kernel memory in the clear after being added. Thus, they are not safe from attacks that compromise kernel memory. However, they cannot be read back in the clear using the system call interface. Choice of keyring ~~~~~~~~~~~~~~~~~ There are several different types of keyrings in which fscrypt master keys may be placed, such as a session keyring, a user session keyring, or a user keyring. Each key must be placed in a keyring that is "attached" to all processes that might need to access files encrypted with it, in the sense that request_key() will find the key. Generally, if only processes belonging to a specific user need to access a given encrypted directory and no session keyring has been installed, then that directory's key should be placed in that user's user session keyring or user keyring. Otherwise, a session keyring should be installed if needed, and the key should be linked into that session keyring, or in a keyring linked into that session keyring. Note: introducing the complex visibility semantics of keyrings here was arguably a mistake --- especially given that by design, after any process successfully opens an encrypted file (thereby setting up the per-file key), possessing the keyring key is not actually required for any process to read/write the file until its in-memory inode is evicted. In the future there probably should be a way to provide keys directly to the filesystem instead, which would make the intended semantics clearer.
On Thu, Jan 18, 2018 at 01:13:59PM +0000, André Draszik wrote: > -static int validate_user_key(struct fscrypt_info *crypt_info, > +static inline struct key *fscrypt_get_encrypted_key(const char *description) > +{ > + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS)) > + return request_key(&key_type_encrypted, description, NULL); > + return ERR_PTR(-ENOKEY); > +} > + > +static int validate_keyring_key(struct fscrypt_info *crypt_info, > struct fscrypt_context *ctx, u8 *raw_key, > const char *prefix, int min_keysize) > { > char *description; > struct key *keyring_key; > - struct fscrypt_key *master_key; > - const struct user_key_payload *ukp; > + const u8 *master_key; > + u32 master_key_len; > int res; > > description = kasprintf(GFP_NOFS, "%s%*phN", prefix, > @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info, > return -ENOMEM; > > keyring_key = request_key(&key_type_logon, description, NULL); > + if (keyring_key == ERR_PTR(-ENOKEY)) > + keyring_key = fscrypt_get_encrypted_key(description); > kfree(description); > if (IS_ERR(keyring_key)) > return PTR_ERR(keyring_key); > down_read(&keyring_key->sem); > > - if (keyring_key->type != &key_type_logon) { > + if (keyring_key->type == &key_type_logon) { > + const struct user_key_payload *ukp; > + const struct fscrypt_key *fk; > + > + ukp = user_key_payload_locked(keyring_key); > + if (!ukp) { > + /* key was revoked before we acquired its semaphore */ > + res = -EKEYREVOKED; > + goto out; > + } > + if (ukp->datalen != sizeof(struct fscrypt_key)) { > + res = -EINVAL; > + goto out; > + } > + fk = (struct fscrypt_key *)ukp->data; > + master_key = fk->raw; > + master_key_len = fk->size; > + } else if (keyring_key->type == &key_type_encrypted) { > + const struct encrypted_key_payload *ekp; > + > + ekp = keyring_key->payload.data[0]; > + master_key = ekp->decrypted_data; > + master_key_len = ekp->decrypted_datalen; > + } else { > printk_once(KERN_WARNING > - "%s: key type must be logon\n", __func__); > + "%s: key type must be logon or encrypted\n", > + __func__); > res = -ENOKEY; > goto out; > } > - ukp = user_key_payload_locked(keyring_key); > - if (!ukp) { > - /* key was revoked before we acquired its semaphore */ > - res = -EKEYREVOKED; > - goto out; > - } > - if (ukp->datalen != sizeof(struct fscrypt_key)) { > - res = -EINVAL; > - goto out; > - } > - master_key = (struct fscrypt_key *)ukp->data; > BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE); > > - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE > - || master_key->size % AES_BLOCK_SIZE != 0) { > + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE > + || master_key_len % AES_BLOCK_SIZE != 0) { > printk_once(KERN_WARNING > - "%s: key size incorrect: %d\n", > - __func__, master_key->size); > + "%s: key size incorrect: %u\n", > + __func__, master_key_len); > res = -ENOKEY; > goto out; > } > - res = derive_key_aes(ctx->nonce, master_key, raw_key); > + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key); > + > out: > up_read(&keyring_key->sem); > key_put(keyring_key); > @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode) > if (!raw_key) > goto out; > > - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX, > - keysize); > + res = validate_keyring_key(crypt_info, &ctx, raw_key, > + FS_KEY_DESC_PREFIX, keysize); > if (res && inode->i_sb->s_cop->key_prefix) { > - int res2 = validate_user_key(crypt_info, &ctx, raw_key, > - inode->i_sb->s_cop->key_prefix, > - keysize); > + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key, > + inode->i_sb->s_cop->key_prefix, > + keysize); > if (res2) { > if (res2 == -ENOKEY) > res = -ENOKEY; > -- > 2.15.1 > > -- > To unsubscribe from this list: send the line "unsubscribe keyrings" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html The code changes here look fine, but unfortunately there is a lock ordering problem exposed by using a key type that implements the key_type ->read() method. The problem is that encrypted_read() can take a page fault during copy_to_user() while holding the key semaphore, which then (with ext4) can trigger the start of a jbd2 transaction. Meanwhile fscrypt_get_encryption_info() can be called from within a jbd2 transaction via fscrypt_inherit_context(), which results in a different locking order. We probably will need to fix this by removing the call to fscrypt_get_encryption_info() from fscrypt_inherit_context(). I've pasted the lockdep report I got below: [ 432.705339] [ 432.705681] ====================================================== [ 432.707015] WARNING: possible circular locking dependency detected [ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted [ 432.709365] ------------------------------------------------------ [ 432.710482] keyctl/877 is trying to acquire lock: [ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0 [ 432.712688] [ 432.712688] but task is already holding lock: [ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.715176] [ 432.715176] which lock already depends on the new lock. [ 432.715176] [ 432.716601] [ 432.716601] the existing dependency chain (in reverse order) is: [ 432.717901] [ 432.717901] -> #3 (&type->lock_class#2){++++}: [ 432.718924] lock_acquire+0xaa/0x170 [ 432.719632] down_read+0x37/0xa0 [ 432.720298] validate_keyring_key.isra.1+0x97/0x410 [ 432.721218] fscrypt_get_encryption_info+0x724/0x1200 [ 432.722233] fscrypt_inherit_context+0x2c1/0x330 [ 432.723067] __ext4_new_inode+0x34f5/0x44d0 [ 432.723830] ext4_create+0x195/0x4a0 [ 432.724499] lookup_open+0xbe2/0x1400 [ 432.725185] path_openat+0xb31/0x1e50 [ 432.725876] do_filp_open+0x175/0x250 [ 432.726572] do_sys_open+0x219/0x3f0 [ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.728153] [ 432.728153] -> #2 (jbd2_handle){.+.+}: [ 432.729008] lock_acquire+0xaa/0x170 [ 432.729680] start_this_handle+0x47b/0x1020 [ 432.730463] jbd2__journal_start+0x32e/0x5c0 [ 432.731276] __ext4_journal_start_sb+0xa8/0x190 [ 432.732183] ext4_truncate+0x4dd/0xd00 [ 432.732868] ext4_setattr+0xa62/0x1ac0 [ 432.733528] notify_change+0x672/0xdb0 [ 432.734198] do_truncate+0x111/0x1c0 [ 432.734831] path_openat+0xee6/0x1e50 [ 432.735476] do_filp_open+0x175/0x250 [ 432.736149] do_sys_open+0x219/0x3f0 [ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.737576] [ 432.737576] -> #1 (&ei->i_mmap_sem){++++}: [ 432.738508] lock_acquire+0xaa/0x170 [ 432.739148] down_read+0x37/0xa0 [ 432.739735] ext4_filemap_fault+0x7b/0xb0 [ 432.740446] __do_fault+0x7a/0x200 [ 432.741060] __handle_mm_fault+0x6e0/0x2040 [ 432.741801] handle_mm_fault+0x194/0x320 [ 432.742494] __do_page_fault+0x4e1/0xa70 [ 432.743184] page_fault+0x2c/0x60 [ 432.743784] __clear_user+0x3d/0x60 [ 432.744409] clear_user+0x76/0xa0 [ 432.745009] load_elf_binary+0x2bf6/0x2f10 [ 432.745753] search_binary_handler+0x136/0x450 [ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0 [ 432.747339] do_execve+0x2c/0x40 [ 432.747881] try_to_run_init_process+0x12/0x40 [ 432.748611] kernel_init+0xf2/0x180 [ 432.749190] ret_from_fork+0x3a/0x50 [ 432.749785] [ 432.749785] -> #0 (&mm->mmap_sem){++++}: [ 432.750576] __lock_acquire+0x8d1/0x12d0 [ 432.751232] lock_acquire+0xaa/0x170 [ 432.751848] __might_fault+0x12b/0x1a0 [ 432.752478] _copy_to_user+0x27/0xc0 [ 432.753080] encrypted_read+0x54d/0x7b0 [ 432.753734] keyctl_read_key+0x1f2/0x240 [ 432.754396] SyS_keyctl+0x184/0x2a0 [ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.755778] [ 432.755778] other info that might help us debug this: [ 432.755778] [ 432.756972] Chain exists of: [ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2 [ 432.756972] [ 432.758498] Possible unsafe locking scenario: [ 432.758498] [ 432.759302] CPU0 CPU1 [ 432.759919] ---- ---- [ 432.760536] lock(&type->lock_class#2); [ 432.761100] lock(jbd2_handle); [ 432.761926] lock(&type->lock_class#2); [ 432.762836] lock(&mm->mmap_sem); [ 432.763348] [ 432.763348] *** DEADLOCK *** [ 432.763348] [ 432.764209] 1 lock held by keyctl/877: [ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240 [ 432.766045] [ 432.766045] stack backtrace: [ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 [ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014 [ 432.769211] Call Trace: [ 432.769559] dump_stack+0x8d/0xd5 [ 432.770041] print_circular_bug.isra.20+0x321/0x5e0 [ 432.770712] ? save_trace+0xd6/0x250 [ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670 [ 432.771867] ? depot_save_stack+0x2d5/0x490 [ 432.772504] ? check_usage+0x13c0/0x13c0 [ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550 [ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60 [ 432.774401] ? depot_save_stack+0x2d5/0x490 [ 432.774979] ? kasan_kmalloc+0x13a/0x170 [ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410 [ 432.776187] validate_chain.isra.24+0x13ee/0x2410 [ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670 [ 432.777546] __lock_acquire+0x8d1/0x12d0 [ 432.778088] lock_acquire+0xaa/0x170 [ 432.778576] ? __might_fault+0xce/0x1a0 [ 432.779096] __might_fault+0x12b/0x1a0 [ 432.779608] ? __might_fault+0xce/0x1a0 [ 432.780029] _copy_to_user+0x27/0xc0 [ 432.780438] encrypted_read+0x54d/0x7b0 [ 432.780858] ? key_task_permission+0x16e/0x3b0 [ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0 [ 432.781851] ? creds_are_invalid+0x9/0x50 [ 432.782287] ? lookup_user_key+0x19a/0xf50 [ 432.782736] ? __lock_acquire+0x8d1/0x12d0 [ 432.783182] ? key_validate+0xb0/0xb0 [ 432.783602] ? keyctl_read_key+0x174/0x240 [ 432.784058] keyctl_read_key+0x1f2/0x240 [ 432.784486] SyS_keyctl+0x184/0x2a0 [ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b [ 432.785374] RIP: 0033:0x7f812b6c8259
On Thu, Jan 25, 2018 at 04:37:48PM -0800, Eric Biggers wrote: > On Thu, Jan 18, 2018 at 01:13:59PM +0000, André Draszik wrote: > > -static int validate_user_key(struct fscrypt_info *crypt_info, > > +static inline struct key *fscrypt_get_encrypted_key(const char *description) > > +{ > > + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS)) > > + return request_key(&key_type_encrypted, description, NULL); > > + return ERR_PTR(-ENOKEY); > > +} > > + > > +static int validate_keyring_key(struct fscrypt_info *crypt_info, > > struct fscrypt_context *ctx, u8 *raw_key, > > const char *prefix, int min_keysize) > > { > > char *description; > > struct key *keyring_key; > > - struct fscrypt_key *master_key; > > - const struct user_key_payload *ukp; > > + const u8 *master_key; > > + u32 master_key_len; > > int res; > > > > description = kasprintf(GFP_NOFS, "%s%*phN", prefix, > > @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info, > > return -ENOMEM; > > > > keyring_key = request_key(&key_type_logon, description, NULL); > > + if (keyring_key == ERR_PTR(-ENOKEY)) > > + keyring_key = fscrypt_get_encrypted_key(description); > > kfree(description); > > if (IS_ERR(keyring_key)) > > return PTR_ERR(keyring_key); > > down_read(&keyring_key->sem); > > > > - if (keyring_key->type != &key_type_logon) { > > + if (keyring_key->type == &key_type_logon) { > > + const struct user_key_payload *ukp; > > + const struct fscrypt_key *fk; > > + > > + ukp = user_key_payload_locked(keyring_key); > > + if (!ukp) { > > + /* key was revoked before we acquired its semaphore */ > > + res = -EKEYREVOKED; > > + goto out; > > + } > > + if (ukp->datalen != sizeof(struct fscrypt_key)) { > > + res = -EINVAL; > > + goto out; > > + } > > + fk = (struct fscrypt_key *)ukp->data; > > + master_key = fk->raw; > > + master_key_len = fk->size; > > + } else if (keyring_key->type == &key_type_encrypted) { > > + const struct encrypted_key_payload *ekp; > > + > > + ekp = keyring_key->payload.data[0]; > > + master_key = ekp->decrypted_data; > > + master_key_len = ekp->decrypted_datalen; > > + } else { > > printk_once(KERN_WARNING > > - "%s: key type must be logon\n", __func__); > > + "%s: key type must be logon or encrypted\n", > > + __func__); > > res = -ENOKEY; > > goto out; > > } > > - ukp = user_key_payload_locked(keyring_key); > > - if (!ukp) { > > - /* key was revoked before we acquired its semaphore */ > > - res = -EKEYREVOKED; > > - goto out; > > - } > > - if (ukp->datalen != sizeof(struct fscrypt_key)) { > > - res = -EINVAL; > > - goto out; > > - } > > - master_key = (struct fscrypt_key *)ukp->data; > > BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE); > > > > - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE > > - || master_key->size % AES_BLOCK_SIZE != 0) { > > + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE > > + || master_key_len % AES_BLOCK_SIZE != 0) { > > printk_once(KERN_WARNING > > - "%s: key size incorrect: %d\n", > > - __func__, master_key->size); > > + "%s: key size incorrect: %u\n", > > + __func__, master_key_len); > > res = -ENOKEY; > > goto out; > > } > > The code changes here look fine, but unfortunately there is a lock ordering > problem exposed by using a key type that implements the key_type ->read() > method. The problem is that encrypted_read() can take a page fault during > copy_to_user() while holding the key semaphore, which then (with ext4) can > trigger the start of a jbd2 transaction. Meanwhile > fscrypt_get_encryption_info() can be called from within a jbd2 transaction via > fscrypt_inherit_context(), which results in a different locking order. We > probably will need to fix this by removing the call to > fscrypt_get_encryption_info() from fscrypt_inherit_context(). > Actually, a better idea might be to access the key payload under rcu_read_lock() rather than the key semaphore. It looks like that's possible with both the "logon" and "encrypted" key types. Note that derive_key_aes() can sleep, so the part under the rcu_read_lock() would have to just copy the payload to a temporary buffer, and derive_key_aes() would be done after rcu_read_unlock(), using the temporary buffer. But I think you can just reuse the 'raw_key' buffer, so that the encryption operation in derive_key_aes() is done in-place. Eric
diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst index 776ddc655f79..852ac2900b66 100644 --- a/Documentation/filesystems/fscrypt.rst +++ b/Documentation/filesystems/fscrypt.rst @@ -368,11 +368,19 @@ Adding keys To provide a master key, userspace must add it to an appropriate keyring using the add_key() system call (see: ``Documentation/security/keys/core.rst``). The key type must be -"logon"; keys of this type are kept in kernel memory and cannot be -read back by userspace. The key description must be "fscrypt:" -followed by the 16-character lower case hex representation of the -``master_key_descriptor`` that was set in the encryption policy. The -key payload must conform to the following structure:: +either "logon" or "encrypted"; "logon" keys are kept in kernel +memory and cannot be read back by userspace while "encrypted" +keys can be rooted in a "trusted" key and thus are protected by +a TPM and cannot be read by userspace in unencrypted form. Note +that while an "encrypted" key can also be rooted in a "user" key, +any "encrypted" key rooted in a "user" key can effectively be +retrieved in the clear, hence only rooting the key in a "trusted" +key has any useful security properties! + +The key description must be "fscrypt:" followed by the 16-character +lower case hex representation of the ``master_key_descriptor`` that +was set in the encryption policy. For a "logon" key, key payload +must conform to the following structure:: #define FS_MAX_KEY_SIZE 64 @@ -386,6 +394,17 @@ key payload must conform to the following structure:: ``raw`` with ``size`` indicating its size in bytes. That is, the bytes ``raw[0..size-1]`` (inclusive) are the actual key. +When using an "encrypted" key, only the actual ``raw`` key from above +fscrypt_key structure is needed:: + + keyctl add encrypted "fscrypt:``master_key_descriptor``" "new default trusted:``master-key-name`` ``size``" ``ring`` + keyctl add encrypted "fscrypt:``master_key_descriptor``" "load ``hex_blob``" ``ring`` + +Where:: + + master-key-name:= name of the trusted key this fscrypt master key + shall be rooted in + The key description prefix "fscrypt:" may alternatively be replaced with a filesystem-specific prefix such as "ext4:". However, the filesystem-specific prefixes are deprecated and should not be used in @@ -412,6 +431,33 @@ evicted. In the future there probably should be a way to provide keys directly to the filesystem instead, which would make the intended semantics clearer. +Complete Examples +------------------ + +Set fscrypt policy on an (empty) encrypted directory, /encrypted:: + + $ fscryptctl set_policy 1234567890123456 /encrypted + +Create an encrypted key "1234567890123456" of length 64 bytes with format +'fscrypt' and root it in a previously loaded trusted "kmk":: + + $ keyctl add encrypted "fscrypt:1234567890123456" "new default trusted:kmk 64" @u + 839715473 + + $ keyctl print 839715473 + default trusted:kmk 64 e98a49dc11eb9312f46530879aac869300ee734035100f4ee + 5441279369a4c9d83d6e59b8158d0a3de01790c0bb99af82e9603cb6977c7d1229338cda + 80375aaf034678405a00c19806d6fb12490e39b1d7ca603c491b58a962345160e344ae51 + 83483e066692d05f5ab3d8b9ea39cab0e + + $ keyctl pipe 839715473 > fscrypt.blob + +The directory policy will remain across reboots, so after a reboot the key +generated earlier will simply have to be loaded into the kernel keyring +again:: + + $ keyctl add encrypted fscrypt:1234567890123456 "load $(cat fscrypt.blob)" @u + Access semantics ================ diff --git a/fs/crypto/keyinfo.c b/fs/crypto/keyinfo.c index 5e6e846f5a24..3d20addadcd4 100644 --- a/fs/crypto/keyinfo.c +++ b/fs/crypto/keyinfo.c @@ -10,6 +10,7 @@ */ #include <keys/user-type.h> +#include <keys/encrypted-type.h> #include <linux/scatterlist.h> #include <linux/ratelimit.h> #include <crypto/aes.h> @@ -20,14 +21,16 @@ static struct crypto_shash *essiv_hash_tfm; /** * derive_key_aes() - Derive a key using AES-128-ECB - * @deriving_key: Encryption key used for derivation. - * @source_key: Source key to which to apply derivation. - * @derived_raw_key: Derived raw key. + * @deriving_key: Encryption key used for derivation. + * @source_key: Raw source key to which to apply derivation. + * @source_key_len: Length of the source key. + * @derived_raw_key: Derived raw key. * * Return: Zero on success; non-zero otherwise. */ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE], - const struct fscrypt_key *source_key, + const u8 source_key[FS_MAX_KEY_SIZE], + u32 source_key_len, u8 derived_raw_key[FS_MAX_KEY_SIZE]) { int res = 0; @@ -55,9 +58,9 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE], if (res < 0) goto out; - sg_init_one(&src_sg, source_key->raw, source_key->size); - sg_init_one(&dst_sg, derived_raw_key, source_key->size); - skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key->size, + sg_init_one(&src_sg, source_key, source_key_len); + sg_init_one(&dst_sg, derived_raw_key, source_key_len); + skcipher_request_set_crypt(req, &src_sg, &dst_sg, source_key_len, NULL); res = crypto_wait_req(crypto_skcipher_encrypt(req), &wait); out: @@ -66,14 +69,21 @@ static int derive_key_aes(u8 deriving_key[FS_AES_128_ECB_KEY_SIZE], return res; } -static int validate_user_key(struct fscrypt_info *crypt_info, +static inline struct key *fscrypt_get_encrypted_key(const char *description) +{ + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS)) + return request_key(&key_type_encrypted, description, NULL); + return ERR_PTR(-ENOKEY); +} + +static int validate_keyring_key(struct fscrypt_info *crypt_info, struct fscrypt_context *ctx, u8 *raw_key, const char *prefix, int min_keysize) { char *description; struct key *keyring_key; - struct fscrypt_key *master_key; - const struct user_key_payload *ukp; + const u8 *master_key; + u32 master_key_len; int res; description = kasprintf(GFP_NOFS, "%s%*phN", prefix, @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info, return -ENOMEM; keyring_key = request_key(&key_type_logon, description, NULL); + if (keyring_key == ERR_PTR(-ENOKEY)) + keyring_key = fscrypt_get_encrypted_key(description); kfree(description); if (IS_ERR(keyring_key)) return PTR_ERR(keyring_key); down_read(&keyring_key->sem); - if (keyring_key->type != &key_type_logon) { + if (keyring_key->type == &key_type_logon) { + const struct user_key_payload *ukp; + const struct fscrypt_key *fk; + + ukp = user_key_payload_locked(keyring_key); + if (!ukp) { + /* key was revoked before we acquired its semaphore */ + res = -EKEYREVOKED; + goto out; + } + if (ukp->datalen != sizeof(struct fscrypt_key)) { + res = -EINVAL; + goto out; + } + fk = (struct fscrypt_key *)ukp->data; + master_key = fk->raw; + master_key_len = fk->size; + } else if (keyring_key->type == &key_type_encrypted) { + const struct encrypted_key_payload *ekp; + + ekp = keyring_key->payload.data[0]; + master_key = ekp->decrypted_data; + master_key_len = ekp->decrypted_datalen; + } else { printk_once(KERN_WARNING - "%s: key type must be logon\n", __func__); + "%s: key type must be logon or encrypted\n", + __func__); res = -ENOKEY; goto out; } - ukp = user_key_payload_locked(keyring_key); - if (!ukp) { - /* key was revoked before we acquired its semaphore */ - res = -EKEYREVOKED; - goto out; - } - if (ukp->datalen != sizeof(struct fscrypt_key)) { - res = -EINVAL; - goto out; - } - master_key = (struct fscrypt_key *)ukp->data; BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE); - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE - || master_key->size % AES_BLOCK_SIZE != 0) { + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE + || master_key_len % AES_BLOCK_SIZE != 0) { printk_once(KERN_WARNING - "%s: key size incorrect: %d\n", - __func__, master_key->size); + "%s: key size incorrect: %u\n", + __func__, master_key_len); res = -ENOKEY; goto out; } - res = derive_key_aes(ctx->nonce, master_key, raw_key); + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key); + out: up_read(&keyring_key->sem); key_put(keyring_key); @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode) if (!raw_key) goto out; - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX, - keysize); + res = validate_keyring_key(crypt_info, &ctx, raw_key, + FS_KEY_DESC_PREFIX, keysize); if (res && inode->i_sb->s_cop->key_prefix) { - int res2 = validate_user_key(crypt_info, &ctx, raw_key, - inode->i_sb->s_cop->key_prefix, - keysize); + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key, + inode->i_sb->s_cop->key_prefix, + keysize); if (res2) { if (res2 == -ENOKEY) res = -ENOKEY;
fscrypt uses a master key for each directory policy from which all further keys for that policy are derived, and at the moment such a master key has to be inserted into a kernel keyring as a 'logon' key by user-space. While 'logon' keys have the nice property of not being readable by user-space (keyctl dump etc.), the fscrypt master key still needs to be generated initially, in a secure way, and it needs to be saved somewhere for subsequent reboots, and hence also needs securing itself. Usage of the 'logon' key means that it is up to user-space to do all that, opening up the possibility of creating cryptographically non-sound master keys, or not storing the master key securely across reboots. One approach for securing the master key would be to do that using a TPM and while one could manually do so e.g. using tpm-tools, that still leaves creation and actual correct usage of tpm-tools up to user-space, though. As an aside, tpm-tools needs the tcsd daemon running, which makes it awkward to use from within an initramfs, and while other libraries for interfacing with a TPM do exist, there appears to be a better way: The kernel already has a concept of trusted as well as encrypted keys. Trusted keys are TPM backed keys, which can be sealed to PCRs and also easily be re-sealed against new future PCRs, and encrypted keys are keys that are encrypted using another key, e.g. a trusted key. All are generated automatically by the kernel using the RNG and never leave kernel memory space (bar any kernel or hardware bugs). So it seems reasonable to allow the fscrypt subsystem to work with encrypted keys as well. This is what this change here does. We can utilise keys that never ever exist in user-space, not even at initial creation time, as well as simplifying usage / configuration. Something very very similar exists for eCrypts already. With these patches, we can kmk_id=$(keyctl add trusted kmk "new 128 [pcrinfo=...]" @u) fscrypt_id=$(keyctl add encrypted fscrypt:1234567890123456 "new default trusted:kmk 64" @u) fscryptctl set_policy 1234567890123456 /encrypted then we can save those keys for after reboot (optionally TPM-sealed as per the pcrinfo= argument above): keyctl pipe ${kmk_id} > /keys/kmk.blob keyctl pipe ${fscrypt_id} > /keys/fscrypt.blob and on subsequent boots we can simply insert them again (optionally benefitting from the TPM's capability to only unseal kmk.blob when the system is in the expected state, thereby also protecting fscrypt.blob): keyctl add trusted kmk "load $(cat /keys/kmk.blob)" @u keyctl add encrypted fscrypt:1234567890123456 "load $(cat /keys/fscrypt.blob)" @u 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: Eric Biggers <ebiggers@google.com> Cc: linux-integrity@vger.kernel.org Cc: keyrings@vger.kernel.org Cc: linux-security-module@vger.kernel.org Cc: linux-fscrypt@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- changes in v3: * merge documentation changes into this commit * update derive_key_aes() signature to not take a struct fscrypt_key and update all callers * try to use the 'encrypted' key as fallback only if the a 'logon' key isn't present (ENOKEY) * update fscrypt_get_encrypted_key() to take a 'description', not a 'sig' * update commit message * re-add keyrings mailing list and encrypted-keys maintainers to Cc changes in v2: * dropped the previously added 'fscrypt' encrypted-key, and just use the 'default' format --- Documentation/filesystems/fscrypt.rst | 56 +++++++++++++++++++-- fs/crypto/keyinfo.c | 92 ++++++++++++++++++++++------------- 2 files changed, 110 insertions(+), 38 deletions(-)