diff mbox series

[v2,02/20] fscrypt: add flag allowing partially-encrypted directories

Message ID 5e762e300535cbb9f04b25a97e1d13fd082f5b0e.1662420176.git.sweettea-kernel@dorminy.me (mailing list archive)
State Superseded
Headers show
Series btrfs: add fscrypt integration | expand

Commit Message

Sweet Tea Dorminy Sept. 6, 2022, 12:35 a.m. UTC 
From: Omar Sandoval <osandov@osandov.com>

Creating several new subvolumes out of snapshots of another subvolume,
each for a different VM's storage, is a important usecase for btrfs.  We
would like to give each VM a unique encryption key to use for new writes
to its subvolume, so that secure deletion of the VM's data is as simple
as securely deleting the key; to avoid needing multiple keys in each VM,
we envision the initial subvolume being unencrypted. However, this means
that the snapshot's directories would have a mix of encrypted and
unencrypted files. During lookup with a key, both unencrypted and
encrypted forms of the desired name must be queried.

To allow this, add another FS_CFLG to allow filesystems to opt into
partially encrypted directories.

Signed-off-by: Omar Sandoval <osandov@osandov.com>
Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
---
 fs/crypto/fname.c       | 17 ++++++++++++++++-
 include/linux/fscrypt.h |  2 ++
 2 files changed, 18 insertions(+), 1 deletion(-)

Comments

Josef Bacik Sept. 8, 2022, 1:43 p.m. UTC | #1 
On Mon, Sep 05, 2022 at 08:35:17PM -0400, Sweet Tea Dorminy wrote:
> From: Omar Sandoval <osandov@osandov.com>
> 
> Creating several new subvolumes out of snapshots of another subvolume,
> each for a different VM's storage, is a important usecase for btrfs.  We
> would like to give each VM a unique encryption key to use for new writes
> to its subvolume, so that secure deletion of the VM's data is as simple
> as securely deleting the key; to avoid needing multiple keys in each VM,
> we envision the initial subvolume being unencrypted. However, this means
> that the snapshot's directories would have a mix of encrypted and
> unencrypted files. During lookup with a key, both unencrypted and
> encrypted forms of the desired name must be queried.
> 
> To allow this, add another FS_CFLG to allow filesystems to opt into
> partially encrypted directories.
> 
> Signed-off-by: Omar Sandoval <osandov@osandov.com>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef
Eric Biggers Sept. 12, 2022, 1:42 a.m. UTC | #2 
On Mon, Sep 05, 2022 at 08:35:17PM -0400, Sweet Tea Dorminy wrote:
> From: Omar Sandoval <osandov@osandov.com>
> 
> Creating several new subvolumes out of snapshots of another subvolume,
> each for a different VM's storage, is a important usecase for btrfs.  We
> would like to give each VM a unique encryption key to use for new writes
> to its subvolume, so that secure deletion of the VM's data is as simple
> as securely deleting the key; to avoid needing multiple keys in each VM,
> we envision the initial subvolume being unencrypted. However, this means
> that the snapshot's directories would have a mix of encrypted and
> unencrypted files. During lookup with a key, both unencrypted and
> encrypted forms of the desired name must be queried.
> 
> To allow this, add another FS_CFLG to allow filesystems to opt into
> partially encrypted directories.
> 
> Signed-off-by: Omar Sandoval <osandov@osandov.com>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>  fs/crypto/fname.c       | 17 ++++++++++++++++-
>  include/linux/fscrypt.h |  2 ++
>  2 files changed, 18 insertions(+), 1 deletion(-)

I'm still trying to wrap my head around what this part involves exactly.  This
is a pretty big change in semantics.

Could this be moved to the end of the patchset, or is this a fundamental part of
the btrfs fscrypt support that the rest of your patchset depends on?  I'd think
it would be a lot easier to review if this change was an add-on at the end.

One thing to keep in mind is that FS_IOC_SET_ENCRYPTION_POLICY failing on
nonempty directories can actually be very useful, since it makes it possible to
detect bugs where people create files in encrypted directories (expecting that
they are encrypted) before an encryption policy actually gets assigned.  Since
FS_IOC_SET_ENCRYPTION_POLICY fails in that case, such bugs can be detected and
fixed.

If it succeeds, then everything would just silently work and some files wouldn't
be encrypted as intended.  That's kind of scary.

It might be warranted to use an encryption policy flag to explicitly indicate
that mixing encrypted and unencrypted files is being allowed.

- Eric
Anand Jain Sept. 13, 2022, 10:07 a.m. UTC | #3 
On 06/09/2022 08:35, Sweet Tea Dorminy wrote:
> From: Omar Sandoval <osandov@osandov.com>
> 
> Creating several new subvolumes out of snapshots of another subvolume,
> each for a different VM's storage, is a important usecase for btrfs.

> We
> would like to give each VM a unique encryption key to use for new writes
> to its subvolume, so that secure deletion of the VM's data is as simple
> as securely deleting the key; to avoid needing multiple keys in each VM,
> we envision the initial subvolume being unencrypted.

In this usecase, you didn't mention if the original subvolume is
encrypted, assuming it is not, so this usecase is the same as mentioned
in the design document. Now, in this usecase, what happens if the
original subvolume is encrypted? Can we still avoid the multiple keys?
How is that going to work? Or we don't yet support subvolumes out of
snapshots of another encrypted subvolume?

Thanks,
-Anand

> However, this means
> that the snapshot's directories would have a mix of encrypted and
> unencrypted files. During lookup with a key, both unencrypted and
> encrypted forms of the desired name must be queried.
> 
> To allow this, add another FS_CFLG to allow filesystems to opt into
> partially encrypted directories.
> 
> Signed-off-by: Omar Sandoval <osandov@osandov.com>
> Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
> ---
>   fs/crypto/fname.c       | 17 ++++++++++++++++-
>   include/linux/fscrypt.h |  2 ++
>   2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 6c092a1533f7..3bdece33e14d 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -414,6 +414,7 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
>   	fname->usr_fname = iname;
>   
>   	if (!IS_ENCRYPTED(dir) || fscrypt_is_dot_dotdot(iname)) {
> +unencrypted:
>   		fname->disk_name.name = (unsigned char *)iname->name;
>   		fname->disk_name.len = iname->len;
>   		return 0;
> @@ -448,8 +449,16 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
>   	 * user-supplied name
>   	 */
>   
> -	if (iname->len > FSCRYPT_NOKEY_NAME_MAX_ENCODED)
> +	if (iname->len > FSCRYPT_NOKEY_NAME_MAX_ENCODED) {
> +		/*
> +		 * This isn't a valid nokey name, but it could be an unencrypted
> +		 * name if the filesystem allows partially encrypted
> +		 * directories.
> +		 */
> +		if (dir->i_sb->s_cop->flags & FS_CFLG_ALLOW_PARTIAL)
> +			goto unencrypted;
>   		return -ENOENT;
> +	}
>   
>   	fname->crypto_buf.name = kmalloc(FSCRYPT_NOKEY_NAME_MAX, GFP_KERNEL);
>   	if (fname->crypto_buf.name == NULL)
> @@ -460,6 +469,12 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
>   	if (ret < (int)offsetof(struct fscrypt_nokey_name, bytes[1]) ||
>   	    (ret > offsetof(struct fscrypt_nokey_name, sha256) &&
>   	     ret != FSCRYPT_NOKEY_NAME_MAX)) {
> +		/* Again, this could be an unencrypted name. */
> +		if (dir->i_sb->s_cop->flags & FS_CFLG_ALLOW_PARTIAL) {
> +			kfree(fname->crypto_buf.name);
> +			fname->crypto_buf.name = NULL;
> +			goto unencrypted;
> +		}
>   		ret = -ENOENT;
>   		goto errout;
>   	}
> diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
> index a236d8c6d0da..a4e00314c91b 100644
> --- a/include/linux/fscrypt.h
> +++ b/include/linux/fscrypt.h
> @@ -102,6 +102,8 @@ struct fscrypt_nokey_name {
>    * pages for writes and therefore won't need the fscrypt bounce page pool.
>    */
>   #define FS_CFLG_OWN_PAGES (1U << 1)
> +/* The filesystem allows partially encrypted directories/files. */
> +#define FS_CFLG_ALLOW_PARTIAL (1U << 2)
>   
>   /* Crypto operations for filesystems */
>   struct fscrypt_operations {
Neal Gompa Sept. 13, 2022, 11:02 a.m. UTC | #4 
On Tue, Sep 13, 2022 at 6:21 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> On 06/09/2022 08:35, Sweet Tea Dorminy wrote:
> > From: Omar Sandoval <osandov@osandov.com>
> >
> > Creating several new subvolumes out of snapshots of another subvolume,
> > each for a different VM's storage, is a important usecase for btrfs.
>
> > We
> > would like to give each VM a unique encryption key to use for new writes
> > to its subvolume, so that secure deletion of the VM's data is as simple
> > as securely deleting the key; to avoid needing multiple keys in each VM,
> > we envision the initial subvolume being unencrypted.
>
> In this usecase, you didn't mention if the original subvolume is
> encrypted, assuming it is not, so this usecase is the same as mentioned
> in the design document. Now, in this usecase, what happens if the
> original subvolume is encrypted? Can we still avoid the multiple keys?
> How is that going to work? Or we don't yet support subvolumes out of
> snapshots of another encrypted subvolume?
>

Count this as the first official "email triggered by Plumbers" I
suppose, but I've been looking forward to Btrfs encryption for Fedora
for a variety of use-cases:

* Converting a disk from unencrypted to encrypted without reinstalling[1]
* Encrypting system and user data with different credentials[2][3]
* Mass managed systems with dual-credential encryption (one rooted in
org, other by user)[1]

The way I envision Fedora's usage of this feature going is as follows:

The system is shipped (OEM install; e.g. Lenovo, Slimbook, etc.) or
installed without encryption (from Anaconda). The firstboot process
helps the user get setup, and asks if they want disk encryption
(provided there is no policy forcing it on). If the answer is yes,
then the root subvolume and the home subvolume are configured to be
encrypted using the on-board secure enclave (TPM or whatever). When
the user creation step occurs, if disk encryption was set on earlier,
then the user subvolume inside the home subvolume gets configured to
be encrypted using the login credentials. If it wasn't set on earlier,
we could also still offer to encrypt the user data here. If this is a
mass-managed system, an additional decryption key would be available
for the system manager to use in the event of credential loss, damaged
TPM, etc.

For the cloudy scenario, we'd probably provide tools to orchestrate
this from cloud-init for confidential computing stuff too.

For all this to work, I expect the following:

* Encryption to support *somehow* any number of decryption keys/methods
* Nesting of subvolumes with differing encryption methods
* The ability to blindly backup and restore subvolumes without needing
to decrypt

On a personal system, I expect at most two credentials in use: one
keyed to the system and one keyed to the user. For a corporate/mass
managed system, I expect a third one: one keyed to the management
platform. This is necessary because in a lot of jurisdictions, you
don't necessarily own the data on your corporate laptop, and the owner
has the right to be able to access that data. But a more prosaic or
banal reason is that people forget their credentials and being able to
reset them without losing all their data is usually a good thing. :)

Hopefully I've transcribed what I said properly into this email. :)

[1]: https://pagure.io/fedora-btrfs/project/issue/10
[2]: https://pagure.io/fedora-workstation/issue/82
[3]: https://pagure.io/fedora-workstation/issue/136



--
Neal Gompa (FAS: ngompa)
Sweet Tea Dorminy Sept. 15, 2022, 6:58 p.m. UTC | #5 
> I'm still trying to wrap my head around what this part involves 
> exactly.  This
> is a pretty big change in semantics.
> 
> Could this be moved to the end of the patchset, or is this a 
> fundamental part of
> the btrfs fscrypt support that the rest of your patchset depends on?  
> I'd think
> it would be a lot easier to review if this change was an add-on at the 
> end.

Definitely.

> 
> One thing to keep in mind is that FS_IOC_SET_ENCRYPTION_POLICY failing 
> on
> nonempty directories can actually be very useful, since it makes it 
> possible to
> detect bugs where people create files in encrypted directories 
> (expecting that
> they are encrypted) before an encryption policy actually gets assigned. 
>  Since
> FS_IOC_SET_ENCRYPTION_POLICY fails in that case, such bugs can be 
> detected and
> fixed.

I agree that this has risks of inadvertent misuse in that fashion.

The usecase I'm oriented towards is: someone builds an unencrypted 
subvolume with a container base filesystem, takes several snapshots of 
the subvolume, starts a container on each subvolume, and has each 
container encrypt its designated subvolume going forward with a 
different key. This usecase needs some way to mark a subvolume/directory 
already containing files as encrypted going forward; I've had a hard 
time coming up with a way to both protect users against such accidental 
misuse, but also allow this container usecase.

> 
> It might be warranted to use an encryption policy flag to explicitly 
> indicate
> that mixing encrypted and unencrypted files is being allowed.

Could it be sufficient to check either empty or read-only, something 
like (is_empty_dir(inode) || (FS_CFLG_PARTIAL && !inode_permission(..., 
inode, MAY_WRITE)))? Then the user is unable to accidentally write 
unencrypted data, since they've taken an action to make the directory 
read-only, until they've set a policy and key and turned the directory 
read-write again.

Thanks!
diff mbox series

Patch

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 6c092a1533f7..3bdece33e14d 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -414,6 +414,7 @@  int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
 	fname->usr_fname = iname;
 
 	if (!IS_ENCRYPTED(dir) || fscrypt_is_dot_dotdot(iname)) {
+unencrypted:
 		fname->disk_name.name = (unsigned char *)iname->name;
 		fname->disk_name.len = iname->len;
 		return 0;
@@ -448,8 +449,16 @@  int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
 	 * user-supplied name
 	 */
 
-	if (iname->len > FSCRYPT_NOKEY_NAME_MAX_ENCODED)
+	if (iname->len > FSCRYPT_NOKEY_NAME_MAX_ENCODED) {
+		/*
+		 * This isn't a valid nokey name, but it could be an unencrypted
+		 * name if the filesystem allows partially encrypted
+		 * directories.
+		 */
+		if (dir->i_sb->s_cop->flags & FS_CFLG_ALLOW_PARTIAL)
+			goto unencrypted;
 		return -ENOENT;
+	}
 
 	fname->crypto_buf.name = kmalloc(FSCRYPT_NOKEY_NAME_MAX, GFP_KERNEL);
 	if (fname->crypto_buf.name == NULL)
@@ -460,6 +469,12 @@  int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
 	if (ret < (int)offsetof(struct fscrypt_nokey_name, bytes[1]) ||
 	    (ret > offsetof(struct fscrypt_nokey_name, sha256) &&
 	     ret != FSCRYPT_NOKEY_NAME_MAX)) {
+		/* Again, this could be an unencrypted name. */
+		if (dir->i_sb->s_cop->flags & FS_CFLG_ALLOW_PARTIAL) {
+			kfree(fname->crypto_buf.name);
+			fname->crypto_buf.name = NULL;
+			goto unencrypted;
+		}
 		ret = -ENOENT;
 		goto errout;
 	}
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index a236d8c6d0da..a4e00314c91b 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -102,6 +102,8 @@  struct fscrypt_nokey_name {
  * pages for writes and therefore won't need the fscrypt bounce page pool.
  */
 #define FS_CFLG_OWN_PAGES (1U << 1)
+/* The filesystem allows partially encrypted directories/files. */
+#define FS_CFLG_ALLOW_PARTIAL (1U << 2)
 
 /* Crypto operations for filesystems */
 struct fscrypt_operations {