[PATCH 0/3] fscrypt: forbid truncate(2) without key

[PATCH 0/3] fscrypt: forbid truncate(2) without key

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

These patches update the various filesystems to forbid userspace from
truncating encrypted files without the encryption key, as it's not
possible to handle this correctly in general.  I believe this may have
been missed because truncate() doesn't require opening the file first,
and therefore it's not prevented by the existing requirement that open()
is only allowed with the encryption key.

It probably makes sense to take this series through the fscrypt tree, as
the changes for each filesystem are basically identical.  (Eventually it
might make sense to add an S_ENCRYPTED flag to struct inode and move
some of these "hooks" up into the VFS; this one might fit nicely into
setattr_prepare(), for example.)

Eric Biggers (3):
  ext4: require key for truncate(2) of encrypted file
  f2fs: require key for truncate(2) of encrypted file
  ubifs: require key for truncate(2) of encrypted file

 fs/ext4/inode.c |  8 ++++++++
 fs/f2fs/file.c  | 10 +++++++---
 fs/ubifs/file.c |  8 ++++++++
 3 files changed, 23 insertions(+), 3 deletions(-)

-- 
2.13.1.508.gb3defc5cc-goog

[PATCH 1/3] ext4: require key for truncate(2) of encrypted file

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Currently, filesystems allow truncate(2) on an encrypted file without
the encryption key.  However, it's impossible to correctly handle the
case where the size being truncated to is not a multiple of the
filesystem block size, because that would require decrypting the final
block, zeroing the part beyond i_size, then encrypting the block.

As other modifications to encrypted file contents are prohibited without
the key, just prohibit truncate(2) as well, making it fail with ENOKEY.

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

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 5cf82d03968c..baf8630de6a5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5307,6 +5307,14 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
 		loff_t oldsize = inode->i_size;
 		int shrink = (attr->ia_size <= inode->i_size);
 
+		if (ext4_encrypted_inode(inode)) {
+			error = fscrypt_get_encryption_info(inode);
+			if (error)
+				return error;
+			if (!fscrypt_has_encryption_key(inode))
+				return -ENOKEY;
+		}
+
 		if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
 			struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 
-- 
2.13.1.508.gb3defc5cc-goog

Re: [PATCH 1/3] ext4: require key for truncate(2) of encrypted file

[Andreas Dilger]

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

On Jun 13, 2017, at 5:47 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> From: Eric Biggers <ebiggers@google.com>
> 
> Currently, filesystems allow truncate(2) on an encrypted file without
> the encryption key.  However, it's impossible to correctly handle the
> case where the size being truncated to is not a multiple of the
> filesystem block size, because that would require decrypting the final
> block, zeroing the part beyond i_size, then encrypting the block.
> 
> As other modifications to encrypted file contents are prohibited without
> the key, just prohibit truncate(2) as well, making it fail with ENOKEY.

Out of curiosity, if an encrypted block is zero-filled at the end when
the key is unavailable, what would happen?  At worst this would result
in garbage at the end of the file when the zeroes are "decrypted"?

Is it possible to unlink files if they are encrypted without the key (if
the user has permission to write to the directory)?

Does file unlink result in the per-file crypto key being erased in the
inode, or is the inode just marked unused but not overwritten?  One of
the desirable features of per-file crypto is "secure erase" so that once
the inode is unlinked the crypto key is gone and there is no way to decrypt
the data after the fact, even if the filesystem/user key is later recovered.
If the per-file key is still sitting in the inode, then it may be possible
to decrypt the data long after the file was deleted if the key is later
compromised and the inode+data blocks were not re-used.

Cheers, Andreas

> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
> fs/ext4/inode.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 5cf82d03968c..baf8630de6a5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5307,6 +5307,14 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
> 		loff_t oldsize = inode->i_size;
> 		int shrink = (attr->ia_size <= inode->i_size);
> 
> +		if (ext4_encrypted_inode(inode)) {
> +			error = fscrypt_get_encryption_info(inode);
> +			if (error)
> +				return error;
> +			if (!fscrypt_has_encryption_key(inode))
> +				return -ENOKEY;
> +		}
> +
> 		if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
> 			struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> 
> --
> 2.13.1.508.gb3defc5cc-goog
> 


Cheers, Andreas






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

Re: [PATCH 1/3] ext4: require key for truncate(2) of encrypted file

[Eric Biggers]

Hi Andreas,

On Tue, Jun 13, 2017 at 06:14:53PM -0600, Andreas Dilger wrote:
> On Jun 13, 2017, at 5:47 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> > 
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Currently, filesystems allow truncate(2) on an encrypted file without
> > the encryption key.  However, it's impossible to correctly handle the
> > case where the size being truncated to is not a multiple of the
> > filesystem block size, because that would require decrypting the final
> > block, zeroing the part beyond i_size, then encrypting the block.
> > 
> > As other modifications to encrypted file contents are prohibited without
> > the key, just prohibit truncate(2) as well, making it fail with ENOKEY.
> 
> Out of curiosity, if an encrypted block is zero-filled at the end when
> the key is unavailable, what would happen?  At worst this would result
> in garbage at the end of the file when the zeroes are "decrypted"?

Currently ext4 just skips zeroing the final block entirely, which is perhaps the
most reasonable behavior, but it's wrong because it breaks truncate() semantics.
If it did zero the final block on-disk, yes it wouldn't actually be zeroes after
being decrypted, so if the file were to be extended later the extended portion
wouldn't be filled with zeroes, and more importantly even without extending, up
to the last 15 bytes of the file would be corrupted since AES-XTS encryption
operates on 16 byte blocks.  Also, the ciphertext page would likely end up in
the pagecache, which is supposed to contain the plaintext.  So if the file were
read later (which requires the key), up to the last PAGE_SIZE bytes of the file
would appear as garbage.

> 
> Is it possible to unlink files if they are encrypted without the key (if
> the user has permission to write to the directory)?
> 

Yes, that's supported.

> Does file unlink result in the per-file crypto key being erased in the
> inode, or is the inode just marked unused but not overwritten?  One of
> the desirable features of per-file crypto is "secure erase" so that once
> the inode is unlinked the crypto key is gone and there is no way to decrypt
> the data after the fact, even if the filesystem/user key is later recovered.
> If the per-file key is still sitting in the inode, then it may be possible
> to decrypt the data long after the file was deleted if the key is later
> compromised and the inode+data blocks were not re-used.

I assume you're talking about the key derivation nonce in the encryption xattr
(which is stored on-disk and is not really a "key"; although it's used as an
AES-ECB key in derive_key_aes(), that's really an implementation detail, and the
key derivation algorithm could/should be replaced with something less
idiosyncratic like an HKDF).  This is a completely separate issue, but no, as
far as I know we aren't securely erasing the nonce following a file deletion so
that someone with access to both the raw disk and the master key can no longer
derive the per-file key.  It's a good idea, though, and I think it should be
done if practical.  Note, though, that the xattr may be stored in an external
xattr block, and it also may have moved around over time --- so it may be
necessary to zero the whole inode and xattr block.

Eric

Re: [PATCH 1/3] ext4: require key for truncate(2) of encrypted file

[Andreas Dilger]

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

On Jun 13, 2017, at 9:12 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> Hi Andreas,
> 
> On Tue, Jun 13, 2017 at 06:14:53PM -0600, Andreas Dilger wrote:
>> On Jun 13, 2017, at 5:47 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
>>> 
>>> From: Eric Biggers <ebiggers@google.com>
>>> 
>>> Currently, filesystems allow truncate(2) on an encrypted file without
>>> the encryption key.  However, it's impossible to correctly handle the
>>> case where the size being truncated to is not a multiple of the
>>> filesystem block size, because that would require decrypting the final
>>> block, zeroing the part beyond i_size, then encrypting the block.
>>> 
>>> As other modifications to encrypted file contents are prohibited without
>>> the key, just prohibit truncate(2) as well, making it fail with ENOKEY.
>> 
>> Out of curiosity, if an encrypted block is zero-filled at the end when
>> the key is unavailable, what would happen?  At worst this would result
>> in garbage at the end of the file when the zeroes are "decrypted"?
> 
> Currently ext4 just skips zeroing the final block entirely, which is perhaps the
> most reasonable behavior, but it's wrong because it breaks truncate() semantics.
> If it did zero the final block on-disk, yes it wouldn't actually be zeroes after
> being decrypted, so if the file were to be extended later the extended portion
> wouldn't be filled with zeroes, and more importantly even without extending, up
> to the last 15 bytes of the file would be corrupted since AES-XTS encryption
> operates on 16 byte blocks.  Also, the ciphertext page would likely end up in
> the pagecache, which is supposed to contain the plaintext.  So if the file were
> read later (which requires the key), up to the last PAGE_SIZE bytes of the file
> would appear as garbage.

It wouldn't be bad to add this kind of explanation to the commit message, so
that it is clear why simple zeroing of the partial block doesn't work.

>> Is it possible to unlink files if they are encrypted without the key (if
>> the user has permission to write to the directory)?
> 
> Yes, that's supported.
> 
>> Does file unlink result in the per-file crypto key being erased in the
>> inode, or is the inode just marked unused but not overwritten?  One of
>> the desirable features of per-file crypto is "secure erase" so that once
>> the inode is unlinked the crypto key is gone and there is no way to decrypt
>> the data after the fact, even if the filesystem/user key is later recovered.
>> If the per-file key is still sitting in the inode, then it may be possible
>> to decrypt the data long after the file was deleted if the key is later
>> compromised and the inode+data blocks were not re-used.
> 
> I assume you're talking about the key derivation nonce in the encryption xattr
> (which is stored on-disk and is not really a "key"; although it's used as an
> AES-ECB key in derive_key_aes(), that's really an implementation detail, and the
> key derivation algorithm could/should be replaced with something less
> idiosyncratic like an HKDF).  This is a completely separate issue, but no, as
> far as I know we aren't securely erasing the nonce following a file deletion so
> that someone with access to both the raw disk and the master key can no longer
> derive the per-file key.  It's a good idea, though, and I think it should be
> done if practical.  Note, though, that the xattr may be stored in an external
> xattr block, and it also may have moved around over time --- so it may be
> necessary to zero the whole inode and xattr block.

While it is true that the crypto xattr _could_ exist in the external xattr block,
this is fairly unlikely since this xattr is created first, so it is most likely
to be in the inode xattr space.  Also, it is common that there is no external
xattr block.  In most cases, setting the xattr with the same size will overwrite
it in-place rather than try to allocate new space for it, so this should be
relatively easy to implement.

In the end, trying to zero this xattr and failing is no worse than not doing it
at all, so I think it reasonable to try.  It shouldn't result in any extra I/O,
since the inode always needs to be written out anyway to store the dtime and
nlink = 0.

Cheers, Andreas






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

Re: [PATCH 1/3] ext4: require key for truncate(2) of encrypted file

[Christoph Hellwig]

On Tue, Jun 13, 2017 at 04:47:53PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Currently, filesystems allow truncate(2) on an encrypted file without
> the encryption key.  However, it's impossible to correctly handle the
> case where the size being truncated to is not a multiple of the
> filesystem block size, because that would require decrypting the final
> block, zeroing the part beyond i_size, then encrypting the block.
> 
> As other modifications to encrypted file contents are prohibited without
> the key, just prohibit truncate(2) as well, making it fail with ENOKEY.

What about hole punches?  What about fallocate which just adds zeroes
but still changes the content.  What about insert or collapse range?

Re: [PATCH 1/3] ext4: require key for truncate(2) of encrypted file

[Eric Biggers]

Hi Christoph,

On Tue, Jun 13, 2017 at 11:52:10PM -0700, Christoph Hellwig wrote:
> On Tue, Jun 13, 2017 at 04:47:53PM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > Currently, filesystems allow truncate(2) on an encrypted file without
> > the encryption key.  However, it's impossible to correctly handle the
> > case where the size being truncated to is not a multiple of the
> > filesystem block size, because that would require decrypting the final
> > block, zeroing the part beyond i_size, then encrypting the block.
> > 
> > As other modifications to encrypted file contents are prohibited without
> > the key, just prohibit truncate(2) as well, making it fail with ENOKEY.
> 
> What about hole punches?  What about fallocate which just adds zeroes
> but still changes the content.  What about insert or collapse range?

None of those are allowed because fallocate() requires a file descriptor, and
open() fails with ENOKEY if the encryption key is not available.  truncate() is
different because it takes a path, not a file descriptor.

Eric

Re: [PATCH 1/3] ext4: require key for truncate(2) of encrypted file

[Christoph Hellwig]

On Wed, Jun 14, 2017 at 12:03:57AM -0700, Eric Biggers wrote:
> None of those are allowed because fallocate() requires a file descriptor, and
> open() fails with ENOKEY if the encryption key is not available.  truncate() is
> different because it takes a path, not a file descriptor.

Ok, that makes sense.

Re: [PATCH 1/3] ext4: require key for truncate(2) of encrypted file

[Theodore Ts'o]

On Tue, Jun 13, 2017 at 04:47:53PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Currently, filesystems allow truncate(2) on an encrypted file without
> the encryption key.  However, it's impossible to correctly handle the
> case where the size being truncated to is not a multiple of the
> filesystem block size, because that would require decrypting the final
> block, zeroing the part beyond i_size, then encrypting the block.
> 
> As other modifications to encrypted file contents are prohibited without
> the key, just prohibit truncate(2) as well, making it fail with ENOKEY.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Thanks, applied.

					- Ted

[PATCH 2/3] f2fs: require key for truncate(2) of encrypted file

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Currently, filesystems allow truncate(2) on an encrypted file without
the encryption key.  However, it's impossible to correctly handle the
case where the size being truncated to is not a multiple of the
filesystem block size, because that would require decrypting the final
block, zeroing the part beyond i_size, then encrypting the block.

As other modifications to encrypted file contents are prohibited without
the key, just prohibit truncate(2) as well, making it fail with ENOKEY.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/file.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 61af721329fa..be0b32bd1297 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -682,9 +682,13 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
 		return err;
 
 	if (attr->ia_valid & ATTR_SIZE) {
-		if (f2fs_encrypted_inode(inode) &&
-				fscrypt_get_encryption_info(inode))
-			return -EACCES;
+		if (f2fs_encrypted_inode(inode)) {
+			err = fscrypt_get_encryption_info(inode);
+			if (err)
+				return err;
+			if (!fscrypt_has_encryption_key(inode))
+				return -ENOKEY;
+		}
 
 		if (attr->ia_size <= i_size_read(inode)) {
 			truncate_setsize(inode, attr->ia_size);
-- 
2.13.1.508.gb3defc5cc-goog

Re: [f2fs-dev] [PATCH 2/3] f2fs: require key for truncate(2) of encrypted file

[Chao Yu]

On 2017/6/14 7:47, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Currently, filesystems allow truncate(2) on an encrypted file without
> the encryption key.  However, it's impossible to correctly handle the
> case where the size being truncated to is not a multiple of the
> filesystem block size, because that would require decrypting the final
> block, zeroing the part beyond i_size, then encrypting the block.
> 
> As other modifications to encrypted file contents are prohibited without
> the key, just prohibit truncate(2) as well, making it fail with ENOKEY.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Acked-by: Chao Yu <yuchao0@huawei.com>

Thanks,

[PATCH 3/3] ubifs: require key for truncate(2) of encrypted file

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Currently, filesystems allow truncate(2) on an encrypted file without
the encryption key.  However, it's impossible to correctly handle the
case where the size being truncated to is not a multiple of the
filesystem block size, because that would require decrypting the final
block, zeroing the part beyond i_size, then encrypting the block.

As other modifications to encrypted file contents are prohibited without
the key, just prohibit truncate(2) as well, making it fail with ENOKEY.

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

diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 2cda3d67e2d0..ee3ff4c6bf4a 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -1284,6 +1284,14 @@ int ubifs_setattr(struct dentry *dentry, struct iattr *attr)
 	if (err)
 		return err;
 
+	if (ubifs_crypt_is_encrypted(inode) && (attr->ia_valid & ATTR_SIZE)) {
+		err = fscrypt_get_encryption_info(inode);
+		if (err)
+			return err;
+		if (!fscrypt_has_encryption_key(inode))
+			return -ENOKEY;
+	}
+
 	if ((attr->ia_valid & ATTR_SIZE) && attr->ia_size < inode->i_size)
 		/* Truncation to a smaller size */
 		err = do_truncation(c, inode, attr);
-- 
2.13.1.508.gb3defc5cc-goog