[PATCH] ext4: mark extents index blocks as dirty to avoid information leakage

[PATCH] ext4: mark extents index blocks as dirty to avoid information leakage

[brookxu]

From: Chunguang Xu <brookxu@tencent.com>

In the scene of deleting a file, the physical block information in the
extent will be cleared to 0, the buffer_head contains these extents is
marked as dirty, and then managed by jbd2, which will clear the
buffer_head's dirty flag by clear_buffer_dirty. However, when the entire
extent block is deleted, it is revoked from the jbd2, but  the extents
block is not redirtied.

Not quite reasonable here, for the following concerns:

1. This has the risk of information leakage and leads to an interesting
phenomenon that deleting the entire file is no more secure than truncate
to 1 byte, because the whole extents physical block clear to zero in cache
will never written back as the page is not redirtied.

2. For large files, the number of index block is usually very small.
Ignoring index pages not get much more benefit in IO performance. But if
we remark the page as dirty, the page is then written back by the system
writeback mechanism asynchronously with little performance impact. As a
result, the risk of information leakage can be avoided. At least not wrose
than truncate file length to 1 byte

Therefore, when the index block is released, we need to remark its page
as dirty, so that the index block on the disk will be updated and the
data is more security.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/ext4.h      | 1 +
 fs/ext4/ext4_jbd2.c | 8 ++++++++
 fs/ext4/extents.c   | 3 ++-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 61b37a0..f9a4d97 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -644,6 +644,7 @@ enum {
 #define EXT4_FREE_BLOCKS_NOFREE_FIRST_CLUSTER    0x0010
 #define EXT4_FREE_BLOCKS_NOFREE_LAST_CLUSTER    0x0020
 #define EXT4_FREE_BLOCKS_RERESERVE_CLUSTER      0x0040
+#define EXT4_FREE_BLOCKS_METADATA_INDEX        0x0080
 
 /*
  * ioctl commands
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index 1f53d64..7974c62 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -275,7 +275,15 @@ int __ext4_forget(const char *where, unsigned int line, handle_t *handle,
         ext4_set_errno(inode->i_sb, -err);
         __ext4_abort(inode->i_sb, where, line,
                "error %d when attempting revoke", err);
+    } else  {
+        /*
+         * we dirtied index block to ensure that related changes to
+         * the index block will be stored to disk.
+         */
+        if (is_metadata & EXT4_FREE_BLOCKS_METADATA_INDEX)
+            mark_buffer_dirty(bh);
     }
+
     BUFFER_TRACE(bh, "exit");
     return err;
 }
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 954013d..2ee0df0 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2431,7 +2431,8 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
     trace_ext4_ext_rm_idx(inode, leaf);
 
     ext4_free_blocks(handle, inode, NULL, leaf, 1,
-             EXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET);
+             EXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET |
+             EXT4_FREE_BLOCKS_METADATA_INDEX);
 
     while (--depth >= 0) {
         if (path->p_idx != EXT_FIRST_INDEX(path->p_hdr))
-- 
1.8.3.1

Re: [PATCH] ext4: mark extents index blocks as dirty to avoid information leakage

[brookxu]

PING

brookxu wrote on 2020/3/3 16:51:
> From: Chunguang Xu <brookxu@tencent.com>
>
> In the scene of deleting a file, the physical block information in the
> extent will be cleared to 0, the buffer_head contains these extents is
> marked as dirty, and then managed by jbd2, which will clear the
> buffer_head's dirty flag by clear_buffer_dirty. However, when the entire
> extent block is deleted, it is revoked from the jbd2, but  the extents
> block is not redirtied.
>
> Not quite reasonable here, for the following concerns:
>
> 1. This has the risk of information leakage and leads to an interesting
> phenomenon that deleting the entire file is no more secure than truncate
> to 1 byte, because the whole extents physical block clear to zero in cache
> will never written back as the page is not redirtied.
>
> 2. For large files, the number of index block is usually very small.
> Ignoring index pages not get much more benefit in IO performance. But if
> we remark the page as dirty, the page is then written back by the system
> writeback mechanism asynchronously with little performance impact. As a
> result, the risk of information leakage can be avoided. At least not wrose
> than truncate file length to 1 byte
>
> Therefore, when the index block is released, we need to remark its page
> as dirty, so that the index block on the disk will be updated and the
> data is more security.
>
> Signed-off-by: Chunguang Xu <brookxu@tencent.com>
> ---
>  fs/ext4/ext4.h      | 1 +
>  fs/ext4/ext4_jbd2.c | 8 ++++++++
>  fs/ext4/extents.c   | 3 ++-
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 61b37a0..f9a4d97 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -644,6 +644,7 @@ enum {
>  #define EXT4_FREE_BLOCKS_NOFREE_FIRST_CLUSTER    0x0010
>  #define EXT4_FREE_BLOCKS_NOFREE_LAST_CLUSTER    0x0020
>  #define EXT4_FREE_BLOCKS_RERESERVE_CLUSTER      0x0040
> +#define EXT4_FREE_BLOCKS_METADATA_INDEX        0x0080
>  
>  /*
>   * ioctl commands
> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> index 1f53d64..7974c62 100644
> --- a/fs/ext4/ext4_jbd2.c
> +++ b/fs/ext4/ext4_jbd2.c
> @@ -275,7 +275,15 @@ int __ext4_forget(const char *where, unsigned int line, handle_t *handle,
>          ext4_set_errno(inode->i_sb, -err);
>          __ext4_abort(inode->i_sb, where, line,
>                 "error %d when attempting revoke", err);
> +    } else  {
> +        /*
> +         * we dirtied index block to ensure that related changes to
> +         * the index block will be stored to disk.
> +         */
> +        if (is_metadata & EXT4_FREE_BLOCKS_METADATA_INDEX)
> +            mark_buffer_dirty(bh);
>      }
> +
>      BUFFER_TRACE(bh, "exit");
>      return err;
>  }
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 954013d..2ee0df0 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2431,7 +2431,8 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
>      trace_ext4_ext_rm_idx(inode, leaf);
>  
>      ext4_free_blocks(handle, inode, NULL, leaf, 1,
> -             EXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET);
> +             EXT4_FREE_BLOCKS_METADATA | EXT4_FREE_BLOCKS_FORGET |
> +             EXT4_FREE_BLOCKS_METADATA_INDEX);
>  
>      while (--depth >= 0) {
>          if (path->p_idx != EXT_FIRST_INDEX(path->p_hdr))

Re: [PATCH] ext4: mark extents index blocks as dirty to avoid information leakage

[Theodore Y. Ts'o]

On Tue, Mar 03, 2020 at 04:51:06PM +0800, brookxu wrote:
> From: Chunguang Xu <brookxu@tencent.com>
> 
> In the scene of deleting a file, the physical block information in the
> extent will be cleared to 0, the buffer_head contains these extents is
> marked as dirty, and then managed by jbd2, which will clear the
> buffer_head's dirty flag by clear_buffer_dirty. However, when the entire
> extent block is deleted, it is revoked from the jbd2, but  the extents
> block is not redirtied.
> 
> Not quite reasonable here, for the following concerns:
> 
> 1. This has the risk of information leakage and leads to an interesting
> phenomenon that deleting the entire file is no more secure than truncate
> to 1 byte, because the whole extents physical block clear to zero in cache
> will never written back as the page is not redirtied.
> 
> 2. For large files, the number of index block is usually very small.
> Ignoring index pages not get much more benefit in IO performance. But if
> we remark the page as dirty, the page is then written back by the system
> writeback mechanism asynchronously with little performance impact. As a
> result, the risk of information leakage can be avoided. At least not wrose
> than truncate file length to 1 byte
> 
> Therefore, when the index block is released, we need to remark its page
> as dirty, so that the index block on the disk will be updated and the
> data is more security.
> 
> Signed-off-by: Chunguang Xu <brookxu@tencent.com>

Trying to zero the extent block is only going to provide pretend
security; the data blocks are still there, and anyone looking for the
data can still find it if they look hard enough.  Also, for most
files, it really doesn't matter.

So, no, I don't think this patch is appropriate.a

If you are really worried about the security for deleted files, I
would suggest trying to implement the secure delete flag (chattr +s)
for ext4, and actually trying to zero out the data blocks for those
files where this kind of security is required.  (Please note that for
SSD's, this probably won't provide as much security as you would like
unless they implement the secure discard operation.)

							- Ted

Re: [PATCH] ext4: mark extents index blocks as dirty to avoid information leakage

[brookxu]

Thanks for your reply, but threre's some points implement inconsistently

1. + s attribute will cause a big performance problem. With only data
   blocks but no index blocks, it is difficult to recover the complete
   data. Therefore, we can also improve data security with the lowest
   cost.
2. In the SSD scenario, discard on some devices cannot guarantee that
   data is erased. but if we update the dirty pages of memory to disk.
   ssd will remap the corresponding lba, then the user will not be able
   to access the old data, it's security.
3. In the small file scenario, the file extent is stored on the inode.
   Because the inode block will not be forgotten by jbd2, the extent on
   the disk is always cleared after the small file is deleted. If security
   is only pretended, why not take effect on the small file.
4. If to facilitate data recovery, why do need to clear extents in
   memory? This operation does not seem to make sense.

I think that the page of the index block is not updated to disk after
the file is deleted, which may be a logical defect.

Theodore Y. Ts'o wrote on 2020/3/12 22:46:
> On Tue, Mar 03, 2020 at 04:51:06PM +0800, brookxu wrote:
>> From: Chunguang Xu <brookxu@tencent.com>
>>
>> In the scene of deleting a file, the physical block information in the
>> extent will be cleared to 0, the buffer_head contains these extents is
>> marked as dirty, and then managed by jbd2, which will clear the
>> buffer_head's dirty flag by clear_buffer_dirty. However, when the entire
>> extent block is deleted, it is revoked from the jbd2, but  the extents
>> block is not redirtied.
>>
>> Not quite reasonable here, for the following concerns:
>>
>> 1. This has the risk of information leakage and leads to an interesting
>> phenomenon that deleting the entire file is no more secure than truncate
>> to 1 byte, because the whole extents physical block clear to zero in cache
>> will never written back as the page is not redirtied.
>>
>> 2. For large files, the number of index block is usually very small.
>> Ignoring index pages not get much more benefit in IO performance. But if
>> we remark the page as dirty, the page is then written back by the system
>> writeback mechanism asynchronously with little performance impact. As a
>> result, the risk of information leakage can be avoided. At least not wrose
>> than truncate file length to 1 byte
>>
>> Therefore, when the index block is released, we need to remark its page
>> as dirty, so that the index block on the disk will be updated and the
>> data is more security.
>>
>> Signed-off-by: Chunguang Xu <brookxu@tencent.com>
> Trying to zero the extent block is only going to provide pretend
> security; the data blocks are still there, and anyone looking for the
> data can still find it if they look hard enough.  Also, for most
> files, it really doesn't matter.
>
> So, no, I don't think this patch is appropriate.a
>
> If you are really worried about the security for deleted files, I
> would suggest trying to implement the secure delete flag (chattr +s)
> for ext4, and actually trying to zero out the data blocks for those
> files where this kind of security is required.  (Please note that for
> SSD's, this probably won't provide as much security as you would like
> unless they implement the secure discard operation.)
>
> 							- Ted