public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v1] SecurityPkg: Improve initialization of default key variables.
@ 2021-10-06 12:25 Grzegorz Bernacki
  2021-12-14 15:43 ` Sunny Wang
  0 siblings, 1 reply; 2+ messages in thread
From: Grzegorz Bernacki @ 2021-10-06 12:25 UTC (permalink / raw)
  To: devel
  Cc: jiewen.yao, jian.j.wang, Samer.El-Haj-Mahmoud, sunny.Wang, mw,
	upstream, Grzegorz Bernacki

This commit allows to use data in  EFI_VARIABLE_AUTHENTICATION_2
structure format to initialize default secure boot variables.
It allows to use revocation list published by UEFI.

Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
---
 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c | 90 ++++++++++++--------
 1 file changed, 56 insertions(+), 34 deletions(-)

diff --git a/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c b/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
index ff65184713..1f8869b1d2 100644
--- a/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
+++ b/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
@@ -73,20 +73,19 @@ CreateSigList (
 
 /** Adds new signature list to signature database.
 
-  @param[in]      SigLists        A pointer to signature database.
-  @param[in]      SigListAppend  A signature list to be added.
-  @param[out]     *SigListOut     Created signature database.
+  @param[in,out]  SigLists        A pointer to signature database.
+  @param[in]      SigListAppend   A signature list to be added.
   @param[in, out] SigListsSize    A size of created signature database.
 
   @retval  EFI_SUCCESS           Signature List was added successfully.
   @retval  EFI_OUT_OF_RESOURCES  Failed to allocate memory.
+  @retval  EFI_INVALID_PARAMETER Invalid parameters.
 **/
 STATIC
 EFI_STATUS
 ConcatenateSigList (
-  IN  EFI_SIGNATURE_LIST *SigLists,
+  IN  EFI_SIGNATURE_LIST **SigLists,
   IN  EFI_SIGNATURE_LIST *SigListAppend,
-  OUT EFI_SIGNATURE_LIST **SigListOut,
   IN OUT UINTN           *SigListsSize
 )
 {
@@ -94,6 +93,10 @@ ConcatenateSigList (
   UINT8              *Offset;
   UINTN              NewSigListsSize;
 
+  if ((SigLists == NULL) || (SigListsSize == NULL) || (SigListAppend == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   NewSigListsSize = *SigListsSize + SigListAppend->SignatureListSize;
 
   TmpSigList = (EFI_SIGNATURE_LIST *) AllocateZeroPool (NewSigListsSize);
@@ -101,14 +104,17 @@ ConcatenateSigList (
     return EFI_OUT_OF_RESOURCES;
   }
 
-  CopyMem (TmpSigList, SigLists, *SigListsSize);
+  if (*SigLists != NULL) {
+    CopyMem (TmpSigList, *SigLists, *SigListsSize);
+    FreePool(*SigLists);
+  }
 
   Offset = (UINT8 *)TmpSigList;
   Offset += *SigListsSize;
   CopyMem ((VOID *)Offset, SigListAppend, SigListAppend->SignatureListSize);
 
   *SigListsSize = NewSigListsSize;
-  *SigListOut = TmpSigList;
+  *SigLists = TmpSigList;
   return EFI_SUCCESS;
 }
 
@@ -133,14 +139,15 @@ SecureBootFetchData (
     OUT EFI_SIGNATURE_LIST **SigListOut
 )
 {
+  EFI_VARIABLE_AUTHENTICATION_2  *Auth2;
   EFI_SIGNATURE_LIST *EfiSig;
   EFI_SIGNATURE_LIST *TmpEfiSig;
-  EFI_SIGNATURE_LIST *TmpEfiSig2;
   EFI_STATUS         Status;
   VOID               *Buffer;
   VOID               *RsaPubKey;
   UINTN               Size;
   UINTN               KeyIndex;
+  UINTN               SigListOffset;
 
 
   KeyIndex = 0;
@@ -154,42 +161,57 @@ SecureBootFetchData (
                &Buffer,
                &Size
                );
+    if (Status == EFI_NOT_FOUND && KeyIndex > 0) {
+      break;
+    } else if (EFI_ERROR(Status)) {
+      if (EfiSig != NULL) {
+        FreePool(EfiSig);
+      }
+      return EFI_INVALID_PARAMETER;
+    }
 
-    if (Status == EFI_SUCCESS) {
-      RsaPubKey = NULL;
-      if (RsaGetPublicKeyFromX509 (Buffer, Size, &RsaPubKey) == FALSE) {
-        DEBUG ((DEBUG_ERROR, "%a: Invalid key format: %d\n", __FUNCTION__, KeyIndex));
+    RsaPubKey = NULL;
+    Auth2 = (EFI_VARIABLE_AUTHENTICATION_2 *)Buffer;
+    if ((Auth2->AuthInfo.Hdr.wCertificateType == WIN_CERT_TYPE_EFI_GUID) &&
+        (CompareGuid (&gEfiCertPkcs7Guid, &Auth2->AuthInfo.CertType) == TRUE)) {
+
+      SigListOffset = Auth2->AuthInfo.Hdr.dwLength - (UINT32) (OFFSET_OF (WIN_CERTIFICATE_UEFI_GUID, CertData));
+      TmpEfiSig = (EFI_SIGNATURE_LIST *) &Auth2->AuthInfo.CertData[SigListOffset];
+      Size -= OFFSET_OF (EFI_VARIABLE_AUTHENTICATION_2, AuthInfo);
+      Size -= OFFSET_OF (WIN_CERTIFICATE_UEFI_GUID, CertData);
+      Size -= SigListOffset;
+
+      while (Size > 0) {
+        ConcatenateSigList (&EfiSig, TmpEfiSig, SigListsSize);
+        Size -= TmpEfiSig->SignatureListSize;
+        TmpEfiSig = (EFI_SIGNATURE_LIST *)((UINT8 *)TmpEfiSig + TmpEfiSig->SignatureListSize);
+      }
+    } else if (RsaGetPublicKeyFromX509 (Buffer, Size, &RsaPubKey) == TRUE) {
+      Status = CreateSigList (Buffer, Size, &TmpEfiSig);
+
+      if (EFI_ERROR(Status)) {
+        DEBUG ((DEBUG_ERROR, "%a: Cannot create a sig list\n", __FUNCTION__));
         if (EfiSig != NULL) {
           FreePool(EfiSig);
         }
         FreePool(Buffer);
-        return EFI_INVALID_PARAMETER;
-      }
 
-      Status = CreateSigList (Buffer, Size, &TmpEfiSig);
-
-      //
-      // Concatenate lists if more than one section found
-      //
-      if (KeyIndex == 0) {
-        EfiSig = TmpEfiSig;
-        *SigListsSize = TmpEfiSig->SignatureListSize;
-      } else {
-        ConcatenateSigList (EfiSig, TmpEfiSig, &TmpEfiSig2, SigListsSize);
-        FreePool (EfiSig);
-        FreePool (TmpEfiSig);
-        EfiSig = TmpEfiSig2;
+        return Status;
       }
 
-      KeyIndex++;
-      FreePool (Buffer);
-    } if (Status == EFI_NOT_FOUND) {
-      break;
+      ConcatenateSigList (&EfiSig, TmpEfiSig, SigListsSize);
+      FreePool (TmpEfiSig);
+    } else {
+      DEBUG ((DEBUG_ERROR, "%a: Invalid key format: %d\n", __FUNCTION__, KeyIndex));
+      if (EfiSig != NULL) {
+        FreePool(EfiSig);
+      }
+      FreePool(Buffer);
+      return EFI_INVALID_PARAMETER;
     }
-  };
 
-  if (KeyIndex == 0) {
-    return EFI_NOT_FOUND;
+    KeyIndex++;
+    FreePool (Buffer);
   }
 
   *SigListOut = EfiSig;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH v1] SecurityPkg: Improve initialization of default key variables.
  2021-10-06 12:25 [PATCH v1] SecurityPkg: Improve initialization of default key variables Grzegorz Bernacki
@ 2021-12-14 15:43 ` Sunny Wang
  0 siblings, 0 replies; 2+ messages in thread
From: Sunny Wang @ 2021-12-14 15:43 UTC (permalink / raw)
  To: Grzegorz Bernacki, devel@edk2.groups.io, Patrick Rudolph
  Cc: jiewen.yao@intel.com, jian.j.wang@intel.com, Samer El-Haj-Mahmoud,
	mw@semihalf.com, upstream@semihalf.com, Sunny Wang

Looks good to me.
Reviewed-by: Sunny Wang <sunny.wang@arm.com>

Hi Patrick,
This patch is to address your comment below. Could you give this patch a try on your side?
https://edk2.groups.io/g/devel/message/79766?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2CSecurityPkg%3A+Create+library+for+enrolling+Secure+Boot+variables.%2C20%2C2%2C0%2C84608356

Best Regards,
Sunny
-----Original Message-----
From: Grzegorz Bernacki <gjb@semihalf.com>
Sent: 06 October 2021 13:25
To: devel@edk2.groups.io
Cc: jiewen.yao@intel.com; jian.j.wang@intel.com; Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; mw@semihalf.com; upstream@semihalf.com; Grzegorz Bernacki <gjb@semihalf.com>
Subject: [PATCH v1] SecurityPkg: Improve initialization of default key variables.

This commit allows to use data in  EFI_VARIABLE_AUTHENTICATION_2
structure format to initialize default secure boot variables.
It allows to use revocation list published by UEFI.

Signed-off-by: Grzegorz Bernacki <gjb@semihalf.com>
---
 SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c | 90 ++++++++++++--------
 1 file changed, 56 insertions(+), 34 deletions(-)

diff --git a/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c b/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
index ff65184713..1f8869b1d2 100644
--- a/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
+++ b/SecurityPkg/Library/SecureBootVariableLib/SecureBootVariableLib.c
@@ -73,20 +73,19 @@ CreateSigList (

 /** Adds new signature list to signature database.

-  @param[in]      SigLists        A pointer to signature database.
-  @param[in]      SigListAppend  A signature list to be added.
-  @param[out]     *SigListOut     Created signature database.
+  @param[in,out]  SigLists        A pointer to signature database.
+  @param[in]      SigListAppend   A signature list to be added.
   @param[in, out] SigListsSize    A size of created signature database.

   @retval  EFI_SUCCESS           Signature List was added successfully.
   @retval  EFI_OUT_OF_RESOURCES  Failed to allocate memory.
+  @retval  EFI_INVALID_PARAMETER Invalid parameters.
 **/
 STATIC
 EFI_STATUS
 ConcatenateSigList (
-  IN  EFI_SIGNATURE_LIST *SigLists,
+  IN  EFI_SIGNATURE_LIST **SigLists,
   IN  EFI_SIGNATURE_LIST *SigListAppend,
-  OUT EFI_SIGNATURE_LIST **SigListOut,
   IN OUT UINTN           *SigListsSize
 )
 {
@@ -94,6 +93,10 @@ ConcatenateSigList (
   UINT8              *Offset;
   UINTN              NewSigListsSize;

+  if ((SigLists == NULL) || (SigListsSize == NULL) || (SigListAppend == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   NewSigListsSize = *SigListsSize + SigListAppend->SignatureListSize;

   TmpSigList = (EFI_SIGNATURE_LIST *) AllocateZeroPool (NewSigListsSize);
@@ -101,14 +104,17 @@ ConcatenateSigList (
     return EFI_OUT_OF_RESOURCES;
   }

-  CopyMem (TmpSigList, SigLists, *SigListsSize);
+  if (*SigLists != NULL) {
+    CopyMem (TmpSigList, *SigLists, *SigListsSize);
+    FreePool(*SigLists);
+  }

   Offset = (UINT8 *)TmpSigList;
   Offset += *SigListsSize;
   CopyMem ((VOID *)Offset, SigListAppend, SigListAppend->SignatureListSize);

   *SigListsSize = NewSigListsSize;
-  *SigListOut = TmpSigList;
+  *SigLists = TmpSigList;
   return EFI_SUCCESS;
 }

@@ -133,14 +139,15 @@ SecureBootFetchData (
     OUT EFI_SIGNATURE_LIST **SigListOut
 )
 {
+  EFI_VARIABLE_AUTHENTICATION_2  *Auth2;
   EFI_SIGNATURE_LIST *EfiSig;
   EFI_SIGNATURE_LIST *TmpEfiSig;
-  EFI_SIGNATURE_LIST *TmpEfiSig2;
   EFI_STATUS         Status;
   VOID               *Buffer;
   VOID               *RsaPubKey;
   UINTN               Size;
   UINTN               KeyIndex;
+  UINTN               SigListOffset;


   KeyIndex = 0;
@@ -154,42 +161,57 @@ SecureBootFetchData (
                &Buffer,
                &Size
                );
+    if (Status == EFI_NOT_FOUND && KeyIndex > 0) {
+      break;
+    } else if (EFI_ERROR(Status)) {
+      if (EfiSig != NULL) {
+        FreePool(EfiSig);
+      }
+      return EFI_INVALID_PARAMETER;
+    }

-    if (Status == EFI_SUCCESS) {
-      RsaPubKey = NULL;
-      if (RsaGetPublicKeyFromX509 (Buffer, Size, &RsaPubKey) == FALSE) {
-        DEBUG ((DEBUG_ERROR, "%a: Invalid key format: %d\n", __FUNCTION__, KeyIndex));
+    RsaPubKey = NULL;
+    Auth2 = (EFI_VARIABLE_AUTHENTICATION_2 *)Buffer;
+    if ((Auth2->AuthInfo.Hdr.wCertificateType == WIN_CERT_TYPE_EFI_GUID) &&
+        (CompareGuid (&gEfiCertPkcs7Guid, &Auth2->AuthInfo.CertType) == TRUE)) {
+
+      SigListOffset = Auth2->AuthInfo.Hdr.dwLength - (UINT32) (OFFSET_OF (WIN_CERTIFICATE_UEFI_GUID, CertData));
+      TmpEfiSig = (EFI_SIGNATURE_LIST *) &Auth2->AuthInfo.CertData[SigListOffset];
+      Size -= OFFSET_OF (EFI_VARIABLE_AUTHENTICATION_2, AuthInfo);
+      Size -= OFFSET_OF (WIN_CERTIFICATE_UEFI_GUID, CertData);
+      Size -= SigListOffset;
+
+      while (Size > 0) {
+        ConcatenateSigList (&EfiSig, TmpEfiSig, SigListsSize);
+        Size -= TmpEfiSig->SignatureListSize;
+        TmpEfiSig = (EFI_SIGNATURE_LIST *)((UINT8 *)TmpEfiSig + TmpEfiSig->SignatureListSize);
+      }
+    } else if (RsaGetPublicKeyFromX509 (Buffer, Size, &RsaPubKey) == TRUE) {
+      Status = CreateSigList (Buffer, Size, &TmpEfiSig);
+
+      if (EFI_ERROR(Status)) {
+        DEBUG ((DEBUG_ERROR, "%a: Cannot create a sig list\n", __FUNCTION__));
         if (EfiSig != NULL) {
           FreePool(EfiSig);
         }
         FreePool(Buffer);
-        return EFI_INVALID_PARAMETER;
-      }

-      Status = CreateSigList (Buffer, Size, &TmpEfiSig);
-
-      //
-      // Concatenate lists if more than one section found
-      //
-      if (KeyIndex == 0) {
-        EfiSig = TmpEfiSig;
-        *SigListsSize = TmpEfiSig->SignatureListSize;
-      } else {
-        ConcatenateSigList (EfiSig, TmpEfiSig, &TmpEfiSig2, SigListsSize);
-        FreePool (EfiSig);
-        FreePool (TmpEfiSig);
-        EfiSig = TmpEfiSig2;
+        return Status;
       }

-      KeyIndex++;
-      FreePool (Buffer);
-    } if (Status == EFI_NOT_FOUND) {
-      break;
+      ConcatenateSigList (&EfiSig, TmpEfiSig, SigListsSize);
+      FreePool (TmpEfiSig);
+    } else {
+      DEBUG ((DEBUG_ERROR, "%a: Invalid key format: %d\n", __FUNCTION__, KeyIndex));
+      if (EfiSig != NULL) {
+        FreePool(EfiSig);
+      }
+      FreePool(Buffer);
+      return EFI_INVALID_PARAMETER;
     }
-  };

-  if (KeyIndex == 0) {
-    return EFI_NOT_FOUND;
+    KeyIndex++;
+    FreePool (Buffer);
   }

   *SigListOut = EfiSig;
--
2.25.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply related	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-12-14 15:43 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-06 12:25 [PATCH v1] SecurityPkg: Improve initialization of default key variables Grzegorz Bernacki
2021-12-14 15:43 ` Sunny Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox