From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web10.16689.1671206606868684499 for ; Fri, 16 Dec 2022 08:03:27 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@posteo.de header.s=2017 header.b=eoSQbb9Q; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id AD99124019B for ; Fri, 16 Dec 2022 17:03:24 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1671206604; bh=y1DrRTnr5pLd6UVqZ17dgRsQHdzkSIe9uYkPaXwLmJI=; h=Subject:From:Date:Cc:To:From; b=eoSQbb9QWjxv4l2RxxiP7ZCYfomcIxNu8nYK6lKCVt1dt8+oIXPHXdmbStU6ySZHK 9MYS/O1LL0xT466jwh4Evc3iLlOX4qEMjgQUnQgP6xqa1rXvKVq1qHWvY+G8XCckEC Ouq5PldX5p4V1ADEqUS4CdAanFKk4O82ETd0ZIdfgTWzvWxszLsI/pX+80S1cmFJqT +C37stI9Z8WCS8dxCy1QZsN2dKGqiLKq0eVBEZJAQ7WFF2e5u5QWCu88XaT0sLM6CF McQfPDNvJTesHypkPdSEddOY/7Bzc4QNwT3ZVGVTwoNsdrRW0xtgok4Ia0V6WiZlLZ jnRNf1E7Fnzug== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4NYYmW5snnz6tmL; Fri, 16 Dec 2022 17:03:23 +0100 (CET) Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\)) Subject: Re: [edk2-platforms][PATCH v2 03/11] Ext4Pkg: Fix global buffer overflow in Ext4ReadDir From: =?utf-8?Q?Marvin_H=C3=A4user?= In-Reply-To: <20221212144654.2650-4-savvamtr@gmail.com> Date: Fri, 16 Dec 2022 16:03:13 +0000 Cc: edk2-devel-groups-io , Pedro Falcato , Vitaly Cheptsov Message-Id: <659B64F8-C2EA-4513-AC47-5CBDDFEC40B7@posteo.de> References: <20221212144654.2650-1-savvamtr@gmail.com> <20221212144654.2650-4-savvamtr@gmail.com> To: Savva Mitrofanov Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 12. Dec 2022, at 15:46, Savva Mitrofanov wrote: >=20 > 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 !=3D 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 <=3D 2 and check for > null-terminator presence >=20 > Cc: Marvin H=C3=A4user > Cc: Pedro Falcato > Cc: Vitaly Cheptsov > Fixes: e55f0527dde48a5f139c1b8f35acc4e6b59dd794 > Signed-off-by: Savva Mitrofanov > --- > Features/Ext4Pkg/Ext4Dxe/Directory.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) >=20 > 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 ( >=20 > // 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 =3D Entry.name_len !=3D 0 && > - (CompareMem (Entry.name, ".", Entry.name_len) =3D=3D= 0 || > - CompareMem (Entry.name, "..", Entry.name_len) =3D=3D= 0); > + IsDotOrDotDot =3D Entry.name_len <=3D 2 && > + ((Entry.name[0] =3D=3D '.') && > + (Entry.name[1] =3D=3D '.' || Entry.name[1] =3D=3D = '\0')); This is definitely borked, names do not need to be 0-terminated. So this = may cause OOB if Entry.name_len =3D=3D 1 and Entry.name[0] =3D=3D '.' = and also may yield a false negative. >=20 > // When inode =3D 0, it's unused. > ShouldSkip =3D Entry.inode =3D=3D 0 || IsDotOrDotDot; > --=20 > 2.38.1 >=20