[Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Liming Gao]

https://bugzilla.tianocore.org/show_bug.cgi?id=686

Liming Gao (3):
  MdePkg: Add more checker in UefiDecompressLib to access the valid
    buffer only
  IntelFrameworkModulePkg: Add more checker in UefiTianoDecompressLib
  BaseTools: Add more checker in Decompress algorithm to access the
    valid buffer

 BaseTools/Source/C/Common/Decompress.c             | 23 +++++++++++++++++--
 BaseTools/Source/C/TianoCompress/TianoCompress.c   | 26 +++++++++++++++++++++-
 .../BaseUefiTianoCustomDecompressLib.c             | 16 +++++++++++--
 .../BaseUefiDecompressLib/BaseUefiDecompressLib.c  | 17 ++++++++++++--
 4 files changed, 75 insertions(+), 7 deletions(-)

-- 
2.10.0.windows.1

[Patch 1/3] MdePkg: Add more checker in UefiDecompressLib to access the valid buffer only

[Liming Gao]

https://bugzilla.tianocore.org/show_bug.cgi?id=686

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Holtsclaw Brent <brent.holtsclaw@intel.com>
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
 .../BaseUefiDecompressLib/BaseUefiDecompressLib.c       | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.c b/MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.c
index dc89157..9fc637e 100644
--- a/MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.c
+++ b/MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.c
@@ -152,6 +152,7 @@ MakeTable (
   UINT16  Mask;
   UINT16  WordOfStart;
   UINT16  WordOfCount;
+  UINT16  MaxTableLength;
 
   //
   // The maximum mapping table width supported by this internal
@@ -164,6 +165,9 @@ MakeTable (
   }
 
   for (Index = 0; Index < NumOfChar; Index++) {
+    if (BitLen[Index] > 16) {
+      return (UINT16) BAD_TABLE;
+    }
     Count[BitLen[Index]]++;
   }
 
@@ -205,6 +209,7 @@ MakeTable (
 
   Avail = NumOfChar;
   Mask  = (UINT16) (1U << (15 - TableBits));
+  MaxTableLength = (UINT16) (1U << TableBits);
 
   for (Char = 0; Char < NumOfChar; Char++) {
 
@@ -218,6 +223,9 @@ MakeTable (
     if (Len <= TableBits) {
 
       for (Index = Start[Len]; Index < NextCode; Index++) {
+        if (Index >= MaxTableLength) {
+          return (UINT16) BAD_TABLE;
+        }
         Table[Index] = Char;
       }
 
@@ -620,11 +628,16 @@ Decode (
       // Write BytesRemain of bytes into mDstBase
       //
       BytesRemain--;
+
       while ((INT16) (BytesRemain) >= 0) {
-        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
         if (Sd->mOutBuf >= Sd->mOrigSize) {
           goto Done;
         }
+        if (DataIdx >= Sd->mOrigSize) {
+          Sd->mBadTableFlag = (UINT16) BAD_TABLE;
+          goto Done;
+        }
+        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
 
         BytesRemain--;
       }
@@ -694,7 +707,7 @@ UefiDecompressGetInfo (
   }
 
   CompressedSize   = ReadUnaligned32 ((UINT32 *)Source);
-  if (SourceSize < (CompressedSize + 8)) {
+  if (SourceSize < (CompressedSize + 8) || (CompressedSize + 8) < 8) {
     return RETURN_INVALID_PARAMETER;
   }
 
-- 
2.10.0.windows.1

[Patch 2/3] IntelFrameworkModulePkg: Add more checker in UefiTianoDecompressLib

[Liming Gao]

https://bugzilla.tianocore.org/show_bug.cgi?id=686
To make sure the valid buffer be accessed only.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Holtsclaw Brent <brent.holtsclaw@intel.com>
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
 .../BaseUefiTianoCustomDecompressLib.c                   | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c b/IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c
index ab7987a..39999a0 100644
--- a/IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c
+++ b/IntelFrameworkModulePkg/Library/BaseUefiTianoCustomDecompressLib/BaseUefiTianoCustomDecompressLib.c
@@ -143,6 +143,7 @@ MakeTable (
   UINT16  Mask;
   UINT16  WordOfStart;
   UINT16  WordOfCount;
+  UINT16  MaxTableLength;
 
   //
   // The maximum mapping table width supported by this internal
@@ -155,6 +156,9 @@ MakeTable (
   }
 
   for (Index = 0; Index < NumOfChar; Index++) {
+    if (BitLen[Index] > 16) {
+      return (UINT16) BAD_TABLE;
+    }
     Count[BitLen[Index]]++;
   }
 
@@ -196,6 +200,7 @@ MakeTable (
 
   Avail = NumOfChar;
   Mask  = (UINT16) (1U << (15 - TableBits));
+  MaxTableLength = (UINT16) (1U << TableBits);
 
   for (Char = 0; Char < NumOfChar; Char++) {
 
@@ -209,6 +214,9 @@ MakeTable (
     if (Len <= TableBits) {
 
       for (Index = Start[Len]; Index < NextCode; Index++) {
+        if (Index >= MaxTableLength) {
+          return (UINT16) BAD_TABLE;
+        }
         Table[Index] = Char;
       }
 
@@ -615,10 +623,14 @@ Decode (
       //
       BytesRemain--;
       while ((INT16) (BytesRemain) >= 0) {
-        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
         if (Sd->mOutBuf >= Sd->mOrigSize) {
           goto Done ;
         }
+        if (DataIdx >= Sd->mOrigSize) {
+          Sd->mBadTableFlag = (UINT16) BAD_TABLE;
+          goto Done ;
+        }
+        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
 
         BytesRemain--;
       }
@@ -688,7 +700,7 @@ UefiDecompressGetInfo (
   }
 
   CompressedSize   = ReadUnaligned32 ((UINT32 *)Source);
-  if (SourceSize < (CompressedSize + 8)) {
+  if (SourceSize < (CompressedSize + 8) || (CompressedSize + 8) < 8) {
     return RETURN_INVALID_PARAMETER;
   }
 
-- 
2.10.0.windows.1

[Patch 3/3] BaseTools: Add more checker in Decompress algorithm to access the valid buffer

[Liming Gao]

https://bugzilla.tianocore.org/show_bug.cgi?id=686

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Holtsclaw Brent <brent.holtsclaw@intel.com>
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
 BaseTools/Source/C/Common/Decompress.c           | 23 +++++++++++++++++++--
 BaseTools/Source/C/TianoCompress/TianoCompress.c | 26 +++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/BaseTools/Source/C/Common/Decompress.c b/BaseTools/Source/C/Common/Decompress.c
index 9906888..71313b1 100644
--- a/BaseTools/Source/C/Common/Decompress.c
+++ b/BaseTools/Source/C/Common/Decompress.c
@@ -194,12 +194,16 @@ Returns:
   UINT16  Avail;
   UINT16  NextCode;
   UINT16  Mask;
+  UINT16  MaxTableLength;
 
   for (Index = 1; Index <= 16; Index++) {
     Count[Index] = 0;
   }
 
   for (Index = 0; Index < NumOfChar; Index++) {
+    if (BitLen[Index] > 16) {
+      return (UINT16) BAD_TABLE;
+    }
     Count[BitLen[Index]]++;
   }
 
@@ -237,6 +241,7 @@ Returns:
 
   Avail = NumOfChar;
   Mask  = (UINT16) (1U << (15 - TableBits));
+  MaxTableLength = (UINT16) (1U << TableBits);
 
   for (Char = 0; Char < NumOfChar; Char++) {
 
@@ -250,6 +255,9 @@ Returns:
     if (Len <= TableBits) {
 
       for (Index = Start[Len]; Index < NextCode; Index++) {
+        if (Index >= MaxTableLength) {
+          return (UINT16) BAD_TABLE;
+        }
         Table[Index] = Char;
       }
 
@@ -643,10 +651,14 @@ Returns: (VOID)
 
       BytesRemain--;
       while ((INT16) (BytesRemain) >= 0) {
-        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
         if (Sd->mOutBuf >= Sd->mOrigSize) {
           return ;
         }
+        if (DataIdx >= Sd->mOrigSize) {
+          Sd->mBadTableFlag = (UINT16) BAD_TABLE;
+          return ;
+        }
+        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
 
         BytesRemain--;
       }
@@ -684,6 +696,7 @@ Returns:
 --*/
 {
   UINT8 *Src;
+  UINT32 CompSize;
 
   *ScratchSize  = sizeof (SCRATCH_DATA);
 
@@ -692,7 +705,13 @@ Returns:
     return EFI_INVALID_PARAMETER;
   }
 
+  CompSize = Src[0] + (Src[1] << 8) + (Src[2] << 16) + (Src[3] << 24);
   *DstSize = Src[4] + (Src[5] << 8) + (Src[6] << 16) + (Src[7] << 24);
+
+  if (SrcSize < CompSize + 8 || (CompSize + 8) < 8) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   return EFI_SUCCESS;
 }
 
@@ -752,7 +771,7 @@ Returns:
   CompSize  = Src[0] + (Src[1] << 8) + (Src[2] << 16) + (Src[3] << 24);
   OrigSize  = Src[4] + (Src[5] << 8) + (Src[6] << 16) + (Src[7] << 24);
 
-  if (SrcSize < CompSize + 8) {
+  if (SrcSize < CompSize + 8 || (CompSize + 8) < 8) {
     return EFI_INVALID_PARAMETER;
   }
 
diff --git a/BaseTools/Source/C/TianoCompress/TianoCompress.c b/BaseTools/Source/C/TianoCompress/TianoCompress.c
index b88d7da..2d6fc4c 100644
--- a/BaseTools/Source/C/TianoCompress/TianoCompress.c
+++ b/BaseTools/Source/C/TianoCompress/TianoCompress.c
@@ -1757,6 +1757,7 @@ Returns:
   SCRATCH_DATA      *Scratch;
   UINT8      *Src;
   UINT32     OrigSize;
+  UINT32     CompSize;
 
   SetUtilityName(UTILITY_NAME);
 
@@ -1765,6 +1766,7 @@ Returns:
   OutBuffer = NULL;
   Scratch   = NULL;
   OrigSize = 0;
+  CompSize = 0;
   InputLength = 0;
   InputFileName = NULL;
   OutputFileName = NULL;
@@ -2006,15 +2008,24 @@ Returns:
     }
     fwrite(OutBuffer, (size_t)(DstSize), 1, OutputFile);
   } else {
+    if (InputLength < 8){
+      Error (NULL, 0, 3000, "Invalid", "The input file %s is too small.", InputFileName);
+      goto ERROR;
+    }
     //
     // Get Compressed file original size
     //
     Src     = (UINT8 *)FileBuffer;
     OrigSize  = Src[4] + (Src[5] << 8) + (Src[6] << 16) + (Src[7] << 24);
+    CompSize  = Src[0] + (Src[1] << 8) + (Src[2] <<16) + (Src[3] <<24);
 
     //
     // Allocate OutputBuffer
     //
+    if (InputLength < CompSize + 8 || (CompSize + 8) < 8) {
+      Error (NULL, 0, 3000, "Invalid", "The input file %s data is invalid.", InputFileName);
+      goto ERROR;
+    }
     OutBuffer = (UINT8 *)malloc(OrigSize);
     if (OutBuffer == NULL) {
       Error (NULL, 0, 4001, "Resource:", "Memory cannot be allocated!");
@@ -2204,12 +2215,16 @@ Returns:
   UINT16  Mask;
   UINT16  WordOfStart;
   UINT16  WordOfCount;
+  UINT16  MaxTableLength;
 
   for (Index = 0; Index <= 16; Index++) {
     Count[Index] = 0;
   }
 
   for (Index = 0; Index < NumOfChar; Index++) {
+    if (BitLen[Index] > 16) {
+      return (UINT16) BAD_TABLE;
+    }
     Count[BitLen[Index]]++;
   }
 
@@ -2253,6 +2268,7 @@ Returns:
 
   Avail = NumOfChar;
   Mask  = (UINT16) (1U << (15 - TableBits));
+  MaxTableLength = (UINT16) (1U << TableBits);
 
   for (Char = 0; Char < NumOfChar; Char++) {
 
@@ -2266,6 +2282,9 @@ Returns:
     if (Len <= TableBits) {
 
       for (Index = Start[Len]; Index < NextCode; Index++) {
+        if (Index >= MaxTableLength) {
+          return (UINT16) BAD_TABLE;
+        }
         Table[Index] = Char;
       }
 
@@ -2650,11 +2669,16 @@ Returns: (VOID)
       DataIdx     = Sd->mOutBuf - DecodeP (Sd) - 1;
 
       BytesRemain--;
+
       while ((INT16) (BytesRemain) >= 0) {
-        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
         if (Sd->mOutBuf >= Sd->mOrigSize) {
           goto Done ;
         }
+        if (DataIdx >= Sd->mOrigSize) {
+          Sd->mBadTableFlag = (UINT16) BAD_TABLE;
+          goto Done ;
+        }
+        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
 
         BytesRemain--;
       }
-- 
2.10.0.windows.1

Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Zeng, Star]

On 2018/10/16 10:06, Liming Gao wrote:
> https://bugzilla.tianocore.org/show_bug.cgi?id=686
> 
> Liming Gao (3):
>    MdePkg: Add more checker in UefiDecompressLib to access the valid
>      buffer only
>    IntelFrameworkModulePkg: Add more checker in UefiTianoDecompressLib
>    BaseTools: Add more checker in Decompress algorithm to access the
>      valid buffer

Hi Liming,

If these patches are not pushed yet, I am glad to broadcast the good 
request "add CVE number in subject line" from Laszlo at 
https://lists.01.org/pipermail/edk2-devel/2018-October/031031.html. :)


Thanks,
Star

> 
>   BaseTools/Source/C/Common/Decompress.c             | 23 +++++++++++++++++--
>   BaseTools/Source/C/TianoCompress/TianoCompress.c   | 26 +++++++++++++++++++++-
>   .../BaseUefiTianoCustomDecompressLib.c             | 16 +++++++++++--
>   .../BaseUefiDecompressLib/BaseUefiDecompressLib.c  | 17 ++++++++++++--
>   4 files changed, 75 insertions(+), 7 deletions(-)
> 

Re: [Patch 3/3] BaseTools: Add more checker in Decompress algorithm to access the valid buffer

[Zhu, Yonghong]

Reviewed-by: Yonghong Zhu <yonghong.zhu@intel.com> 

Best Regards,
Zhu Yonghong


-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Liming Gao
Sent: Tuesday, October 16, 2018 10:06 AM
To: edk2-devel@lists.01.org
Cc: Holtsclaw, Brent <brent.holtsclaw@intel.com>
Subject: [edk2] [Patch 3/3] BaseTools: Add more checker in Decompress algorithm to access the valid buffer

https://bugzilla.tianocore.org/show_bug.cgi?id=686

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Holtsclaw Brent <brent.holtsclaw@intel.com>
Signed-off-by: Liming Gao <liming.gao@intel.com>
---
 BaseTools/Source/C/Common/Decompress.c           | 23 +++++++++++++++++++--
 BaseTools/Source/C/TianoCompress/TianoCompress.c | 26 +++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/BaseTools/Source/C/Common/Decompress.c b/BaseTools/Source/C/Common/Decompress.c
index 9906888..71313b1 100644
--- a/BaseTools/Source/C/Common/Decompress.c
+++ b/BaseTools/Source/C/Common/Decompress.c
@@ -194,12 +194,16 @@ Returns:
   UINT16  Avail;
   UINT16  NextCode;
   UINT16  Mask;
+  UINT16  MaxTableLength;
 
   for (Index = 1; Index <= 16; Index++) {
     Count[Index] = 0;
   }
 
   for (Index = 0; Index < NumOfChar; Index++) {
+    if (BitLen[Index] > 16) {
+      return (UINT16) BAD_TABLE;
+    }
     Count[BitLen[Index]]++;
   }
 
@@ -237,6 +241,7 @@ Returns:
 
   Avail = NumOfChar;
   Mask  = (UINT16) (1U << (15 - TableBits));
+  MaxTableLength = (UINT16) (1U << TableBits);
 
   for (Char = 0; Char < NumOfChar; Char++) {
 
@@ -250,6 +255,9 @@ Returns:
     if (Len <= TableBits) {
 
       for (Index = Start[Len]; Index < NextCode; Index++) {
+        if (Index >= MaxTableLength) {
+          return (UINT16) BAD_TABLE;
+        }
         Table[Index] = Char;
       }
 
@@ -643,10 +651,14 @@ Returns: (VOID)
 
       BytesRemain--;
       while ((INT16) (BytesRemain) >= 0) {
-        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
         if (Sd->mOutBuf >= Sd->mOrigSize) {
           return ;
         }
+        if (DataIdx >= Sd->mOrigSize) {
+          Sd->mBadTableFlag = (UINT16) BAD_TABLE;
+          return ;
+        }
+        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
 
         BytesRemain--;
       }
@@ -684,6 +696,7 @@ Returns:
 --*/
 {
   UINT8 *Src;
+  UINT32 CompSize;
 
   *ScratchSize  = sizeof (SCRATCH_DATA);
 
@@ -692,7 +705,13 @@ Returns:
     return EFI_INVALID_PARAMETER;
   }
 
+  CompSize = Src[0] + (Src[1] << 8) + (Src[2] << 16) + (Src[3] << 24);
   *DstSize = Src[4] + (Src[5] << 8) + (Src[6] << 16) + (Src[7] << 24);
+
+  if (SrcSize < CompSize + 8 || (CompSize + 8) < 8) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   return EFI_SUCCESS;
 }
 
@@ -752,7 +771,7 @@ Returns:
   CompSize  = Src[0] + (Src[1] << 8) + (Src[2] << 16) + (Src[3] << 24);
   OrigSize  = Src[4] + (Src[5] << 8) + (Src[6] << 16) + (Src[7] << 24);
 
-  if (SrcSize < CompSize + 8) {
+  if (SrcSize < CompSize + 8 || (CompSize + 8) < 8) {
     return EFI_INVALID_PARAMETER;
   }
 
diff --git a/BaseTools/Source/C/TianoCompress/TianoCompress.c b/BaseTools/Source/C/TianoCompress/TianoCompress.c
index b88d7da..2d6fc4c 100644
--- a/BaseTools/Source/C/TianoCompress/TianoCompress.c
+++ b/BaseTools/Source/C/TianoCompress/TianoCompress.c
@@ -1757,6 +1757,7 @@ Returns:
   SCRATCH_DATA      *Scratch;
   UINT8      *Src;
   UINT32     OrigSize;
+  UINT32     CompSize;
 
   SetUtilityName(UTILITY_NAME);
 
@@ -1765,6 +1766,7 @@ Returns:
   OutBuffer = NULL;
   Scratch   = NULL;
   OrigSize = 0;
+  CompSize = 0;
   InputLength = 0;
   InputFileName = NULL;
   OutputFileName = NULL;
@@ -2006,15 +2008,24 @@ Returns:
     }
     fwrite(OutBuffer, (size_t)(DstSize), 1, OutputFile);
   } else {
+    if (InputLength < 8){
+      Error (NULL, 0, 3000, "Invalid", "The input file %s is too small.", InputFileName);
+      goto ERROR;
+    }
     //
     // Get Compressed file original size
     //
     Src     = (UINT8 *)FileBuffer;
     OrigSize  = Src[4] + (Src[5] << 8) + (Src[6] << 16) + (Src[7] << 24);
+    CompSize  = Src[0] + (Src[1] << 8) + (Src[2] <<16) + (Src[3] <<24);
 
     //
     // Allocate OutputBuffer
     //
+    if (InputLength < CompSize + 8 || (CompSize + 8) < 8) {
+      Error (NULL, 0, 3000, "Invalid", "The input file %s data is invalid.", InputFileName);
+      goto ERROR;
+    }
     OutBuffer = (UINT8 *)malloc(OrigSize);
     if (OutBuffer == NULL) {
       Error (NULL, 0, 4001, "Resource:", "Memory cannot be allocated!"); @@ -2204,12 +2215,16 @@ Returns:
   UINT16  Mask;
   UINT16  WordOfStart;
   UINT16  WordOfCount;
+  UINT16  MaxTableLength;
 
   for (Index = 0; Index <= 16; Index++) {
     Count[Index] = 0;
   }
 
   for (Index = 0; Index < NumOfChar; Index++) {
+    if (BitLen[Index] > 16) {
+      return (UINT16) BAD_TABLE;
+    }
     Count[BitLen[Index]]++;
   }
 
@@ -2253,6 +2268,7 @@ Returns:
 
   Avail = NumOfChar;
   Mask  = (UINT16) (1U << (15 - TableBits));
+  MaxTableLength = (UINT16) (1U << TableBits);
 
   for (Char = 0; Char < NumOfChar; Char++) {
 
@@ -2266,6 +2282,9 @@ Returns:
     if (Len <= TableBits) {
 
       for (Index = Start[Len]; Index < NextCode; Index++) {
+        if (Index >= MaxTableLength) {
+          return (UINT16) BAD_TABLE;
+        }
         Table[Index] = Char;
       }
 
@@ -2650,11 +2669,16 @@ Returns: (VOID)
       DataIdx     = Sd->mOutBuf - DecodeP (Sd) - 1;
 
       BytesRemain--;
+
       while ((INT16) (BytesRemain) >= 0) {
-        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
         if (Sd->mOutBuf >= Sd->mOrigSize) {
           goto Done ;
         }
+        if (DataIdx >= Sd->mOrigSize) {
+          Sd->mBadTableFlag = (UINT16) BAD_TABLE;
+          goto Done ;
+        }
+        Sd->mDstBase[Sd->mOutBuf++] = Sd->mDstBase[DataIdx++];
 
         BytesRemain--;
       }
--
2.10.0.windows.1

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

Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Laszlo Ersek]

On 10/18/18 05:04, Zeng, Star wrote:
> On 2018/10/16 10:06, Liming Gao wrote:
>> https://bugzilla.tianocore.org/show_bug.cgi?id=686
>>
>> Liming Gao (3):
>>    MdePkg: Add more checker in UefiDecompressLib to access the valid
>>      buffer only
>>    IntelFrameworkModulePkg: Add more checker in UefiTianoDecompressLib
>>    BaseTools: Add more checker in Decompress algorithm to access the
>>      valid buffer
> 
> Hi Liming,
> 
> If these patches are not pushed yet, I am glad to broadcast the good
> request "add CVE number in subject line" from Laszlo at
> https://lists.01.org/pipermail/edk2-devel/2018-October/031031.html. :)

Indeed; I now see on the BZ that no fewer than five CVEs are associated
with this series / BZ. Thank you Star for pointing that out.

Can we get a more detailed attack / threat analysis on the BZ, please?
Comment 7 says, "Impact: Elevation of Privilege". What does that mean
precisely?

For example, I'd like to evaluate the practical impact on ArmVirtQemu
and OVMF. From the build reports, those platforms use the
UefiDecompressLib class in "DxeIpl.inf" and "DxeMain.inf" only.

In turn, the DXE IPL PEIM seems to expose the UEFI decompress facility
via EFI_PEI_DECOMPRESS_PPI, and the DXE core does the same via
EFI_DECOMPRESS_PROTOCOL.

I don't think 3rd party drivers / applications / OS-es have access to
the PEI phase, so I think a buffer overflow in EFI_PEI_DECOMPRESS_PPI
might be exploitable. (Or is perhaps EFI_PEI_DECOMPRESS_PPI used for
update capsule processing on some platforms?)

Regarding EFI_DECOMPRESS_PROTOCOL; any 3rd party UEFI driver or app can
locate and call it. But how can the protocol's vulnerability exploited
for "Elevation of Privilege"? Can it be used to attack SMM somehow? I
don't see any SMM module in the edk2 tree consuming
"gEfiDecompressProtocolGuid".

Is UefiDecompressLib perhaps used for extracting GUID-ed sections as
well (on some other platforms)?

Thanks
Laszlo

Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Gao, Liming]

Laszlo and Star:
  Thank your notes. I will add CVE number in patch subject although it will make subject long than 80 characters. Here is my proposed patch subject: CVE-2017-5731..5735 MdePkg: Add more checker in UefiDecompressLib to access the valid buffer only. 

  In PEI phase, the recovery image is from the external device. If the recovery image has the corrupt EFI compression section, they will be handled by EFI Decompression PPI. In DXE phase, UEFI option ROM is the third party code. If it is EFI compression option ROM, EFI decompression protocol will be used to decode its data. I don't think SMM uses EFI decompression protocol. UefiDecompressionLib is used as EFI compression PPI/Protocol. It matches PI EFI compression section instead of GUID section. So, it has no GUID extraction PPI/Protocol. 

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, October 18, 2018 9:03 PM
> To: Zeng, Star <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [Patch 0/3] Add more checker for Tianocompress and Ueficompress
> 
> On 10/18/18 05:04, Zeng, Star wrote:
> > On 2018/10/16 10:06, Liming Gao wrote:
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=686
> >>
> >> Liming Gao (3):
> >>    MdePkg: Add more checker in UefiDecompressLib to access the valid
> >>      buffer only
> >>    IntelFrameworkModulePkg: Add more checker in UefiTianoDecompressLib
> >>    BaseTools: Add more checker in Decompress algorithm to access the
> >>      valid buffer
> >
> > Hi Liming,
> >
> > If these patches are not pushed yet, I am glad to broadcast the good
> > request "add CVE number in subject line" from Laszlo at
> > https://lists.01.org/pipermail/edk2-devel/2018-October/031031.html. :)
> 
> Indeed; I now see on the BZ that no fewer than five CVEs are associated
> with this series / BZ. Thank you Star for pointing that out.
> 
> Can we get a more detailed attack / threat analysis on the BZ, please?
> Comment 7 says, "Impact: Elevation of Privilege". What does that mean
> precisely?
> 
> For example, I'd like to evaluate the practical impact on ArmVirtQemu
> and OVMF. From the build reports, those platforms use the
> UefiDecompressLib class in "DxeIpl.inf" and "DxeMain.inf" only.
> 
> In turn, the DXE IPL PEIM seems to expose the UEFI decompress facility
> via EFI_PEI_DECOMPRESS_PPI, and the DXE core does the same via
> EFI_DECOMPRESS_PROTOCOL.
> 
> I don't think 3rd party drivers / applications / OS-es have access to
> the PEI phase, so I think a buffer overflow in EFI_PEI_DECOMPRESS_PPI
> might be exploitable. (Or is perhaps EFI_PEI_DECOMPRESS_PPI used for
> update capsule processing on some platforms?)
> 
> Regarding EFI_DECOMPRESS_PROTOCOL; any 3rd party UEFI driver or app can
> locate and call it. But how can the protocol's vulnerability exploited
> for "Elevation of Privilege"? Can it be used to attack SMM somehow? I
> don't see any SMM module in the edk2 tree consuming
> "gEfiDecompressProtocolGuid".
> 
> Is UefiDecompressLib perhaps used for extracting GUID-ed sections as
> well (on some other platforms)?
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Laszlo Ersek]

On 10/18/18 15:36, Gao, Liming wrote:
> Laszlo and Star:
>   Thank your notes. I will add CVE number in patch subject although it
>   will make subject long than 80 characters.

I agree the subject will be overlong, but I also think that including
the CVE numbers is important enough for that.

> Here is my proposed patch subject: CVE-2017-5731..5735 MdePkg: Add
> more checker in UefiDecompressLib to access the valid buffer only.

I suggest (based on tradition) that we keep the normal subject at the
front, and then we append the CVE numbers at the end. Also, we should
spell out all those CVE identifiers individually, if the same patch
solves them all. It should be possible to search the subject line for
any one of these CVE numbers in separation, using the official CVE
number format.

>   In PEI phase, the recovery image is from the external device. If the
>   recovery image has the corrupt EFI compression section, they will be
>   handled by EFI Decompression PPI.

In the PEI phase, if the recovery image is crafted, it could cause a
buffer overflow during decompression. However, if the recovery image is
crafted, it might as well decompress cleanly, and once it is dispatched,
do "bad things". Do the decompression and the dispatch occur at
different privilege levels?

> In DXE phase, UEFI option ROM is the third party code. If it is EFI
> compression option ROM, EFI decompression protocol will be used to
> decode its data. I don't think SMM uses EFI decompression protocol.
> UefiDecompressionLib is used as EFI compression PPI/Protocol. It
> matches PI EFI compression section instead of GUID section. So, it has
> no GUID extraction PPI/Protocol.

In the DXE phase, if the option ROM is crafted, it could cause a buffer
overflow when it is decompressed. But, again, how is that different from
when a crafted oprom decompresses cleanly, and then does "bad things"
when it is dispatched?

Here (in the DXE phase), I can imagine two answers myself:

(1) Decompression occurs before Secure Boot validation, but dispatch
occurs only after. Therefore a crafted UEFI image could cause problems
via decompression even if it would fail SB verification later.

(2) Decompression of UEFI option ROMs occurs before PlatformBDS locks
down SMRAM and lockboxes. However, the execution of UEFI option ROMs
is deferred until after the lockdown.

Do these scenarios apply? Because, if they do, I agree the issue
qualifies as privilege escalation.

Thank you!
Laszlo

Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Gao, Liming]

Laszlo: 
  I try to answer your question. I also include the BZ submitter brent.holtsclaw@intel.com. Holtsclaw, please add your comments if my info is not enough. 

Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Friday, October 19, 2018 12:01 AM
>To: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>;
>edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Subject: Re: [edk2] [Patch 0/3] Add more checker for Tianocompress and
>Ueficompress
>
>On 10/18/18 15:36, Gao, Liming wrote:
>> Laszlo and Star:
>>   Thank your notes. I will add CVE number in patch subject although it
>>   will make subject long than 80 characters.
>
>I agree the subject will be overlong, but I also think that including
>the CVE numbers is important enough for that.
>
>> Here is my proposed patch subject: CVE-2017-5731..5735 MdePkg: Add
>> more checker in UefiDecompressLib to access the valid buffer only.
>
>I suggest (based on tradition) that we keep the normal subject at the
>front, and then we append the CVE numbers at the end. Also, we should
>spell out all those CVE identifiers individually, if the same patch
>solves them all. It should be possible to search the subject line for
>any one of these CVE numbers in separation, using the official CVE
>number format.
>

So, your proposal is like:  MdePkg: Add more checker in UefiDecompressLib to access the valid buffer only CVE-2017-5731 CVE-2017-5732 CVE-2017-5733 CVE-2017-5734 CVE-2017-5735

>>   In PEI phase, the recovery image is from the external device. If the
>>   recovery image has the corrupt EFI compression section, they will be
>>   handled by EFI Decompression PPI.
>
>In the PEI phase, if the recovery image is crafted, it could cause a
>buffer overflow during decompression. However, if the recovery image is
>crafted, it might as well decompress cleanly, and once it is dispatched,
>do "bad things". Do the decompression and the dispatch occur at
>different privilege levels?
>

This patch focuses on the wrong decompression data that cause the decompression failure or hang.  The data content can be signed and verified.  

>> In DXE phase, UEFI option ROM is the third party code. If it is EFI
>> compression option ROM, EFI decompression protocol will be used to
>> decode its data. I don't think SMM uses EFI decompression protocol.
>> UefiDecompressionLib is used as EFI compression PPI/Protocol. It
>> matches PI EFI compression section instead of GUID section. So, it has
>> no GUID extraction PPI/Protocol.
>
>In the DXE phase, if the option ROM is crafted, it could cause a buffer
>overflow when it is decompressed. But, again, how is that different from
>when a crafted oprom decompresses cleanly, and then does "bad things"
>when it is dispatched?
>
>Here (in the DXE phase), I can imagine two answers myself:
>
>(1) Decompression occurs before Secure Boot validation, but dispatch
>occurs only after. Therefore a crafted UEFI image could cause problems
>via decompression even if it would fail SB verification later.
>
>(2) Decompression of UEFI option ROMs occurs before PlatformBDS locks
>down SMRAM and lockboxes. However, the execution of UEFI option ROMs
>is deferred until after the lockdown.
>
>Do these scenarios apply? Because, if they do, I agree the issue
>qualifies as privilege escalation.
>

Yes. Decompression happen early. After decompression, PE image will be verified. 

>Thank you!
>Laszlo

Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Laszlo Ersek]

On 10/19/18 08:40, Gao, Liming wrote:
> Laszlo:
>   I try to answer your question. I also include the BZ submitter
>   brent.holtsclaw@intel.com. Holtsclaw, please add your comments if my
>   info is not enough.
>
> Thanks
> Liming
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Friday, October 19, 2018 12:01 AM
>> To: Gao, Liming <liming.gao@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>> Subject: Re: [edk2] [Patch 0/3] Add more checker for Tianocompress
>> and Ueficompress
>>
>> On 10/18/18 15:36, Gao, Liming wrote:
>>> Laszlo and Star:
>>>   Thank your notes. I will add CVE number in patch subject although
>>>   it will make subject long than 80 characters.
>>
>> I agree the subject will be overlong, but I also think that including
>> the CVE numbers is important enough for that.
>>
>>> Here is my proposed patch subject: CVE-2017-5731..5735 MdePkg: Add
>>> more checker in UefiDecompressLib to access the valid buffer only.
>>
>> I suggest (based on tradition) that we keep the normal subject at the
>> front, and then we append the CVE numbers at the end. Also, we should
>> spell out all those CVE identifiers individually, if the same patch
>> solves them all. It should be possible to search the subject line for
>> any one of these CVE numbers in separation, using the official CVE
>> number format.
>>
>
> So, your proposal is like:  MdePkg: Add more checker in
> UefiDecompressLib to access the valid buffer only CVE-2017-5731
> CVE-2017-5732 CVE-2017-5733 CVE-2017-5734 CVE-2017-5735

Yes:

  MdePkg: Add more checker in UefiDecompressLib to access the valid buffer only (CVE-2017-5731 CVE-2017-5732 CVE-2017-5733 CVE-2017-5734 CVE-2017-5735)

It looks terrible, but the real subject is still readable to the left,
and subjects with searchable CVE numbers take priority (in my opinion
anyway).

Actually: I wonder why we needed five different CVEs, if they can all be
fixed with a small, single patch.

More precisely: looking at the patch in more detail, I see that the
patch fixes multiple functions / separate buffer overflows. Is it
possible to associate each CVE with a specific, small code change in the
patch? Because if it is possible, then I think we should split the patch
*per CVE*. The subjects would go:

- MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5731)
- MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5732)
- MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5733)
- MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5734)
- MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5735)

(71 characters, in each subject)

If such separation is technically possible, then I think it would be an
improvement; minimally for documentation purposes.

>>>   In PEI phase, the recovery image is from the external device. If
>>>   the recovery image has the corrupt EFI compression section, they
>>>   will be handled by EFI Decompression PPI.
>>
>> In the PEI phase, if the recovery image is crafted, it could cause a
>> buffer overflow during decompression. However, if the recovery image
>> is crafted, it might as well decompress cleanly, and once it is
>> dispatched, do "bad things". Do the decompression and the dispatch
>> occur at different privilege levels?
>>
>
> This patch focuses on the wrong decompression data that cause the
> decompression failure or hang.  The data content can be signed and
> verified.
>
>>> In DXE phase, UEFI option ROM is the third party code. If it is EFI
>>> compression option ROM, EFI decompression protocol will be used to
>>> decode its data. I don't think SMM uses EFI decompression protocol.
>>> UefiDecompressionLib is used as EFI compression PPI/Protocol. It
>>> matches PI EFI compression section instead of GUID section. So, it
>>> has no GUID extraction PPI/Protocol.
>>
>> In the DXE phase, if the option ROM is crafted, it could cause a
>> buffer overflow when it is decompressed. But, again, how is that
>> different from when a crafted oprom decompresses cleanly, and then
>> does "bad things" when it is dispatched?
>>
>> Here (in the DXE phase), I can imagine two answers myself:
>>
>> (1) Decompression occurs before Secure Boot validation, but dispatch
>> occurs only after. Therefore a crafted UEFI image could cause
>> problems via decompression even if it would fail SB verification
>> later.
>>
>> (2) Decompression of UEFI option ROMs occurs before PlatformBDS locks
>> down SMRAM and lockboxes. However, the execution of UEFI option ROMs
>> is deferred until after the lockdown.
>>
>> Do these scenarios apply? Because, if they do, I agree the issue
>> qualifies as privilege escalation.
>>
>
> Yes. Decompression happen early. After decompression, PE image will be
> verified.

Got it now. Thanks!
Laszlo

Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Gao, Liming]

Laszlo:

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, October 19, 2018 7:25 PM
> To: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Holtsclaw, Brent <brent.holtsclaw@intel.com>
> Subject: Re: [edk2] [Patch 0/3] Add more checker for Tianocompress and Ueficompress
> 
> On 10/19/18 08:40, Gao, Liming wrote:
> > Laszlo:
> >   I try to answer your question. I also include the BZ submitter
> >   brent.holtsclaw@intel.com. Holtsclaw, please add your comments if my
> >   info is not enough.
> >
> > Thanks
> > Liming
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Friday, October 19, 2018 12:01 AM
> >> To: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> >> <star.zeng@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [Patch 0/3] Add more checker for Tianocompress
> >> and Ueficompress
> >>
> >> On 10/18/18 15:36, Gao, Liming wrote:
> >>> Laszlo and Star:
> >>>   Thank your notes. I will add CVE number in patch subject although
> >>>   it will make subject long than 80 characters.
> >>
> >> I agree the subject will be overlong, but I also think that including
> >> the CVE numbers is important enough for that.
> >>
> >>> Here is my proposed patch subject: CVE-2017-5731..5735 MdePkg: Add
> >>> more checker in UefiDecompressLib to access the valid buffer only.
> >>
> >> I suggest (based on tradition) that we keep the normal subject at the
> >> front, and then we append the CVE numbers at the end. Also, we should
> >> spell out all those CVE identifiers individually, if the same patch
> >> solves them all. It should be possible to search the subject line for
> >> any one of these CVE numbers in separation, using the official CVE
> >> number format.
> >>
> >
> > So, your proposal is like:  MdePkg: Add more checker in
> > UefiDecompressLib to access the valid buffer only CVE-2017-5731
> > CVE-2017-5732 CVE-2017-5733 CVE-2017-5734 CVE-2017-5735
> 
> Yes:
> 
>   MdePkg: Add more checker in UefiDecompressLib to access the valid buffer only (CVE-2017-5731 CVE-2017-5732 CVE-2017-5733
> CVE-2017-5734 CVE-2017-5735)
> 
> It looks terrible, but the real subject is still readable to the left,
> and subjects with searchable CVE numbers take priority (in my opinion
> anyway).
> 
> Actually: I wonder why we needed five different CVEs, if they can all be
> fixed with a small, single patch.
> 
> More precisely: looking at the patch in more detail, I see that the
> patch fixes multiple functions / separate buffer overflows. Is it
> possible to associate each CVE with a specific, small code change in the
> patch? Because if it is possible, then I think we should split the patch
> *per CVE*. The subjects would go:
> 
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5731)
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5732)
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5733)
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5734)
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5735)
> 
> (71 characters, in each subject)
> 
> If such separation is technically possible, then I think it would be an
> improvement; minimally for documentation purposes.
> 

I don't find the detail information for each CVE. BZ 686 attaches one doc to list all issues. So, I fix them together. I think one patch is allowed to include more than one CVEs. Even if with single CVE, patch subject may be longer than 80 characters. If we need strictly follow subject length rule, I suggest to mention CVE FIX in subject, and list CVE number info in the commit message. User can use git command to get full commit log and know which commit is CVE fix. For example:
MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE FIX)

> >>>   In PEI phase, the recovery image is from the external device. If
> >>>   the recovery image has the corrupt EFI compression section, they
> >>>   will be handled by EFI Decompression PPI.
> >>
> >> In the PEI phase, if the recovery image is crafted, it could cause a
> >> buffer overflow during decompression. However, if the recovery image
> >> is crafted, it might as well decompress cleanly, and once it is
> >> dispatched, do "bad things". Do the decompression and the dispatch
> >> occur at different privilege levels?
> >>
> >
> > This patch focuses on the wrong decompression data that cause the
> > decompression failure or hang.  The data content can be signed and
> > verified.
> >
> >>> In DXE phase, UEFI option ROM is the third party code. If it is EFI
> >>> compression option ROM, EFI decompression protocol will be used to
> >>> decode its data. I don't think SMM uses EFI decompression protocol.
> >>> UefiDecompressionLib is used as EFI compression PPI/Protocol. It
> >>> matches PI EFI compression section instead of GUID section. So, it
> >>> has no GUID extraction PPI/Protocol.
> >>
> >> In the DXE phase, if the option ROM is crafted, it could cause a
> >> buffer overflow when it is decompressed. But, again, how is that
> >> different from when a crafted oprom decompresses cleanly, and then
> >> does "bad things" when it is dispatched?
> >>
> >> Here (in the DXE phase), I can imagine two answers myself:
> >>
> >> (1) Decompression occurs before Secure Boot validation, but dispatch
> >> occurs only after. Therefore a crafted UEFI image could cause
> >> problems via decompression even if it would fail SB verification
> >> later.
> >>
> >> (2) Decompression of UEFI option ROMs occurs before PlatformBDS locks
> >> down SMRAM and lockboxes. However, the execution of UEFI option ROMs
> >> is deferred until after the lockdown.
> >>
> >> Do these scenarios apply? Because, if they do, I agree the issue
> >> qualifies as privilege escalation.
> >>
> >
> > Yes. Decompression happen early. After decompression, PE image will be
> > verified.
> 
> Got it now. Thanks!
> Laszlo

Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress

[Laszlo Ersek]

On 10/19/18 16:40, Gao, Liming wrote:

> 
> I don't find the detail information for each CVE. BZ 686 attaches one
> doc to list all issues. So, I fix them together. I think one patch is
> allowed to include more than one CVEs. Even if with single CVE, patch
> subject may be longer than 80 characters. If we need strictly follow
> subject length rule, I suggest to mention CVE FIX in subject, and
> list CVE number info in the commit message. User can use git command
> to get full commit log and know which commit is CVE fix. For
> example: MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE
> FIX)

OK.

Thanks!
Laszlo