From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 9222A210C66A0 for ; Tue, 31 Jul 2018 19:32:36 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Jul 2018 19:32:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,430,1526367600"; d="scan'208";a="61396234" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga008.jf.intel.com with ESMTP; 31 Jul 2018 19:32:35 -0700 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 31 Jul 2018 19:32:34 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 31 Jul 2018 19:32:34 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.124]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.57]) with mapi id 14.03.0319.002; Wed, 1 Aug 2018 10:32:32 +0800 From: "Dong, Eric" To: "edk2-devel@lists.01.org" CC: "Wu, Hao A" , "Yao, Jiewen" Thread-Topic: [edk2] [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add check to avoid potential buffer overflow. Thread-Index: AQHUKI2WN1Qc9ytTWkSfZuUsaTlBDaSqLj6w Date: Wed, 1 Aug 2018 02:32:31 +0000 Message-ID: References: <20180731051556.15980-1-eric.dong@intel.com> In-Reply-To: <20180731051556.15980-1-eric.dong@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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:32:37 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Hao & Jiewen, This patch has been verified by the original reporter, also pass regression= test done by myself. Thanks, Eric > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Er= ic > 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. >=20 > Current code not check the CommunicationBuffer size before use it. Attack= er > can read beyond the end of the (untrusted) commbuffer into controlled > memory. Attacker can get access outside of valid SMM memory regions. This > patch add check before use it. >=20 > 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(-) >=20 > 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 >=20 > +#define OPAL_PASSWORD_MAX_LENGTH 32 >=20 > #pragma pack(1) >=20 > @@ -76,7 +77,7 @@ typedef struct { > typedef struct { > LIST_ENTRY Link; >=20 > - UINT8 Password[32]; > + UINT8 Password[OPAL_PASSWORD_MAX_LENGTH]; > UINT8 PasswordLength; >=20 > 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. >=20 > #include "OpalPasswordSupportNotify.h" >=20 > -#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; >=20 > if (CommBuffer =3D=3D NULL || CommBufferSize =3D=3D NULL) { > return EFI_SUCCESS; > } >=20 > + 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; >=20 > 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_OF > (OPAL_SMM_COMMUNICATE_HEADER, Data), SmmFunctionHeader->Data); > + if (DeviceBuffer =3D=3D NULL) { > + Status =3D EFI_OUT_OF_RESOURCES; > + goto EXIT; > + } >=20 > - 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 (&DeviceBuffer- > >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; >=20 > default: > @@ -701,6 +718,10 @@ SmmOpalPasswordHandler ( > EXIT: > SmmFunctionHeader->ReturnStatus =3D Status; >=20 > + 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; >=20 > typedef struct { > - UINT8 Password[32]; > + UINT8 Password[OPAL_PASSWORD_MAX_LENGTH]; > UINT8 PasswordLength; >=20 > 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 >=20 > extern VOID *mBuffer; >=20 > @@ -124,7 +120,7 @@ typedef struct { >=20 > UINT32 NvmeNamespaceId; >=20 > - UINT8 Password[32]; > + UINT8 Password[OPAL_PASSWORD_MAX_LE= NGTH]; > UINT8 PasswordLength; >=20 > UINT32 Length; > -- > 2.15.0.windows.1 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel