[TCBZ3925] Correct the implementation of Pkcs7Sign

The documentation says "DER formatted", but the code was expecting it to be a pre-processed X509 structure. Add the additional processing.
microsoft · Jun 25, 2024 · a30086a · a30086a
1 parent 3f20eee
commit a30086a
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 14 deletions.
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -2393,6 +2393,7 @@ Pkcs7GetCertificatesList (
   @param[in]  InData           Pointer to the content to be signed.
   @param[in]  InDataSize       Size of InData in bytes.
   @param[in]  SignCert         Pointer to signer's DER-encoded certificate to sign with.
+  @param[in]  SignCertSize     Size of signer's DER-encoded certificate to sign with.  // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
   @param[in]  OtherCerts       Pointer to an optional additional set of certificates to
                                include in the PKCS#7 signedData (e.g. any intermediate
                                CAs in the chain).
@@ -2413,7 +2414,10 @@ Pkcs7Sign (
   IN   CONST UINT8  *KeyPassword,
   IN   UINT8        *InData,
   IN   UINTN        InDataSize,
-  IN   UINT8        *SignCert,
+  // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
+  IN   CONST UINT8  *SignCert,
+  // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
+  IN   UINTN        SignCertSize,
   IN   UINT8        *OtherCerts      OPTIONAL,
   OUT  UINT8        **SignedData,
   OUT  UINTN        *SignedDataSize

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c
@@ -25,6 +25,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
   @param[in]  InData           Pointer to the content to be signed.
   @param[in]  InDataSize       Size of InData in bytes.
   @param[in]  SignCert         Pointer to signer's DER-encoded certificate to sign with.
+  @param[in]  SignCertSize     Size of signer's DER-encoded certificate to sign with.  // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
   @param[in]  OtherCerts       Pointer to an optional additional set of certificates to
                                include in the PKCS#7 signedData (e.g. any intermediate
                                CAs in the chain).
@@ -44,13 +45,15 @@ Pkcs7Sign (
   IN   CONST UINT8  *KeyPassword,
   IN   UINT8        *InData,
   IN   UINTN        InDataSize,
-  IN   UINT8        *SignCert,
+  IN   CONST UINT8  *SignCert,      // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
+  IN   UINTN        SignCertSize,   // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
   IN   UINT8        *OtherCerts      OPTIONAL,
   OUT  UINT8        **SignedData,
   OUT  UINTN        *SignedDataSize
   )
 {
   BOOLEAN   Status;
+  X509      *Cert;      // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
   EVP_PKEY  *Key;
   BIO       *DataBio;
   PKCS7     *Pkcs7;
@@ -69,6 +72,7 @@ Pkcs7Sign (
   }
 
   RsaContext = NULL;
+  Cert       = NULL;    // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
   Key        = NULL;
   Pkcs7      = NULL;
   DataBio    = NULL;
@@ -106,6 +110,17 @@ Pkcs7Sign (
 
   RandomSeed (NULL, 0);
 
+  // MU_CHANGE [TCBZ3925] [BEGIN] - Pkcs7Sign is broken
+  //
+  // Read DER-encoded root certificate and Construct X509 Certificate
+  //
+  Cert = d2i_X509 (NULL, &SignCert, (long)SignCertSize);
+  if (Cert == NULL) {
+    goto _Exit;
+  }
+
+  // MU_CHANGE [TCBZ3925] [END] - Pkcs7Sign is broken
+
   //
   // Construct OpenSSL EVP_PKEY for private key.
   //
@@ -134,7 +149,7 @@ Pkcs7Sign (
   // Create the PKCS#7 signedData structure.
   //
   Pkcs7 = PKCS7_sign (
-            (X509 *)SignCert,
+            Cert,     // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
             Key,
             (STACK_OF (X509) *) OtherCerts,
             DataBio,
@@ -182,6 +197,13 @@ Pkcs7Sign (
   //
   // Release Resources
   //
+  // MU_CHANGE [TCBZ3925] [BEGIN] - Pkcs7Sign is broken
+  if (Cert != NULL) {
+    X509_free (Cert);
+  }
+
+  // MU_CHANGE [TCBZ3925] [END] - Pkcs7Sign is broken
+
   if (Key != NULL) {
     EVP_PKEY_free (Key);
   }

diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c
@@ -24,6 +24,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
   @param[in]  InData           Pointer to the content to be signed.
   @param[in]  InDataSize       Size of InData in bytes.
   @param[in]  SignCert         Pointer to signer's DER-encoded certificate to sign with.
+  @param[in]  SignCertSize     Size of signer's DER-encoded certificate to sign with.  // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
   @param[in]  OtherCerts       Pointer to an optional additional set of certificates to
                                include in the PKCS#7 signedData (e.g. any intermediate
                                CAs in the chain).
@@ -42,7 +43,8 @@ Pkcs7Sign (
   IN   CONST UINT8  *KeyPassword,
   IN   UINT8        *InData,
   IN   UINTN        InDataSize,
-  IN   UINT8        *SignCert,
+  IN   CONST UINT8  *SignCert,      // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
+  IN   UINTN        SignCertSize,   // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
   IN   UINT8        *OtherCerts      OPTIONAL,
   OUT  UINT8        **SignedData,
   OUT  UINTN        *SignedDataSize

diff --git a/CryptoPkg/Library/BaseCryptLibNull/Pk/CryptPkcs7SignNull.c b/CryptoPkg/Library/BaseCryptLibNull/Pk/CryptPkcs7SignNull.c
@@ -24,6 +24,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
   @param[in]  InData           Pointer to the content to be signed.
   @param[in]  InDataSize       Size of InData in bytes.
   @param[in]  SignCert         Pointer to signer's DER-encoded certificate to sign with.
+  @param[in]  SignCertSize     Size of signer's DER-encoded certificate to sign with.  // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
   @param[in]  OtherCerts       Pointer to an optional additional set of certificates to
                                include in the PKCS#7 signedData (e.g. any intermediate
                                CAs in the chain).
@@ -42,7 +43,8 @@ Pkcs7Sign (
   IN   CONST UINT8  *KeyPassword,
   IN   UINT8        *InData,
   IN   UINTN        InDataSize,
-  IN   UINT8        *SignCert,
+  IN   CONST UINT8  *SignCert,    // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
+  IN   UINTN        SignCertSize, // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
   IN   UINT8        *OtherCerts      OPTIONAL,
   OUT  UINT8        **SignedData,
   OUT  UINTN        *SignedDataSize

diff --git a/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/RsaPkcs7Tests.c b/CryptoPkg/Test/UnitTest/Library/BaseCryptLib/RsaPkcs7Tests.c
@@ -344,17 +344,20 @@ TestVerifyPkcs7SignVerify (
   BOOLEAN  Status;
   UINT8    *P7SignedData;
   UINTN    P7SignedDataSize;
-  UINT8    *SignCert;
+
+  // UINT8    *SignCert;  // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
 
   P7SignedData = NULL;
-  SignCert     = NULL;
+  // SignCert     = NULL; // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
 
+  // MU_CHANGE [TCBZ3925] [BEGIN] - Pkcs7Sign is broken
   //
   // Construct Signer Certificate from RAW data.
   //
-  Status = X509ConstructCertificate (TestCert, sizeof (TestCert), (UINT8 **)&SignCert);
-  UT_ASSERT_TRUE (Status);
-  UT_ASSERT_NOT_NULL (SignCert);
+  // Status = X509ConstructCertificate (TestCert, sizeof (TestCert), (UINT8 **)&SignCert);
+  // UT_ASSERT_TRUE (Status);
+  // UT_ASSERT_NOT_NULL (SignCert);
+  // MU_CHANGE [TCBZ3925] [END] - Pkcs7Sign is broken
 
   //
   // Create PKCS#7 signedData on Payload.
@@ -366,7 +369,8 @@ TestVerifyPkcs7SignVerify (
              (CONST UINT8 *)PemPass,
              (UINT8 *)Payload,
              AsciiStrLen (Payload),
-             SignCert,
+             TestCert,            // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
+             sizeof (TestCert),   // MU_CHANGE [TCBZ3925] - Pkcs7Sign is broken
              NULL,
              &P7SignedData,
              &P7SignedDataSize
@@ -388,9 +392,11 @@ TestVerifyPkcs7SignVerify (
     FreePool (P7SignedData);
   }
 
-  if (SignCert != NULL) {
-    X509Free (SignCert);
-  }
+  // MU_CHANGE [TCBZ3925] [BEGIN] - Pkcs7Sign is broken
+  // if (SignCert != NULL) {
+  //   X509Free (SignCert);
+  // }
+  // MU_CHANGE [TCBZ3925] [END] - Pkcs7Sign is broken
 
   return UNIT_TEST_PASSED;
 }