[PATCH v4] ext4: wipe ext4_dir_entry2 upon file deletion

[PATCH v4] ext4: wipe ext4_dir_entry2 upon file deletion

[Leah Rumancik]

Upon file deletion, zero out all fields in ext4_dir_entry2 besides rec_len.
In case sensitive data is stored in filenames, this ensures no potentially
sensitive data is left in the directory entry upon deletion. Also, wipe
these fields upon moving a directory entry during the conversion to an
htree and when splitting htree nodes.

The data wiped may still exist in the journal, but there are future
commits planned to address this.

Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com>
---
 fs/ext4/namei.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 883e2a7cd4ab..0cfb1278ce1b 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1777,7 +1777,14 @@ dx_move_dirents(char *from, char *to, struct dx_map_entry *map, int count,
 		memcpy (to, de, rec_len);
 		((struct ext4_dir_entry_2 *) to)->rec_len =
 				ext4_rec_len_to_disk(rec_len, blocksize);
+
+		/* wipe dir_entry excluding the rec_len field */
 		de->inode = 0;
+		memset(&de->name_len, 0, ext4_rec_len_from_disk(de->rec_len,
+								blocksize) -
+					 offsetof(struct ext4_dir_entry_2,
+								name_len));
+
 		map++;
 		to += rec_len;
 	}
@@ -2102,6 +2109,7 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
 	data2 = bh2->b_data;
 
 	memcpy(data2, de, len);
+	memset(de, 0, len); /* wipe old data */
 	de = (struct ext4_dir_entry_2 *) data2;
 	top = data2 + len;
 	while ((char *)(de2 = ext4_next_entry(de, blocksize)) < top)
@@ -2482,15 +2490,27 @@ int ext4_generic_delete_entry(struct inode *dir,
 					 entry_buf, buf_size, i))
 			return -EFSCORRUPTED;
 		if (de == de_del)  {
-			if (pde)
+			if (pde) {
 				pde->rec_len = ext4_rec_len_to_disk(
 					ext4_rec_len_from_disk(pde->rec_len,
 							       blocksize) +
 					ext4_rec_len_from_disk(de->rec_len,
 							       blocksize),
 					blocksize);
-			else
+
+				/* wipe entire dir_entry */
+				memset(de, 0, ext4_rec_len_from_disk(de->rec_len,
+								blocksize));
+			} else {
+				/* wipe dir_entry excluding the rec_len field */
 				de->inode = 0;
+				memset(&de->name_len, 0,
+					ext4_rec_len_from_disk(de->rec_len,
+								blocksize) -
+					offsetof(struct ext4_dir_entry_2,
+								name_len));
+			}
+
 			inode_inc_iversion(dir);
 			return 0;
 		}
-- 
2.31.1.498.g6c1eba8ee3d-goog

Re: [PATCH v4] ext4: wipe ext4_dir_entry2 upon file deletion

[Theodore Ts'o]

On Thu, Apr 22, 2021 at 06:08:34PM +0000, Leah Rumancik wrote:
> Upon file deletion, zero out all fields in ext4_dir_entry2 besides rec_len.
> In case sensitive data is stored in filenames, this ensures no potentially
> sensitive data is left in the directory entry upon deletion. Also, wipe
> these fields upon moving a directory entry during the conversion to an
> htree and when splitting htree nodes.
> 
> The data wiped may still exist in the journal, but there are future
> commits planned to address this.
> 
> Signed-off-by: Leah Rumancik <leah.rumancik@gmail.com>

Applied, thanks.

					- Ted

Re: [PATCH v4] ext4: wipe ext4_dir_entry2 upon file deletion

[Theodore Ts'o]

On Fri, Jul 02, 2021 at 01:29:52PM +0000, Artem S. Tashkinov wrote:
> Hi,
> 
> I'm curious about the nature of this feature. Does it make restoring
> accidentally deleted files more difficult or even impossible? OK, data
> remains but the patch description makes it sound like all the metadata
> is being wiped completely.

It doesn't make any worse, but that's because ever since ext3 (e.g.,
since 2001), when we unlink a file, we endup zeroing i_blocks[] as
well as the indirect or extent tree blocks.  We *could* have fixed
this by special casing the path where the entire unlink fits inside
the current transaction, instead of always allowing for the
transaction to split across more than one transaction.

No one complained about the fact we were zero'ing the logical ->
physical block maps for two decades, by which I think we can assume
almost no one has tried used the "lsdel" + "link" hack in debugfs for
years and years.

> If it's the case, is it possible to make this new security feature user
> configurable? I'm OK with it being on by default but I'd be glad if
> there were a mount option to disable it.

We considered this, but given that all this would do is allow people
to recover the timestamps, user/group ownerships, etc., but not the
data blocks, it was deemed not to be worth the extra complexity.

If someone really wanted to allow undelete support, they could use the
various userspace solutions, or we *could* have an optional feature
which moves inodes whose link couint is about to go to zero into a
magic "trash can" directory, with some kind of automated auto-delete
mechanism when free blocks or free inodes all below some threshold, or
when free space falls below some threshold.  The trash can directory
would have to be readable only by root, in order to preserve security
when users delete files in a mode 0700 directory.  I'm not really
convinced it's worth it to implement such a thing, though....

	       	     	   	     	  - Ted

Re: [PATCH v4] ext4: wipe ext4_dir_entry2 upon file deletion

[Artem S. Tashkinov]

Hi,

I'm curious about the nature of this feature. Does it make restoring
accidentally deleted files more difficult or even impossible? OK, data
remains but the patch description makes it sound like all the metadata
is being wiped completely.

 > Upon file deletion, zero out all fields in ext4_dir_entry2
 > besides rec_len. In case sensitive data is stored in filenames,
 > this ensures no potentially sensitive data is left in the directory
 > entry upon deletion. Also, wipe these fields upon moving a directory
 > entry during the conversion to an htree and when splitting htree
 > nodes.

If it's the case, is it possible to make this new security feature user
configurable? I'm OK with it being on by default but I'd be glad if
there were a mount option to disable it.

Best regards,
Artem