[PATCH] CryptoPkg/BaseCryptLib: Fix potential integer overflow issue.

[PATCH] CryptoPkg/BaseCryptLib: Fix potential integer overflow issue.

[Long Qin]

The LookupFreeMemRegion() in RuntimeMemAllocate.c is used to look-up
free memory region for runtime resource allocation, which was designed
to support runtime authenticated variable service.
The direct offset subtractions in this function may bring possible
integer overflow issue.

This patch is to add the extra parameter checks to remove this possible
overflow risk.

Cc: Ye Ting <ting.ye@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Long Qin <qin.long@intel.com>
---
 .../Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c    | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
index 463f2bf855..92bb9ddccd 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
@@ -2,7 +2,7 @@
   Light-weight Memory Management Routines for OpenSSL-based Crypto
   Library at Runtime Phase.
 
-Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -141,6 +141,12 @@ LookupFreeMemRegion (
 
   StartPageIndex = RT_SIZE_TO_PAGES (mRTPageTable->LastEmptyPageOffset);
   ReqPages       = RT_SIZE_TO_PAGES (AllocationSize);
+  if (ReqPages > mRTPageTable->PageCount) {
+    //
+    // No enough region for object allocation.
+    //
+    return (UINTN)(-1);
+  }
 
   //
   // Look up the free memory region with in current memory map table.
@@ -176,6 +182,12 @@ LookupFreeMemRegion (
   // Look up the free memory region from the beginning of the memory table
   // until the StartCursorOffset
   //
+  if (ReqPages > StartPageIndex) {
+    //
+    // No enough region for object allocation.
+    //
+    return (UINTN)(-1);
+  }
   for (Index = 0; Index < (StartPageIndex - ReqPages); ) {
     //
     // Check Consecutive ReqPages Pages.
-- 
2.16.1.windows.1

Re: [PATCH] CryptoPkg/BaseCryptLib: Fix potential integer overflow issue.

[Laszlo Ersek]

On 10/24/18 15:22, Long Qin wrote:
> The LookupFreeMemRegion() in RuntimeMemAllocate.c is used to look-up
> free memory region for runtime resource allocation, which was designed
> to support runtime authenticated variable service.
> The direct offset subtractions in this function may bring possible
> integer overflow issue.
> 
> This patch is to add the extra parameter checks to remove this possible
> overflow risk.
> 
> Cc: Ye Ting <ting.ye@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Long Qin <qin.long@intel.com>
> ---
>  .../Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c    | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
> index 463f2bf855..92bb9ddccd 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
> @@ -2,7 +2,7 @@
>    Light-weight Memory Management Routines for OpenSSL-based Crypto
>    Library at Runtime Phase.
>  
> -Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -141,6 +141,12 @@ LookupFreeMemRegion (
>  
>    StartPageIndex = RT_SIZE_TO_PAGES (mRTPageTable->LastEmptyPageOffset);
>    ReqPages       = RT_SIZE_TO_PAGES (AllocationSize);
> +  if (ReqPages > mRTPageTable->PageCount) {
> +    //
> +    // No enough region for object allocation.
> +    //
> +    return (UINTN)(-1);
> +  }
>  
>    //
>    // Look up the free memory region with in current memory map table.
> @@ -176,6 +182,12 @@ LookupFreeMemRegion (
>    // Look up the free memory region from the beginning of the memory table
>    // until the StartCursorOffset
>    //
> +  if (ReqPages > StartPageIndex) {
> +    //
> +    // No enough region for object allocation.
> +    //
> +    return (UINTN)(-1);
> +  }
>    for (Index = 0; Index < (StartPageIndex - ReqPages); ) {
>      //
>      // Check Consecutive ReqPages Pages.
> 

As far as I can see, "RuntimeCryptLib.inf" (where this file is used) is
only linked into runtime DXE modules -- not SMM modules. That means this
issue is not a security bug, because runtime DXE modules can be
overwritten by the OS anyway. (They reside in normal RAM.) Can you
please confirm?

Nonetheless, it would be nice to explain in the commit message, what
exactly "ReqPages" depends on.

If needed, please file a BZ as well. (I'm not saying it's required, but
you might want to consider it, and reference it in the commit message.)

Thanks
Laszlo

Re: [PATCH] CryptoPkg/BaseCryptLib: Fix potential integer overflow issue.

[Long, Qin]

Thanks, Laszlo.

From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, October 25, 2018 12:59 AM
To: Long, Qin <qin.long@intel.com>; edk2-devel@lists.01.org
Cc: Ye, Ting <ting.ye@intel.com>
Subject: Re: [edk2] [PATCH] CryptoPkg/BaseCryptLib: Fix potential integer overflow issue.

On 10/24/18 15:22, Long Qin wrote:
> The LookupFreeMemRegion() in RuntimeMemAllocate.c is used to look-up
> free memory region for runtime resource allocation, which was designed
> to support runtime authenticated variable service.
> The direct offset subtractions in this function may bring possible
> integer overflow issue.
>
> This patch is to add the extra parameter checks to remove this possible
> overflow risk.
>
> Cc: Ye Ting <ting.ye@intel.com<mailto:ting.ye@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Long Qin <qin.long@intel.com<mailto:qin.long@intel.com>>
> ---
>  .../Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c    | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
> index 463f2bf855..92bb9ddccd 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
> @@ -2,7 +2,7 @@
>    Light-weight Memory Management Routines for OpenSSL-based Crypto
>    Library at Runtime Phase.
>
> -Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD License
>  which accompanies this distribution.  The full text of the license may be found at
> @@ -141,6 +141,12 @@ LookupFreeMemRegion (
>
>    StartPageIndex = RT_SIZE_TO_PAGES (mRTPageTable->LastEmptyPageOffset);
>    ReqPages       = RT_SIZE_TO_PAGES (AllocationSize);
> +  if (ReqPages > mRTPageTable->PageCount) {
> +    //
> +    // No enough region for object allocation.
> +    //
> +    return (UINTN)(-1);
> +  }
>
>    //
>    // Look up the free memory region with in current memory map table.
> @@ -176,6 +182,12 @@ LookupFreeMemRegion (
>    // Look up the free memory region from the beginning of the memory table
>    // until the StartCursorOffset
>    //
> +  if (ReqPages > StartPageIndex) {
> +    //
> +    // No enough region for object allocation.
> +    //
> +    return (UINTN)(-1);
> +  }
>    for (Index = 0; Index < (StartPageIndex - ReqPages); ) {
>      //
>      // Check Consecutive ReqPages Pages.
>

As far as I can see, "RuntimeCryptLib.inf" (where this file is used) is
only linked into runtime DXE modules -- not SMM modules. That means this
issue is not a security bug, because runtime DXE modules can be
overwritten by the OS anyway. (They reside in normal RAM.) Can you
please confirm?

[qlong] Yes, this library instance is only linked into runtime DXE driver, not SMM.
It was designed to provide the runtime authentication / verification support
(for variable service) in early implementation (non-SMM variable driver).
But the memory used in runtime dxe modules will not overwritten since
It was marked as “EfiRuntimeServicesData”. The RuntimeCryptLib applied
one light-weight memory management routines to meet the internal memory
allocation / free usage when openssl handle PKCS7 verification.
The possible integer overflow issue was found from code review. Yes, I think
it’s low risk since most runtime variable service was updated to use smm solution.

Nonetheless, it would be nice to explain in the commit message, what
exactly "ReqPages" depends on.
[qlong] ReqPages is one variable to describe the required pages for memory allocation
(from the malloc() call in OpenSSL codes when handling pkcs7 verification).
It’s hard to state the specific dependency (which include the PKCS7 data and some
openssl internal data structure).

If needed, please file a BZ as well. (I'm not saying it's required, but
you might want to consider it, and reference it in the commit message.)

[qlong] Sure. It make sense.
             And create one: https://bugzilla.tianocore.org/show_bug.cgi?id=1275

Thanks
Laszlo

Re: [PATCH] CryptoPkg/BaseCryptLib: Fix potential integer overflow issue.

[Ye, Ting]

Reviewed-by: Ye Ting <ting.ye@intel.com> 

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Long Qin
Sent: Wednesday, October 24, 2018 9:22 PM
To: edk2-devel@lists.01.org
Cc: Ye, Ting <ting.ye@intel.com>
Subject: [edk2] [PATCH] CryptoPkg/BaseCryptLib: Fix potential integer overflow issue.

The LookupFreeMemRegion() in RuntimeMemAllocate.c is used to look-up free memory region for runtime resource allocation, which was designed to support runtime authenticated variable service.
The direct offset subtractions in this function may bring possible integer overflow issue.

This patch is to add the extra parameter checks to remove this possible overflow risk.

Cc: Ye Ting <ting.ye@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Long Qin <qin.long@intel.com>
---
 .../Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c    | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
index 463f2bf855..92bb9ddccd 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
@@ -2,7 +2,7 @@
   Light-weight Memory Management Routines for OpenSSL-based Crypto
   Library at Runtime Phase.
 
-Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials  are licensed and made available under the terms and conditions of the BSD License  which accompanies this distribution.  The full text of the license may be found at @@ -141,6 +141,12 @@ LookupFreeMemRegion (
 
   StartPageIndex = RT_SIZE_TO_PAGES (mRTPageTable->LastEmptyPageOffset);
   ReqPages       = RT_SIZE_TO_PAGES (AllocationSize);
+  if (ReqPages > mRTPageTable->PageCount) {
+    //
+    // No enough region for object allocation.
+    //
+    return (UINTN)(-1);
+  }
 
   //
   // Look up the free memory region with in current memory map table.
@@ -176,6 +182,12 @@ LookupFreeMemRegion (
   // Look up the free memory region from the beginning of the memory table
   // until the StartCursorOffset
   //
+  if (ReqPages > StartPageIndex) {
+    //
+    // No enough region for object allocation.
+    //
+    return (UINTN)(-1);
+  }
   for (Index = 0; Index < (StartPageIndex - ReqPages); ) {
     //
     // Check Consecutive ReqPages Pages.
--
2.16.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel