Re: [patch] SecurityPkg: enhance secure boot Config Dxe & Time Based AuthVariable.

["Long, Qin" <qin.long@intel.com>]

Hi, Lubo,

Please use "mSha256OidValue", instead of "mHash256OidValue", to identify the SHA-256 cipher name.

Reviewed-by: Long Qin <qin.long@intel.com>


Best Regards & Thanks,
LONG, Qin

> -----Original Message-----
> From: Zhang, Lubo
> Sent: Thursday, February 16, 2017 11:05 PM
> To: edk2-devel@lists.01.org
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Long, Qin
> <qin.long@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [patch] SecurityPkg: enhance secure boot Config Dxe & Time Based
> AuthVariable.
> 
> V3: code clean up
> 
> prohibit Image SHA-1 hash option in SecureBootConfigDxe.
> Timebased Auth Variable driver should ensure AuthAlgorithm is SHA256
> before further verification
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Zhang Lubo <lubo.zhang@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Long Qin <qin.long@intel.com>
> Cc: Yao Jiewen <jiewen.yao@intel.com>
> ---
>  SecurityPkg/Library/AuthVariableLib/AuthService.c  | 27
> +++++++++++++++++++++-
>   .../Library/AuthVariableLib/AuthServiceInternal.h  |  4 +++-
>  .../SecureBootConfigDxe/SecureBootConfigImpl.c     | 14 ++++-------
>  .../SecureBootConfigDxe/SecureBootConfigImpl.h     |  8 ++-----
>  4 files changed, 35 insertions(+), 18 deletions(-)
> 
> diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c
> b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> index b013d42..1cdfadc 100644
> --- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
> +++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> @@ -16,11 +16,11 @@
> 
>    VerifyTimeBasedPayloadAndUpdate() and VerifyCounterBasedPayload()
> are sub function to do verification.
>    They will do basic validation for authentication data structure, then call
> crypto library
>    to verify the signature.
> 
> -Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2009 - 2017, 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
> 
> @@ -34,10 +34,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
>  //
>  // Public Exponent of RSA Key.
>  //
>  CONST UINT8 mRsaE[] = { 0x01, 0x00, 0x01 };
> 
> +CONST UINT8 mHash256OidValue[] = { 0x60, 0x86, 0x48, 0x01, 0x65, 0x03,
> +0x04, 0x02, 0x01 };
> +
>  //
>  // Requirement for different signature type which have been defined in
> UEFI spec.
>  // These data are used to perform SignatureList format check while setting
> PK/KEK variable.
>  //
>  EFI_SIGNATURE_ITEM mSupportSigItem[] = { @@ -2243,10 +2245,33 @@
> VerifyTimeBasedPayload (
>    //
>    SigData = CertData->AuthInfo.CertData;
>    SigDataSize = CertData->AuthInfo.Hdr.dwLength - (UINT32) (OFFSET_OF
> (WIN_CERTIFICATE_UEFI_GUID, CertData));
> 
>    //
> +  // SignedData.digestAlgorithms shall contain the digest algorithm
> + used when preparing the  // signature. Only a digest algorithm of SHA-256
> is accepted.
> +  //
> +  //    According to PKCS#7 Definition:
> +  //        SignedData ::= SEQUENCE {
> +  //            version Version,
> +  //            digestAlgorithms DigestAlgorithmIdentifiers,
> +  //            contentInfo ContentInfo,
> +  //            .... }
> +  //    The DigestAlgorithmIdentifiers can be used to determine the hash
> algorithm
> +  //    in VARIABLE_AUTHENTICATION_2 descriptor.
> +  //    This field has the fixed offset (+13) and be calculated based on two
> bytes of length encoding.
> +  //
> +  if ((Attributes &
> EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) {
> +    if (SigDataSize >= (13 + sizeof (mHash256OidValue))) {
> +      if (((*(SigData + 1) & TWO_BYTE_ENCODE) != TWO_BYTE_ENCODE) ||
> +           (CompareMem (SigData + 13, &mHash256OidValue, sizeof
> (mHash256OidValue)) != 0)) {
> +          return EFI_SECURITY_VIOLATION;
> +        }
> +    }
> +  }
> +
> +  //
>    // Find out the new data payload which follows Pkcs7 SignedData directly.
>    //
>    PayloadPtr = SigData + SigDataSize;
>    PayloadSize = DataSize - OFFSET_OF_AUTHINFO2_CERT_DATA - (UINTN)
> SigDataSize;
> 
> diff --git a/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h
> b/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h
> index e7c4bf0..e9b7cf3 100644
> --- a/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h
> +++ b/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h
> @@ -10,11 +10,11 @@
>       The whole SMM authentication variable design relies on the integrity of
> flash part and SMM.
>    which is assumed to be protected by platform.  All variable code and
> metadata in flash/SMM Memory
>    may not be modified without authorization. If platform fails to protect
> these resources,
>    the authentication service provided in this driver will be broken, and the
> behavior is undefined.
> 
> -Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2009 - 2017, 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
> 
> @@ -35,10 +35,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/PlatformSecureLib.h>
> 
>  #include <Guid/AuthenticatedVariableFormat.h>
>  #include <Guid/ImageAuthentication.h>
> 
> +#define TWO_BYTE_ENCODE       0x82
> +
>  ///
>  /// Struct to record signature requirement defined by UEFI spec.
>  /// For SigHeaderSize and SigDataSize, ((UINT32) ~0) means NO exact length
> requirement for this field.
>  ///
>  typedef struct {
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
> figImpl.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
> figImpl.c
> index 0d96185..6f58729 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
> figImpl.c
> +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
> +++ nfigImpl.c
> @@ -1,9 +1,9 @@
>  /** @file
>    HII Config Access protocol implementation of SecureBoot configuration
> module.
> 
> -Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2011 - 2017, 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
> 
> @@ -61,11 +61,10 @@ UINT8 mHashOidValue[] = {
>    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x02,   // OBJ_sha384
>    0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03,   // OBJ_sha512
>    };
> 
>  HASH_TABLE mHash[] = {
> -  { L"SHA1",   20, &mHashOidValue[8],  5, Sha1GetContextSize,   Sha1Init,
> Sha1Update,   Sha1Final  },
>    { L"SHA224", 28, &mHashOidValue[13], 9, NULL,                 NULL,       NULL,
> NULL       },
>    { L"SHA256", 32, &mHashOidValue[22], 9, Sha256GetContextSize,
> Sha256Init, Sha256Update, Sha256Final},
>    { L"SHA384", 48, &mHashOidValue[31], 9, Sha384GetContextSize,
> Sha384Init, Sha384Update, Sha384Final},
>    { L"SHA512", 64, &mHashOidValue[40], 9, Sha512GetContextSize,
> Sha512Init, Sha512Update, Sha512Final}  }; @@ -1784,26 +1783,21 @@
> HashPeImage (
> 
>    HashCtx       = NULL;
>    SectionHeader = NULL;
>    Status        = FALSE;
> 
> -  if ((HashAlg != HASHALG_SHA1) && (HashAlg != HASHALG_SHA256)) {
> +  if (HashAlg != HASHALG_SHA256) {
>      return FALSE;
>    }
> 
>    //
>    // Initialize context of hash.
>    //
>    ZeroMem (mImageDigest, MAX_DIGEST_SIZE);
> 
> -  if (HashAlg == HASHALG_SHA1) {
> -    mImageDigestSize  = SHA1_DIGEST_SIZE;
> -    mCertType         = gEfiCertSha1Guid;
> -  } else if (HashAlg == HASHALG_SHA256) {
> -    mImageDigestSize  = SHA256_DIGEST_SIZE;
> -    mCertType         = gEfiCertSha256Guid;
> -  }
> +  mImageDigestSize  = SHA256_DIGEST_SIZE;
> +  mCertType         = gEfiCertSha256Guid;
> 
>    CtxSize   = mHash[HashAlg].GetContextSize();
> 
>    HashCtx = AllocatePool (CtxSize);
>    ASSERT (HashCtx != NULL);
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
> figImpl.h
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
> figImpl.h
> index 5055a9e..bea9470 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon
> figImpl.h
> +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
> +++ nfigImpl.h
> @@ -1,10 +1,10 @@
>  /** @file
>    The header file of HII Config Access protocol implementation of SecureBoot
>    configuration module.
> 
> -Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2011 - 2017, 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
> 
> @@ -65,14 +65,11 @@ extern  EFI_IFR_GUID_LABEL         *mStartLabel;
>  extern  EFI_IFR_GUID_LABEL         *mEndLabel;
> 
>  #define MAX_CHAR              480
>  #define TWO_BYTE_ENCODE       0x82
> 
> -//
> -// SHA-1 digest size in bytes.
> -//
> -#define SHA1_DIGEST_SIZE    20
> +
>  //
>  // SHA-256 digest size in bytes
>  //
>  #define SHA256_DIGEST_SIZE  32
>  //
> @@ -92,11 +89,10 @@ extern  EFI_IFR_GUID_LABEL         *mEndLabel;
>  #define WIN_CERT_UEFI_RSA2048_SIZE               256
> 
>  //
>  // Support hash types
>  //
> -#define HASHALG_SHA1                           0x00000000
>  #define HASHALG_SHA224                         0x00000001
>  #define HASHALG_SHA256                         0x00000002
>  #define HASHALG_SHA384                         0x00000003
>  #define HASHALG_SHA512                         0x00000004
>  #define HASHALG_RAW                            0x00000005
> --
> 1.9.5.msysgit.1