Re: [edk2-platforms][PATCH v2 03/11] Ext4Pkg: Fix global buffer overflow in Ext4ReadDir

["Marvin Häuser" <mhaeuser@posteo.de>]

On 12. Dec 2022, at 15:46, Savva Mitrofanov <savvamtr@gmail.com> wrote:
> 
> Directory entry structure can contain name_len bigger than size of "."
> or "..", that's why CompareMem in such cases leads to global buffer
> overflow. So there are two problems. The first is that statement doesn't
> check cases when name_len != 0 but > 2 and the second is that we passing
> big Length to CompareMem routine.
> The correct way here is to check that name_len <= 2 and check for
> null-terminator presence
> 
> Cc: Marvin Häuser <mhaeuser@posteo.de>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
> Fixes: e55f0527dde48a5f139c1b8f35acc4e6b59dd794
> Signed-off-by: Savva Mitrofanov <savvamtr@gmail.com>
> ---
> Features/Ext4Pkg/Ext4Dxe/Directory.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> index 8b8fce568e43..ffc0e8043076 100644
> --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c
> +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c
> @@ -491,11 +491,9 @@ Ext4ReadDir (
> 
>     // Entry.name_len may be 0 if it's a nameless entry, like an unused entry
>     // or a checksum at the end of the directory block.
> -    // memcmp (and CompareMem) return 0 when the passed length is 0.
> -
> -    IsDotOrDotDot = Entry.name_len != 0 &&
> -                    (CompareMem (Entry.name, ".", Entry.name_len) == 0 ||
> -                     CompareMem (Entry.name, "..", Entry.name_len) == 0);
> +    IsDotOrDotDot = Entry.name_len <= 2 &&
> +                    ((Entry.name[0] == '.') &&
> +                     (Entry.name[1] == '.' || Entry.name[1] == '\0'));

This is definitely borked, names do not need to be 0-terminated. So this may cause OOB if Entry.name_len == 1 and Entry.name[0] == '.' and also may yield a false negative.

> 
>     // When inode = 0, it's unused.
>     ShouldSkip = Entry.inode == 0 || IsDotOrDotDot;
> -- 
> 2.38.1
>