From nobody Sun Apr 28 09:14:31 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; spf=none (zoho.com: 198.145.21.10 is neither permitted nor denied by domain of lists.01.org) smtp.mailfrom=edk2-devel-bounces@lists.01.org Return-Path: Received: from ml01.01.org (ml01.01.org [198.145.21.10]) by mx.zohomail.com with SMTPS id 1509439228282750.2367506765382; Tue, 31 Oct 2017 01:40:28 -0700 (PDT) Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id 474732034AB3C; Tue, 31 Oct 2017 01:36:35 -0700 (PDT) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 72C142034AB35 for ; Tue, 31 Oct 2017 01:36:33 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Oct 2017 01:40:24 -0700 Received: from shwdepsi940.ccr.corp.intel.com ([10.239.9.122]) by fmsmga006.fm.intel.com with ESMTP; 31 Oct 2017 01:40:23 -0700 X-Original-To: edk2-devel@lists.01.org Received-SPF: none (zoho.com: 198.145.21.10 is neither permitted nor denied by domain of lists.01.org) client-ip=198.145.21.10; envelope-from=edk2-devel-bounces@lists.01.org; helo=ml01.01.org; Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=qin.long@intel.com; receiver=edk2-devel@lists.01.org X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,323,1505804400"; d="scan'208";a="169920605" From: Long Qin To: edk2-devel@lists.01.org Date: Tue, 31 Oct 2017 16:39:29 +0800 Message-Id: <20171031083930.12800-2-qin.long@intel.com> X-Mailer: git-send-email 2.14.1.windows.1 In-Reply-To: <20171031083930.12800-1-qin.long@intel.com> References: <20171031083930.12800-1-qin.long@intel.com> Subject: [edk2] [PATCH 1/2] CryptoPkg/BaseCryptLib: Fix buffer overflow issue in realloc wrapper X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: ting.ye@intel.com, lersek@redhat.com, Qin Long MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Errors-To: edk2-devel-bounces@lists.01.org Sender: "edk2-devel" X-ZohoMail: RSF_4 Z_629925259 SPT_0 Content-Type: text/plain; charset="utf-8" There is one long-standing problem in CRT realloc wrapper, which will cause the obvious buffer overflow issue when re-allocating one bigger memory block: void *realloc (void *ptr, size_t size) { // // BUG: hardcode OldSize =3D=3D size! We have no any knowledge about // memory size of original pointer ptr. // return ReallocatePool ((UINTN) size, (UINTN) size, ptr); } This patch introduces one extra header to record the memory buffer size information when allocating memory block from malloc routine, and re-wrap the realloc() and free() routines to remove this BUG. Cc: Laszlo Ersek Cc: Ting Ye Cc: Jian J Wang Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Qin Long --- .../BaseCryptLib/SysCall/BaseMemAllocation.c | 72 ++++++++++++++++++= +--- 1 file changed, 65 insertions(+), 7 deletions(-) diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c b/C= ryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c index f390e0d449..ed37a0ff39 100644 --- a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c @@ -16,6 +16,18 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHE= R EXPRESS OR IMPLIED. #include #include =20 +// +// Extra header to record the memory buffer size from malloc routine. +// +#define CRYPTMEM_HEAD_SIGNATURE SIGNATURE_32('c','m','h','d') +typedef struct { + UINT32 Signature; + UINT32 Reserved; + UINTN Size; +} CRYPTMEM_HEAD; + +#define CRYPTMEM_OVERHEAD sizeof(CRYPTMEM_HEAD) + // // -- Memory-Allocation Routines -- // @@ -23,27 +35,73 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITH= ER EXPRESS OR IMPLIED. /* Allocates memory blocks */ void *malloc (size_t size) { - return AllocatePool ((UINTN) size); + CRYPTMEM_HEAD *PoolHdr; + UINTN NewSize; + VOID *Data; + + // + // Adjust the size by the buffer header overhead + // + NewSize =3D (UINTN)(size) + CRYPTMEM_OVERHEAD; + + Data =3D AllocatePool (NewSize); + if (Data !=3D NULL) { + PoolHdr =3D (CRYPTMEM_HEAD *)Data; + // + // Record the memory brief information + // + PoolHdr->Signature =3D CRYPTMEM_HEAD_SIGNATURE; + PoolHdr->Size =3D size; + } + return (VOID *)(PoolHdr + 1); } =20 /* Reallocate memory blocks */ void *realloc (void *ptr, size_t size) { - // - // BUG: hardcode OldSize =3D=3D size! We have no any knowledge about - // memory size of original pointer ptr. - // - return ReallocatePool ((UINTN) size, (UINTN) size, ptr); + CRYPTMEM_HEAD *OldPoolHdr; + CRYPTMEM_HEAD *NewPoolHdr; + UINTN OldSize; + UINTN NewSize; + VOID *Data; + + NewSize =3D (UINTN)size + CRYPTMEM_OVERHEAD; + Data =3D AllocatePool (NewSize); + if (Data !=3D NULL) { + NewPoolHdr =3D (CRYPTMEM_HEAD *)Data; + NewPoolHdr->Signature =3D CRYPTMEM_HEAD_SIGNATURE; + NewPoolHdr->Size =3D size; + if (ptr !=3D NULL) { + // + // Retrieve the original size from the buffer header. + // + OldPoolHdr =3D (CRYPTMEM_HEAD *)ptr - 1; + ASSERT (OldPoolHdr->Signature =3D=3D CRYPTMEM_HEAD_SIGNATURE); + OldSize =3D OldPoolHdr->Size; + + // + // Duplicate the buffer content. + // + CopyMem ((VOID *)(NewPoolHdr + 1), ptr, MIN (OldSize, size)); + FreePool ((VOID *)OldPoolHdr); + } + } + + return (VOID *)(NewPoolHdr + 1); } =20 /* De-allocates or frees a memory block */ void free (void *ptr) { + CRYPTMEM_HEAD *PoolHdr; + // // In Standard C, free() handles a null pointer argument transparently. = This // is not true of FreePool() below, so protect it. // if (ptr !=3D NULL) { - FreePool (ptr); + PoolHdr =3D (CRYPTMEM_HEAD *)ptr - 1; + ASSERT (PoolHdr->Signature =3D=3D CRYPTMEM_HEAD_SIGNATURE); + FreePool (PoolHdr); } } --=20 2.14.1.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel From nobody Sun Apr 28 09:14:31 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; spf=none (zoho.com: 198.145.21.10 is neither permitted nor denied by domain of lists.01.org) smtp.mailfrom=edk2-devel-bounces@lists.01.org Return-Path: Received: from ml01.01.org (ml01.01.org [198.145.21.10]) by mx.zohomail.com with SMTPS id 1509439230002323.5226425374608; Tue, 31 Oct 2017 01:40:30 -0700 (PDT) Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id 85F1D2034CF7C; Tue, 31 Oct 2017 01:36:36 -0700 (PDT) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 950112034C0A9 for ; Tue, 31 Oct 2017 01:36:35 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Oct 2017 01:40:27 -0700 Received: from shwdepsi940.ccr.corp.intel.com ([10.239.9.122]) by fmsmga006.fm.intel.com with ESMTP; 31 Oct 2017 01:40:25 -0700 X-Original-To: edk2-devel@lists.01.org Received-SPF: none (zoho.com: 198.145.21.10 is neither permitted nor denied by domain of lists.01.org) client-ip=198.145.21.10; envelope-from=edk2-devel-bounces@lists.01.org; helo=ml01.01.org; Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=qin.long@intel.com; receiver=edk2-devel@lists.01.org X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,323,1505804400"; d="scan'208";a="169920613" From: Long Qin To: edk2-devel@lists.01.org Date: Tue, 31 Oct 2017 16:39:30 +0800 Message-Id: <20171031083930.12800-3-qin.long@intel.com> X-Mailer: git-send-email 2.14.1.windows.1 In-Reply-To: <20171031083930.12800-1-qin.long@intel.com> References: <20171031083930.12800-1-qin.long@intel.com> Subject: [edk2] [PATCH 2/2] CryptoPkg/BaseCryptLib: Fix mismatched memory allocation/free X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: ting.ye@intel.com, lersek@redhat.com, Qin Long MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Errors-To: edk2-devel-bounces@lists.01.org Sender: "edk2-devel" X-ZohoMail: RSF_4 Z_629925259 SPT_0 Content-Type: text/plain; charset="utf-8" The malloc/free (instead of AllocatePool/FreePool) were used directly in some wrapper implementations, which was designed to leverage the light-weight memory management routines at Runtime phase. The malloc/free and AllocatePool/FreePool usages are required to be matched, after extra memory size info header was introduced in malloc wrapper. This patch corrects two memory allocation cases, which requires the caller to free the buffer with FreePool() outside the function call. And some comments were also added to clarify the correct memory release functions if it's the caller's responsibility to free the memory buffer. Cc: Laszlo Ersek Cc: Ting Ye Cc: Jian J Wang Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Qin Long Reviewed-by: Jian J Wang --- CryptoPkg/Include/Library/BaseCryptLib.h | 16 ++++++++++--= ---- CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c | 5 +++-- CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c | 3 ++- CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c | 15 +++++++++---= --- CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c | 13 ++++++++----- 5 files changed, 32 insertions(+), 20 deletions(-) diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/L= ibrary/BaseCryptLib.h index 5f67ecb709..e2b6a95666 100644 --- a/CryptoPkg/Include/Library/BaseCryptLib.h +++ b/CryptoPkg/Include/Library/BaseCryptLib.h @@ -2388,10 +2388,12 @@ Pkcs5HashPassword ( @param[in] P7Data Pointer to the PKCS#7 message to verify. @param[in] P7Length Length of the PKCS#7 message in bytes. @param[out] CertStack Pointer to Signer's certificates retrieved from= P7Data. - It's caller's responsibility to free the buffer. + It's caller's responsibility to free the buffer= with + Pkcs7FreeSigners(). @param[out] StackLength Length of signer's certificates in bytes. @param[out] TrustedCert Pointer to a trusted certificate from Signer's = certificates. - It's caller's responsibility to free the buffer. + It's caller's responsibility to free the buffer= with + Pkcs7FreeSigners(). @param[out] CertLength Length of the trusted certificate in bytes. =20 @retval TRUE The operation is finished successfully. @@ -2433,10 +2435,11 @@ Pkcs7FreeSigners ( @param[in] P7Data Pointer to the PKCS#7 message. @param[in] P7Length Length of the PKCS#7 message in bytes. @param[out] SignerChainCerts Pointer to the certificates list chained t= o signer's - certificate. It's caller's responsibility = to free the buffer. + certificate. It's caller's responsibility = to free the buffer + with Pkcs7FreeSigners(). @param[out] ChainLength Length of the chained certificates list bu= ffer in bytes. @param[out] UnchainCerts Pointer to the unchained certificates list= s. It's caller's - responsibility to free the buffer. + responsibility to free the buffer with Pkc= s7FreeSigners(). @param[out] UnchainLength Length of the unchained certificates list = buffer in bytes. =20 @retval TRUE The operation is finished successfully. @@ -2472,7 +2475,8 @@ Pkcs7GetCertificatesList ( @param[in] OtherCerts Pointer to an optional additional set of ce= rtificates to include in the PKCS#7 signedData (e.g. any = intermediate CAs in the chain). - @param[out] SignedData Pointer to output PKCS#7 signedData. + @param[out] SignedData Pointer to output PKCS#7 signedData. It's c= aller's + responsibility to free the buffer with Free= Pool(). @param[out] SignedDataSize Size of SignedData in bytes. =20 @retval TRUE PKCS#7 data signing succeeded. @@ -2540,7 +2544,7 @@ Pkcs7Verify ( @param[in] P7Data Pointer to the PKCS#7 signed data to process. @param[in] P7Length Length of the PKCS#7 signed data in bytes. @param[out] Content Pointer to the extracted content from the PKCS= #7 signedData. - It's caller's responsibility to free the buffe= r. + It's caller's responsibility to free the buffe= r with FreePool(). @param[out] ContentSize The size of the extracted content in bytes. =20 @retval TRUE The P7Data was correctly formatted for process= ing. diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c b/CryptoPkg= /Library/BaseCryptLib/Pk/CryptPkcs7Sign.c index d3b1a907aa..0f61d4b4ad 100644 --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c @@ -34,7 +34,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER= EXPRESS OR IMPLIED. @param[in] OtherCerts Pointer to an optional additional set of ce= rtificates to include in the PKCS#7 signedData (e.g. any = intermediate CAs in the chain). - @param[out] SignedData Pointer to output PKCS#7 signedData. + @param[out] SignedData Pointer to output PKCS#7 signedData. It's c= aller's + responsibility to free the buffer with Free= Pool(). @param[out] SignedDataSize Size of SignedData in bytes. =20 @retval TRUE PKCS#7 data signing succeeded. @@ -167,7 +168,7 @@ Pkcs7Sign ( // is totally 19 bytes. // *SignedDataSize =3D P7DataSize - 19; - *SignedData =3D malloc (*SignedDataSize); + *SignedData =3D AllocatePool (*SignedDataSize); if (*SignedData =3D=3D NULL) { OPENSSL_free (P7Data); goto _Exit; diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c b/Crypt= oPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c index 539bb6b7d5..1ce7202d91 100644 --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7SignNull.c @@ -33,7 +33,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER= EXPRESS OR IMPLIED. @param[in] OtherCerts Pointer to an optional additional set of ce= rtificates to include in the PKCS#7 signedData (e.g. any = intermediate CAs in the chain). - @param[out] SignedData Pointer to output PKCS#7 signedData. + @param[out] SignedData Pointer to output PKCS#7 signedData. It's c= aller's + responsibility to free the buffer with Free= Pool(). @param[out] SignedDataSize Size of SignedData in bytes. =20 @retval FALSE This interface is not supported. diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c b/CryptoP= kg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c index bf67a1f569..296df028b1 100644 --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c @@ -240,10 +240,12 @@ _Exit: @param[in] P7Data Pointer to the PKCS#7 message to verify. @param[in] P7Length Length of the PKCS#7 message in bytes. @param[out] CertStack Pointer to Signer's certificates retrieved from= P7Data. - It's caller's responsibility to free the buffer. + It's caller's responsibility to free the buffer= with + Pkcs7FreeSigners(). @param[out] StackLength Length of signer's certificates in bytes. @param[out] TrustedCert Pointer to a trusted certificate from Signer's = certificates. - It's caller's responsibility to free the buffer. + It's caller's responsibility to free the buffer= with + Pkcs7FreeSigners(). @param[out] CertLength Length of the trusted certificate in bytes. =20 @retval TRUE The operation is finished successfully. @@ -438,10 +440,11 @@ Pkcs7FreeSigners ( @param[in] P7Data Pointer to the PKCS#7 message. @param[in] P7Length Length of the PKCS#7 message in bytes. @param[out] SignerChainCerts Pointer to the certificates list chained t= o signer's - certificate. It's caller's responsibility = to free the buffer. + certificate. It's caller's responsibility = to free the buffer + with Pkcs7FreeSigners(). @param[out] ChainLength Length of the chained certificates list bu= ffer in bytes. @param[out] UnchainCerts Pointer to the unchained certificates list= s. It's caller's - responsibility to free the buffer. + responsibility to free the buffer with Pkc= s7FreeSigners(). @param[out] UnchainLength Length of the unchained certificates list = buffer in bytes. =20 @retval TRUE The operation is finished successfully. @@ -921,7 +924,7 @@ _Exit: @param[in] P7Data Pointer to the PKCS#7 signed data to process. @param[in] P7Length Length of the PKCS#7 signed data in bytes. @param[out] Content Pointer to the extracted content from the PKCS= #7 signedData. - It's caller's responsibility to free the buffe= r. + It's caller's responsibility to free the buffe= r with FreePool(). @param[out] ContentSize The size of the extracted content in bytes. =20 @retval TRUE The P7Data was correctly formatted for process= ing. @@ -996,7 +999,7 @@ Pkcs7GetAttachedContent ( OctStr =3D Pkcs7->d.sign->contents->d.data; if ((OctStr->length > 0) && (OctStr->data !=3D NULL)) { *ContentSize =3D OctStr->length; - *Content =3D malloc (*ContentSize); + *Content =3D AllocatePool (*ContentSize); if (*Content =3D=3D NULL) { *ContentSize =3D 0; goto _Exit; diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c b/Cry= ptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c index 06602ec535..d3e8ec89a7 100644 --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyNull.c @@ -25,10 +25,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITH= ER EXPRESS OR IMPLIED. @param[in] P7Data Pointer to the PKCS#7 message to verify. @param[in] P7Length Length of the PKCS#7 message in bytes. @param[out] CertStack Pointer to Signer's certificates retrieved from= P7Data. - It's caller's responsibility to free the buffer. + It's caller's responsibility to free the buffer= with + Pkcs7FreeSigners(). @param[out] StackLength Length of signer's certificates in bytes. @param[out] TrustedCert Pointer to a trusted certificate from Signer's = certificates. - It's caller's responsibility to free the buffer. + It's caller's responsibility to free the buffer= with + Pkcs7FreeSigners(). @param[out] CertLength Length of the trusted certificate in bytes. =20 @retval FALSE This interface is not supported. @@ -75,10 +77,11 @@ Pkcs7FreeSigners ( @param[in] P7Data Pointer to the PKCS#7 message. @param[in] P7Length Length of the PKCS#7 message in bytes. @param[out] SignerChainCerts Pointer to the certificates list chained t= o signer's - certificate. It's caller's responsibility = to free the buffer. + certificate. It's caller's responsibility = to free the buffer + with Pkcs7FreeSigners(). @param[out] ChainLength Length of the chained certificates list bu= ffer in bytes. @param[out] UnchainCerts Pointer to the unchained certificates list= s. It's caller's - responsibility to free the buffer. + responsibility to free the buffer with Pkc= s7FreeSigners(). @param[out] UnchainLength Length of the unchained certificates list = buffer in bytes. =20 @retval TRUE The operation is finished successfully. @@ -142,7 +145,7 @@ Pkcs7Verify ( @param[in] P7Data Pointer to the PKCS#7 signed data to process. @param[in] P7Length Length of the PKCS#7 signed data in bytes. @param[out] Content Pointer to the extracted content from the PKCS= #7 signedData. - It's caller's responsibility to free the buffe= r. + It's caller's responsibility to free the buffe= r with FreePool(). @param[out] ContentSize The size of the extracted content in bytes. =20 @retval TRUE The P7Data was correctly formatted for process= ing. --=20 2.14.1.windows.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel