From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5738921180F4C for ; Sat, 27 Oct 2018 06:19:51 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Oct 2018 06:19:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,432,1534834800"; d="scan'208";a="91685034" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by FMSMGA003.fm.intel.com with ESMTP; 27 Oct 2018 06:19:50 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sat, 27 Oct 2018 06:19:50 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sat, 27 Oct 2018 06:19:49 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.84]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.199]) with mapi id 14.03.0415.000; Sat, 27 Oct 2018 21:19:47 +0800 From: "Zeng, Star" To: "Wu, Hao A" , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" , "Zeng, Star" Thread-Topic: [PATCH v2] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink() Thread-Index: AQHUbfCBL2TsGiDg8UajHpH3Sr+A06UzE3pw Date: Sat, 27 Oct 2018 13:19:46 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103BC5C1DC@shsmsx102.ccr.corp.intel.com> References: <20181027122756.34508-1-hao.a.wu@intel.com> In-Reply-To: <20181027122756.34508-1-hao.a.wu@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNmE5Y2MwNWEtYjJlYS00YzVmLTgxMWQtZWIwZTA5ZmM3NTA0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoibzZsZFlNa1dOWks0cVdYNzNSSjJuMVZlYjdIS3VucDdvMENXM0F2MUoyVEs0VHByaWpFNXY2SGVwaFpqVnFqciJ9 dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v2] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 27 Oct 2018 13:19:52 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Star Zeng -----Original Message----- From: Wu, Hao A=20 Sent: Saturday, October 27, 2018 8:28 PM To: edk2-devel@lists.01.org Cc: Wu, Hao A ; Ni, Ruiyu ; Zeng, S= tar Subject: [PATCH v2] MdeModulePkg/UdfDxe: Additional checks for ResolveSymli= nk() REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D1279 V2 change: Refine type C check (refer to V1 history below) to eliminate the unnecessar= y CopyMem() call. V1 history: The commit will add 3 types of checks for function ResolveSymlink(): A. Check for the value of 'Component Type' field within a Path Component According to the ECMA-167 standard (3rd Edition - June 1997), Section 14.16= .1.1, valid values are 1 to 5. All other values will be treated as a corrup= ted volume. B. Check for the content pointed by 'File' Since content within 'File' is the output data for ResolveSymlink(). Checks is added to ensure the content in 'File' is valid. Otherwise, possib= le null pointer dereference issue will occur during the subsequent usage of= the data returned by ResolveSymlink(). C. Check for possible memory double free/use after free case For codes: if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent, sizeof (UDF_FILE_INFO)) !=3D 0) { CleanupFileInformation (&PreviousFile); } CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO)); If the contents in 'PreviousFile' and 'File' are the same, call to "Cleanup= FileInformation (&PreviousFile);" will free the buffers in 'File' as well. This will lead to potential memory double free/use after free issu= es. Cc: Ruiyu Ni Cc: Star Zeng Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Hao Wu Reviewed-by: Paulo Alcantara --- MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 38 +++++++++++= +++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/Md= eModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c index b9ebddfe62..971f562ae2 100644 --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c @@ -2144,6 +2144,10 @@ ResolveSymlink ( UINTN Index; UINT8 CompressionId; UDF_FILE_INFO PreviousFile; + BOOLEAN NotParent; + BOOLEAN NotFile; + + ZeroMem ((VOID *)File, sizeof (UDF_FILE_INFO)); =20 // // Symlink files on UDF volumes do not contain so much data other than @= @ -2257,6 +2261,13 @@ ResolveSymlink ( } FileName[Index] =3D L'\0'; break; + default: + // + // Accoring to the ECMA-167 standard (3rd Edition - June 1997), Sect= ion + // 14.16.1.1, all other values are reserved. + // + Status =3D EFI_VOLUME_CORRUPTED; + goto Error_Find_File; } =20 // @@ -2281,12 +2292,26 @@ ResolveSymlink ( break; } =20 - if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent, - sizeof (UDF_FILE_INFO)) !=3D 0) { + // + // Check the content in the file info pointed by File. + // + if ((File->FileEntry =3D=3D NULL) || (File->FileIdentifierDesc =3D=3D = NULL)) { + Status =3D EFI_VOLUME_CORRUPTED; + goto Error_Find_File; + } + + NotParent =3D (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent, + sizeof (UDF_FILE_INFO)) !=3D 0); + NotFile =3D (CompareMem ((VOID *)&PreviousFile, (VOID *)File, + sizeof (UDF_FILE_INFO)) !=3D 0); + + if (NotParent && NotFile) { CleanupFileInformation (&PreviousFile); } =20 - CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO)); + if (NotFile) { + CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO)= ); + } } =20 // @@ -2294,6 +2319,13 @@ ResolveSymlink ( // FreePool (ReadFileInfo.FileData); =20 + // + // Check the content in the resolved file info. + // + if ((File->FileEntry =3D=3D NULL) || (File->FileIdentifierDesc =3D=3D NU= LL)) { + return EFI_VOLUME_CORRUPTED; + } + return EFI_SUCCESS; =20 Error_Find_File: -- 2.12.0.windows.1