From 41129e136b621728eb5cb1c81aafcc5fedc53a12 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Wed, 24 Oct 2018 21:03:43 +0200 Subject: [PATCH 2/4] MdePkg: Add more checker in UefiDecompressLib to access the valid buffer only (CVE FIX) Message-id: <20181024190345.15288-3-lersek@redhat.com> Patchwork-id: 82883 O-Subject: [RHEL8 edk2 PATCH 2/4] MdePkg: Add more checker in UefiDecompressLib to access the valid buffer only (CVE FIX) Bugzilla: 1641449 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=1641449 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-5732 - CVE-2017-5733 - CVE-2017-5734 - CVE-2017-5735 but not CVE-2017-5731 (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 trivial context difference (whitespace) is due to RHEL8 lacking upstream commit 9095d37b8fe5 ("MdePkg: Clean up source files", 2018-06-28). I've considered backporting that (since it only cleans up whitespace). However, the diffstat on that commit convinced me otherwise: "729 files changed, 15667 insertions(+), 15667 deletions(-)". I've decided not to do a partial backport of that (i.e. just for "BaseUefiDecompressLib.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 2ec7953d49677142c5f7552e9e3d96fb406ba0c4) Signed-off-by: Laszlo Ersek --- .../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 e818543..0c6b1fe 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; } -- 1.8.3.1