[PATCH] ext4: forbid encrypting root directory

[PATCH] ext4: forbid encrypting root directory

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Currently it's possible to encrypt all files and directories on an ext4
filesystem by deleting everything, including lost+found, then setting an
encryption policy on the root directory.  However, this is incompatible
with e2fsck because e2fsck expects to find, create, and/or write to
lost+found and does not have access to any encryption keys.  Especially
problematic is that if e2fsck can't find lost+found, it will create it
without regard for whether the root directory is encrypted.  This is
wrong for obvious reasons, and it causes a later run of e2fsck to
consider the lost+found directory entry to be corrupted.

It's not clear it would be useful to support encrypting the root
directory, because in that scenario dm-crypt might as well be used
instead.  But in any case, it's broken currently and must not be
allowed; so start returning an error if userspace tries to set an
encryption policy on the root directory.

For now do this in ext4 only, because f2fs and ubifs do not appear to
have the lost+found requirement.  We could move it into
fscrypt_ioctl_set_policy() later if desired, though.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/ext4/super.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index d37c81f327e7..ed8eacdf61da 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1145,6 +1145,15 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
 	handle_t *handle = fs_data;
 	int res, res2, retries = 0;
 
+	/*
+	 * Encrypting the root directory is not allowed because e2fsck expects
+	 * lost+found to exist and be unencrypted, and encrypting the root
+	 * directory would imply encrypting the lost+found directory as well as
+	 * the filename "lost+found" itself.
+	 */
+	if (inode->i_ino == EXT4_ROOT_INO)
+		return -EBUSY;
+
 	res = ext4_convert_inline_data(inode);
 	if (res)
 		return res;
-- 
2.13.1.508.gb3defc5cc-goog

Re: [PATCH] ext4: forbid encrypting root directory

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

On Jun 14, 2017, at 5:17 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> From: Eric Biggers <ebiggers@google.com>
> 
> Currently it's possible to encrypt all files and directories on an ext4
> filesystem by deleting everything, including lost+found, then setting an
> encryption policy on the root directory.  However, this is incompatible
> with e2fsck because e2fsck expects to find, create, and/or write to
> lost+found and does not have access to any encryption keys.  Especially
> problematic is that if e2fsck can't find lost+found, it will create it
> without regard for whether the root directory is encrypted.  This is
> wrong for obvious reasons, and it causes a later run of e2fsck to
> consider the lost+found directory entry to be corrupted.
> 
> It's not clear it would be useful to support encrypting the root
> directory, because in that scenario dm-crypt might as well be used
> instead.

The benefit of per-file encryption over dm-crypt is that it isn't an
all-or-nothing usage case like dm-crypt (e.g. there could be different
users/keys for different subdirectories), and secure file delete (as
mentioned earlier) works properly with per-file encryption, and doesn't
work at all with dm-crypt.

> But in any case, it's broken currently and must not be allowed; so
> start returning an error if userspace tries to set an encryption
> policy on the root directory.
> 
> For now do this in ext4 only, because f2fs and ubifs do not appear to
> have the lost+found requirement.  We could move it into
> fscrypt_ioctl_set_policy() later if desired, though.

What about a special case to handle an unencrypted lost+found inode?

There was also a patch series a while ago to explicitly add lost+found
into the superblock so that it could be found even if the root directory
was corrupted, and to allow flexibility in whether it is always shown in
the root directory or not (e.g. allowing ".lost+found" or similar).

Cheers, Andreas

> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> fs/ext4/super.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index d37c81f327e7..ed8eacdf61da 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1145,6 +1145,15 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> 	handle_t *handle = fs_data;
> 	int res, res2, retries = 0;
> 
> +	/*
> +	 * Encrypting the root directory is not allowed because e2fsck expects
> +	 * lost+found to exist and be unencrypted, and encrypting the root
> +	 * directory would imply encrypting the lost+found directory as well as
> +	 * the filename "lost+found" itself.
> +	 */
> +	if (inode->i_ino == EXT4_ROOT_INO)
> +		return -EBUSY;

Why -EBUSY and not -EPERM?

> 	res = ext4_convert_inline_data(inode);
> 	if (res)
> 		return res;
> --
> 2.13.1.508.gb3defc5cc-goog
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] ext4: forbid encrypting root directory

[Eric Biggers]

Hi Andreas,

On Wed, Jun 14, 2017 at 06:00:33PM -0600, Andreas Dilger wrote:
> On Jun 14, 2017, at 5:17 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > 
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Currently it's possible to encrypt all files and directories on an ext4
> > filesystem by deleting everything, including lost+found, then setting an
> > encryption policy on the root directory.  However, this is incompatible
> > with e2fsck because e2fsck expects to find, create, and/or write to
> > lost+found and does not have access to any encryption keys.  Especially
> > problematic is that if e2fsck can't find lost+found, it will create it
> > without regard for whether the root directory is encrypted.  This is
> > wrong for obvious reasons, and it causes a later run of e2fsck to
> > consider the lost+found directory entry to be corrupted.
> > 
> > It's not clear it would be useful to support encrypting the root
> > directory, because in that scenario dm-crypt might as well be used
> > instead.
> 
> The benefit of per-file encryption over dm-crypt is that it isn't an
> all-or-nothing usage case like dm-crypt (e.g. there could be different
> users/keys for different subdirectories), and secure file delete (as
> mentioned earlier) works properly with per-file encryption, and doesn't
> work at all with dm-crypt.
> 

Well, encrypting the root directory *is* the all-or-nothing use case --- all
files and directories on the filesystem would be encrypted with the same key,
and by design it cannot be overridden for individual files/directories.

"Secure file delete" would be an advantage if/when it's implemented, though.

> > But in any case, it's broken currently and must not be allowed; so
> > start returning an error if userspace tries to set an encryption
> > policy on the root directory.
> > 
> > For now do this in ext4 only, because f2fs and ubifs do not appear to
> > have the lost+found requirement.  We could move it into
> > fscrypt_ioctl_set_policy() later if desired, though.
> 
> What about a special case to handle an unencrypted lost+found inode?
> 
> There was also a patch series a while ago to explicitly add lost+found
> into the superblock so that it could be found even if the root directory
> was corrupted, and to allow flexibility in whether it is always shown in
> the root directory or not (e.g. allowing ".lost+found" or similar).
> 

It could be done if the lost+found inode was not linked to from any directory
and instead had its inode number stored in the superblock so that e2fsck could
still find it.  However, if e2fsck put files in a lost+found directory that
doesn't exist in the filesystem directory structure, how would users retrieve
those files?  Would users be required to run a special e2fsprogs command to
list/read/delete the files in lost+found?

> > +	/*
> > +	 * Encrypting the root directory is not allowed because e2fsck expects
> > +	 * lost+found to exist and be unencrypted, and encrypting the root
> > +	 * directory would imply encrypting the lost+found directory as well as
> > +	 * the filename "lost+found" itself.
> > +	 */
> > +	if (inode->i_ino == EXT4_ROOT_INO)
> > +		return -EBUSY;
> 
> Why -EBUSY and not -EPERM?
> 

No strong reason; I had in mind that EBUSY is returned when running a command
like 'rmdir /mnt'.  Probably EPERM would be better.

Eric

Re: [PATCH] ext4: forbid encrypting root directory

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3907 bytes --]

On Jun 14, 2017, at 6:24 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> Hi Andreas,
> 
> On Wed, Jun 14, 2017 at 06:00:33PM -0600, Andreas Dilger wrote:
>> On Jun 14, 2017, at 5:17 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
>>> 
>>> From: Eric Biggers <ebiggers@google.com>
>>> 
>>> Currently it's possible to encrypt all files and directories on an ext4
>>> filesystem by deleting everything, including lost+found, then setting an
>>> encryption policy on the root directory.  However, this is incompatible
>>> with e2fsck because e2fsck expects to find, create, and/or write to
>>> lost+found and does not have access to any encryption keys.  Especially
>>> problematic is that if e2fsck can't find lost+found, it will create it
>>> without regard for whether the root directory is encrypted.  This is
>>> wrong for obvious reasons, and it causes a later run of e2fsck to
>>> consider the lost+found directory entry to be corrupted.
>>> 
>>> It's not clear it would be useful to support encrypting the root
>>> directory, because in that scenario dm-crypt might as well be used
>>> instead.
>> 
>> The benefit of per-file encryption over dm-crypt is that it isn't an
>> all-or-nothing usage case like dm-crypt (e.g. there could be different
>> users/keys for different subdirectories), and secure file delete (as
>> mentioned earlier) works properly with per-file encryption, and doesn't
>> work at all with dm-crypt.
> 
> Well, encrypting the root directory *is* the all-or-nothing use case --- all
> files and directories on the filesystem would be encrypted with the same key,
> and by design it cannot be overridden for individual files/directories.
> 
> "Secure file delete" would be an advantage if/when it's implemented, though.
> 
>>> But in any case, it's broken currently and must not be allowed; so
>>> start returning an error if userspace tries to set an encryption
>>> policy on the root directory.
>>> 
>>> For now do this in ext4 only, because f2fs and ubifs do not appear to
>>> have the lost+found requirement.  We could move it into
>>> fscrypt_ioctl_set_policy() later if desired, though.
>> 
>> What about a special case to handle an unencrypted lost+found inode?
>> 
>> There was also a patch series a while ago to explicitly add lost+found
>> into the superblock so that it could be found even if the root directory
>> was corrupted, and to allow flexibility in whether it is always shown in
>> the root directory or not (e.g. allowing ".lost+found" or similar).
> 
> It could be done if the lost+found inode was not linked to from any directory
> and instead had its inode number stored in the superblock so that e2fsck could
> still find it.  However, if e2fsck put files in a lost+found directory that
> doesn't exist in the filesystem directory structure, how would users retrieve
> those files?  Would users be required to run a special e2fsprogs command to
> list/read/delete the files in lost+found?

I was thinking that readdir on the root inode could insert the "lost+found"
or ".lost+found" entry dynamically, or (a bit less pleasant) is to add a
special case that this entry is just never encrypted (could compare the
inode number to the one stored in the superblock, instead of comparing names)?

Cheers, Andreas

>>> +	/*
>>> +	 * Encrypting the root directory is not allowed because e2fsck expects
>>> +	 * lost+found to exist and be unencrypted, and encrypting the root
>>> +	 * directory would imply encrypting the lost+found directory as well as
>>> +	 * the filename "lost+found" itself.
>>> +	 */
>>> +	if (inode->i_ino == EXT4_ROOT_INO)
>>> +		return -EBUSY;
>> 
>> Why -EBUSY and not -EPERM?
>> 
> 
> No strong reason; I had in mind that EBUSY is returned when running a command
> like 'rmdir /mnt'.  Probably EPERM would be better.
> 
> Eric


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] ext4: forbid encrypting root directory

[Eric Biggers]

On Wed, Jun 14, 2017 at 08:02:05PM -0600, Andreas Dilger wrote:
> >> 
> >> What about a special case to handle an unencrypted lost+found inode?
> >> 
> >> There was also a patch series a while ago to explicitly add lost+found
> >> into the superblock so that it could be found even if the root directory
> >> was corrupted, and to allow flexibility in whether it is always shown in
> >> the root directory or not (e.g. allowing ".lost+found" or similar).
> > 
> > It could be done if the lost+found inode was not linked to from any directory
> > and instead had its inode number stored in the superblock so that e2fsck could
> > still find it.  However, if e2fsck put files in a lost+found directory that
> > doesn't exist in the filesystem directory structure, how would users retrieve
> > those files?  Would users be required to run a special e2fsprogs command to
> > list/read/delete the files in lost+found?
> 
> I was thinking that readdir on the root inode could insert the "lost+found"
> or ".lost+found" entry dynamically,

I think this is possible, but not trivial.  It's not just readdir; ->lookup()
would also have to special-case lookups of "lost+found", and we'd have to
override the fscrypt_permitted_context() check.  It would also have to be a
RO_COMPAT filesystem feature, lest an unaware ext4 driver or e2fsprogs create a
lost+found directory which would then be ambiguous with the "real" one.

> or (a bit less pleasant) is to add a
> special case that this entry is just never encrypted (could compare the
> inode number to the one stored in the superblock, instead of comparing names)?

I think that would be similarly difficult, as it would still require special
logic in readdir and ->lookup(), and would still require a RO_COMPAT filesystem
feature.

Either way, not all filesystems will have the implicit "lost+found" directory
feature, so we're still going to need to forbid encrypting the root directory on
some filesystems anyway.

Eric