From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web11.120389.1673715970783712200 for ; Sat, 14 Jan 2023 09:06:11 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=PnQh6XZg; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id E89CC2400E3 for ; Sat, 14 Jan 2023 18:06:08 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1673715968; bh=U/gGKwH/22gwZHVFnCPFjuN31/WB2XCPtB1W//f+4Fg=; h=From:Subject:Date:Cc:To:From; b=PnQh6XZgAuGGY4fHNZuF3ZlCBlYtj4pm8U54t5J5TfgMSN/jGdjlxB+6RWoQXJl2L UX6RQ1VZRbUuYiYAEce5LM38xHm79UpLXbDiyYMKCrgPgMdST3xVFOEirNhOvKTcEJ Fo+LiPzdK7f4ZtYcPVhLbRLBHWSaKhq9n5K7kQpzUNhg+3lUMSQd76BsIL4e5dMQHP DX0O3DkvmFv9pOLGUMsDVq7uhZRYO3A1A29HrYAXlbpgs3z0hnZvnjWJGADi8Z87de XrNUWk5yFc3mi+/+NwTPEL6y7Ky3/5AZalX03CiQpVp0VRgNJeRIdVpPcu1Q6KNFwP 3S+S7MDx8g1Ew== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4NvPnW6pB4z6tmS; Sat, 14 Jan 2023 18:06:07 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-Id: <7597DAF0-A613-4DA1-AC47-38C562425CE7@posteo.de> Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\)) Subject: Re: [PATCH 1/3] Ext4Pkg: Fix out-of-bounds read in Ext4ReadDir Date: Sat, 14 Jan 2023 17:05:57 +0000 In-Reply-To: <20230111235920.252317-3-pedro.falcato@gmail.com> Cc: edk2-devel-groups-io , Savva Mitrofanov To: Pedro Falcato References: <20230111235920.252317-1-pedro.falcato@gmail.com> <20230111235920.252317-3-pedro.falcato@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_4AA4843D-7825-407F-A224-C78FDD17840E" --Apple-Mail=_4AA4843D-7825-407F-A224-C78FDD17840E Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Reviewed-by: Marvin H=C3=A4user > (Still two comments for next time) > On 12. Jan 2023, at 00:59, Pedro Falcato = wrote: >=20 > Fix an out-of-bounds read inside CompareMem() when > checking for "." or ".." by explicitly bounding name_len > to [0, 2] beforehand. >=20 > Reported-by: Savva Mitrofanov > Fixes: 45e37d8533ca8 ("Ext4Pkg: Hide "." and ".." entries from Read() = callers.") > Cc: Marvin H=C3=A4user > Signed-off-by: Pedro Falcato > --- > Features/Ext4Pkg/Ext4Dxe/Directory.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) >=20 > diff --git a/Features/Ext4Pkg/Ext4Dxe/Directory.c = b/Features/Ext4Pkg/Ext4Dxe/Directory.c > index 4441e6d192b6..6ed664fc632f 100644 > --- a/Features/Ext4Pkg/Ext4Dxe/Directory.c > +++ b/Features/Ext4Pkg/Ext4Dxe/Directory.c > @@ -491,12 +491,14 @@ Ext4ReadDir ( > // or a checksum at the end of the directory block. > // memcmp (and CompareMem) return 0 when the passed length is 0. >=20 > - IsDotOrDotDot =3D Entry.name_len !=3D 0 && > - (CompareMem (Entry.name, ".", Entry.name_len) =3D=3D= 0 || > - CompareMem (Entry.name, "..", Entry.name_len) =3D=3D= 0); > + // We must bound name_len as > 0 and <=3D 2 to avoid any = out-of-bounds accesses or bad detection of "bad detection" =3D false positive? > + // "." and "..". > + IsDotOrDotDot =3D Entry.name_len > 0 && Entry.name_len <=3D 2 && > + CompareMem (Entry.name, "..", Entry.name_len) =3D=3D= 0; >=20 > - // When inode =3D 0, it's unused. > - ShouldSkip =3D Entry.inode =3D=3D 0 || IsDotOrDotDot; > + // When inode =3D 0, it's unused. When name_len =3D=3D 0, it's a = nameless entry Inconsistency between "=3D" and "=3D=3D". > + // (which we should not expose to ReadDir). > + ShouldSkip =3D Entry.inode =3D=3D 0 || Entry.name_len =3D=3D 0 || = IsDotOrDotDot; >=20 > if (ShouldSkip) { > Offset +=3D Entry.rec_len; > --=20 > 2.39.0 >=20 --Apple-Mail=_4AA4843D-7825-407F-A224-C78FDD17840E Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Reviewed-by: = Marvin H=C3=A4user <mhaeuser@posteo.de>

(Still two comments for next time)

On 12. Jan 2023, at 00:59, Pedro Falcato = <pedro.falcato@gmail.com> wrote:

Fix an out-of-bounds read = inside CompareMem() when
checking for "." or ".." by explicitly = bounding name_len
to [0, 2] beforehand.

Reported-by: Savva = Mitrofanov <savvamtr@gmail.com>
Fixes: 45e37d8533ca8 ("Ext4Pkg: = Hide "." and ".." entries from Read() callers.")
Cc: Marvin H=C3=A4user= <mhaeuser@posteo.de>
Signed-off-by: Pedro Falcato = <pedro.falcato@gmail.com>
---
= Features/Ext4Pkg/Ext4Dxe/Directory.c | 12 +++++++-----
1 file = changed, 7 insertions(+), 5 deletions(-)

diff --git = a/Features/Ext4Pkg/Ext4Dxe/Directory.c = b/Features/Ext4Pkg/Ext4Dxe/Directory.c
index = 4441e6d192b6..6ed664fc632f 100644
--- = a/Features/Ext4Pkg/Ext4Dxe/Directory.c
+++ = b/Features/Ext4Pkg/Ext4Dxe/Directory.c
@@ -491,12 +491,14 @@ = Ext4ReadDir (
    // or a checksum at the end of = the directory block.
    // memcmp (and = CompareMem) return 0 when the passed length is 0.

- =    IsDotOrDotDot =3D Entry.name_len !=3D 0 = &&
- =             &n= bsp;      (CompareMem (Entry.name, ".", = Entry.name_len) =3D=3D 0 ||
- =             &n= bsp;       CompareMem (Entry.name, = "..", Entry.name_len) =3D=3D 0);
+    // We must bound = name_len as > 0 and <=3D 2 to avoid any out-of-bounds accesses or = bad detection of

"bad = detection" =3D false positive?

+    // "." and "..".
+ =    IsDotOrDotDot =3D Entry.name_len > 0 && = Entry.name_len <=3D 2 &&
+ =             &n= bsp;      CompareMem (Entry.name, "..", = Entry.name_len) =3D=3D 0;

-    // When inode =3D = 0, it's unused.
-    ShouldSkip =3D Entry.inode =3D=3D = 0 || IsDotOrDotDot;
+    // When inode =3D 0, it's = unused. When name_len =3D=3D 0, it's a nameless = entry

Inconsistency = between "=3D" and "=3D=3D".

+=    // (which we should not expose to ReadDir).
+ =    ShouldSkip =3D Entry.inode =3D=3D 0 || Entry.name_len = =3D=3D 0 || IsDotOrDotDot;

    if = (ShouldSkip) {
      Offset +=3D = Entry.rec_len;
-- =
2.39.0


= --Apple-Mail=_4AA4843D-7825-407F-A224-C78FDD17840E--