Re: [PATCH] ext4: Don't leak old mountpoint samples

[Theodore Y. Ts'o]

On Tue, Dec 01, 2020 at 04:13:01PM +0100, Richard Weinberger wrote:
> As soon the first file is opened, ext4 samples the mountpoint
> of the filesystem in 64 bytes of the super block.
> It does so using strlcpy(), this means that the remaining bytes
> in the super block string buffer are untouched.
> If the mount point before had a longer path than the current one,
> it can be reconstructed.
> 
> Consider the case where the fs was mounted to "/media/johnjdeveloper"
> and later to "/".
> The the super block buffer then contains "/\x00edia/johnjdeveloper".
> 
> This case was seen in the wild and caused confusion how the name
> of a developer ands up on the super block of a filesystem used
> in production...
> 
> Fix this by clearing the string buffer before writing to it,
> 
> Signed-off-by: Richard Weinberger <richard@xxxxxx>

Thank for reporting this issue.  In fact, the better fix is to use
strncpy().  See my revised patch for an explanation of why....

commit cdc9ad7d3f201a77749432878fb4caa490862de6
Author: Theodore Ts'o <tytso@xxxxxxx>
Date:   Thu Dec 17 13:24:15 2020 -0500

    ext4: don't leak old mountpoint samples
    
    When the first file is opened, ext4 samples the mountpoint of the
    filesystem in 64 bytes of the super block.  It does so using
    strlcpy(), this means that the remaining bytes in the super block
    string buffer are untouched.  If the mount point before had a longer
    path than the current one, it can be reconstructed.
    
    Consider the case where the fs was mounted to "/media/johnjdeveloper"
    and later to "/".  The super block buffer then contains
    "/\x00edia/johnjdeveloper".
    
    This case was seen in the wild and caused confusion how the name
    of a developer ands up on the super block of a filesystem used
    in production...
    
    Fix this by using strncpy() instead of strlcpy().  The superblock
    field is defined to be a fixed-size char array, and it is already
    marked using __nonstring in fs/ext4/ext4.h.  The consumer of the field
    in e2fsprogs already assumes that in the case of a 64+ byte mount
    path, that s_last_mounted will not be NUL terminated.
    
    Reported-by: Richard Weinberger <richard@xxxxxx>
    Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 1cd3d26e3217..349b27f0dda0 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -810,7 +810,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
 	if (err)
 		goto out_journal;
 	lock_buffer(sbi->s_sbh);
-	strlcpy(sbi->s_es->s_last_mounted, cp,
+	strncpy(sbi->s_es->s_last_mounted, cp,
 		sizeof(sbi->s_es->s_last_mounted));
 	ext4_superblock_csum_set(sb);
 	unlock_buffer(sbi->s_sbh);