From 29c394f110b1f769e629e8775874261e33d4abd9 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Wed, 24 Oct 2018 21:03:45 +0200
Subject: [PATCH 4/4] BaseTools: Add more checker in Decompress algorithm to
access the valid buffer (CVE FIX)
Message-id: <20181024190345.15288-5-lersek@redhat.com>
Patchwork-id: 82886
O-Subject: [RHEL8 edk2 PATCH 4/4] BaseTools: Add more checker in Decompress
algorithm to access the valid buffer (CVE FIX)
Bugzilla: 1641445
1641453
1641464
1641469
Acked-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Acked-by: Thomas Huth <thuth@redhat.com>
From: Liming Gao <liming.gao@intel.com>
--v-- RHEL8 note start --v--
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1641445
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1641453
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1641464
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1641469
Unfortunately, the upstream patch series was not structured according to
the CVE reports. This patch contributes to fixing:
- CVE-2017-5731
- CVE-2017-5733
- CVE-2017-5734
- CVE-2017-5735
but not CVE-2017-5732 (contrarily to the upstream commit message). The
best I could achieve up-stream was to get the "CVE FIX" expression into
the subject, and a whole-sale dump of the CVEs into the body. I had not
been invited to the original (off-list, embargoed) analysis and review.
The differences that "git-backport-diff" reports as "functional" for this
backport aren't actually functional differences. They are due to
downstream lacking two upstream commits:
- f7496d717357 ("BaseTools: Clean up source files", 2018-07-09), with the
"usual" diffstat "289 files changed, 10645 insertions(+), 10645
deletions(-)";
- more importantly, 472eb3b89682 ("BaseTools: Add --uefi option to enable
UefiCompress method", 2018-10-13).
(Side note: in upstream, commit 472eb3b89682 was incorrectly reverted as
part of 1ccc4d895dd8 ("Revert BaseTools: PYTHON3 migration", 2018-10-15),
but then it was re-applied in f1400101a732.)
In commit 472eb3b89682, the "UEFI" compression/decompression method was
added to BaseTools, beyond the original "Tiano" method. This caused the
Tiano method to be indented more deeply, in the main() function of
"TianoCompress.c". (Also the original Decompress() function was renamed to
TDecompress().) The CVE fix applies to the "Tiano" method, which RHEL8
does have, but at a different nesting level. Therefore the changes have
been backported manually, and the difference in indentation is also why
"git-backport-diff" thinks the changes are functional.
This backport, once applied, can be diffed against the upstream tree more
easily as follows:
git diff -b HEAD..041d89bc0f01 -- \
BaseTools/Source/C/Common/Decompress.c \
BaseTools/Source/C/TianoCompress/TianoCompress.c
--^-- RHEL8 note end --^--
Fix CVE-2017-5731,CVE-2017-5732,CVE-2017-5733,CVE-2017-5734,CVE-2017-5735
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>
Reviewed-by: Star Zeng <star.zeng@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
(cherry picked from commit 041d89bc0f0119df37a5fce1d0f16495ff905089)
---
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 8f1afb4..bdc10f5 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 046fb36..d07fd9e 100644
--- a/BaseTools/Source/C/TianoCompress/TianoCompress.c
+++ b/BaseTools/Source/C/TianoCompress/TianoCompress.c
@@ -1753,6 +1753,7 @@ Returns:
SCRATCH_DATA *Scratch;
UINT8 *Src;
UINT32 OrigSize;
+ UINT32 CompSize;
SetUtilityName(UTILITY_NAME);
@@ -1761,6 +1762,7 @@ Returns:
OutBuffer = NULL;
Scratch = NULL;
OrigSize = 0;
+ CompSize = 0;
InputLength = 0;
InputFileName = NULL;
OutputFileName = NULL;
@@ -1979,15 +1981,24 @@ Returns:
if (DebugMode) {
DebugMsg(UTILITY_NAME, 0, DebugLevel, "Decoding\n", NULL);
}
+ 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!");
@@ -2171,12 +2182,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]]++;
}
@@ -2220,6 +2235,7 @@ Returns:
Avail = NumOfChar;
Mask = (UINT16) (1U << (15 - TableBits));
+ MaxTableLength = (UINT16) (1U << TableBits);
for (Char = 0; Char < NumOfChar; Char++) {
@@ -2233,6 +2249,9 @@ Returns:
if (Len <= TableBits) {
for (Index = Start[Len]; Index < NextCode; Index++) {
+ if (Index >= MaxTableLength) {
+ return (UINT16) BAD_TABLE;
+ }
Table[Index] = Char;
}
@@ -2617,11 +2636,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--;
}
--
1.8.3.1