[edk2-devel] [PATCH v2 2/2] SecurityPkg/SecureBootConfigDxe: Fix certificate lookup algorithm

[Min Xu]

On August 9, 2021 5:51 PM, Marvin Häuser wrote:
> The current certificate lookup code does not check the bounds of the
> authentication data before accessing it. Abort if the header cannot fit, and
> proceed to the next hashing algortihm if the OID of the current one exceeds the
> authentication data bounds.
> 
> Additionally move the two-byte encoding check out of the loop as the data is
> invariant.
> 
> Cc: Jiewen Yao <jiewen.yao at intel.com>
> Cc: Jian J Wang <jian.j.wang at intel.com>
> Cc: Min Xu <min.m.xu at intel.com>
> Cc: Vitaly Cheptsov <vit9696 at protonmail.com>
> Signed-off-by: Marvin Häuser <mhaeuser at posteo.de>
> ---
> 
> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigIm
> pl.c | 45 ++++++++++++--------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.c
> index 65a8188d6d03..fd7629f61862 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
> +++ nfigImpl.c
> @@ -1969,30 +1969,41 @@ HashPeImageByType (  {
> 
>    UINT8                     Index;
> 
>    WIN_CERTIFICATE_EFI_PKCS  *PkcsCertData;
> 
> +  UINT32                    AuthDataSize;
> 
> 
> 
>    PkcsCertData = (WIN_CERTIFICATE_EFI_PKCS *) (mImageBase + mSecDataDir-
> >Offset);
> 
> +  if (PkcsCertData->Hdr.dwLength <= sizeof (PkcsCertData->Hdr)) {
> 
> +    return EFI_UNSUPPORTED;
> 
> +  }
> 
> +
> 
> +  AuthDataSize = PkcsCertData->Hdr.dwLength - sizeof
> + (PkcsCertData->Hdr);
> 
> +  if (AuthDataSize < 32) {
> 
> +    return EFI_UNSUPPORTED;
> 
> +  }
> 
> +  //
> 
> +  // Check the Hash algorithm in PE/COFF Authenticode.
> 
> +  //    According to PKCS#7 Definition:
> 
> +  //        SignedData ::= SEQUENCE {
> 
> +  //            version Version,
> 
> +  //            digestAlgorithms DigestAlgorithmIdentifiers,
> 
> +  //            contentInfo ContentInfo,
> 
> +  //            .... }
> 
> +  //    The DigestAlgorithmIdentifiers can be used to determine the hash
> algorithm in PE/COFF hashing
> 
> +  //    This field has the fixed offset (+32) in final Authenticode ASN.1 data.
> 
> +  //    Fixed offset (+32) is calculated based on two bytes of length encoding.
> 
> +  //
> 
> +  if ((*(PkcsCertData->CertData + 1) & TWO_BYTE_ENCODE) !=
> + TWO_BYTE_ENCODE) {
> 
> +    //
> 
> +    // Only support two bytes of Long Form of Length Encoding.
> 
> +    //
> 
> +    return EFI_UNSUPPORTED;
> 
> +  }
> 
> 
> 
>    for (Index = 0; Index < HASHALG_MAX; Index++) {
> 
> -    //
> 
> -    // Check the Hash algorithm in PE/COFF Authenticode.
> 
> -    //    According to PKCS#7 Definition:
> 
> -    //        SignedData ::= SEQUENCE {
> 
> -    //            version Version,
> 
> -    //            digestAlgorithms DigestAlgorithmIdentifiers,
> 
> -    //            contentInfo ContentInfo,
> 
> -    //            .... }
> 
> -    //    The DigestAlgorithmIdentifiers can be used to determine the hash
> algorithm in PE/COFF hashing
> 
> -    //    This field has the fixed offset (+32) in final Authenticode ASN.1 data.
> 
> -    //    Fixed offset (+32) is calculated based on two bytes of length encoding.
> 
> -     //
> 
> -    if ((*(PkcsCertData->CertData + 1) & TWO_BYTE_ENCODE) !=
> TWO_BYTE_ENCODE) {
> 
> -      //
> 
> -      // Only support two bytes of Long Form of Length Encoding.
> 
> -      //
> 
> +    if (AuthDataSize - 32 < mHash[Index].OidLength) {
> 
>        continue;
> 
>      }
> 
> 
> 
> -    //
> 
>      if (CompareMem (PkcsCertData->CertData + 32, mHash[Index].OidValue,
> mHash[Index].OidLength) == 0) {
> 
>        break;
> 
>      }
> 
> --
> 2.31.1

Reviewed-by: Min Xu <min.m.xu at intel.com>

Thanks!
Xu, Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79131): https://edk2.groups.io/g/devel/message/79131
Mute This Topic: https://groups.io/mt/84764905/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-