public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable
@ 2017-10-12  9:14 Zhang, Chao B
  2017-10-13  6:50 ` Chen, Chen A
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Zhang, Chao B @ 2017-10-12  9:14 UTC (permalink / raw)
  To: edk2-devel; +Cc: qin.long, chen.a.chen, Chao Zhang

ECR1707 for UEFI2.7 clarified certificate management rule for private time-based
AuthVariable.Trusted cert rule changed from whole signer's certificate stack to
top-level issuer cert tbscertificate + SignerCert CN for better management compatibility.
Hash is used to reduce storage overhead.

Cc: Long Qin <qin.long@intel.com>
Cc: Chen Chen <chen.a.chen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang <chao.b.zhang@intel.com>
---
 SecurityPkg/Library/AuthVariableLib/AuthService.c | 208 ++++++++++++++++++----
 1 file changed, 171 insertions(+), 37 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index a37ec0b..7188ff6 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1530,6 +1530,85 @@ AuthServiceInternalCompareTimeStamp (
 }
 
 /**
+  Calculate SHA256 digest of SignerCert CommonName + ToplevelCert tbsCertificate
+  SignerCert and ToplevelCert are inside the signer certificate chain.
+
+  @param[in]  SignerCert          A pointer to SignerCert data.
+  @param[in]  SignerCertSize      Length of SignerCert data.
+  @param[in]  TopLevelCert        A pointer to TopLevelCert data.
+  @param[in]  TopLevelCertSize    Length of TopLevelCert data.
+  @param[out] Sha256Digest       Sha256 digest calculated.
+
+  @return EFI_ABORTED          Digest process failed.
+  @return EFI_SUCCESS          SHA256 Digest is succesfully calculated.
+
+**/
+EFI_STATUS
+CalculatePrivAuthVarSignChainSHA256Digest(
+  IN     UINT8            *SignerCert,
+  IN     UINTN            SignerCertSize,
+  IN     UINT8            *TopLevelCert,
+  IN     UINTN            TopLevelCertSize,
+  OUT    UINT8            *Sha256Digest
+  )
+{
+  UINT8                   *TbsCert;
+  UINTN                   TbsCertSize;
+  UINT8                   CertCommonName[128];
+  UINTN                   CertCommonNameSize;
+  BOOLEAN                 CryptoStatus;
+  EFI_STATUS              Status;
+
+  CertCommonNameSize = sizeof(CertCommonName);
+
+  //
+  // Get SignerCert CommonName
+  //
+  Status = X509GetCommonName(SignerCert, SignerCertSize, CertCommonName, &CertCommonNameSize);
+  if (EFI_ERROR(Status)) {
+    DEBUG((DEBUG_INFO, "%a Get SignerCert CommonName failed with status %x\n", __FUNCTION__, Status));
+    return EFI_ABORTED;
+  }
+
+  //
+  // Get TopLevelCert tbsCertificate
+  //
+  if (!X509GetTBSCert(TopLevelCert, TopLevelCertSize, &TbsCert, &TbsCertSize)) {
+    DEBUG((DEBUG_INFO, "%a Get Top-level Cert tbsCertificate failed!\n", __FUNCTION__));
+    return EFI_ABORTED;
+  }
+
+  //
+  // Digest SignerCert CN + TopLevelCert tbsCertificate
+  //
+  ZeroMem (Sha256Digest, SHA256_DIGEST_SIZE);
+  CryptoStatus = Sha256Init (mHashCtx);
+  if (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  //
+  // '\0' is forced in CertCommonName. No overflow issue
+  //
+  CryptoStatus = Sha256Update (mHashCtx, CertCommonName, AsciiStrLen((CHAR8 *)CertCommonName));
+  if (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  CryptoStatus = Sha256Update (mHashCtx, TbsCert, TbsCertSize);
+  if (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  CryptoStatus  = Sha256Final (mHashCtx, Sha256Digest);
+  if (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
   Find matching signer's certificates for common authenticated variable
   by corresponding VariableName and VendorGuid from "certdb" or "certdbv".
 
@@ -1872,13 +1951,16 @@ DeleteCertsFromDb (
 /**
   Insert signer's certificates for common authenticated variable with VariableName
   and VendorGuid in AUTH_CERT_DB_DATA to "certdb" or "certdbv" according to
-  time based authenticated variable attributes.
+  time based authenticated variable attributes. CertData is the SHA256 digest of
+  SignerCert CommonName + TopLevelCert tbsCertificate.
 
-  @param[in]  VariableName   Name of authenticated Variable.
-  @param[in]  VendorGuid     Vendor GUID of authenticated Variable.
-  @param[in]  Attributes     Attributes of authenticated variable.
-  @param[in]  CertData       Pointer to signer's certificates.
-  @param[in]  CertDataSize   Length of CertData in bytes.
+  @param[in]  VariableName      Name of authenticated Variable.
+  @param[in]  VendorGuid        Vendor GUID of authenticated Variable.
+  @param[in]  Attributes        Attributes of authenticated variable.
+  @param[in]  SignerCert        Signer certificate data.
+  @param[in]  SignerCertSize    Length of signer certificate.
+  @param[in]  TopLevelCert      Top-level certificate data.
+  @param[in]  TopLevelCertSize  Length of top-level certificate.
 
   @retval  EFI_INVALID_PARAMETER Any input parameter is invalid.
   @retval  EFI_ACCESS_DENIED     An AUTH_CERT_DB_DATA entry with same VariableName
@@ -1892,8 +1974,10 @@ InsertCertsToDb (
   IN     CHAR16           *VariableName,
   IN     EFI_GUID         *VendorGuid,
   IN     UINT32           Attributes,
-  IN     UINT8            *CertData,
-  IN     UINTN            CertDataSize
+  IN     UINT8            *SignerCert,
+  IN     UINTN            SignerCertSize,
+  IN     UINT8            *TopLevelCert,
+  IN     UINTN            TopLevelCertSize
   )
 {
   EFI_STATUS              Status;
@@ -1904,10 +1988,12 @@ InsertCertsToDb (
   UINT32                  NewCertDbSize;
   UINT32                  CertNodeSize;
   UINT32                  NameSize;
+  UINT32                  CertDataSize;
   AUTH_CERT_DB_DATA       *Ptr;
   CHAR16                  *DbName;
+  UINT8                   Sha256Digest[SHA256_DIGEST_SIZE];
 
-  if ((VariableName == NULL) || (VendorGuid == NULL) || (CertData == NULL)) {
+  if ((VariableName == NULL) || (VendorGuid == NULL) || (SignerCert == NULL) ||(TopLevelCert == NULL)) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -1967,11 +2053,24 @@ InsertCertsToDb (
   // Construct new data content of variable "certdb" or "certdbv".
   //
   NameSize      = (UINT32) StrLen (VariableName);
+  CertDataSize  = sizeof(Sha256Digest);
   CertNodeSize  = sizeof (AUTH_CERT_DB_DATA) + (UINT32) CertDataSize + NameSize * sizeof (CHAR16);
   NewCertDbSize = (UINT32) DataSize + CertNodeSize;
   if (NewCertDbSize > mMaxCertDbSize) {
     return EFI_OUT_OF_RESOURCES;
   }
+
+  Status = CalculatePrivAuthVarSignChainSHA256Digest(
+             SignerCert,
+             SignerCertSize,
+             TopLevelCert,
+             TopLevelCertSize,
+             Sha256Digest
+             );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
   NewCertDb     = (UINT8*) mCertDbStore;
 
   //
@@ -1999,7 +2098,7 @@ InsertCertsToDb (
 
   CopyMem (
     (UINT8 *) Ptr +  sizeof (AUTH_CERT_DB_DATA) + NameSize * sizeof (CHAR16),
-    CertData,
+    Sha256Digest,
     CertDataSize
     );
 
@@ -2181,19 +2280,27 @@ VerifyTimeBasedPayload (
   UINTN                            NewDataSize;
   UINT8                            *Buffer;
   UINTN                            Length;
-  UINT8                            *RootCert;
-  UINTN                            RootCertSize;
+  UINT8                            *TopLevelCert;
+  UINTN                            TopLevelCertSize;
+  UINT8                            *TrustedCert;
+  UINTN                            TrustedCertSize;
   UINT8                            *SignerCerts;
   UINTN                            CertStackSize;
   UINT8                            *CertsInCertDb;
   UINT32                           CertsSizeinDb;
+  UINT8                            Sha256Digest[SHA256_DIGEST_SIZE];
 
+  //
+  // 1. TopLevelCert is the top-level issuer certificate in signature Signer Cert Chain
+  // 2. TrustedCert is the certificate which firmware trusts. It could be saved in protected
+  //     storage or PK payload on PK init
+  //
   VerifyStatus           = FALSE;
   CertData               = NULL;
   NewData                = NULL;
   Attr                   = Attributes;
   SignerCerts            = NULL;
-  RootCert               = NULL;
+  TopLevelCert           = NULL;
   CertsInCertDb          = NULL;
 
   //
@@ -2325,8 +2432,8 @@ VerifyTimeBasedPayload (
                      SigDataSize,
                      &SignerCerts,
                      &CertStackSize,
-                     &RootCert,
-                     &RootCertSize
+                     &TopLevelCert,
+                     &TopLevelCertSize
                      );
     if (!VerifyStatus) {
       goto Exit;
@@ -2348,8 +2455,8 @@ VerifyTimeBasedPayload (
     }
     CertList = (EFI_SIGNATURE_LIST *) Data;
     Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
-    if ((RootCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
-        (CompareMem (Cert->SignatureData, RootCert, RootCertSize) != 0)) {
+    if ((TopLevelCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
+        (CompareMem (Cert->SignatureData, TopLevelCert, TopLevelCertSize) != 0)) {
       VerifyStatus = FALSE;
       goto Exit;
     }
@@ -2360,8 +2467,8 @@ VerifyTimeBasedPayload (
     VerifyStatus = Pkcs7Verify (
                      SigData,
                      SigDataSize,
-                     RootCert,
-                     RootCertSize,
+                     TopLevelCert,
+                     TopLevelCertSize,
                      NewData,
                      NewDataSize
                      );
@@ -2394,8 +2501,8 @@ VerifyTimeBasedPayload (
           //
           // Iterate each Signature Data Node within this CertList for a verify
           //
-          RootCert      = Cert->SignatureData;
-          RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
+          TrustedCert      = Cert->SignatureData;
+          TrustedCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
 
           //
           // Verify Pkcs7 SignedData via Pkcs7Verify library.
@@ -2403,8 +2510,8 @@ VerifyTimeBasedPayload (
           VerifyStatus = Pkcs7Verify (
                            SigData,
                            SigDataSize,
-                           RootCert,
-                           RootCertSize,
+                           TrustedCert,
+                           TrustedCertSize,
                            NewData,
                            NewDataSize
                            );
@@ -2428,8 +2535,8 @@ VerifyTimeBasedPayload (
                      SigDataSize,
                      &SignerCerts,
                      &CertStackSize,
-                     &RootCert,
-                     &RootCertSize
+                     &TopLevelCert,
+                     &TopLevelCertSize
                      );
     if (!VerifyStatus) {
       goto Exit;
@@ -2448,17 +2555,36 @@ VerifyTimeBasedPayload (
         goto Exit;
       }
 
-      if ((CertStackSize != CertsSizeinDb) ||
-          (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
-        goto Exit;
+      if (CertsSizeinDb == SHA256_DIGEST_SIZE) {
+        //
+        // Check hash of signer cert CommonName + Top-level issuer tbsCertificate against data in CertDb
+        //
+        Status = CalculatePrivAuthVarSignChainSHA256Digest(
+                   SignerCerts + sizeof(UINT8) + sizeof(UINT32),
+                   ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
+                   TopLevelCert,
+                   TopLevelCertSize,
+                   Sha256Digest
+                   );
+        if (EFI_ERROR(Status) || CompareMem (Sha256Digest, CertsInCertDb, CertsSizeinDb) != 0){
+          goto Exit;
+        }
+      } else {
+         //
+         // Keep backward compatible with previous solution which saves whole signer certs stack in CertDb
+         //
+         if ((CertStackSize != CertsSizeinDb) ||
+             (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
+              goto Exit;
+         }
       }
     }
 
     VerifyStatus = Pkcs7Verify (
                      SigData,
                      SigDataSize,
-                     RootCert,
-                     RootCertSize,
+                     TopLevelCert,
+                     TopLevelCertSize,
                      NewData,
                      NewDataSize
                      );
@@ -2468,9 +2594,17 @@ VerifyTimeBasedPayload (
 
     if ((OrgTimeStamp == NULL) && (PayloadSize != 0)) {
       //
-      // Insert signer's certificates when adding a new common authenticated variable.
+      // When adding a new common authenticated variable, always save Hash of cn of signer cert + tbsCertificate of Top-level issuer
       //
-      Status = InsertCertsToDb (VariableName, VendorGuid, Attributes, SignerCerts, CertStackSize);
+      Status = InsertCertsToDb (
+                 VariableName,
+                 VendorGuid,
+                 Attributes,
+                 SignerCerts + sizeof(UINT8) + sizeof(UINT32),
+                 ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
+                 TopLevelCert,
+                 TopLevelCertSize
+                 );
       if (EFI_ERROR (Status)) {
         VerifyStatus = FALSE;
         goto Exit;
@@ -2479,16 +2613,16 @@ VerifyTimeBasedPayload (
   } else if (AuthVarType == AuthVarTypePayload) {
     CertList = (EFI_SIGNATURE_LIST *) PayloadPtr;
     Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
-    RootCert      = Cert->SignatureData;
-    RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
+    TrustedCert     = Cert->SignatureData;
+    TrustedCertSize = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
     //
     // Verify Pkcs7 SignedData via Pkcs7Verify library.
     //
     VerifyStatus = Pkcs7Verify (
                      SigData,
                      SigDataSize,
-                     RootCert,
-                     RootCertSize,
+                     TrustedCert,
+                     TrustedCertSize,
                      NewData,
                      NewDataSize
                      );
@@ -2499,7 +2633,7 @@ VerifyTimeBasedPayload (
 Exit:
 
   if (AuthVarType == AuthVarTypePk || AuthVarType == AuthVarTypePriv) {
-    Pkcs7FreeSigners (RootCert);
+    Pkcs7FreeSigners (TopLevelCert);
     Pkcs7FreeSigners (SignerCerts);
   }
 
-- 
1.9.5.msysgit.1



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

* Re: [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable
  2017-10-12  9:14 [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable Zhang, Chao B
@ 2017-10-13  6:50 ` Chen, Chen A
  2017-10-13  8:43 ` Long, Qin
  2017-10-16  2:15 ` Gary Lin
  2 siblings, 0 replies; 5+ messages in thread
From: Chen, Chen A @ 2017-10-13  6:50 UTC (permalink / raw)
  To: Zhang, Chao B, edk2-devel@lists.01.org; +Cc: Long, Qin

Reviewed-by: Chen A Chen <chen.a.chen@intel.com>

-----Original Message-----
From: Zhang, Chao B 
Sent: Thursday, October 12, 2017 5:14 PM
To: edk2-devel@lists.01.org
Cc: Long, Qin <qin.long@intel.com>; Chen, Chen A <chen.a.chen@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Subject: [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable

ECR1707 for UEFI2.7 clarified certificate management rule for private time-based AuthVariable.Trusted cert rule changed from whole signer's certificate stack to top-level issuer cert tbscertificate + SignerCert CN for better management compatibility.
Hash is used to reduce storage overhead.

Cc: Long Qin <qin.long@intel.com>
Cc: Chen Chen <chen.a.chen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang <chao.b.zhang@intel.com>
---
 SecurityPkg/Library/AuthVariableLib/AuthService.c | 208 ++++++++++++++++++----
 1 file changed, 171 insertions(+), 37 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index a37ec0b..7188ff6 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1530,6 +1530,85 @@ AuthServiceInternalCompareTimeStamp (  }
 
 /**
+  Calculate SHA256 digest of SignerCert CommonName + ToplevelCert 
+ tbsCertificate  SignerCert and ToplevelCert are inside the signer certificate chain.
+
+  @param[in]  SignerCert          A pointer to SignerCert data.
+  @param[in]  SignerCertSize      Length of SignerCert data.
+  @param[in]  TopLevelCert        A pointer to TopLevelCert data.
+  @param[in]  TopLevelCertSize    Length of TopLevelCert data.
+  @param[out] Sha256Digest       Sha256 digest calculated.
+
+  @return EFI_ABORTED          Digest process failed.
+  @return EFI_SUCCESS          SHA256 Digest is succesfully calculated.
+
+**/
+EFI_STATUS
+CalculatePrivAuthVarSignChainSHA256Digest(
+  IN     UINT8            *SignerCert,
+  IN     UINTN            SignerCertSize,
+  IN     UINT8            *TopLevelCert,
+  IN     UINTN            TopLevelCertSize,
+  OUT    UINT8            *Sha256Digest
+  )
+{
+  UINT8                   *TbsCert;
+  UINTN                   TbsCertSize;
+  UINT8                   CertCommonName[128];
+  UINTN                   CertCommonNameSize;
+  BOOLEAN                 CryptoStatus;
+  EFI_STATUS              Status;
+
+  CertCommonNameSize = sizeof(CertCommonName);
+
+  //
+  // Get SignerCert CommonName
+  //
+  Status = X509GetCommonName(SignerCert, SignerCertSize, 
+ CertCommonName, &CertCommonNameSize);  if (EFI_ERROR(Status)) {
+    DEBUG((DEBUG_INFO, "%a Get SignerCert CommonName failed with status %x\n", __FUNCTION__, Status));
+    return EFI_ABORTED;
+  }
+
+  //
+  // Get TopLevelCert tbsCertificate
+  //
+  if (!X509GetTBSCert(TopLevelCert, TopLevelCertSize, &TbsCert, &TbsCertSize)) {
+    DEBUG((DEBUG_INFO, "%a Get Top-level Cert tbsCertificate failed!\n", __FUNCTION__));
+    return EFI_ABORTED;
+  }
+
+  //
+  // Digest SignerCert CN + TopLevelCert tbsCertificate  //  ZeroMem 
+ (Sha256Digest, SHA256_DIGEST_SIZE);  CryptoStatus = Sha256Init 
+ (mHashCtx);  if (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  //
+  // '\0' is forced in CertCommonName. No overflow issue  //  
+ CryptoStatus = Sha256Update (mHashCtx, CertCommonName, 
+ AsciiStrLen((CHAR8 *)CertCommonName));  if (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  CryptoStatus = Sha256Update (mHashCtx, TbsCert, TbsCertSize);  if 
+ (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  CryptoStatus  = Sha256Final (mHashCtx, Sha256Digest);  if 
+ (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
   Find matching signer's certificates for common authenticated variable
   by corresponding VariableName and VendorGuid from "certdb" or "certdbv".
 
@@ -1872,13 +1951,16 @@ DeleteCertsFromDb (
 /**
   Insert signer's certificates for common authenticated variable with VariableName
   and VendorGuid in AUTH_CERT_DB_DATA to "certdb" or "certdbv" according to
-  time based authenticated variable attributes.
+  time based authenticated variable attributes. CertData is the SHA256 
+ digest of  SignerCert CommonName + TopLevelCert tbsCertificate.
 
-  @param[in]  VariableName   Name of authenticated Variable.
-  @param[in]  VendorGuid     Vendor GUID of authenticated Variable.
-  @param[in]  Attributes     Attributes of authenticated variable.
-  @param[in]  CertData       Pointer to signer's certificates.
-  @param[in]  CertDataSize   Length of CertData in bytes.
+  @param[in]  VariableName      Name of authenticated Variable.
+  @param[in]  VendorGuid        Vendor GUID of authenticated Variable.
+  @param[in]  Attributes        Attributes of authenticated variable.
+  @param[in]  SignerCert        Signer certificate data.
+  @param[in]  SignerCertSize    Length of signer certificate.
+  @param[in]  TopLevelCert      Top-level certificate data.
+  @param[in]  TopLevelCertSize  Length of top-level certificate.
 
   @retval  EFI_INVALID_PARAMETER Any input parameter is invalid.
   @retval  EFI_ACCESS_DENIED     An AUTH_CERT_DB_DATA entry with same VariableName
@@ -1892,8 +1974,10 @@ InsertCertsToDb (
   IN     CHAR16           *VariableName,
   IN     EFI_GUID         *VendorGuid,
   IN     UINT32           Attributes,
-  IN     UINT8            *CertData,
-  IN     UINTN            CertDataSize
+  IN     UINT8            *SignerCert,
+  IN     UINTN            SignerCertSize,
+  IN     UINT8            *TopLevelCert,
+  IN     UINTN            TopLevelCertSize
   )
 {
   EFI_STATUS              Status;
@@ -1904,10 +1988,12 @@ InsertCertsToDb (
   UINT32                  NewCertDbSize;
   UINT32                  CertNodeSize;
   UINT32                  NameSize;
+  UINT32                  CertDataSize;
   AUTH_CERT_DB_DATA       *Ptr;
   CHAR16                  *DbName;
+  UINT8                   Sha256Digest[SHA256_DIGEST_SIZE];
 
-  if ((VariableName == NULL) || (VendorGuid == NULL) || (CertData == NULL)) {
+  if ((VariableName == NULL) || (VendorGuid == NULL) || (SignerCert == 
+ NULL) ||(TopLevelCert == NULL)) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -1967,11 +2053,24 @@ InsertCertsToDb (
   // Construct new data content of variable "certdb" or "certdbv".
   //
   NameSize      = (UINT32) StrLen (VariableName);
+  CertDataSize  = sizeof(Sha256Digest);
   CertNodeSize  = sizeof (AUTH_CERT_DB_DATA) + (UINT32) CertDataSize + NameSize * sizeof (CHAR16);
   NewCertDbSize = (UINT32) DataSize + CertNodeSize;
   if (NewCertDbSize > mMaxCertDbSize) {
     return EFI_OUT_OF_RESOURCES;
   }
+
+  Status = CalculatePrivAuthVarSignChainSHA256Digest(
+             SignerCert,
+             SignerCertSize,
+             TopLevelCert,
+             TopLevelCertSize,
+             Sha256Digest
+             );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
   NewCertDb     = (UINT8*) mCertDbStore;
 
   //
@@ -1999,7 +2098,7 @@ InsertCertsToDb (
 
   CopyMem (
     (UINT8 *) Ptr +  sizeof (AUTH_CERT_DB_DATA) + NameSize * sizeof (CHAR16),
-    CertData,
+    Sha256Digest,
     CertDataSize
     );
 
@@ -2181,19 +2280,27 @@ VerifyTimeBasedPayload (
   UINTN                            NewDataSize;
   UINT8                            *Buffer;
   UINTN                            Length;
-  UINT8                            *RootCert;
-  UINTN                            RootCertSize;
+  UINT8                            *TopLevelCert;
+  UINTN                            TopLevelCertSize;
+  UINT8                            *TrustedCert;
+  UINTN                            TrustedCertSize;
   UINT8                            *SignerCerts;
   UINTN                            CertStackSize;
   UINT8                            *CertsInCertDb;
   UINT32                           CertsSizeinDb;
+  UINT8                            Sha256Digest[SHA256_DIGEST_SIZE];
 
+  //
+  // 1. TopLevelCert is the top-level issuer certificate in signature 
+ Signer Cert Chain  // 2. TrustedCert is the certificate which firmware trusts. It could be saved in protected
+  //     storage or PK payload on PK init
+  //
   VerifyStatus           = FALSE;
   CertData               = NULL;
   NewData                = NULL;
   Attr                   = Attributes;
   SignerCerts            = NULL;
-  RootCert               = NULL;
+  TopLevelCert           = NULL;
   CertsInCertDb          = NULL;
 
   //
@@ -2325,8 +2432,8 @@ VerifyTimeBasedPayload (
                      SigDataSize,
                      &SignerCerts,
                      &CertStackSize,
-                     &RootCert,
-                     &RootCertSize
+                     &TopLevelCert,
+                     &TopLevelCertSize
                      );
     if (!VerifyStatus) {
       goto Exit;
@@ -2348,8 +2455,8 @@ VerifyTimeBasedPayload (
     }
     CertList = (EFI_SIGNATURE_LIST *) Data;
     Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
-    if ((RootCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
-        (CompareMem (Cert->SignatureData, RootCert, RootCertSize) != 0)) {
+    if ((TopLevelCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
+        (CompareMem (Cert->SignatureData, TopLevelCert, 
+ TopLevelCertSize) != 0)) {
       VerifyStatus = FALSE;
       goto Exit;
     }
@@ -2360,8 +2467,8 @@ VerifyTimeBasedPayload (
     VerifyStatus = Pkcs7Verify (
                      SigData,
                      SigDataSize,
-                     RootCert,
-                     RootCertSize,
+                     TopLevelCert,
+                     TopLevelCertSize,
                      NewData,
                      NewDataSize
                      );
@@ -2394,8 +2501,8 @@ VerifyTimeBasedPayload (
           //
           // Iterate each Signature Data Node within this CertList for a verify
           //
-          RootCert      = Cert->SignatureData;
-          RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
+          TrustedCert      = Cert->SignatureData;
+          TrustedCertSize  = CertList->SignatureSize - (sizeof 
+ (EFI_SIGNATURE_DATA) - 1);
 
           //
           // Verify Pkcs7 SignedData via Pkcs7Verify library.
@@ -2403,8 +2510,8 @@ VerifyTimeBasedPayload (
           VerifyStatus = Pkcs7Verify (
                            SigData,
                            SigDataSize,
-                           RootCert,
-                           RootCertSize,
+                           TrustedCert,
+                           TrustedCertSize,
                            NewData,
                            NewDataSize
                            );
@@ -2428,8 +2535,8 @@ VerifyTimeBasedPayload (
                      SigDataSize,
                      &SignerCerts,
                      &CertStackSize,
-                     &RootCert,
-                     &RootCertSize
+                     &TopLevelCert,
+                     &TopLevelCertSize
                      );
     if (!VerifyStatus) {
       goto Exit;
@@ -2448,17 +2555,36 @@ VerifyTimeBasedPayload (
         goto Exit;
       }
 
-      if ((CertStackSize != CertsSizeinDb) ||
-          (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
-        goto Exit;
+      if (CertsSizeinDb == SHA256_DIGEST_SIZE) {
+        //
+        // Check hash of signer cert CommonName + Top-level issuer tbsCertificate against data in CertDb
+        //
+        Status = CalculatePrivAuthVarSignChainSHA256Digest(
+                   SignerCerts + sizeof(UINT8) + sizeof(UINT32),
+                   ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
+                   TopLevelCert,
+                   TopLevelCertSize,
+                   Sha256Digest
+                   );
+        if (EFI_ERROR(Status) || CompareMem (Sha256Digest, CertsInCertDb, CertsSizeinDb) != 0){
+          goto Exit;
+        }
+      } else {
+         //
+         // Keep backward compatible with previous solution which saves whole signer certs stack in CertDb
+         //
+         if ((CertStackSize != CertsSizeinDb) ||
+             (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
+              goto Exit;
+         }
       }
     }
 
     VerifyStatus = Pkcs7Verify (
                      SigData,
                      SigDataSize,
-                     RootCert,
-                     RootCertSize,
+                     TopLevelCert,
+                     TopLevelCertSize,
                      NewData,
                      NewDataSize
                      );
@@ -2468,9 +2594,17 @@ VerifyTimeBasedPayload (
 
     if ((OrgTimeStamp == NULL) && (PayloadSize != 0)) {
       //
-      // Insert signer's certificates when adding a new common authenticated variable.
+      // When adding a new common authenticated variable, always save 
+ Hash of cn of signer cert + tbsCertificate of Top-level issuer
       //
-      Status = InsertCertsToDb (VariableName, VendorGuid, Attributes, SignerCerts, CertStackSize);
+      Status = InsertCertsToDb (
+                 VariableName,
+                 VendorGuid,
+                 Attributes,
+                 SignerCerts + sizeof(UINT8) + sizeof(UINT32),
+                 ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
+                 TopLevelCert,
+                 TopLevelCertSize
+                 );
       if (EFI_ERROR (Status)) {
         VerifyStatus = FALSE;
         goto Exit;
@@ -2479,16 +2613,16 @@ VerifyTimeBasedPayload (
   } else if (AuthVarType == AuthVarTypePayload) {
     CertList = (EFI_SIGNATURE_LIST *) PayloadPtr;
     Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
-    RootCert      = Cert->SignatureData;
-    RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
+    TrustedCert     = Cert->SignatureData;
+    TrustedCertSize = CertList->SignatureSize - (sizeof 
+ (EFI_SIGNATURE_DATA) - 1);
     //
     // Verify Pkcs7 SignedData via Pkcs7Verify library.
     //
     VerifyStatus = Pkcs7Verify (
                      SigData,
                      SigDataSize,
-                     RootCert,
-                     RootCertSize,
+                     TrustedCert,
+                     TrustedCertSize,
                      NewData,
                      NewDataSize
                      );
@@ -2499,7 +2633,7 @@ VerifyTimeBasedPayload (
 Exit:
 
   if (AuthVarType == AuthVarTypePk || AuthVarType == AuthVarTypePriv) {
-    Pkcs7FreeSigners (RootCert);
+    Pkcs7FreeSigners (TopLevelCert);
     Pkcs7FreeSigners (SignerCerts);
   }
 
--
1.9.5.msysgit.1



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

* Re: [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable
  2017-10-12  9:14 [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable Zhang, Chao B
  2017-10-13  6:50 ` Chen, Chen A
@ 2017-10-13  8:43 ` Long, Qin
  2017-10-16  2:15 ` Gary Lin
  2 siblings, 0 replies; 5+ messages in thread
From: Long, Qin @ 2017-10-13  8:43 UTC (permalink / raw)
  To: Zhang, Chao B, edk2-devel@lists.01.org

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


Best Regards & Thanks,
LONG, Qin

-----Original Message-----
From: Zhang, Chao B 
Sent: Thursday, October 12, 2017 5:14 PM
To: edk2-devel@lists.01.org
Cc: Long, Qin <qin.long@intel.com>; Chen, Chen A <chen.a.chen@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Subject: [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable

ECR1707 for UEFI2.7 clarified certificate management rule for private time-based AuthVariable.Trusted cert rule changed from whole signer's certificate stack to top-level issuer cert tbscertificate + SignerCert CN for better management compatibility.
Hash is used to reduce storage overhead.

Cc: Long Qin <qin.long@intel.com>
Cc: Chen Chen <chen.a.chen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang <chao.b.zhang@intel.com>
---
 SecurityPkg/Library/AuthVariableLib/AuthService.c | 208 ++++++++++++++++++----
 1 file changed, 171 insertions(+), 37 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index a37ec0b..7188ff6 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1530,6 +1530,85 @@ AuthServiceInternalCompareTimeStamp (  }
 
 /**
+  Calculate SHA256 digest of SignerCert CommonName + ToplevelCert 
+ tbsCertificate  SignerCert and ToplevelCert are inside the signer certificate chain.
+
+  @param[in]  SignerCert          A pointer to SignerCert data.
+  @param[in]  SignerCertSize      Length of SignerCert data.
+  @param[in]  TopLevelCert        A pointer to TopLevelCert data.
+  @param[in]  TopLevelCertSize    Length of TopLevelCert data.
+  @param[out] Sha256Digest       Sha256 digest calculated.
+
+  @return EFI_ABORTED          Digest process failed.
+  @return EFI_SUCCESS          SHA256 Digest is succesfully calculated.
+
+**/
+EFI_STATUS
+CalculatePrivAuthVarSignChainSHA256Digest(
+  IN     UINT8            *SignerCert,
+  IN     UINTN            SignerCertSize,
+  IN     UINT8            *TopLevelCert,
+  IN     UINTN            TopLevelCertSize,
+  OUT    UINT8            *Sha256Digest
+  )
+{
+  UINT8                   *TbsCert;
+  UINTN                   TbsCertSize;
+  UINT8                   CertCommonName[128];
+  UINTN                   CertCommonNameSize;
+  BOOLEAN                 CryptoStatus;
+  EFI_STATUS              Status;
+
+  CertCommonNameSize = sizeof(CertCommonName);
+
+  //
+  // Get SignerCert CommonName
+  //
+  Status = X509GetCommonName(SignerCert, SignerCertSize, 
+ CertCommonName, &CertCommonNameSize);  if (EFI_ERROR(Status)) {
+    DEBUG((DEBUG_INFO, "%a Get SignerCert CommonName failed with status %x\n", __FUNCTION__, Status));
+    return EFI_ABORTED;
+  }
+
+  //
+  // Get TopLevelCert tbsCertificate
+  //
+  if (!X509GetTBSCert(TopLevelCert, TopLevelCertSize, &TbsCert, &TbsCertSize)) {
+    DEBUG((DEBUG_INFO, "%a Get Top-level Cert tbsCertificate failed!\n", __FUNCTION__));
+    return EFI_ABORTED;
+  }
+
+  //
+  // Digest SignerCert CN + TopLevelCert tbsCertificate  //  ZeroMem 
+ (Sha256Digest, SHA256_DIGEST_SIZE);  CryptoStatus = Sha256Init 
+ (mHashCtx);  if (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  //
+  // '\0' is forced in CertCommonName. No overflow issue  //  
+ CryptoStatus = Sha256Update (mHashCtx, CertCommonName, 
+ AsciiStrLen((CHAR8 *)CertCommonName));  if (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  CryptoStatus = Sha256Update (mHashCtx, TbsCert, TbsCertSize);  if 
+ (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  CryptoStatus  = Sha256Final (mHashCtx, Sha256Digest);  if 
+ (!CryptoStatus) {
+    return EFI_ABORTED;
+  }
+
+  return EFI_SUCCESS;
+}
+
+/**
   Find matching signer's certificates for common authenticated variable
   by corresponding VariableName and VendorGuid from "certdb" or "certdbv".
 
@@ -1872,13 +1951,16 @@ DeleteCertsFromDb (
 /**
   Insert signer's certificates for common authenticated variable with VariableName
   and VendorGuid in AUTH_CERT_DB_DATA to "certdb" or "certdbv" according to
-  time based authenticated variable attributes.
+  time based authenticated variable attributes. CertData is the SHA256 
+ digest of  SignerCert CommonName + TopLevelCert tbsCertificate.
 
-  @param[in]  VariableName   Name of authenticated Variable.
-  @param[in]  VendorGuid     Vendor GUID of authenticated Variable.
-  @param[in]  Attributes     Attributes of authenticated variable.
-  @param[in]  CertData       Pointer to signer's certificates.
-  @param[in]  CertDataSize   Length of CertData in bytes.
+  @param[in]  VariableName      Name of authenticated Variable.
+  @param[in]  VendorGuid        Vendor GUID of authenticated Variable.
+  @param[in]  Attributes        Attributes of authenticated variable.
+  @param[in]  SignerCert        Signer certificate data.
+  @param[in]  SignerCertSize    Length of signer certificate.
+  @param[in]  TopLevelCert      Top-level certificate data.
+  @param[in]  TopLevelCertSize  Length of top-level certificate.
 
   @retval  EFI_INVALID_PARAMETER Any input parameter is invalid.
   @retval  EFI_ACCESS_DENIED     An AUTH_CERT_DB_DATA entry with same VariableName
@@ -1892,8 +1974,10 @@ InsertCertsToDb (
   IN     CHAR16           *VariableName,
   IN     EFI_GUID         *VendorGuid,
   IN     UINT32           Attributes,
-  IN     UINT8            *CertData,
-  IN     UINTN            CertDataSize
+  IN     UINT8            *SignerCert,
+  IN     UINTN            SignerCertSize,
+  IN     UINT8            *TopLevelCert,
+  IN     UINTN            TopLevelCertSize
   )
 {
   EFI_STATUS              Status;
@@ -1904,10 +1988,12 @@ InsertCertsToDb (
   UINT32                  NewCertDbSize;
   UINT32                  CertNodeSize;
   UINT32                  NameSize;
+  UINT32                  CertDataSize;
   AUTH_CERT_DB_DATA       *Ptr;
   CHAR16                  *DbName;
+  UINT8                   Sha256Digest[SHA256_DIGEST_SIZE];
 
-  if ((VariableName == NULL) || (VendorGuid == NULL) || (CertData == NULL)) {
+  if ((VariableName == NULL) || (VendorGuid == NULL) || (SignerCert == 
+ NULL) ||(TopLevelCert == NULL)) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -1967,11 +2053,24 @@ InsertCertsToDb (
   // Construct new data content of variable "certdb" or "certdbv".
   //
   NameSize      = (UINT32) StrLen (VariableName);
+  CertDataSize  = sizeof(Sha256Digest);
   CertNodeSize  = sizeof (AUTH_CERT_DB_DATA) + (UINT32) CertDataSize + NameSize * sizeof (CHAR16);
   NewCertDbSize = (UINT32) DataSize + CertNodeSize;
   if (NewCertDbSize > mMaxCertDbSize) {
     return EFI_OUT_OF_RESOURCES;
   }
+
+  Status = CalculatePrivAuthVarSignChainSHA256Digest(
+             SignerCert,
+             SignerCertSize,
+             TopLevelCert,
+             TopLevelCertSize,
+             Sha256Digest
+             );
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
   NewCertDb     = (UINT8*) mCertDbStore;
 
   //
@@ -1999,7 +2098,7 @@ InsertCertsToDb (
 
   CopyMem (
     (UINT8 *) Ptr +  sizeof (AUTH_CERT_DB_DATA) + NameSize * sizeof (CHAR16),
-    CertData,
+    Sha256Digest,
     CertDataSize
     );
 
@@ -2181,19 +2280,27 @@ VerifyTimeBasedPayload (
   UINTN                            NewDataSize;
   UINT8                            *Buffer;
   UINTN                            Length;
-  UINT8                            *RootCert;
-  UINTN                            RootCertSize;
+  UINT8                            *TopLevelCert;
+  UINTN                            TopLevelCertSize;
+  UINT8                            *TrustedCert;
+  UINTN                            TrustedCertSize;
   UINT8                            *SignerCerts;
   UINTN                            CertStackSize;
   UINT8                            *CertsInCertDb;
   UINT32                           CertsSizeinDb;
+  UINT8                            Sha256Digest[SHA256_DIGEST_SIZE];
 
+  //
+  // 1. TopLevelCert is the top-level issuer certificate in signature 
+ Signer Cert Chain  // 2. TrustedCert is the certificate which firmware trusts. It could be saved in protected
+  //     storage or PK payload on PK init
+  //
   VerifyStatus           = FALSE;
   CertData               = NULL;
   NewData                = NULL;
   Attr                   = Attributes;
   SignerCerts            = NULL;
-  RootCert               = NULL;
+  TopLevelCert           = NULL;
   CertsInCertDb          = NULL;
 
   //
@@ -2325,8 +2432,8 @@ VerifyTimeBasedPayload (
                      SigDataSize,
                      &SignerCerts,
                      &CertStackSize,
-                     &RootCert,
-                     &RootCertSize
+                     &TopLevelCert,
+                     &TopLevelCertSize
                      );
     if (!VerifyStatus) {
       goto Exit;
@@ -2348,8 +2455,8 @@ VerifyTimeBasedPayload (
     }
     CertList = (EFI_SIGNATURE_LIST *) Data;
     Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
-    if ((RootCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
-        (CompareMem (Cert->SignatureData, RootCert, RootCertSize) != 0)) {
+    if ((TopLevelCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
+        (CompareMem (Cert->SignatureData, TopLevelCert, 
+ TopLevelCertSize) != 0)) {
       VerifyStatus = FALSE;
       goto Exit;
     }
@@ -2360,8 +2467,8 @@ VerifyTimeBasedPayload (
     VerifyStatus = Pkcs7Verify (
                      SigData,
                      SigDataSize,
-                     RootCert,
-                     RootCertSize,
+                     TopLevelCert,
+                     TopLevelCertSize,
                      NewData,
                      NewDataSize
                      );
@@ -2394,8 +2501,8 @@ VerifyTimeBasedPayload (
           //
           // Iterate each Signature Data Node within this CertList for a verify
           //
-          RootCert      = Cert->SignatureData;
-          RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
+          TrustedCert      = Cert->SignatureData;
+          TrustedCertSize  = CertList->SignatureSize - (sizeof 
+ (EFI_SIGNATURE_DATA) - 1);
 
           //
           // Verify Pkcs7 SignedData via Pkcs7Verify library.
@@ -2403,8 +2510,8 @@ VerifyTimeBasedPayload (
           VerifyStatus = Pkcs7Verify (
                            SigData,
                            SigDataSize,
-                           RootCert,
-                           RootCertSize,
+                           TrustedCert,
+                           TrustedCertSize,
                            NewData,
                            NewDataSize
                            );
@@ -2428,8 +2535,8 @@ VerifyTimeBasedPayload (
                      SigDataSize,
                      &SignerCerts,
                      &CertStackSize,
-                     &RootCert,
-                     &RootCertSize
+                     &TopLevelCert,
+                     &TopLevelCertSize
                      );
     if (!VerifyStatus) {
       goto Exit;
@@ -2448,17 +2555,36 @@ VerifyTimeBasedPayload (
         goto Exit;
       }
 
-      if ((CertStackSize != CertsSizeinDb) ||
-          (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
-        goto Exit;
+      if (CertsSizeinDb == SHA256_DIGEST_SIZE) {
+        //
+        // Check hash of signer cert CommonName + Top-level issuer tbsCertificate against data in CertDb
+        //
+        Status = CalculatePrivAuthVarSignChainSHA256Digest(
+                   SignerCerts + sizeof(UINT8) + sizeof(UINT32),
+                   ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
+                   TopLevelCert,
+                   TopLevelCertSize,
+                   Sha256Digest
+                   );
+        if (EFI_ERROR(Status) || CompareMem (Sha256Digest, CertsInCertDb, CertsSizeinDb) != 0){
+          goto Exit;
+        }
+      } else {
+         //
+         // Keep backward compatible with previous solution which saves whole signer certs stack in CertDb
+         //
+         if ((CertStackSize != CertsSizeinDb) ||
+             (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
+              goto Exit;
+         }
       }
     }
 
     VerifyStatus = Pkcs7Verify (
                      SigData,
                      SigDataSize,
-                     RootCert,
-                     RootCertSize,
+                     TopLevelCert,
+                     TopLevelCertSize,
                      NewData,
                      NewDataSize
                      );
@@ -2468,9 +2594,17 @@ VerifyTimeBasedPayload (
 
     if ((OrgTimeStamp == NULL) && (PayloadSize != 0)) {
       //
-      // Insert signer's certificates when adding a new common authenticated variable.
+      // When adding a new common authenticated variable, always save 
+ Hash of cn of signer cert + tbsCertificate of Top-level issuer
       //
-      Status = InsertCertsToDb (VariableName, VendorGuid, Attributes, SignerCerts, CertStackSize);
+      Status = InsertCertsToDb (
+                 VariableName,
+                 VendorGuid,
+                 Attributes,
+                 SignerCerts + sizeof(UINT8) + sizeof(UINT32),
+                 ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
+                 TopLevelCert,
+                 TopLevelCertSize
+                 );
       if (EFI_ERROR (Status)) {
         VerifyStatus = FALSE;
         goto Exit;
@@ -2479,16 +2613,16 @@ VerifyTimeBasedPayload (
   } else if (AuthVarType == AuthVarTypePayload) {
     CertList = (EFI_SIGNATURE_LIST *) PayloadPtr;
     Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
-    RootCert      = Cert->SignatureData;
-    RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
+    TrustedCert     = Cert->SignatureData;
+    TrustedCertSize = CertList->SignatureSize - (sizeof 
+ (EFI_SIGNATURE_DATA) - 1);
     //
     // Verify Pkcs7 SignedData via Pkcs7Verify library.
     //
     VerifyStatus = Pkcs7Verify (
                      SigData,
                      SigDataSize,
-                     RootCert,
-                     RootCertSize,
+                     TrustedCert,
+                     TrustedCertSize,
                      NewData,
                      NewDataSize
                      );
@@ -2499,7 +2633,7 @@ VerifyTimeBasedPayload (
 Exit:
 
   if (AuthVarType == AuthVarTypePk || AuthVarType == AuthVarTypePriv) {
-    Pkcs7FreeSigners (RootCert);
+    Pkcs7FreeSigners (TopLevelCert);
     Pkcs7FreeSigners (SignerCerts);
   }
 
--
1.9.5.msysgit.1



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

* Re: [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable
  2017-10-12  9:14 [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable Zhang, Chao B
  2017-10-13  6:50 ` Chen, Chen A
  2017-10-13  8:43 ` Long, Qin
@ 2017-10-16  2:15 ` Gary Lin
  2017-10-16 14:09   ` Zhang, Chao B
  2 siblings, 1 reply; 5+ messages in thread
From: Gary Lin @ 2017-10-16  2:15 UTC (permalink / raw)
  To: Zhang, Chao B; +Cc: edk2-devel, qin.long

On Thu, Oct 12, 2017 at 05:14:25PM +0800, Zhang, Chao B wrote:
> ECR1707 for UEFI2.7 clarified certificate management rule for private time-based
> AuthVariable.Trusted cert rule changed from whole signer's certificate stack to
> top-level issuer cert tbscertificate + SignerCert CN for better management compatibility.
> Hash is used to reduce storage overhead.
> 
> Cc: Long Qin <qin.long@intel.com>
> Cc: Chen Chen <chen.a.chen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chao Zhang <chao.b.zhang@intel.com>
> ---
>  SecurityPkg/Library/AuthVariableLib/AuthService.c | 208 ++++++++++++++++++----
>  1 file changed, 171 insertions(+), 37 deletions(-)
> 
> diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> index a37ec0b..7188ff6 100644
> --- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
> +++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> @@ -1530,6 +1530,85 @@ AuthServiceInternalCompareTimeStamp (
>  }
>  
>  /**
> +  Calculate SHA256 digest of SignerCert CommonName + ToplevelCert tbsCertificate
> +  SignerCert and ToplevelCert are inside the signer certificate chain.
> +
> +  @param[in]  SignerCert          A pointer to SignerCert data.
> +  @param[in]  SignerCertSize      Length of SignerCert data.
> +  @param[in]  TopLevelCert        A pointer to TopLevelCert data.
> +  @param[in]  TopLevelCertSize    Length of TopLevelCert data.
> +  @param[out] Sha256Digest       Sha256 digest calculated.
> +
> +  @return EFI_ABORTED          Digest process failed.
> +  @return EFI_SUCCESS          SHA256 Digest is succesfully calculated.
> +
> +**/
> +EFI_STATUS
> +CalculatePrivAuthVarSignChainSHA256Digest(
> +  IN     UINT8            *SignerCert,
> +  IN     UINTN            SignerCertSize,
> +  IN     UINT8            *TopLevelCert,
> +  IN     UINTN            TopLevelCertSize,
> +  OUT    UINT8            *Sha256Digest
> +  )
> +{
> +  UINT8                   *TbsCert;
> +  UINTN                   TbsCertSize;
> +  UINT8                   CertCommonName[128];
> +  UINTN                   CertCommonNameSize;
> +  BOOLEAN                 CryptoStatus;
> +  EFI_STATUS              Status;
> +
> +  CertCommonNameSize = sizeof(CertCommonName);
> +
> +  //
> +  // Get SignerCert CommonName
> +  //
> +  Status = X509GetCommonName(SignerCert, SignerCertSize, CertCommonName, &CertCommonNameSize);
An error showed while building ovmf with gcc 7.2.1:

SecurityPkg/Library/AuthVariableLib/AuthService.c: In function ‘CalculatePrivAuthVarSignChainSHA256Digest’:
SecurityPkg/Library/AuthVariableLib/AuthService.c:1567:58: error: pointer targets in passing argument 3 of ‘X509GetCommonName’ differ in signedness [-Werror=pointer-sign]
   Status = X509GetCommonName(SignerCert, SignerCertSize, CertCommonName, &CertCommonNameSize);
                                                          ^~~~~~~~~~~~~~
In file included from SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h:34:0,
                 from SecurityPkg/Library/AuthVariableLib/AuthService.c:32:
CryptoPkg/Include/Library/BaseCryptLib.h:2202:1: note: expected ‘CHAR8 * {aka char *}’ but argument is of type ‘UINT8 * {aka unsigned char *}’
 X509GetCommonName (
 ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Changing the type of CertCommonName from UINT8 to CHAR8 fixes the
warning.

Cheers,

Gary Lin

> +  if (EFI_ERROR(Status)) {
> +    DEBUG((DEBUG_INFO, "%a Get SignerCert CommonName failed with status %x\n", __FUNCTION__, Status));
> +    return EFI_ABORTED;
> +  }
> +
> +  //
> +  // Get TopLevelCert tbsCertificate
> +  //
> +  if (!X509GetTBSCert(TopLevelCert, TopLevelCertSize, &TbsCert, &TbsCertSize)) {
> +    DEBUG((DEBUG_INFO, "%a Get Top-level Cert tbsCertificate failed!\n", __FUNCTION__));
> +    return EFI_ABORTED;
> +  }
> +
> +  //
> +  // Digest SignerCert CN + TopLevelCert tbsCertificate
> +  //
> +  ZeroMem (Sha256Digest, SHA256_DIGEST_SIZE);
> +  CryptoStatus = Sha256Init (mHashCtx);
> +  if (!CryptoStatus) {
> +    return EFI_ABORTED;
> +  }
> +
> +  //
> +  // '\0' is forced in CertCommonName. No overflow issue
> +  //
> +  CryptoStatus = Sha256Update (mHashCtx, CertCommonName, AsciiStrLen((CHAR8 *)CertCommonName));
> +  if (!CryptoStatus) {
> +    return EFI_ABORTED;
> +  }
> +
> +  CryptoStatus = Sha256Update (mHashCtx, TbsCert, TbsCertSize);
> +  if (!CryptoStatus) {
> +    return EFI_ABORTED;
> +  }
> +
> +  CryptoStatus  = Sha256Final (mHashCtx, Sha256Digest);
> +  if (!CryptoStatus) {
> +    return EFI_ABORTED;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Find matching signer's certificates for common authenticated variable
>    by corresponding VariableName and VendorGuid from "certdb" or "certdbv".
>  
> @@ -1872,13 +1951,16 @@ DeleteCertsFromDb (
>  /**
>    Insert signer's certificates for common authenticated variable with VariableName
>    and VendorGuid in AUTH_CERT_DB_DATA to "certdb" or "certdbv" according to
> -  time based authenticated variable attributes.
> +  time based authenticated variable attributes. CertData is the SHA256 digest of
> +  SignerCert CommonName + TopLevelCert tbsCertificate.
>  
> -  @param[in]  VariableName   Name of authenticated Variable.
> -  @param[in]  VendorGuid     Vendor GUID of authenticated Variable.
> -  @param[in]  Attributes     Attributes of authenticated variable.
> -  @param[in]  CertData       Pointer to signer's certificates.
> -  @param[in]  CertDataSize   Length of CertData in bytes.
> +  @param[in]  VariableName      Name of authenticated Variable.
> +  @param[in]  VendorGuid        Vendor GUID of authenticated Variable.
> +  @param[in]  Attributes        Attributes of authenticated variable.
> +  @param[in]  SignerCert        Signer certificate data.
> +  @param[in]  SignerCertSize    Length of signer certificate.
> +  @param[in]  TopLevelCert      Top-level certificate data.
> +  @param[in]  TopLevelCertSize  Length of top-level certificate.
>  
>    @retval  EFI_INVALID_PARAMETER Any input parameter is invalid.
>    @retval  EFI_ACCESS_DENIED     An AUTH_CERT_DB_DATA entry with same VariableName
> @@ -1892,8 +1974,10 @@ InsertCertsToDb (
>    IN     CHAR16           *VariableName,
>    IN     EFI_GUID         *VendorGuid,
>    IN     UINT32           Attributes,
> -  IN     UINT8            *CertData,
> -  IN     UINTN            CertDataSize
> +  IN     UINT8            *SignerCert,
> +  IN     UINTN            SignerCertSize,
> +  IN     UINT8            *TopLevelCert,
> +  IN     UINTN            TopLevelCertSize
>    )
>  {
>    EFI_STATUS              Status;
> @@ -1904,10 +1988,12 @@ InsertCertsToDb (
>    UINT32                  NewCertDbSize;
>    UINT32                  CertNodeSize;
>    UINT32                  NameSize;
> +  UINT32                  CertDataSize;
>    AUTH_CERT_DB_DATA       *Ptr;
>    CHAR16                  *DbName;
> +  UINT8                   Sha256Digest[SHA256_DIGEST_SIZE];
>  
> -  if ((VariableName == NULL) || (VendorGuid == NULL) || (CertData == NULL)) {
> +  if ((VariableName == NULL) || (VendorGuid == NULL) || (SignerCert == NULL) ||(TopLevelCert == NULL)) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> @@ -1967,11 +2053,24 @@ InsertCertsToDb (
>    // Construct new data content of variable "certdb" or "certdbv".
>    //
>    NameSize      = (UINT32) StrLen (VariableName);
> +  CertDataSize  = sizeof(Sha256Digest);
>    CertNodeSize  = sizeof (AUTH_CERT_DB_DATA) + (UINT32) CertDataSize + NameSize * sizeof (CHAR16);
>    NewCertDbSize = (UINT32) DataSize + CertNodeSize;
>    if (NewCertDbSize > mMaxCertDbSize) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> +
> +  Status = CalculatePrivAuthVarSignChainSHA256Digest(
> +             SignerCert,
> +             SignerCertSize,
> +             TopLevelCert,
> +             TopLevelCertSize,
> +             Sha256Digest
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
>    NewCertDb     = (UINT8*) mCertDbStore;
>  
>    //
> @@ -1999,7 +2098,7 @@ InsertCertsToDb (
>  
>    CopyMem (
>      (UINT8 *) Ptr +  sizeof (AUTH_CERT_DB_DATA) + NameSize * sizeof (CHAR16),
> -    CertData,
> +    Sha256Digest,
>      CertDataSize
>      );
>  
> @@ -2181,19 +2280,27 @@ VerifyTimeBasedPayload (
>    UINTN                            NewDataSize;
>    UINT8                            *Buffer;
>    UINTN                            Length;
> -  UINT8                            *RootCert;
> -  UINTN                            RootCertSize;
> +  UINT8                            *TopLevelCert;
> +  UINTN                            TopLevelCertSize;
> +  UINT8                            *TrustedCert;
> +  UINTN                            TrustedCertSize;
>    UINT8                            *SignerCerts;
>    UINTN                            CertStackSize;
>    UINT8                            *CertsInCertDb;
>    UINT32                           CertsSizeinDb;
> +  UINT8                            Sha256Digest[SHA256_DIGEST_SIZE];
>  
> +  //
> +  // 1. TopLevelCert is the top-level issuer certificate in signature Signer Cert Chain
> +  // 2. TrustedCert is the certificate which firmware trusts. It could be saved in protected
> +  //     storage or PK payload on PK init
> +  //
>    VerifyStatus           = FALSE;
>    CertData               = NULL;
>    NewData                = NULL;
>    Attr                   = Attributes;
>    SignerCerts            = NULL;
> -  RootCert               = NULL;
> +  TopLevelCert           = NULL;
>    CertsInCertDb          = NULL;
>  
>    //
> @@ -2325,8 +2432,8 @@ VerifyTimeBasedPayload (
>                       SigDataSize,
>                       &SignerCerts,
>                       &CertStackSize,
> -                     &RootCert,
> -                     &RootCertSize
> +                     &TopLevelCert,
> +                     &TopLevelCertSize
>                       );
>      if (!VerifyStatus) {
>        goto Exit;
> @@ -2348,8 +2455,8 @@ VerifyTimeBasedPayload (
>      }
>      CertList = (EFI_SIGNATURE_LIST *) Data;
>      Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
> -    if ((RootCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
> -        (CompareMem (Cert->SignatureData, RootCert, RootCertSize) != 0)) {
> +    if ((TopLevelCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
> +        (CompareMem (Cert->SignatureData, TopLevelCert, TopLevelCertSize) != 0)) {
>        VerifyStatus = FALSE;
>        goto Exit;
>      }
> @@ -2360,8 +2467,8 @@ VerifyTimeBasedPayload (
>      VerifyStatus = Pkcs7Verify (
>                       SigData,
>                       SigDataSize,
> -                     RootCert,
> -                     RootCertSize,
> +                     TopLevelCert,
> +                     TopLevelCertSize,
>                       NewData,
>                       NewDataSize
>                       );
> @@ -2394,8 +2501,8 @@ VerifyTimeBasedPayload (
>            //
>            // Iterate each Signature Data Node within this CertList for a verify
>            //
> -          RootCert      = Cert->SignatureData;
> -          RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
> +          TrustedCert      = Cert->SignatureData;
> +          TrustedCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
>  
>            //
>            // Verify Pkcs7 SignedData via Pkcs7Verify library.
> @@ -2403,8 +2510,8 @@ VerifyTimeBasedPayload (
>            VerifyStatus = Pkcs7Verify (
>                             SigData,
>                             SigDataSize,
> -                           RootCert,
> -                           RootCertSize,
> +                           TrustedCert,
> +                           TrustedCertSize,
>                             NewData,
>                             NewDataSize
>                             );
> @@ -2428,8 +2535,8 @@ VerifyTimeBasedPayload (
>                       SigDataSize,
>                       &SignerCerts,
>                       &CertStackSize,
> -                     &RootCert,
> -                     &RootCertSize
> +                     &TopLevelCert,
> +                     &TopLevelCertSize
>                       );
>      if (!VerifyStatus) {
>        goto Exit;
> @@ -2448,17 +2555,36 @@ VerifyTimeBasedPayload (
>          goto Exit;
>        }
>  
> -      if ((CertStackSize != CertsSizeinDb) ||
> -          (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
> -        goto Exit;
> +      if (CertsSizeinDb == SHA256_DIGEST_SIZE) {
> +        //
> +        // Check hash of signer cert CommonName + Top-level issuer tbsCertificate against data in CertDb
> +        //
> +        Status = CalculatePrivAuthVarSignChainSHA256Digest(
> +                   SignerCerts + sizeof(UINT8) + sizeof(UINT32),
> +                   ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
> +                   TopLevelCert,
> +                   TopLevelCertSize,
> +                   Sha256Digest
> +                   );
> +        if (EFI_ERROR(Status) || CompareMem (Sha256Digest, CertsInCertDb, CertsSizeinDb) != 0){
> +          goto Exit;
> +        }
> +      } else {
> +         //
> +         // Keep backward compatible with previous solution which saves whole signer certs stack in CertDb
> +         //
> +         if ((CertStackSize != CertsSizeinDb) ||
> +             (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
> +              goto Exit;
> +         }
>        }
>      }
>  
>      VerifyStatus = Pkcs7Verify (
>                       SigData,
>                       SigDataSize,
> -                     RootCert,
> -                     RootCertSize,
> +                     TopLevelCert,
> +                     TopLevelCertSize,
>                       NewData,
>                       NewDataSize
>                       );
> @@ -2468,9 +2594,17 @@ VerifyTimeBasedPayload (
>  
>      if ((OrgTimeStamp == NULL) && (PayloadSize != 0)) {
>        //
> -      // Insert signer's certificates when adding a new common authenticated variable.
> +      // When adding a new common authenticated variable, always save Hash of cn of signer cert + tbsCertificate of Top-level issuer
>        //
> -      Status = InsertCertsToDb (VariableName, VendorGuid, Attributes, SignerCerts, CertStackSize);
> +      Status = InsertCertsToDb (
> +                 VariableName,
> +                 VendorGuid,
> +                 Attributes,
> +                 SignerCerts + sizeof(UINT8) + sizeof(UINT32),
> +                 ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
> +                 TopLevelCert,
> +                 TopLevelCertSize
> +                 );
>        if (EFI_ERROR (Status)) {
>          VerifyStatus = FALSE;
>          goto Exit;
> @@ -2479,16 +2613,16 @@ VerifyTimeBasedPayload (
>    } else if (AuthVarType == AuthVarTypePayload) {
>      CertList = (EFI_SIGNATURE_LIST *) PayloadPtr;
>      Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
> -    RootCert      = Cert->SignatureData;
> -    RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
> +    TrustedCert     = Cert->SignatureData;
> +    TrustedCertSize = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
>      //
>      // Verify Pkcs7 SignedData via Pkcs7Verify library.
>      //
>      VerifyStatus = Pkcs7Verify (
>                       SigData,
>                       SigDataSize,
> -                     RootCert,
> -                     RootCertSize,
> +                     TrustedCert,
> +                     TrustedCertSize,
>                       NewData,
>                       NewDataSize
>                       );
> @@ -2499,7 +2633,7 @@ VerifyTimeBasedPayload (
>  Exit:
>  
>    if (AuthVarType == AuthVarTypePk || AuthVarType == AuthVarTypePriv) {
> -    Pkcs7FreeSigners (RootCert);
> +    Pkcs7FreeSigners (TopLevelCert);
>      Pkcs7FreeSigners (SignerCerts);
>    }
>  
> -- 
> 1.9.5.msysgit.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


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

* Re: [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable
  2017-10-16  2:15 ` Gary Lin
@ 2017-10-16 14:09   ` Zhang, Chao B
  0 siblings, 0 replies; 5+ messages in thread
From: Zhang, Chao B @ 2017-10-16 14:09 UTC (permalink / raw)
  To: Gary Lin; +Cc: edk2-devel@lists.01.org, Long, Qin

Gary:
  Thanks for pointing this. Please help to review the patch.

-----Original Message-----
From: Gary Lin [mailto:glin@suse.com] 
Sent: Monday, October 16, 2017 10:15 AM
To: Zhang, Chao B <chao.b.zhang@intel.com>
Cc: edk2-devel@lists.01.org; Long, Qin <qin.long@intel.com>
Subject: Re: [edk2] [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable

On Thu, Oct 12, 2017 at 05:14:25PM +0800, Zhang, Chao B wrote:
> ECR1707 for UEFI2.7 clarified certificate management rule for private time-based
> AuthVariable.Trusted cert rule changed from whole signer's certificate stack to
> top-level issuer cert tbscertificate + SignerCert CN for better management compatibility.
> Hash is used to reduce storage overhead.
> 
> Cc: Long Qin <qin.long@intel.com>
> Cc: Chen Chen <chen.a.chen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chao Zhang <chao.b.zhang@intel.com>
> ---
>  SecurityPkg/Library/AuthVariableLib/AuthService.c | 208 ++++++++++++++++++----
>  1 file changed, 171 insertions(+), 37 deletions(-)
> 
> diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> index a37ec0b..7188ff6 100644
> --- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
> +++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> @@ -1530,6 +1530,85 @@ AuthServiceInternalCompareTimeStamp (
>  }
>  
>  /**
> +  Calculate SHA256 digest of SignerCert CommonName + ToplevelCert tbsCertificate
> +  SignerCert and ToplevelCert are inside the signer certificate chain.
> +
> +  @param[in]  SignerCert          A pointer to SignerCert data.
> +  @param[in]  SignerCertSize      Length of SignerCert data.
> +  @param[in]  TopLevelCert        A pointer to TopLevelCert data.
> +  @param[in]  TopLevelCertSize    Length of TopLevelCert data.
> +  @param[out] Sha256Digest       Sha256 digest calculated.
> +
> +  @return EFI_ABORTED          Digest process failed.
> +  @return EFI_SUCCESS          SHA256 Digest is succesfully calculated.
> +
> +**/
> +EFI_STATUS
> +CalculatePrivAuthVarSignChainSHA256Digest(
> +  IN     UINT8            *SignerCert,
> +  IN     UINTN            SignerCertSize,
> +  IN     UINT8            *TopLevelCert,
> +  IN     UINTN            TopLevelCertSize,
> +  OUT    UINT8            *Sha256Digest
> +  )
> +{
> +  UINT8                   *TbsCert;
> +  UINTN                   TbsCertSize;
> +  UINT8                   CertCommonName[128];
> +  UINTN                   CertCommonNameSize;
> +  BOOLEAN                 CryptoStatus;
> +  EFI_STATUS              Status;
> +
> +  CertCommonNameSize = sizeof(CertCommonName);
> +
> +  //
> +  // Get SignerCert CommonName
> +  //
> +  Status = X509GetCommonName(SignerCert, SignerCertSize, CertCommonName, &CertCommonNameSize);
An error showed while building ovmf with gcc 7.2.1:

SecurityPkg/Library/AuthVariableLib/AuthService.c: In function ‘CalculatePrivAuthVarSignChainSHA256Digest’:
SecurityPkg/Library/AuthVariableLib/AuthService.c:1567:58: error: pointer targets in passing argument 3 of ‘X509GetCommonName’ differ in signedness [-Werror=pointer-sign]
   Status = X509GetCommonName(SignerCert, SignerCertSize, CertCommonName, &CertCommonNameSize);
                                                          ^~~~~~~~~~~~~~
In file included from SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h:34:0,
                 from SecurityPkg/Library/AuthVariableLib/AuthService.c:32:
CryptoPkg/Include/Library/BaseCryptLib.h:2202:1: note: expected ‘CHAR8 * {aka char *}’ but argument is of type ‘UINT8 * {aka unsigned char *}’
 X509GetCommonName (
 ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Changing the type of CertCommonName from UINT8 to CHAR8 fixes the
warning.

Cheers,

Gary Lin

> +  if (EFI_ERROR(Status)) {
> +    DEBUG((DEBUG_INFO, "%a Get SignerCert CommonName failed with status %x\n", __FUNCTION__, Status));
> +    return EFI_ABORTED;
> +  }
> +
> +  //
> +  // Get TopLevelCert tbsCertificate
> +  //
> +  if (!X509GetTBSCert(TopLevelCert, TopLevelCertSize, &TbsCert, &TbsCertSize)) {
> +    DEBUG((DEBUG_INFO, "%a Get Top-level Cert tbsCertificate failed!\n", __FUNCTION__));
> +    return EFI_ABORTED;
> +  }
> +
> +  //
> +  // Digest SignerCert CN + TopLevelCert tbsCertificate
> +  //
> +  ZeroMem (Sha256Digest, SHA256_DIGEST_SIZE);
> +  CryptoStatus = Sha256Init (mHashCtx);
> +  if (!CryptoStatus) {
> +    return EFI_ABORTED;
> +  }
> +
> +  //
> +  // '\0' is forced in CertCommonName. No overflow issue
> +  //
> +  CryptoStatus = Sha256Update (mHashCtx, CertCommonName, AsciiStrLen((CHAR8 *)CertCommonName));
> +  if (!CryptoStatus) {
> +    return EFI_ABORTED;
> +  }
> +
> +  CryptoStatus = Sha256Update (mHashCtx, TbsCert, TbsCertSize);
> +  if (!CryptoStatus) {
> +    return EFI_ABORTED;
> +  }
> +
> +  CryptoStatus  = Sha256Final (mHashCtx, Sha256Digest);
> +  if (!CryptoStatus) {
> +    return EFI_ABORTED;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
>    Find matching signer's certificates for common authenticated variable
>    by corresponding VariableName and VendorGuid from "certdb" or "certdbv".
>  
> @@ -1872,13 +1951,16 @@ DeleteCertsFromDb (
>  /**
>    Insert signer's certificates for common authenticated variable with VariableName
>    and VendorGuid in AUTH_CERT_DB_DATA to "certdb" or "certdbv" according to
> -  time based authenticated variable attributes.
> +  time based authenticated variable attributes. CertData is the SHA256 digest of
> +  SignerCert CommonName + TopLevelCert tbsCertificate.
>  
> -  @param[in]  VariableName   Name of authenticated Variable.
> -  @param[in]  VendorGuid     Vendor GUID of authenticated Variable.
> -  @param[in]  Attributes     Attributes of authenticated variable.
> -  @param[in]  CertData       Pointer to signer's certificates.
> -  @param[in]  CertDataSize   Length of CertData in bytes.
> +  @param[in]  VariableName      Name of authenticated Variable.
> +  @param[in]  VendorGuid        Vendor GUID of authenticated Variable.
> +  @param[in]  Attributes        Attributes of authenticated variable.
> +  @param[in]  SignerCert        Signer certificate data.
> +  @param[in]  SignerCertSize    Length of signer certificate.
> +  @param[in]  TopLevelCert      Top-level certificate data.
> +  @param[in]  TopLevelCertSize  Length of top-level certificate.
>  
>    @retval  EFI_INVALID_PARAMETER Any input parameter is invalid.
>    @retval  EFI_ACCESS_DENIED     An AUTH_CERT_DB_DATA entry with same VariableName
> @@ -1892,8 +1974,10 @@ InsertCertsToDb (
>    IN     CHAR16           *VariableName,
>    IN     EFI_GUID         *VendorGuid,
>    IN     UINT32           Attributes,
> -  IN     UINT8            *CertData,
> -  IN     UINTN            CertDataSize
> +  IN     UINT8            *SignerCert,
> +  IN     UINTN            SignerCertSize,
> +  IN     UINT8            *TopLevelCert,
> +  IN     UINTN            TopLevelCertSize
>    )
>  {
>    EFI_STATUS              Status;
> @@ -1904,10 +1988,12 @@ InsertCertsToDb (
>    UINT32                  NewCertDbSize;
>    UINT32                  CertNodeSize;
>    UINT32                  NameSize;
> +  UINT32                  CertDataSize;
>    AUTH_CERT_DB_DATA       *Ptr;
>    CHAR16                  *DbName;
> +  UINT8                   Sha256Digest[SHA256_DIGEST_SIZE];
>  
> -  if ((VariableName == NULL) || (VendorGuid == NULL) || (CertData == NULL)) {
> +  if ((VariableName == NULL) || (VendorGuid == NULL) || (SignerCert == NULL) ||(TopLevelCert == NULL)) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> @@ -1967,11 +2053,24 @@ InsertCertsToDb (
>    // Construct new data content of variable "certdb" or "certdbv".
>    //
>    NameSize      = (UINT32) StrLen (VariableName);
> +  CertDataSize  = sizeof(Sha256Digest);
>    CertNodeSize  = sizeof (AUTH_CERT_DB_DATA) + (UINT32) CertDataSize + NameSize * sizeof (CHAR16);
>    NewCertDbSize = (UINT32) DataSize + CertNodeSize;
>    if (NewCertDbSize > mMaxCertDbSize) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> +
> +  Status = CalculatePrivAuthVarSignChainSHA256Digest(
> +             SignerCert,
> +             SignerCertSize,
> +             TopLevelCert,
> +             TopLevelCertSize,
> +             Sha256Digest
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
>    NewCertDb     = (UINT8*) mCertDbStore;
>  
>    //
> @@ -1999,7 +2098,7 @@ InsertCertsToDb (
>  
>    CopyMem (
>      (UINT8 *) Ptr +  sizeof (AUTH_CERT_DB_DATA) + NameSize * sizeof (CHAR16),
> -    CertData,
> +    Sha256Digest,
>      CertDataSize
>      );
>  
> @@ -2181,19 +2280,27 @@ VerifyTimeBasedPayload (
>    UINTN                            NewDataSize;
>    UINT8                            *Buffer;
>    UINTN                            Length;
> -  UINT8                            *RootCert;
> -  UINTN                            RootCertSize;
> +  UINT8                            *TopLevelCert;
> +  UINTN                            TopLevelCertSize;
> +  UINT8                            *TrustedCert;
> +  UINTN                            TrustedCertSize;
>    UINT8                            *SignerCerts;
>    UINTN                            CertStackSize;
>    UINT8                            *CertsInCertDb;
>    UINT32                           CertsSizeinDb;
> +  UINT8                            Sha256Digest[SHA256_DIGEST_SIZE];
>  
> +  //
> +  // 1. TopLevelCert is the top-level issuer certificate in signature Signer Cert Chain
> +  // 2. TrustedCert is the certificate which firmware trusts. It could be saved in protected
> +  //     storage or PK payload on PK init
> +  //
>    VerifyStatus           = FALSE;
>    CertData               = NULL;
>    NewData                = NULL;
>    Attr                   = Attributes;
>    SignerCerts            = NULL;
> -  RootCert               = NULL;
> +  TopLevelCert           = NULL;
>    CertsInCertDb          = NULL;
>  
>    //
> @@ -2325,8 +2432,8 @@ VerifyTimeBasedPayload (
>                       SigDataSize,
>                       &SignerCerts,
>                       &CertStackSize,
> -                     &RootCert,
> -                     &RootCertSize
> +                     &TopLevelCert,
> +                     &TopLevelCertSize
>                       );
>      if (!VerifyStatus) {
>        goto Exit;
> @@ -2348,8 +2455,8 @@ VerifyTimeBasedPayload (
>      }
>      CertList = (EFI_SIGNATURE_LIST *) Data;
>      Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
> -    if ((RootCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
> -        (CompareMem (Cert->SignatureData, RootCert, RootCertSize) != 0)) {
> +    if ((TopLevelCertSize != (CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1))) ||
> +        (CompareMem (Cert->SignatureData, TopLevelCert, TopLevelCertSize) != 0)) {
>        VerifyStatus = FALSE;
>        goto Exit;
>      }
> @@ -2360,8 +2467,8 @@ VerifyTimeBasedPayload (
>      VerifyStatus = Pkcs7Verify (
>                       SigData,
>                       SigDataSize,
> -                     RootCert,
> -                     RootCertSize,
> +                     TopLevelCert,
> +                     TopLevelCertSize,
>                       NewData,
>                       NewDataSize
>                       );
> @@ -2394,8 +2501,8 @@ VerifyTimeBasedPayload (
>            //
>            // Iterate each Signature Data Node within this CertList for a verify
>            //
> -          RootCert      = Cert->SignatureData;
> -          RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
> +          TrustedCert      = Cert->SignatureData;
> +          TrustedCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
>  
>            //
>            // Verify Pkcs7 SignedData via Pkcs7Verify library.
> @@ -2403,8 +2510,8 @@ VerifyTimeBasedPayload (
>            VerifyStatus = Pkcs7Verify (
>                             SigData,
>                             SigDataSize,
> -                           RootCert,
> -                           RootCertSize,
> +                           TrustedCert,
> +                           TrustedCertSize,
>                             NewData,
>                             NewDataSize
>                             );
> @@ -2428,8 +2535,8 @@ VerifyTimeBasedPayload (
>                       SigDataSize,
>                       &SignerCerts,
>                       &CertStackSize,
> -                     &RootCert,
> -                     &RootCertSize
> +                     &TopLevelCert,
> +                     &TopLevelCertSize
>                       );
>      if (!VerifyStatus) {
>        goto Exit;
> @@ -2448,17 +2555,36 @@ VerifyTimeBasedPayload (
>          goto Exit;
>        }
>  
> -      if ((CertStackSize != CertsSizeinDb) ||
> -          (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
> -        goto Exit;
> +      if (CertsSizeinDb == SHA256_DIGEST_SIZE) {
> +        //
> +        // Check hash of signer cert CommonName + Top-level issuer tbsCertificate against data in CertDb
> +        //
> +        Status = CalculatePrivAuthVarSignChainSHA256Digest(
> +                   SignerCerts + sizeof(UINT8) + sizeof(UINT32),
> +                   ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
> +                   TopLevelCert,
> +                   TopLevelCertSize,
> +                   Sha256Digest
> +                   );
> +        if (EFI_ERROR(Status) || CompareMem (Sha256Digest, CertsInCertDb, CertsSizeinDb) != 0){
> +          goto Exit;
> +        }
> +      } else {
> +         //
> +         // Keep backward compatible with previous solution which saves whole signer certs stack in CertDb
> +         //
> +         if ((CertStackSize != CertsSizeinDb) ||
> +             (CompareMem (SignerCerts, CertsInCertDb, CertsSizeinDb) != 0)) {
> +              goto Exit;
> +         }
>        }
>      }
>  
>      VerifyStatus = Pkcs7Verify (
>                       SigData,
>                       SigDataSize,
> -                     RootCert,
> -                     RootCertSize,
> +                     TopLevelCert,
> +                     TopLevelCertSize,
>                       NewData,
>                       NewDataSize
>                       );
> @@ -2468,9 +2594,17 @@ VerifyTimeBasedPayload (
>  
>      if ((OrgTimeStamp == NULL) && (PayloadSize != 0)) {
>        //
> -      // Insert signer's certificates when adding a new common authenticated variable.
> +      // When adding a new common authenticated variable, always save Hash of cn of signer cert + tbsCertificate of Top-level issuer
>        //
> -      Status = InsertCertsToDb (VariableName, VendorGuid, Attributes, SignerCerts, CertStackSize);
> +      Status = InsertCertsToDb (
> +                 VariableName,
> +                 VendorGuid,
> +                 Attributes,
> +                 SignerCerts + sizeof(UINT8) + sizeof(UINT32),
> +                 ReadUnaligned32 ((UINT32 *)(SignerCerts + sizeof(UINT8))),
> +                 TopLevelCert,
> +                 TopLevelCertSize
> +                 );
>        if (EFI_ERROR (Status)) {
>          VerifyStatus = FALSE;
>          goto Exit;
> @@ -2479,16 +2613,16 @@ VerifyTimeBasedPayload (
>    } else if (AuthVarType == AuthVarTypePayload) {
>      CertList = (EFI_SIGNATURE_LIST *) PayloadPtr;
>      Cert     = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
> -    RootCert      = Cert->SignatureData;
> -    RootCertSize  = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
> +    TrustedCert     = Cert->SignatureData;
> +    TrustedCertSize = CertList->SignatureSize - (sizeof (EFI_SIGNATURE_DATA) - 1);
>      //
>      // Verify Pkcs7 SignedData via Pkcs7Verify library.
>      //
>      VerifyStatus = Pkcs7Verify (
>                       SigData,
>                       SigDataSize,
> -                     RootCert,
> -                     RootCertSize,
> +                     TrustedCert,
> +                     TrustedCertSize,
>                       NewData,
>                       NewDataSize
>                       );
> @@ -2499,7 +2633,7 @@ VerifyTimeBasedPayload (
>  Exit:
>  
>    if (AuthVarType == AuthVarTypePk || AuthVarType == AuthVarTypePriv) {
> -    Pkcs7FreeSigners (RootCert);
> +    Pkcs7FreeSigners (TopLevelCert);
>      Pkcs7FreeSigners (SignerCerts);
>    }
>  
> -- 
> 1.9.5.msysgit.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

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

end of thread, other threads:[~2017-10-16 14:05 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-12  9:14 [PATCH] SecurityPkg:AuthVariableLib:Implement ECR1707 for Private Auth Variable Zhang, Chao B
2017-10-13  6:50 ` Chen, Chen A
2017-10-13  8:43 ` Long, Qin
2017-10-16  2:15 ` Gary Lin
2017-10-16 14:09   ` Zhang, Chao B

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