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

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

[Richard Weinberger]

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@nod.at>
---
 fs/ext4/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 3ed8c048fb12..dba521250d01 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -809,6 +809,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
 	err = ext4_journal_get_write_access(handle, sbi->s_sbh);
 	if (err)
 		goto out_journal;
+	memset(sbi->s_es->s_last_mounted, 0x00, sizeof(sbi->s_es->s_last_mounted));
 	strlcpy(sbi->s_es->s_last_mounted, cp,
 		sizeof(sbi->s_es->s_last_mounted));
 	ext4_handle_dirty_super(handle, sb);
-- 
2.26.2

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

[harshad shirwadkar]

Thanks for the patch Richard, it looks good to me.

Reviewed-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>


On Tue, Dec 1, 2020 at 7:29 AM Richard Weinberger <richard@nod.at> 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@nod.at>
> ---
>  fs/ext4/file.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 3ed8c048fb12..dba521250d01 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -809,6 +809,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
>         err = ext4_journal_get_write_access(handle, sbi->s_sbh);
>         if (err)
>                 goto out_journal;
> +       memset(sbi->s_es->s_last_mounted, 0x00, sizeof(sbi->s_es->s_last_mounted));
>         strlcpy(sbi->s_es->s_last_mounted, cp,
>                 sizeof(sbi->s_es->s_last_mounted));
>         ext4_handle_dirty_super(handle, sb);
> --
> 2.26.2
>

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@nod.at>

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@mit.edu>
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@nod.at>
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

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

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

[Andreas Dilger]

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

On Dec 17, 2020, at 11:27 AM, Theodore Y. Ts'o <tytso@mit.edu> 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@nod.at>
> 
> 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@mit.edu>
> 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@nod.at>
>    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

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 #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

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

[Richard Weinberger]

----- Ursprüngliche Mail -----
>>    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@nod.at>
>>    Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> 
> 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.

strncpy() zeros all remaining bytes.

Thanks,
//richard