From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 2AB5A1A1E8E for ; Tue, 11 Oct 2016 07:28:36 -0700 (PDT) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga105.fm.intel.com with ESMTP; 11 Oct 2016 07:28:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,329,1473145200"; d="scan'208,217";a="1052340367" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga001.fm.intel.com with ESMTP; 11 Oct 2016 07:28:34 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 11 Oct 2016 07:28:34 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 11 Oct 2016 07:28:33 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.15]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.101]) with mapi id 14.03.0248.002; Tue, 11 Oct 2016 22:28:31 +0800 From: "Yao, Jiewen" To: Sean Brogan , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" , "Zeng, Star" , "Tian, Feng" , "Gao, Liming" , "Zhang, Chao B" Thread-Topic: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance. Thread-Index: AQHSGxVfqpx4fuGpN0ymTNjw43Ajj6CjD7tggABOtOA= Date: Tue, 11 Oct 2016 14:28:31 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C50386A142C@shsmsx102.ccr.corp.intel.com> References: <1475238128-22448-1-git-send-email-jiewen.yao@intel.com> <1475238128-22448-10-git-send-email-jiewen.yao@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Oct 2016 14:28:36 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable In ExecuteFmpAuthenticationHandler and other functions you use asserts to h= andle parameter validation. I didn't see in the caller any protections aga= inst bad parameters so on builds with ASSERT disabled this code will not be= safe. [Jiewen] The code uses 4 ASSERT. ASSERT (Image !=3D NULL); // This is filtered in FmpSetImage(). ASSERT (ImageSize !=3D 0); // I miss that check in FmpSetImage(). I will = fix that. ASSERT (LastAttemptStatus !=3D NULL); // This is an internal input. Calle= r may not able to control it. ASSERT (mNumberOfFmpAuthenticationHandler !=3D 0); // This is an internal= state check. Caller may not able to control it. I already follow EDKII style to add comment on ASSERT() in function comment= . So it is caller's responsibility to make sure they are valid. Anyway, I think it is OK change ASSERT(Image) and ASSERT(ImageSize) to erro= r handling, if you insist. But LastAttempStatus is just internal input. Caller cannot control it. I do= not see the need to use error handling. Also mNumberOfFmpAuthenticationHandler is internal status. If it is 0, it m= eans developer does not configure it successfully. But the last issue will gone if we remove registration pattern. Can you explain how you are using monotonic count? Your comment says you a= re incrementing the PCD to avoid rollback (line 246: It is incremented duri= ng each firmware image operation.) Looks like it is just being compared to make sure capsule counter not less = than PCD based value? [Jiewen] You are right. The purpose is to follow UEFI spec: MonotonicCount It is included in the signature of AuthInfo. It is used to e= nsure freshness/no replay. It is incremented during each firmware image ope= ration. If I misunderstand something, please let me know. Same comments as patch 3. In my opinion library registration pattern is ov= erkill for this usage. [Jiewen] See my comment in previous mail. From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sean= Brogan Sent: Tuesday, October 11, 2016 5:51 PM To: Yao, Jiewen ; edk2-devel@lists.01.org Cc: Kinney, Michael D ; Zeng, Star ; Tian, Feng ; Gao, Liming ; Zhang, Chao B Subject: Re: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add= FmpAuthenticationLib instance. In ExecuteFmpAuthenticationHandler and other functions you use asserts to h= andle parameter validation. I didn't see in the caller any protections aga= inst bad parameters so on builds with ASSERT disabled this code will not be= safe. Can you explain how you are using monotonic count? Your comment says you a= re incrementing the PCD to avoid rollback (line 246: It is incremented duri= ng each firmware image operation.) Looks like it is just being compared to make sure capsule counter not less = than PCD based value? Same comments as patch 3. In my opinion library registration pattern is ov= erkill for this usage. Thanks Sean > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Jiewen Yao > Sent: Friday, September 30, 2016 5:21 AM > To: edk2-devel@lists.01.org > Cc: Michael D Kinney >; Feng Tian > >; Chao Zhang >; Liming Gao > >; Star Zeng > > Subject: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add > FmpAuthenticationLib instance. > > This library is used to authenticate a UEFI defined FMP Capsule. > > Cc: Feng Tian > > Cc: Star Zeng > > Cc: Michael D Kinney > > Cc: Liming Gao > > Cc: Chao Zhang > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao > > Reviewed-by: Liming Gao > > --- > MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c | 277 > ++++++++++++++++++++ > MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.inf | 47 > ++++ MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.uni > | 22 ++ > 3 files changed, 346 insertions(+) > > diff --git > a/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c > b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c > new file mode 100644 > index 0000000..878cbf9 > --- /dev/null > +++ b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.c > @@ -0,0 +1,277 @@ > +/** @file > + Provide generic FMP authentication functions for DXE/PEI post memory > phase. > + > + ExecuteFmpAuthenticationHandler() will receive untrusted input and do > + basic validation. > + > + Copyright (c) 2016, Intel Corporation. All rights reserved.
This > + program and the accompanying materials are licensed and made > + available under the terms and conditions of the BSD License which > + accompanies this distribution. The full text of the license may be > + found at http://opensource.org/licenses/bsd-license.php. > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > EXPRESS OR IMPLIED. > + > +**/ > + > +#include > + > +#include > +#include > +#include #include > + #include #include > + #include > + > +#define FMP_AUTHENTICATION_HANDLER_TABLE_SIZE 0x10 > + > +UINT64 mMonotonicCount; > + > +UINT32 mNumberOfFmpAuthenticationHandler =3D 0; > +UINT32 mMaxNumberOfFmpAuthenticationHandler =3D 0; > + > +GUID *mFmpAuthenticationHandlerGuidTable =3D NUL= L; > +FMP_AUTHENTICATION_HANDLER *mFmpAuthenticationHandlerTable =3D > NULL; > + > +/** > + Reallocates more global memory to store the registered guid and Handle= r > list. > + > + @retval RETURN_SUCCESS Reallocated more global memory spac= e to > store guid and function tables. > + @retval RETURN_OUT_OF_RESOURCES Not enough memory to allocate. > +**/ > +RETURN_STATUS > +EFIAPI > +ReallocateFmpAuthenticationHandlerTable ( > + ) > +{ > + // > + // Reallocate memory for GuidTable > + // > + mFmpAuthenticationHandlerGuidTable =3D ReallocatePool ( > + mMaxNumberOfFmpAuthenticationHa= ndler * sizeof > (GUID), > + (mMaxNumberOfFmpAuthenticationH= andler + > FMP_AUTHENTICATION_HANDLER_TABLE_SIZE) * sizeof (GUID), > + mFmpAuthenticationHandlerGuidTa= ble > + ); > + if (mFmpAuthenticationHandlerGuidTable =3D=3D NULL) { > + goto Done; > + } > + > + // > + // Reallocate memory for Authentication handler Table // > + mFmpAuthenticationHandlerTable =3D ReallocatePool ( > + mMaxNumberOfFmpAuthenticationHandle= r * sizeof > (FMP_AUTHENTICATION_HANDLER), > + (mMaxNumberOfFmpAuthenticationHandl= er + > FMP_AUTHENTICATION_HANDLER_TABLE_SIZE) * sizeof > (FMP_AUTHENTICATION_HANDLER), > + mFmpAuthenticationHandlerTable > + ); if > + (mFmpAuthenticationHandlerTable =3D=3D NULL) { > + goto Done; > + } > + > + // > + // Increase max handler number > + // > + mMaxNumberOfFmpAuthenticationHandler =3D > + mMaxNumberOfFmpAuthenticationHandler + > + FMP_AUTHENTICATION_HANDLER_TABLE_SIZE; > + return RETURN_SUCCESS; > + > +Done: > + if (mFmpAuthenticationHandlerGuidTable !=3D NULL) { > + FreePool (mFmpAuthenticationHandlerGuidTable); > + } > + if (mFmpAuthenticationHandlerTable !=3D NULL) { > + FreePool (mFmpAuthenticationHandlerTable); > + } > + > + return RETURN_OUT_OF_RESOURCES; > +} > + > +/** > + Constructor allocates the global memory to store the registered guid a= nd > Handler list. > + > + @param ImageHandle The firmware allocated handle for the EFI image. > + @param SystemTable A pointer to the EFI System Table. > + > + @retval RETURN_SUCCESS Allocated the global memory space t= o > store guid and function tables. > + @retval RETURN_OUT_OF_RESOURCES Not enough memory to allocate. > +**/ > +RETURN_STATUS > +EFIAPI > +FmpAuthenticationLibConstructor ( > + VOID > + ) > +{ > + mMonotonicCount =3D > PcdGet64(PcdEdkiiSystemFmpCapsuleMonotonicCount); > + > + return ReallocateFmpAuthenticationHandlerTable (); } > + > +/** > + Register FMP authentication handler with CertType. > + > + If CertType is NULL, then ASSERT(). > + If FmpAuthenticationHandler is NULL, then ASSERT(). > + > + @param[in] CertType The certificate type associated= with the > FMP auth handler. > + @param[in] FmpAuthenticationHandler The FMP authentication handler = to > be registered. > + > + @retval RETURN_SUCCESS The handlers were registered. > + @retval RETURN_OUT_OF_RESOURCES There are not enough resources > available to register the handlers. > +**/ > +RETURN_STATUS > +EFIAPI > +RegisterFmpAuthenticationHandler( > + IN GUID *CertType, > + IN FMP_AUTHENTICATION_HANDLER FmpAuthenticationHandler > + ) > +{ > + UINTN Index; > + > + // > + // Check input paramter. > + // > + ASSERT (CertType !=3D NULL); > + ASSERT (FmpAuthenticationHandler !=3D NULL); > + > + // > + // Search the match registered GetInfo handler for the input guided se= ction. > + // > + for (Index =3D 0; Index < mNumberOfFmpAuthenticationHandler; Index ++)= { > + if (CompareGuid (&mFmpAuthenticationHandlerGuidTable[Index], > CertType)) { > + // > + // If the guided handler has been registered before, only update i= ts > handler. > + // > + mFmpAuthenticationHandlerTable[Index] =3D FmpAuthenticationHandler= ; > + return RETURN_SUCCESS; > + } > + } > + > + // > + // Check the global table is enough to contain new Handler. > + // > + if (mNumberOfFmpAuthenticationHandler >=3D > mMaxNumberOfFmpAuthenticationHandler) { > + if (ReallocateFmpAuthenticationHandlerTable () !=3D RETURN_SUCCESS) = { > + return RETURN_OUT_OF_RESOURCES; > + } > + } > + > + // > + // Register new Handler and guid value. > + // > + CopyGuid (&mFmpAuthenticationHandlerGuidTable > + [mNumberOfFmpAuthenticationHandler], CertType); > + mFmpAuthenticationHandlerTable [mNumberOfFmpAuthenticationHandler] > =3D > + FmpAuthenticationHandler; mNumberOfFmpAuthenticationHandler++; > + > + return RETURN_SUCCESS; > +} > + > +/** > + Execute FMP authentication handlers. > + > + Caution: This function may receive untrusted input. > + > + If Image is NULL, then ASSERT(). > + If ImageSize is 0, then ASSERT(). > + If LastAttemptStatus is NULL, then ASSERT(). > + > + @param[in] Image Points to the new FMP authentication = image, > + start from EFI_FIRMWARE_IMAGE_AUTHENT= ICATION. > + @param[in] ImageSize Size of the authentication image in b= ytes. > + @param[out] LastAttemptStatus The last attempt status, which will b= e > recorded > + in ESRT and FMP EFI_FIRMWARE_IMAGE_DE= SCRIPTOR. > + > + @retval RETURN_SUCCESS Authentication pass. > + @retval RETURN_SECURITY_VIOLATION Authentication fail. > + The detail reson is recorded in Last= AttemptStatus. > + @retval RETURN_UNSUPPORTED No Authentication handler associated > with CertType. > +**/ > +RETURN_STATUS > +EFIAPI > +ExecuteFmpAuthenticationHandler( > + IN VOID *Image, > + IN UINTN ImageSize, > + OUT UINT32 *LastAttemptStatus > + ) > +{ > + EFI_FIRMWARE_IMAGE_AUTHENTICATION *ImageAuthentication; > + UINTN ImageOffset; > + UINTN Index; > + GUID *CertType; > + > + // > + // Check the input parameters > + // > + ASSERT (Image !=3D NULL); > + ASSERT (ImageSize !=3D 0); > + ASSERT (LastAttemptStatus !=3D NULL); > + > + ASSERT (mNumberOfFmpAuthenticationHandler !=3D 0); > + > + ImageAuthentication =3D (EFI_FIRMWARE_IMAGE_AUTHENTICATION *)Image; > if > + (ImageSize < sizeof(EFI_FIRMWARE_IMAGE_AUTHENTICATION)) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - ImageSize > too small\n")); > + *LastAttemptStatus =3D LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + if (ImageAuthentication->AuthInfo.Hdr.dwLength <=3D > sizeof(WIN_CERTIFICATE) + sizeof(EFI_GUID)) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - dwLength > too small\n")); > + *LastAttemptStatus =3D LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + if (ImageAuthentication->AuthInfo.Hdr.dwLength > MAX_UINTN - > sizeof(UINT64)) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - dwLength > too big\n")); > + *LastAttemptStatus =3D LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + ImageOffset =3D sizeof(UINT64) + > + ImageAuthentication->AuthInfo.Hdr.dwLength; > + if (ImageSize <=3D ImageOffset) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - ImageSize > too small\n")); > + *LastAttemptStatus =3D LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + if (ImageAuthentication->AuthInfo.Hdr.wRevision !=3D 0x0200) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - wRevision: > 0x%02x, expect - 0x%02x\n", (UINTN)ImageAuthentication- > >AuthInfo.Hdr.wRevision, (UINTN)0x0200)); > + *LastAttemptStatus =3D LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + if (ImageAuthentication->AuthInfo.Hdr.wCertificateType !=3D > WIN_CERT_TYPE_EFI_GUID) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - > wCertificateType: 0x%02x, expect - 0x%02x\n", (UINTN)ImageAuthentication- > >AuthInfo.Hdr.wCertificateType, (UINTN)WIN_CERT_TYPE_EFI_GUID)); > + *LastAttemptStatus =3D LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + > + // > + // It is used to ensure freshness / no replay. > + // It is incremented during each firmware image operation. > + // > + if (ImageAuthentication->MonotonicCount < mMonotonicCount) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - > MonotonicCount: 0x%016lx, current - 0x%016lx\n", ImageAuthentication- > >MonotonicCount, mMonotonicCount)); > + *LastAttemptStatus =3D > LAST_ATTEMPT_STATUS_ERROR_INCORRECT_VERSION; > + return RETURN_SECURITY_VIOLATION; > + } > + > + CertType =3D &ImageAuthentication->AuthInfo.CertType; > + DEBUG((DEBUG_INFO, "ExecuteFmpAuthenticationHandler - CertType: > + %g\n", CertType)); > + > + // > + // Search the match registered extract handler for the input guided se= ction. > + // > + for (Index =3D 0; Index < mNumberOfFmpAuthenticationHandler; Index ++)= { > + if (CompareGuid (&mFmpAuthenticationHandlerGuidTable[Index], > CertType)) { > + // > + // Call the match handler to extract raw data for the input sectio= n data. > + // > + return mFmpAuthenticationHandlerTable [Index] ( > + Image, > + ImageSize, > + LastAttemptStatus > + ); > + } > + } > + > + // > + // Not found, the input guided section is not supported. > + // > + return RETURN_UNSUPPORTED; > +} > diff --git > a/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.inf > b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.inf > new file mode 100644 > index 0000000..edd0a61 > --- /dev/null > +++ b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.inf > @@ -0,0 +1,47 @@ > +## @file > +# FmpAuthenitcation Library > +# > +# Instance of FmpAuthenitcation Library for DXE/PEI post memory phase. > +# > +# Copyright (c) 2016, Intel Corporation. All rights reserved.
# # > +This program and the accompanying materials # are licensed and made > +available under the terms and conditions of the BSD License # which > +accompanies this distribution. The full text of the license may be > +found at # http://opensource.org/licenses/bsd-license.php. > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > +BASIS, # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > EXPRESS OR IMPLIED. > +# > +## > + > +[Defines] > + INF_VERSION =3D 0x00010005 > + BASE_NAME =3D FmpAuthenitcationLib > + MODULE_UNI_FILE =3D FmpAuthenitcationLib.uni > + FILE_GUID =3D 5011522C-7B0E-4ACB-8E30-9B1D133CF2E= 0 > + MODULE_TYPE =3D BASE > + VERSION_STRING =3D 1.0 > + LIBRARY_CLASS =3D FmpAuthenitcationLib > + CONSTRUCTOR =3D FmpAuthenticationLibConstructor > + > +# > +# The following information is for reference only and not required by th= e build > tools. > +# > +# VALID_ARCHITECTURES =3D IA32 X64 IPF EBC > +# > + > +[Sources] > + FmpAuthenitcationLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + > +[LibraryClasses] > + MemoryAllocationLib > + BaseMemoryLib > + DebugLib > + PcdLib > + > +[Pcd] > + > gEfiMdeModulePkgTokenSpaceGuid.PcdEdkiiSystemFmpCapsuleMonotonicCou > nt > diff --git > a/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.uni > b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.uni > new file mode 100644 > index 0000000..ea660c0 > --- /dev/null > +++ > b/MdeModulePkg/Library/FmpAuthenitcationLib/FmpAuthenitcationLib.uni > @@ -0,0 +1,22 @@ > +// /** @file > +// FmpAuthenitcation Library > +// > +// Instance of FmpAuthenitcation Library for DXE/PEI post memory phase. > +// > +// Copyright (c) 2016, Intel Corporation. All rights reserved.
// > +// This program and the accompanying materials // are licensed and made > +available under the terms and conditions of the BSD License // which > +accompanies this distribution. The full text of the license may be > +found at // http://opensource.org/licenses/bsd-license.php > +// > +// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > +BASIS, // WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > EXPRESS OR IMPLIED. > +// > +// **/ > + > + > +#string STR_MODULE_ABSTRACT #language en-US "FmpAuthenitcati= on > Library" > + > +#string STR_MODULE_DESCRIPTION #language en-US "Instance of > FmpAuthenitcation Library for DXE/PEI post memory phase." > + > -- > 2.7.4.windows.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel