Re: [PATCH V2 09/50] MdeModulePkg/FmpAuthenticationLib: Add FmpAuthenticationLib instance.

[Sean Brogan <sean.brogan@microsoft.com>]

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: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 <michael.d.kinney@intel.com>; Feng Tian
> <feng.tian@intel.com>; Chao Zhang <chao.b.zhang@intel.com>; Liming Gao
> <liming.gao@intel.com>; Star Zeng <star.zeng@intel.com>
> 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 <feng.tian@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> Reviewed-by: Liming Gao <liming.gao@intel.com>
> ---
>  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
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel