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

[Andreas Dilger]

On Dec 17, 2020, at 11:27 AM, Theodore Y. Ts'o <tytso@xxxxxxx> wrote:
> 
> 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>

Color me confused, but I don't see how this change makes any difference?
If "cp" is only "/" then it will *still* not overwrite "edia/johnjdeveloper"
at the end of the s_last_mounted array.  To my mind, the only difference
between using strlcpy() and strncpy() would be whether the last byte in
the array can be used or not, but doesn't affect the remaining bytes.

> 
> 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);


Cheers, Andreas

Attachment: signature.asc
Description: Message signed with OpenPGP