From 29c394f110b1f769e629e8775874261e33d4abd9 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek 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 Acked-by: Thomas Huth From: Liming Gao --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 Signed-off-by: Liming Gao Reviewed-by: Star Zeng Acked-by: Laszlo Ersek (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