[RFC] fscrypt_key_required mount option

[RFC] fscrypt_key_required mount option

[Richard Weinberger]

This series is an RFC, it outlines how an filesystem, in this case UBIFS,
could implement a mount option to disable filesystem operations on encrypted
files where no key is present.
The desired byproduct is that in this case ->d_revalidate() is not needed
anymore and VFS folks are in less fear about the dcache implications.

Thanks,
//richard

[PATCH 1/4] fscrypt: Implement FS_CFLG_OWN_D_OPS
[PATCH 2/4] fscrypt: Export fscrypt_d_ops
[PATCH 3/4] ubifs: Simplify fscrypt_get_encryption_info() error
[PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/4] fscrypt: Implement FS_CFLG_OWN_D_OPS

[Richard Weinberger]

If a filesystem sets FS_CFLG_OWN_D_OPS it manages dentry operations
itself and fscrypt is not allowed to set them.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/crypto/hooks.c       | 4 +++-
 include/linux/fscrypt.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c
index 56debb1fcf5e..3ec925405fbe 100644
--- a/fs/crypto/hooks.c
+++ b/fs/crypto/hooks.c
@@ -107,7 +107,9 @@ int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry)
 		spin_unlock(&dentry->d_lock);
 	}
 
-	d_set_d_op(dentry, &fscrypt_d_ops);
+	if ((dir->i_sb->s_cop->flags & FS_CFLG_OWN_D_OPS) == 0)
+		d_set_d_op(dentry, &fscrypt_d_ops);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__fscrypt_prepare_lookup);
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index e5194fc3983e..7139a110ac4f 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -48,6 +48,7 @@ struct fscrypt_name {
  * fscrypt superblock flags
  */
 #define FS_CFLG_OWN_PAGES (1U << 1)
+#define FS_CFLG_OWN_D_OPS (1U << 2)
 
 /*
  * crypto operations for filesystems
-- 
2.21.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/4] fscrypt: Export fscrypt_d_ops

[Richard Weinberger]

If a filesystem manages dentry operations itself it might want to
re-use fscrypt_d_ops.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/crypto/crypto.c          | 1 +
 fs/crypto/fscrypt_private.h | 1 -
 include/linux/fscrypt.h     | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index 4dc788e3bc96..8018f8bba50d 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -357,6 +357,7 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
 const struct dentry_operations fscrypt_d_ops = {
 	.d_revalidate = fscrypt_d_revalidate,
 };
+EXPORT_SYMBOL(fscrypt_d_ops);
 
 void fscrypt_restore_control_page(struct page *page)
 {
diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
index 7da276159593..bced1ee4fd64 100644
--- a/fs/crypto/fscrypt_private.h
+++ b/fs/crypto/fscrypt_private.h
@@ -125,7 +125,6 @@ extern int fscrypt_do_page_crypto(const struct inode *inode,
 				  gfp_t gfp_flags);
 extern struct page *fscrypt_alloc_bounce_page(struct fscrypt_ctx *ctx,
 					      gfp_t gfp_flags);
-extern const struct dentry_operations fscrypt_d_ops;
 
 extern void __printf(3, 4) __cold
 fscrypt_msg(struct super_block *sb, const char *level, const char *fmt, ...);
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 7139a110ac4f..2b9577e4707f 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -231,6 +231,7 @@ extern int __fscrypt_encrypt_symlink(struct inode *inode, const char *target,
 extern const char *fscrypt_get_symlink(struct inode *inode, const void *caddr,
 				       unsigned int max_size,
 				       struct delayed_call *done);
+extern const struct dentry_operations fscrypt_d_ops;
 #else  /* !CONFIG_FS_ENCRYPTION */
 
 static inline bool fscrypt_has_encryption_key(const struct inode *inode)
-- 
2.21.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/4] ubifs: Simplify fscrypt_get_encryption_info() error handling

[Richard Weinberger]

fscrypt_get_encryption_info() does not return -ENOKEY,
there is no need to handle this case.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/dir.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 5767b373a8ff..b0cb913697c5 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -526,7 +526,7 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 
 	if (encrypted) {
 		err = fscrypt_get_encryption_info(dir);
-		if (err && err != -ENOKEY)
+		if (err)
 			return err;
 
 		err = fscrypt_fname_alloc_buffer(dir, UBIFS_MAX_NLEN, &fstr);
@@ -794,7 +794,7 @@ static int ubifs_unlink(struct inode *dir, struct dentry *dentry)
 
 	if (ubifs_crypt_is_encrypted(dir)) {
 		err = fscrypt_get_encryption_info(dir);
-		if (err && err != -ENOKEY)
+		if (err)
 			return err;
 	}
 
@@ -904,7 +904,7 @@ static int ubifs_rmdir(struct inode *dir, struct dentry *dentry)
 
 	if (ubifs_crypt_is_encrypted(dir)) {
 		err = fscrypt_get_encryption_info(dir);
-		if (err && err != -ENOKEY)
+		if (err)
 			return err;
 	}
 
-- 
2.21.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[Richard Weinberger]

Usually fscrypt allows limited access to encrypted files even
if no key is available.
Encrypted filenames are shown and based on this names users
can unlink and move files.

This is not always what people expect. The fscrypt_key_required mount
option disables this feature.
If no key is present all access is denied with the -ENOKEY error code.

The side benefit of this is that we don't need ->d_revalidate().
Not having ->d_revalidate() makes an encrypted ubifs usable
as overlayfs upper directory.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 fs/ubifs/crypto.c |  2 +-
 fs/ubifs/dir.c    | 29 ++++++++++++++++++++++++++---
 fs/ubifs/super.c  | 15 +++++++++++++++
 fs/ubifs/ubifs.h  |  1 +
 4 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c
index 4aaedf2d7f44..a6a48c5dc058 100644
--- a/fs/ubifs/crypto.c
+++ b/fs/ubifs/crypto.c
@@ -76,7 +76,7 @@ int ubifs_decrypt(const struct inode *inode, struct ubifs_data_node *dn,
 }
 
 const struct fscrypt_operations ubifs_crypt_operations = {
-	.flags			= FS_CFLG_OWN_PAGES,
+	.flags			= FS_CFLG_OWN_PAGES | FS_CFLG_OWN_D_OPS,
 	.key_prefix		= "ubifs:",
 	.get_context		= ubifs_crypt_get_context,
 	.set_context		= ubifs_crypt_set_context,
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index b0cb913697c5..4d029f08b80d 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -208,6 +208,16 @@ static int dbg_check_name(const struct ubifs_info *c,
 	return 0;
 }
 
+static void ubifs_set_dentry_ops(struct inode *dir, struct dentry *dentry)
+{
+#ifdef CONFIG_FS_ENCRYPTION
+	struct ubifs_info *c = dir->i_sb->s_fs_info;
+
+	if (IS_ENCRYPTED(dir) && !c->fscrypt_key_required)
+		d_set_d_op(dentry, &fscrypt_d_ops);
+#endif
+}
+
 static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 				   unsigned int flags)
 {
@@ -224,7 +234,10 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 	if (err)
 		return ERR_PTR(err);
 
-	err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm);
+	ubifs_set_dentry_ops(dir, dentry);
+
+	err = fscrypt_setup_filename(dir, &dentry->d_name,
+				     !c->fscrypt_key_required, &nm);
 	if (err)
 		return ERR_PTR(err);
 
@@ -240,6 +253,11 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
 	}
 
 	if (nm.hash) {
+		if (c->fscrypt_key_required) {
+			inode = ERR_PTR(-ENOKEY);
+			goto done;
+		}
+
 		ubifs_assert(c, fname_len(&nm) == 0);
 		ubifs_assert(c, fname_name(&nm) == NULL);
 		dent_key_init_hash(c, &key, dir->i_ino, nm.hash);
@@ -529,6 +547,9 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
 		if (err)
 			return err;
 
+		if (c->fscrypt_key_required && !dir->i_crypt_info)
+			return -ENOKEY;
+
 		err = fscrypt_fname_alloc_buffer(dir, UBIFS_MAX_NLEN, &fstr);
 		if (err)
 			return err;
@@ -798,7 +819,8 @@ static int ubifs_unlink(struct inode *dir, struct dentry *dentry)
 			return err;
 	}
 
-	err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm);
+	err = fscrypt_setup_filename(dir, &dentry->d_name, !c->fscrypt_key_required,
+				     &nm);
 	if (err)
 		return err;
 
@@ -908,7 +930,8 @@ static int ubifs_rmdir(struct inode *dir, struct dentry *dentry)
 			return err;
 	}
 
-	err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm);
+	err = fscrypt_setup_filename(dir, &dentry->d_name,
+				     !c->fscrypt_key_required, &nm);
 	if (err)
 		return err;
 
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 8dc2818fdd84..e081b00236b6 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -445,6 +445,9 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root)
 			   ubifs_compr_name(c, c->mount_opts.compr_type));
 	}
 
+	if (c->fscrypt_key_required)
+		seq_puts(s, ",fscrypt_key_required");
+
 	seq_printf(s, ",assert=%s", ubifs_assert_action_name(c));
 	seq_printf(s, ",ubi=%d,vol=%d", c->vi.ubi_num, c->vi.vol_id);
 
@@ -952,6 +955,7 @@ enum {
 	Opt_assert,
 	Opt_auth_key,
 	Opt_auth_hash_name,
+	Opt_fscrypt_key_required,
 	Opt_ignore,
 	Opt_err,
 };
@@ -969,6 +973,7 @@ static const match_table_t tokens = {
 	{Opt_ignore, "ubi=%s"},
 	{Opt_ignore, "vol=%s"},
 	{Opt_assert, "assert=%s"},
+	{Opt_fscrypt_key_required, "fscrypt_key_required"},
 	{Opt_err, NULL},
 };
 
@@ -1008,6 +1013,7 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
 {
 	char *p;
 	substring_t args[MAX_OPT_ARGS];
+	unsigned int old_fscrypt_key_required = c->fscrypt_key_required;
 
 	if (!options)
 		return 0;
@@ -1099,6 +1105,15 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
 			if (!c->auth_hash_name)
 				return -ENOMEM;
 			break;
+		case Opt_fscrypt_key_required:
+			c->fscrypt_key_required = 1;
+
+			if (is_remount && (old_fscrypt_key_required != c->fscrypt_key_required)) {
+				ubifs_err(c, "fscrypt_key_required cannot changed by remount");
+				return -EINVAL;
+			}
+
+			break;
 		case Opt_ignore:
 			break;
 		default:
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 1ae12900e01d..71b03a6798ae 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -1304,6 +1304,7 @@ struct ubifs_info {
 	unsigned int rw_incompat:1;
 	unsigned int assert_action:2;
 	unsigned int authenticated:1;
+	unsigned int fscrypt_key_required:1;
 
 	struct mutex tnc_mutex;
 	struct ubifs_zbranch zroot;
-- 
2.21.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[Eric Biggers]

Hi Richard,

On Thu, Mar 14, 2019 at 06:15:59PM +0100, Richard Weinberger wrote:
> Usually fscrypt allows limited access to encrypted files even
> if no key is available.
> Encrypted filenames are shown and based on this names users
> can unlink and move files.

Actually, fscrypt doesn't allow moving files without the key.  It would only be
possible for cross-renames, i.e. renames with the RENAME_EXCHANGE flag.  So for
consistency with regular renames, fscrypt also forbids cross-renames if the key
for either the source or destination directory is missing.

So the main use case for the ciphertext view is *deleting* files.  For example,
deleting a user's home directory after that user has been removed from the
system.  Or the system freeing up space by deleting cache files from a user who
isn't currently logged in.

> 
> This is not always what people expect. The fscrypt_key_required mount
> option disables this feature.
> If no key is present all access is denied with the -ENOKEY error code.

The problem with this mount option is that it allows users to create undeletable
files.  So I'm not really convinced yet this is a good change.  And though the
fscrypt_key_required semantics are easier to implement, we'd still have to
support the existing semantics too, thus increasing the maintenance cost.

> 
> The side benefit of this is that we don't need ->d_revalidate().
> Not having ->d_revalidate() makes an encrypted ubifs usable
> as overlayfs upper directory.
> 

It would be preferable if we could get overlayfs to work without providing a
special mount option.

> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  fs/ubifs/crypto.c |  2 +-
>  fs/ubifs/dir.c    | 29 ++++++++++++++++++++++++++---
>  fs/ubifs/super.c  | 15 +++++++++++++++
>  fs/ubifs/ubifs.h  |  1 +
>  4 files changed, 43 insertions(+), 4 deletions(-)
> 

Shouldn't readlink() honor the mount option too?

> diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c
> index 4aaedf2d7f44..a6a48c5dc058 100644
> --- a/fs/ubifs/crypto.c
> +++ b/fs/ubifs/crypto.c
> @@ -76,7 +76,7 @@ int ubifs_decrypt(const struct inode *inode, struct ubifs_data_node *dn,
>  }
>  
>  const struct fscrypt_operations ubifs_crypt_operations = {
> -	.flags			= FS_CFLG_OWN_PAGES,
> +	.flags			= FS_CFLG_OWN_PAGES | FS_CFLG_OWN_D_OPS,
>  	.key_prefix		= "ubifs:",
>  	.get_context		= ubifs_crypt_get_context,
>  	.set_context		= ubifs_crypt_set_context,
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index b0cb913697c5..4d029f08b80d 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -208,6 +208,16 @@ static int dbg_check_name(const struct ubifs_info *c,
>  	return 0;
>  }
>  
> +static void ubifs_set_dentry_ops(struct inode *dir, struct dentry *dentry)
> +{
> +#ifdef CONFIG_FS_ENCRYPTION
> +	struct ubifs_info *c = dir->i_sb->s_fs_info;
> +
> +	if (IS_ENCRYPTED(dir) && !c->fscrypt_key_required)
> +		d_set_d_op(dentry, &fscrypt_d_ops);
> +#endif
> +}
> +
>  static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
>  				   unsigned int flags)
>  {
> @@ -224,7 +234,10 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
>  	if (err)
>  		return ERR_PTR(err);
>  
> -	err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm);
> +	ubifs_set_dentry_ops(dir, dentry);
> +
> +	err = fscrypt_setup_filename(dir, &dentry->d_name,
> +				     !c->fscrypt_key_required, &nm);
>  	if (err)
>  		return ERR_PTR(err);
>  
> @@ -240,6 +253,11 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,
>  	}
>  
>  	if (nm.hash) {
> +		if (c->fscrypt_key_required) {
> +			inode = ERR_PTR(-ENOKEY);
> +			goto done;
> +		}
> +
>  		ubifs_assert(c, fname_len(&nm) == 0);
>  		ubifs_assert(c, fname_name(&nm) == NULL);
>  		dent_key_init_hash(c, &key, dir->i_ino, nm.hash);
> @@ -529,6 +547,9 @@ static int ubifs_readdir(struct file *file, struct dir_context *ctx)
>  		if (err)
>  			return err;
>  
> +		if (c->fscrypt_key_required && !dir->i_crypt_info)
> +			return -ENOKEY;
> +

How about returning -ENOKEY when trying to open the directory in the first
place, rather than allowing getting to readdir()?  That would match the behavior
of regular files.

>  		err = fscrypt_fname_alloc_buffer(dir, UBIFS_MAX_NLEN, &fstr);
>  		if (err)
>  			return err;
> @@ -798,7 +819,8 @@ static int ubifs_unlink(struct inode *dir, struct dentry *dentry)
>  			return err;
>  	}
>  
> -	err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm);
> +	err = fscrypt_setup_filename(dir, &dentry->d_name, !c->fscrypt_key_required,
> +				     &nm);
>  	if (err)
>  		return err;
>  
> @@ -908,7 +930,8 @@ static int ubifs_rmdir(struct inode *dir, struct dentry *dentry)
>  			return err;
>  	}
>  
> -	err = fscrypt_setup_filename(dir, &dentry->d_name, 1, &nm);
> +	err = fscrypt_setup_filename(dir, &dentry->d_name,
> +				     !c->fscrypt_key_required, &nm);
>  	if (err)
>  		return err;
>  
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 8dc2818fdd84..e081b00236b6 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -445,6 +445,9 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root)
>  			   ubifs_compr_name(c, c->mount_opts.compr_type));
>  	}
>  
> +	if (c->fscrypt_key_required)
> +		seq_puts(s, ",fscrypt_key_required");
> +
>  	seq_printf(s, ",assert=%s", ubifs_assert_action_name(c));
>  	seq_printf(s, ",ubi=%d,vol=%d", c->vi.ubi_num, c->vi.vol_id);
>  
> @@ -952,6 +955,7 @@ enum {
>  	Opt_assert,
>  	Opt_auth_key,
>  	Opt_auth_hash_name,
> +	Opt_fscrypt_key_required,
>  	Opt_ignore,
>  	Opt_err,
>  };
> @@ -969,6 +973,7 @@ static const match_table_t tokens = {
>  	{Opt_ignore, "ubi=%s"},
>  	{Opt_ignore, "vol=%s"},
>  	{Opt_assert, "assert=%s"},
> +	{Opt_fscrypt_key_required, "fscrypt_key_required"},
>  	{Opt_err, NULL},
>  };
>  
> @@ -1008,6 +1013,7 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
>  {
>  	char *p;
>  	substring_t args[MAX_OPT_ARGS];
> +	unsigned int old_fscrypt_key_required = c->fscrypt_key_required;
>  
>  	if (!options)
>  		return 0;
> @@ -1099,6 +1105,15 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options,
>  			if (!c->auth_hash_name)
>  				return -ENOMEM;
>  			break;
> +		case Opt_fscrypt_key_required:
> +			c->fscrypt_key_required = 1;
> +
> +			if (is_remount && (old_fscrypt_key_required != c->fscrypt_key_required)) {
> +				ubifs_err(c, "fscrypt_key_required cannot changed by remount");
> +				return -EINVAL;
> +			}
> +
> +			break;
>  		case Opt_ignore:
>  			break;
>  		default:
> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
> index 1ae12900e01d..71b03a6798ae 100644
> --- a/fs/ubifs/ubifs.h
> +++ b/fs/ubifs/ubifs.h
> @@ -1304,6 +1304,7 @@ struct ubifs_info {
>  	unsigned int rw_incompat:1;
>  	unsigned int assert_action:2;
>  	unsigned int authenticated:1;
> +	unsigned int fscrypt_key_required:1;
>  
>  	struct mutex tnc_mutex;
>  	struct ubifs_zbranch zroot;
> -- 
> 2.21.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[Richard Weinberger]

Eric,

Am Donnerstag, 14. März 2019, 18:49:14 CET schrieb Eric Biggers:
> Hi Richard,
> 
> On Thu, Mar 14, 2019 at 06:15:59PM +0100, Richard Weinberger wrote:
> > Usually fscrypt allows limited access to encrypted files even
> > if no key is available.
> > Encrypted filenames are shown and based on this names users
> > can unlink and move files.
> 
> Actually, fscrypt doesn't allow moving files without the key.  It would only be
> possible for cross-renames, i.e. renames with the RENAME_EXCHANGE flag.  So for
> consistency with regular renames, fscrypt also forbids cross-renames if the key
> for either the source or destination directory is missing.
> 
> So the main use case for the ciphertext view is *deleting* files.  For example,
> deleting a user's home directory after that user has been removed from the
> system.  Or the system freeing up space by deleting cache files from a user who
> isn't currently logged in.

Right, I somehow thought beside of deleting you can do more.

> > 
> > This is not always what people expect. The fscrypt_key_required mount
> > option disables this feature.
> > If no key is present all access is denied with the -ENOKEY error code.
> 
> The problem with this mount option is that it allows users to create undeletable
> files.  So I'm not really convinced yet this is a good change.  And though the
> fscrypt_key_required semantics are easier to implement, we'd still have to
> support the existing semantics too, thus increasing the maintenance cost.

The undeletable-file argument is a good point. Thanks for bringing this up.
To get rid of such files root needs to mount without the new mount parameter. ;-\

> > 
> > The side benefit of this is that we don't need ->d_revalidate().
> > Not having ->d_revalidate() makes an encrypted ubifs usable
> > as overlayfs upper directory.
> > 
> 
> It would be preferable if we could get overlayfs to work without providing a
> special mount option.

Yes, but let's see what Al finds in his review.

> > Signed-off-by: Richard Weinberger <richard@nod.at>
> > ---
> >  fs/ubifs/crypto.c |  2 +-
> >  fs/ubifs/dir.c    | 29 ++++++++++++++++++++++++++---
> >  fs/ubifs/super.c  | 15 +++++++++++++++
> >  fs/ubifs/ubifs.h  |  1 +
> >  4 files changed, 43 insertions(+), 4 deletions(-)
> > 
> 
> Shouldn't readlink() honor the mount option too?

Hmmm, yes. We need to honor it in ->get_link() too.

> > +		if (c->fscrypt_key_required && !dir->i_crypt_info)
> > +			return -ENOKEY;
> > +
> 
> How about returning -ENOKEY when trying to open the directory in the first
> place, rather than allowing getting to readdir()?  That would match the behavior
> of regular files.

I'm not sure what the best approach is.
We could also do it in ->permission().

Thanks,
//richard



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[Theodore Ts'o]

Richard --- stepping back for a moment, in your use case, are you
assuming that the encryption key is always going to be present while
the system is running?

Ubifs can't use dm-crypt, since it doesn't have a block device, but if
you could, is much more like dm-crypt, in that you have the key
*before* the file system is mounted, and you don't really expect the
key to ever be expunged from the system while it is mounted?

If that's true, maybe the real mismatch is in using fscrypt in the
first place --- and in fact, something where you encrypt everything,
including the file system metadata (ala dm-crypt), would actually give
you much better security properties.

       	       		     	 	- Ted

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[Richard Weinberger]

Ted,

Am Freitag, 15. März 2019, 00:07:02 CET schrieb Theodore Ts'o:
> Richard --- stepping back for a moment, in your use case, are you
> assuming that the encryption key is always going to be present while
> the system is running?

it is not a hard requirement, it is something what is common on embedded
systems that utilize UBIFS and fscrypt.

> Ubifs can't use dm-crypt, since it doesn't have a block device, but if
> you could, is much more like dm-crypt, in that you have the key
> *before* the file system is mounted, and you don't really expect the
> key to ever be expunged from the system while it is mounted?
> 
> If that's true, maybe the real mismatch is in using fscrypt in the
> first place --- and in fact, something where you encrypt everything,
> including the file system metadata (ala dm-crypt), would actually give
> you much better security properties.

Well, fscrypt was chosen as UBIFS encryption backend because per-file encryption
with derived keys makes a lot of sense.
Also the implementation was not super hard, David and I weren't keen to reinvent
dm-crypt für UBI/MTD.

That said, I'm happy with fscrypt, it works well in production.
But being not able to use UBIFS as lower dir on overlayfs hurts.
On embedded systems where the key is always present the proposed hack works
fine. If we can get overlayfs work without that I'll be more than happy.

Thanks,
//richard



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[Theodore Ts'o]

On Fri, Mar 15, 2019 at 08:48:10AM +0100, Richard Weinberger wrote:
> Ted,
> 
> Am Freitag, 15. März 2019, 00:07:02 CET schrieb Theodore Ts'o:
> > Richard --- stepping back for a moment, in your use case, are you
> > assuming that the encryption key is always going to be present while
> > the system is running?
> 
> it is not a hard requirement, it is something what is common on embedded
> systems that utilize UBIFS and fscrypt.
> 
> Well, fscrypt was chosen as UBIFS encryption backend because per-file encryption
> with derived keys makes a lot of sense.
> Also the implementation was not super hard, David and I weren't keen to reinvent
> dm-crypt für UBI/MTD.
> 
> That said, I'm happy with fscrypt, it works well in production.

OK, but please note that fscrypt leaks i_size and timestamp
information; dm-crypt doesn't.  An enterprising attacker could very
easily be able to do something interesting with that information, so
be sure you've thought through what the threat model for users of
ubifs is going to be.

If you need per-user keying, and you need to be able to mount the file
system and access some of the files without having any keys, and if
it's useful for an admin to be able to delete files without having the
key, then fscrypt is a great fit.

You are proposing changes that (optionally) eliminate that last
advantage of fscrypt.  So I just wanted to sanity check whether or not
the other advantages are useful to you, and worth the security
tradeoffs that are inherent in such a choice.  If it's worth it, then
great.  But if it isn't, I'd much rather that you appropriately
protect your users and your customers rather than be an additional
user of fscrypt.  :-)

Cheers,

						- Ted

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[Richard Weinberger]

Ted,

Am Freitag, 15. März 2019, 14:51:28 CET schrieb Theodore Ts'o:
> On Fri, Mar 15, 2019 at 08:48:10AM +0100, Richard Weinberger wrote:
> > Ted,
> > 
> > Am Freitag, 15. März 2019, 00:07:02 CET schrieb Theodore Ts'o:
> > > Richard --- stepping back for a moment, in your use case, are you
> > > assuming that the encryption key is always going to be present while
> > > the system is running?
> > 
> > it is not a hard requirement, it is something what is common on embedded
> > systems that utilize UBIFS and fscrypt.
> > 
> > Well, fscrypt was chosen as UBIFS encryption backend because per-file encryption
> > with derived keys makes a lot of sense.
> > Also the implementation was not super hard, David and I weren't keen to reinvent
> > dm-crypt für UBI/MTD.
> > 
> > That said, I'm happy with fscrypt, it works well in production.
> 
> OK, but please note that fscrypt leaks i_size and timestamp
> information; dm-crypt doesn't.  An enterprising attacker could very
> easily be able to do something interesting with that information, so
> be sure you've thought through what the threat model for users of
> ubifs is going to be.

No need to worry, I'm fully aware of all this.
 
> If you need per-user keying, and you need to be able to mount the file
> system and access some of the files without having any keys, and if
> it's useful for an admin to be able to delete files without having the
> key, then fscrypt is a great fit.
> 
> You are proposing changes that (optionally) eliminate that last
> advantage of fscrypt.  So I just wanted to sanity check whether or not
> the other advantages are useful to you, and worth the security
> tradeoffs that are inherent in such a choice.  If it's worth it, then
> great.  But if it isn't, I'd much rather that you appropriately
> protect your users and your customers rather than be an additional
> user of fscrypt.  :-)

Like I said, this patch series is an RFC, the mount option was suggested
by Amir and Miklos, so I assumed showing some code is a good base for further
discussion.
For most of *my* use-cases it works but having general support for fscrypt+overlayfs
would be the ultimate goal.

Thanks,
//richard



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[James Bottomley]

On Thu, 2019-03-14 at 18:15 +0100, Richard Weinberger wrote:
> Usually fscrypt allows limited access to encrypted files even
> if no key is available.
> Encrypted filenames are shown and based on this names users
> can unlink and move files.

Shouldn't they be able to read/write and create as well (all with the
ciphertext name and contents, of course) ... otherwise how does backup
of encrypted files by admin without the key ever work?

James


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[Theodore Ts'o]

On Thu, Mar 14, 2019 at 04:15:11PM -0700, James Bottomley wrote:
> On Thu, 2019-03-14 at 18:15 +0100, Richard Weinberger wrote:
> > Usually fscrypt allows limited access to encrypted files even
> > if no key is available.
> > Encrypted filenames are shown and based on this names users
> > can unlink and move files.
> 
> Shouldn't they be able to read/write and create as well (all with the
> ciphertext name and contents, of course) ... otherwise how does backup
> of encrypted files by admin without the key ever work?

That's not currently supported.  Michael Halcrow and I worked out some
designs on how to do this, and I even had some prototype patches, but
it's really, really, messy, and requires a lot of complex machinations
in userspace on the save *and* restore, since there's a lot of crypto
metadata that has to be saved, and you have to handle backing up the
directory per-file keys, and restoring them when you recreate the
directory on a restore, etc.

The simpler approach would have allowed backup of encrypted files
without the key, but would require the user's key to do the restore,
and if we ever actually tried to get this feature supported, that's
the approach I'd suggest.


The fundamental reason why we never went did anything with this was
that the original use case of fscrypt was for Chrome OS, where the
original design premise was that you don't need to do backups, since
everything is in the cloud.  A video from 2010:

	https://www.youtube.com/watch?v=lm-Vnx58UYo

And with Android, backups happen automatically while you have the
encryption key.

There was talk about using fscrypt for Ubuntu desktops as an ecryptfs
replacement, and in that case, we would have use case that would have
required backups of a shared desktop where you don't have all of the
encryption keys for all of the users.  Maybe someday as a Google
Summer of Code project or maybe if some potential corporate user of
fscrypt would be willing to fund the necessary engineering work?

But again, I'll repeat this because it's so important: In the vast
majority of cases, including single-user laptops, desktops, etc., the
real right answer is dm-crypt and *not* fscrypt.  Especially since
time-sharing systems are *so* 1980's.  :-)

So I don't use fscrypt on my upstream development laptop (except for
testing purposes) because it's simply not the right tool for the job.

		      	 	    - Ted

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/4] ubifs: Implement new mount option, fscrypt_key_required

[James Bottomley]

On Thu, 2019-03-14 at 19:42 -0400, Theodore Ts'o wrote:
> On Thu, Mar 14, 2019 at 04:15:11PM -0700, James Bottomley wrote:
> > On Thu, 2019-03-14 at 18:15 +0100, Richard Weinberger wrote:
> > > Usually fscrypt allows limited access to encrypted files even
> > > if no key is available.
> > > Encrypted filenames are shown and based on this names users
> > > can unlink and move files.
> > 
> > Shouldn't they be able to read/write and create as well (all with
> > the ciphertext name and contents, of course) ... otherwise how does
> > backup of encrypted files by admin without the key ever work?
> 
> That's not currently supported.  Michael Halcrow and I worked out
> some designs on how to do this, and I even had some prototype
> patches, but it's really, really, messy, and requires a lot of
> complex machinations in userspace on the save *and* restore, since
> there's a lot of crypto metadata that has to be saved, and you have
> to handle backing up the directory per-file keys, and restoring them
> when you recreate the directory on a restore, etc.
> 
> The simpler approach would have allowed backup of encrypted files
> without the key, but would require the user's key to do the restore,
> and if we ever actually tried to get this feature supported, that's
> the approach I'd suggest.

Well, as I said above encrypted file backup by an admin who doesn't
have the key was what I was thinking of.

> The fundamental reason why we never went did anything with this was
> that the original use case of fscrypt was for Chrome OS, where the
> original design premise was that you don't need to do backups, since
> everything is in the cloud.  A video from 2010:
> 
> 	https://www.youtube.com/watch?v=lm-Vnx58UYo
> 
> And with Android, backups happen automatically while you have the
> encryption key.

Heh, colour me paranoid, but if I backup my sensitive data to any
medium (including the cloud) I *want* the backup to be encrypted so
that if someone is careless with my backup data at least they don't get
the contents.

> There was talk about using fscrypt for Ubuntu desktops as an ecryptfs
> replacement, and in that case, we would have use case that would have
> required backups of a shared desktop where you don't have all of the
> encryption keys for all of the users.  Maybe someday as a Google
> Summer of Code project or maybe if some potential corporate user of
> fscrypt would be willing to fund the necessary engineering work?
> 
> But again, I'll repeat this because it's so important: In the vast
> majority of cases, including single-user laptops, desktops, etc., the
> real right answer is dm-crypt and *not* fscrypt.  Especially since
> time-sharing systems are *so* 1980's.  :-)
> 
> So I don't use fscrypt on my upstream development laptop (except for
> testing purposes) because it's simply not the right tool for the job.

I was thinking of the container use case again.  It's not really backup
per se but it is serialization: if you can't do a backup ... i.e. save
and restore an encrypted tar image, you can't use the filesystem for
container image protection.  But it also strikes me that inability to
do backup and restore without the key really restricts the use cases
for the entire filesystem.

James


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/