In ExecuteFmpAuthenticationHandler and other functions you use asserts to handle parameter validation. I didn't see in the caller any protections against bad parameters so on builds with ASSERT disabled this code will not be safe. [Jiewen] The code uses 4 ASSERT. ASSERT (Image != NULL); // This is filtered in FmpSetImage(). ASSERT (ImageSize != 0); // I miss that check in FmpSetImage(). I will fix that. ASSERT (LastAttemptStatus != NULL); // This is an internal input. Caller may not able to control it.
ASSERT (mNumberOfFmpAuthenticationHandler != 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 error 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 means 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 are incrementing the PCD to avoid rollback (line 246: It is incremented during 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 ensure freshness/no replay. It is incremented during each firmware image operation. If I misunderstand something, please let me know. Same comments as patch 3. In my opinion library registration pattern is overkill for this usage. [Jiewen] See my comment in previous mail. From: edk2-devel [mailto:[email protected]] On Behalf Of Sean Brogan Sent: Tuesday, October 11, 2016 5:51 PM To: Yao, Jiewen <[email protected]>; [email protected] Cc: Kinney, Michael D <[email protected]>; Zeng, Star <[email protected]>; Tian, Feng <[email protected]>; Gao, Liming <[email protected]>; Zhang, Chao B <[email protected]> Subject: Re: [edk2] [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance. In ExecuteFmpAuthenticationHandler and other functions you use asserts to handle parameter validation. I didn't see in the caller any protections against 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 are incrementing the PCD to avoid rollback (line 246: It is incremented during 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 overkill for this usage. Thanks Sean > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of > Jiewen Yao > Sent: Friday, September 30, 2016 5:21 AM > To: [email protected]<mailto:[email protected]> > Cc: Michael D Kinney > <[email protected]<mailto:[email protected]>>; Feng Tian > <[email protected]<mailto:[email protected]>>; Chao Zhang > <[email protected]<mailto:[email protected]>>; Liming Gao > <[email protected]<mailto:[email protected]>>; Star Zeng > <[email protected]<mailto:[email protected]>> > 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 <[email protected]<mailto:[email protected]>> > Cc: Star Zeng <[email protected]<mailto:[email protected]>> > Cc: Michael D Kinney > <[email protected]<mailto:[email protected]>> > Cc: Liming Gao <[email protected]<mailto:[email protected]>> > Cc: Chao Zhang <[email protected]<mailto:[email protected]>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao <[email protected]<mailto:[email protected]>> > Reviewed-by: Liming Gao <[email protected]<mailto:[email protected]>> > --- > 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.<BR> 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 <Uefi.h> > + > +#include <Library/DebugLib.h> > +#include <Library/BaseMemoryLib.h> > +#include <Library/MemoryAllocationLib.h> #include > +<Library/FmpAuthenticationLib.h> #include <Library/PcdLib.h> #include > +<Protocol/FirmwareManagement.h> #include <Guid/SystemResourceTable.h> > + > +#define FMP_AUTHENTICATION_HANDLER_TABLE_SIZE 0x10 > + > +UINT64 mMonotonicCount; > + > +UINT32 mNumberOfFmpAuthenticationHandler = 0; > +UINT32 mMaxNumberOfFmpAuthenticationHandler = 0; > + > +GUID *mFmpAuthenticationHandlerGuidTable = NULL; > +FMP_AUTHENTICATION_HANDLER *mFmpAuthenticationHandlerTable = > NULL; > + > +/** > + Reallocates more global memory to store the registered guid and Handler > list. > + > + @retval RETURN_SUCCESS Reallocated more global memory space 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 = ReallocatePool ( > + > mMaxNumberOfFmpAuthenticationHandler * sizeof > (GUID), > + > (mMaxNumberOfFmpAuthenticationHandler + > FMP_AUTHENTICATION_HANDLER_TABLE_SIZE) * sizeof (GUID), > + mFmpAuthenticationHandlerGuidTable > + ); > + if (mFmpAuthenticationHandlerGuidTable == NULL) { > + goto Done; > + } > + > + // > + // Reallocate memory for Authentication handler Table // > + mFmpAuthenticationHandlerTable = ReallocatePool ( > + mMaxNumberOfFmpAuthenticationHandler * > sizeof > (FMP_AUTHENTICATION_HANDLER), > + (mMaxNumberOfFmpAuthenticationHandler + > FMP_AUTHENTICATION_HANDLER_TABLE_SIZE) * sizeof > (FMP_AUTHENTICATION_HANDLER), > + mFmpAuthenticationHandlerTable > + ); if > + (mFmpAuthenticationHandlerTable == NULL) { > + goto Done; > + } > + > + // > + // Increase max handler number > + // > + mMaxNumberOfFmpAuthenticationHandler = > + mMaxNumberOfFmpAuthenticationHandler + > + FMP_AUTHENTICATION_HANDLER_TABLE_SIZE; > + return RETURN_SUCCESS; > + > +Done: > + if (mFmpAuthenticationHandlerGuidTable != NULL) { > + FreePool (mFmpAuthenticationHandlerGuidTable); > + } > + if (mFmpAuthenticationHandlerTable != NULL) { > + FreePool (mFmpAuthenticationHandlerTable); > + } > + > + return RETURN_OUT_OF_RESOURCES; > +} > + > +/** > + Constructor allocates the global memory to store the registered guid and > 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 to > store guid and function tables. > + @retval RETURN_OUT_OF_RESOURCES Not enough memory to allocate. > +**/ > +RETURN_STATUS > +EFIAPI > +FmpAuthenticationLibConstructor ( > + VOID > + ) > +{ > + mMonotonicCount = > 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 != NULL); > + ASSERT (FmpAuthenticationHandler != NULL); > + > + // > + // Search the match registered GetInfo handler for the input guided > section. > + // > + for (Index = 0; Index < mNumberOfFmpAuthenticationHandler; Index ++) { > + if (CompareGuid (&mFmpAuthenticationHandlerGuidTable[Index], > CertType)) { > + // > + // If the guided handler has been registered before, only update its > handler. > + // > + mFmpAuthenticationHandlerTable[Index] = FmpAuthenticationHandler; > + return RETURN_SUCCESS; > + } > + } > + > + // > + // Check the global table is enough to contain new Handler. > + // > + if (mNumberOfFmpAuthenticationHandler >= > mMaxNumberOfFmpAuthenticationHandler) { > + if (ReallocateFmpAuthenticationHandlerTable () != RETURN_SUCCESS) { > + return RETURN_OUT_OF_RESOURCES; > + } > + } > + > + // > + // Register new Handler and guid value. > + // > + CopyGuid (&mFmpAuthenticationHandlerGuidTable > + [mNumberOfFmpAuthenticationHandler], CertType); > + mFmpAuthenticationHandlerTable [mNumberOfFmpAuthenticationHandler] > = > + 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_AUTHENTICATION. > + @param[in] ImageSize Size of the authentication image in bytes. > + @param[out] LastAttemptStatus The last attempt status, which will be > recorded > + in ESRT and FMP > EFI_FIRMWARE_IMAGE_DESCRIPTOR. > + > + @retval RETURN_SUCCESS Authentication pass. > + @retval RETURN_SECURITY_VIOLATION Authentication fail. > + The detail reson is recorded in > LastAttemptStatus. > + @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 != NULL); > + ASSERT (ImageSize != 0); > + ASSERT (LastAttemptStatus != NULL); > + > + ASSERT (mNumberOfFmpAuthenticationHandler != 0); > + > + ImageAuthentication = (EFI_FIRMWARE_IMAGE_AUTHENTICATION *)Image; > if > + (ImageSize < sizeof(EFI_FIRMWARE_IMAGE_AUTHENTICATION)) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - ImageSize > too small\n")); > + *LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + if (ImageAuthentication->AuthInfo.Hdr.dwLength <= > sizeof(WIN_CERTIFICATE) + sizeof(EFI_GUID)) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - dwLength > too small\n")); > + *LastAttemptStatus = 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 = LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + ImageOffset = sizeof(UINT64) + > + ImageAuthentication->AuthInfo.Hdr.dwLength; > + if (ImageSize <= ImageOffset) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - ImageSize > too small\n")); > + *LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + if (ImageAuthentication->AuthInfo.Hdr.wRevision != 0x0200) { > + DEBUG((DEBUG_ERROR, "ExecuteFmpAuthenticationHandler - wRevision: > 0x%02x, expect - 0x%02x\n", (UINTN)ImageAuthentication- > >AuthInfo.Hdr.wRevision, (UINTN)0x0200)); > + *LastAttemptStatus = LAST_ATTEMPT_STATUS_ERROR_INVALID_FORMAT; > + return RETURN_SECURITY_VIOLATION; > + } > + if (ImageAuthentication->AuthInfo.Hdr.wCertificateType != > 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 = 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 = > LAST_ATTEMPT_STATUS_ERROR_INCORRECT_VERSION; > + return RETURN_SECURITY_VIOLATION; > + } > + > + CertType = &ImageAuthentication->AuthInfo.CertType; > + DEBUG((DEBUG_INFO, "ExecuteFmpAuthenticationHandler - CertType: > + %g\n", CertType)); > + > + // > + // Search the match registered extract handler for the input guided > section. > + // > + for (Index = 0; Index < mNumberOfFmpAuthenticationHandler; Index ++) { > + if (CompareGuid (&mFmpAuthenticationHandlerGuidTable[Index], > CertType)) { > + // > + // Call the match handler to extract raw data for the input section > 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.<BR> # # > +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 = 0x00010005 > + BASE_NAME = FmpAuthenitcationLib > + MODULE_UNI_FILE = FmpAuthenitcationLib.uni > + FILE_GUID = 5011522C-7B0E-4ACB-8E30-9B1D133CF2E0 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = FmpAuthenitcationLib > + CONSTRUCTOR = FmpAuthenticationLibConstructor > + > +# > +# The following information is for reference only and not required by the > build > tools. > +# > +# VALID_ARCHITECTURES = 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.<BR> // > +// 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 "FmpAuthenitcation > 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 > [email protected]<mailto:[email protected]> > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected]<mailto:[email protected]> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
