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.115; helo=mga14.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 0F7D4210C66A6 for ; Tue, 31 Jul 2018 19:44:19 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Jul 2018 19:44:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,430,1526367600"; d="scan'208";a="79487612" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 31 Jul 2018 19:44:18 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 31 Jul 2018 19:44:17 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 31 Jul 2018 19:44:17 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.81]) with mapi id 14.03.0319.002; Wed, 1 Aug 2018 10:44:15 +0800 From: "Yao, Jiewen" To: "Dong, Eric" , "edk2-devel@lists.01.org" CC: "Wu, Hao A" Thread-Topic: [edk2] [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add check to avoid potential buffer overflow. Thread-Index: AQHUKI2V0d1Y0bOLmk2Pv2WB513Ol6SpqJOAgACJVgA= Date: Wed, 1 Aug 2018 02:44:14 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503ACDBCD6@shsmsx102.ccr.corp.intel.com> References: <20180731051556.15980-1-eric.dong@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYzY5NTA5ZmYtMTMzYi00YTNkLWE1MTQtNTVmNDhhNzQ2NjQ5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoicHJ0U0FQSFRIdlwvOUt0ZHZHeWdPeGVWTDBqMEw5UzlVYUt1UGNybmsxcmJhNG1EdytWN1BFS2JKZXdaS0FGNk4ifQ== x-ctpclassification: CTP_NT 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] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add check to avoid potential buffer overflow. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Aug 2018 02:44:20 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: jiewen.yao@intel.com > -----Original Message----- > From: Dong, Eric > Sent: Wednesday, August 1, 2018 10:33 AM > To: edk2-devel@lists.01.org > Cc: Wu, Hao A ; Yao, Jiewen > Subject: RE: [edk2] [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: > Add check to avoid potential buffer overflow. >=20 > Hi Hao & Jiewen, >=20 > This patch has been verified by the original reporter, also pass regressi= on test > done by myself. >=20 > Thanks, > Eric >=20 > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Eric > > Dong > > Sent: Tuesday, July 31, 2018 1:16 PM > > To: edk2-devel@lists.01.org > > Cc: Wu, Hao A ; Yao, Jiewen > > Subject: [edk2] [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: > Add > > check to avoid potential buffer overflow. > > > > Current code not check the CommunicationBuffer size before use it. Atta= cker > > can read beyond the end of the (untrusted) commbuffer into controlled > > memory. Attacker can get access outside of valid SMM memory regions. Th= is > > patch add check before use it. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Eric Dong > > Cc: Yao Jiewen > > Cc: Wu Hao > > --- > > .../Include/Library/OpalPasswordSupportLib.h | 3 +- > > .../OpalPasswordSupportLib.c | 55 > +++++++++++++++------- > > .../OpalPasswordSupportNotify.h | 2 +- > > .../Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h | 6 +-- > > 4 files changed, 42 insertions(+), 24 deletions(-) > > > > diff --git a/SecurityPkg/Include/Library/OpalPasswordSupportLib.h > > b/SecurityPkg/Include/Library/OpalPasswordSupportLib.h > > index e616c763f0..ad45e9e3b7 100644 > > --- a/SecurityPkg/Include/Library/OpalPasswordSupportLib.h > > +++ b/SecurityPkg/Include/Library/OpalPasswordSupportLib.h > > @@ -19,6 +19,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY > > KIND, EITHER EXPRESS OR IMPLIED. > > #include > > #include > > > > +#define OPAL_PASSWORD_MAX_LENGTH 32 > > > > #pragma pack(1) > > > > @@ -76,7 +77,7 @@ typedef struct { > > typedef struct { > > LIST_ENTRY Link; > > > > - UINT8 Password[32]; > > + UINT8 > Password[OPAL_PASSWORD_MAX_LENGTH]; > > UINT8 PasswordLength; > > > > EFI_DEVICE_PATH_PROTOCOL OpalDevicePath; > > diff --git > > a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.c > > b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.c > > index 837582359e..e377e9ca79 100644 > > --- > a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.c > > +++ > b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib. > > +++ c > > @@ -14,8 +14,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY > > KIND, EITHER EXPRESS OR IMPLIED. > > > > #include "OpalPasswordSupportNotify.h" > > > > -#define OPAL_PASSWORD_MAX_LENGTH 32 > > - > > LIST_ENTRY mDeviceList =3D INITIALIZE_LIST_HEAD_VARIABLE > > (mDeviceList); > > BOOLEAN gInSmm =3D FALSE; > > EFI_GUID gOpalPasswordNotifyProtocolGuid =3D > > OPAL_PASSWORD_NOTIFY_PROTOCOL_GUID; > > @@ -663,34 +661,53 @@ SmmOpalPasswordHandler ( > > EFI_STATUS Status; > > OPAL_SMM_COMMUNICATE_HEADER *SmmFunctionHeader; > > UINTN TempCommBufferSize; > > - UINT8 *NewPassword; > > - UINT8 PasswordLength; > > - EFI_DEVICE_PATH_PROTOCOL *DevicePath; > > + UINTN RemainedDevicePathSize; > > + OPAL_COMM_DEVICE_LIST *DeviceBuffer; > > > > if (CommBuffer =3D=3D NULL || CommBufferSize =3D=3D NULL) { > > return EFI_SUCCESS; > > } > > > > + Status =3D EFI_SUCCESS; > > + DeviceBuffer =3D NULL; > > + > > TempCommBufferSize =3D *CommBufferSize; > > if (TempCommBufferSize < OFFSET_OF > > (OPAL_SMM_COMMUNICATE_HEADER, Data)) { > > return EFI_SUCCESS; > > } > > - > > - Status =3D EFI_SUCCESS; > > - SmmFunctionHeader =3D (OPAL_SMM_COMMUNICATE_HEADER > > *)CommBuffer; > > - > > - DevicePath =3D &((OPAL_COMM_DEVICE_LIST*)(SmmFunctionHeader- > > >Data))->OpalDevicePath; > > - PasswordLength =3D ((OPAL_COMM_DEVICE_LIST*)(SmmFunctionHeader- > > >Data))->PasswordLength; > > - NewPassword =3D ((OPAL_COMM_DEVICE_LIST*)(SmmFunctionHeader- > > >Data))->Password; > > + SmmFunctionHeader =3D (OPAL_SMM_COMMUNICATE_HEADER > > *)CommBuffer; > > > > switch (SmmFunctionHeader->Function) { > > case SMM_FUNCTION_SET_OPAL_PASSWORD: > > - if (OpalPasswordIsFullZero (NewPassword) || PasswordLength =3D= =3D 0) > { > > - Status =3D EFI_INVALID_PARAMETER; > > - goto EXIT; > > - } > > + if (TempCommBufferSize <=3D OFFSET_OF > > (OPAL_SMM_COMMUNICATE_HEADER, Data) + OFFSET_OF > > (OPAL_COMM_DEVICE_LIST, OpalDevicePath)) { > > + return EFI_SUCCESS; > > + } > > + > > + DeviceBuffer =3D AllocateCopyPool (TempCommBufferSize - OFFSET_O= F > > (OPAL_SMM_COMMUNICATE_HEADER, Data), SmmFunctionHeader->Data); > > + if (DeviceBuffer =3D=3D NULL) { > > + Status =3D EFI_OUT_OF_RESOURCES; > > + goto EXIT; > > + } > > > > - Status =3D OpalSavePasswordToSmm (DevicePath, NewPassword, > > PasswordLength); > > + // > > + // Validate the DevicePath. > > + // > > + RemainedDevicePathSize =3D TempCommBufferSize - OFFSET_OF > > (OPAL_SMM_COMMUNICATE_HEADER, Data) > > + - OFFSET_OF > (OPAL_COMM_DEVICE_LIST, > > OpalDevicePath); > > + if (!IsDevicePathValid(&DeviceBuffer->OpalDevicePath, > > RemainedDevicePathSize) || > > + (RemainedDevicePathSize !=3D GetDevicePathSize (&DeviceBuffe= r- > > >OpalDevicePath))) { > > + Status =3D EFI_SUCCESS; > > + goto EXIT; > > + } > > + > > + if (OpalPasswordIsFullZero (DeviceBuffer->Password) || > > + DeviceBuffer->PasswordLength =3D=3D 0 || > > + DeviceBuffer->PasswordLength > > OPAL_PASSWORD_MAX_LENGTH) { > > + Status =3D EFI_INVALID_PARAMETER; > > + goto EXIT; > > + } > > + > > + Status =3D OpalSavePasswordToSmm (&DeviceBuffer->OpalDevicePath, > > + DeviceBuffer->Password, DeviceBuffer->PasswordLength); > > break; > > > > default: > > @@ -701,6 +718,10 @@ SmmOpalPasswordHandler ( > > EXIT: > > SmmFunctionHeader->ReturnStatus =3D Status; > > > > + if (DeviceBuffer !=3D NULL) { > > + FreePool (DeviceBuffer); > > + } > > + > > // > > // Return EFI_SUCCESS cause only one handler can be trigged. > > // so return EFI_WARN_INTERRUPT_SOURCE_PENDING to make all > handler > > can be trigged. > > diff --git > > > a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNotify.h > > > b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNotify. > h > > index f0ad3a1136..d543faed5d 100644 > > --- > > > a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNotify.h > > +++ > > b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNoti > > +++ fy.h > > @@ -41,7 +41,7 @@ typedef struct { > > } OPAL_SMM_COMMUNICATE_HEADER; > > > > typedef struct { > > - UINT8 Password[32]; > > + UINT8 > Password[OPAL_PASSWORD_MAX_LENGTH]; > > UINT8 PasswordLength; > > > > EFI_DEVICE_PATH_PROTOCOL OpalDevicePath; > > diff --git a/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h > > b/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h > > index ce88786fab..bc559f0bd1 100644 > > --- a/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h > > +++ b/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h > > @@ -64,10 +64,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF > > ANY KIND, EITHER EXPRESS OR IMPLIED. > > // The payload Length of HDD related ATA commands // > > #define HDD_PAYLOAD 512 > > -// > > -// According to ATA spec, the max Length of hdd password is 32 bytes -= // > > -#define OPAL_PASSWORD_MAX_LENGTH 32 > > > > extern VOID *mBuffer; > > > > @@ -124,7 +120,7 @@ typedef struct { > > > > UINT32 NvmeNamespaceId; > > > > - UINT8 Password[32]; > > + UINT8 > Password[OPAL_PASSWORD_MAX_LENGTH]; > > UINT8 PasswordLength; > > > > UINT32 Length; > > -- > > 2.15.0.windows.1 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel