From d10c68d628fe5342d400a999a6d10c5c7c0cef41 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 19 Oct 2020 13:22:33 +0200 Subject: CVE-2020-27350: arfile: Integer overflow in parsing GHSL-2020-169: This first hunk adds a check that we have more files left to read in the file than the size of the member, ensuring that (a) the number is not negative, which caused the crash here and (b) ensures that we similarly avoid other issues with trying to read too much data. GHSL-2020-168: Long file names are encoded by a special marker in the filename and then the real filename is part of what is normally the data. We did not check that the length of the file name is within the length of the member, which means that we got a overflow later when subtracting the length from the member size to get the remaining member size. The file createdeb-lp1899193.cc was provided by GitHub Security Lab and reformatted using apt coding style for inclusion in the test case, both of these issues have an automated test case in test/integration/test-ubuntu-bug-1899193-security-issues. LP: #1899193 --- apt-pkg/contrib/arfile.cc | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/arfile.cc b/apt-pkg/contrib/arfile.cc index 5cb43c690..6d4a1f158 100644 --- a/apt-pkg/contrib/arfile.cc +++ b/apt-pkg/contrib/arfile.cc @@ -94,7 +94,12 @@ bool ARArchive::LoadHeaders() delete Memb; return _error->Error(_("Invalid archive member header")); } - + + if (Left < 0 || Memb->Size > static_cast(Left)) + { + delete Memb; + return _error->Error(_("Invalid archive member header")); + } // Check for an extra long name string if (memcmp(Head.Name,"#1/",3) == 0) { @@ -106,6 +111,13 @@ bool ARArchive::LoadHeaders() delete Memb; return _error->Error(_("Invalid archive member header")); } + + if (Len > Memb->Size) + { + delete Memb; + return _error->Error(_("Invalid archive member header")); + } + if (File.Read(S,Len) == false) { delete Memb; -- cgit v1.2.3 From 822db13d68658a1a20df2d19c688c18faa331616 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Fri, 4 Dec 2020 12:37:19 +0100 Subject: tarfile: OOM hardening: Limit size of long names/links to 1 MiB Tarballs have long names and long link targets structured by a special tar header with a GNU extension followed by the actual content (padded to 512 bytes). Essentially, think of a name as a special kind of file. The limit of a file size in a header is 12 bytes, aka 10**12 or 1 TB. While this works OK-ish for file content that we stream to extractors, we need to copy file names into memory, and this opens us up to an OOM DoS attack. Limit the file name size to 1 MiB, as libarchive does, to make things safer. --- apt-pkg/contrib/extracttar.cc | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/extracttar.cc b/apt-pkg/contrib/extracttar.cc index 1616c9f12..f653e227e 100644 --- a/apt-pkg/contrib/extracttar.cc +++ b/apt-pkg/contrib/extracttar.cc @@ -55,7 +55,12 @@ struct ExtractTar::TarHeader char Major[8]; char Minor[8]; }; - + +// We need to read long names (names and link targets) into memory, so let's +// have a limit (shamelessly stolen from libarchive) to avoid people OOMing +// us with large streams. +static const unsigned long long APT_LONGNAME_LIMIT = 1048576llu; + // ExtractTar::ExtractTar - Constructor /*{{{*/ // --------------------------------------------------------------------- /* */ @@ -218,6 +223,8 @@ bool ExtractTar::Go(pkgDirStream &Stream) { unsigned long long Length = Itm.Size; unsigned char Block[512]; + if (Length > APT_LONGNAME_LIMIT) + return _error->Error("Long name to large: %llu bytes > %llu bytes", Length, APT_LONGNAME_LIMIT); while (Length > 0) { if (InFd.Read(Block,sizeof(Block),true) == false) @@ -237,6 +244,8 @@ bool ExtractTar::Go(pkgDirStream &Stream) { unsigned long long Length = Itm.Size; unsigned char Block[512]; + if (Length > APT_LONGNAME_LIMIT) + return _error->Error("Long name to large: %llu bytes > %llu bytes", Length, APT_LONGNAME_LIMIT); while (Length > 0) { if (InFd.Read(Block,sizeof(Block),true) == false) -- cgit v1.2.3 From 0444f9dd52c2bc7bec315f6f1ecad76a30713fa0 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Sat, 5 Dec 2020 20:17:56 +0100 Subject: CVE-2020-27350: debfile: integer overflow: Limit control size to 64 MiB Like the code in arfile.cc, MemControlExtract also has buffer overflows, in code allocating memory for parsing control files. Specify an upper limit of 64 MiB for control files to both protect against the Size overflowing (we allocate Size + 2 bytes), and protect a bit against control files consisting only of zeroes. --- apt-pkg/deb/debfile.cc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'apt-pkg') diff --git a/apt-pkg/deb/debfile.cc b/apt-pkg/deb/debfile.cc index f8d752e7f..bef0cd0d8 100644 --- a/apt-pkg/deb/debfile.cc +++ b/apt-pkg/deb/debfile.cc @@ -184,11 +184,23 @@ bool debDebFile::ControlExtract::DoItem(Item &Itm,int &Fd) // --------------------------------------------------------------------- /* This sets up to extract the control block member file into a memory block of just the right size. All other files go into the bit bucket. */ + +// Upper size limit for control files. Two reasons for having a limit here: +// +// 1. We read those files into memory and want to avoid being killed by OOM +// +// 2. We allocate (Itm.Size+2)-large arrays, so this can overflow if Itm.Size +// becomes 2**64-2 or larger. This is obviously +// +// 64 MiB seems like a terribly large size that everyone should be happy with. +static const unsigned long long DEB_CONTROL_SIZE_LIMIT = 64 * 1024 * 1024; bool debDebFile::MemControlExtract::DoItem(Item &Itm,int &Fd) { // At the control file, allocate buffer memory. if (Member == Itm.Name) { + if (Itm.Size > DEB_CONTROL_SIZE_LIMIT) + return _error->Error("Control file too large: %llu > %llu bytes", Itm.Size, DEB_CONTROL_SIZE_LIMIT); delete [] Control; Control = new char[Itm.Size+2]; IsControl = true; @@ -237,6 +249,9 @@ bool debDebFile::MemControlExtract::Read(debDebFile &Deb) record. */ bool debDebFile::MemControlExtract::TakeControl(const void *Data,unsigned long long Size) { + if (Size > DEB_CONTROL_SIZE_LIMIT) + return _error->Error("Control file too large: %llu > %llu bytes", Size, DEB_CONTROL_SIZE_LIMIT); + delete [] Control; Control = new char[Size+2]; Length = Size; -- cgit v1.2.3 From df81895bce764dd02fbb4d67b92d28a730b5281f Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Sat, 5 Dec 2020 19:55:30 +0100 Subject: CVE-2020-27350: tarfile: integer overflow: Limit tar items to 128 GiB The integer overflow was detected by DonKult who added a check like this: (std::numeric_limits::max() - (2 * sizeof(Block))) Which deals with the code as is, but also still is a fairly big limit, and could become fragile if we change the code. Let's limit our file sizes to 128 GiB, which should be sufficient for everyone. Original comment by DonKult: The code assumes that it can add sizeof(Block)-1 to the size of the item later on, but if we are close to a 64bit overflow this is not possible. Fixing this seems too complex compared to just ensuring there is enough room left given that we will have a lot more problems the moment we will be acting on files that large as if the item is that large, the (valid) tar including it probably doesn't fit in 64bit either. --- apt-pkg/contrib/extracttar.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/extracttar.cc b/apt-pkg/contrib/extracttar.cc index f653e227e..dc2437528 100644 --- a/apt-pkg/contrib/extracttar.cc +++ b/apt-pkg/contrib/extracttar.cc @@ -61,6 +61,11 @@ struct ExtractTar::TarHeader // us with large streams. static const unsigned long long APT_LONGNAME_LIMIT = 1048576llu; +// A file size limit that we allow extracting. Currently, that's 128 GB. +// We also should leave some wiggle room for code adding files to it, and +// possibly conversion for signed, so this should not be larger than like 2**62. +static const unsigned long long APT_FILESIZE_LIMIT = 1llu << 37; + // ExtractTar::ExtractTar - Constructor /*{{{*/ // --------------------------------------------------------------------- /* */ @@ -171,6 +176,11 @@ bool ExtractTar::Go(pkgDirStream &Stream) StrToNum(Tar->Minor,Itm.Minor,sizeof(Tar->Minor),8) == false) return _error->Error(_("Corrupted archive")); + // Security check. Prevents overflows below the code when rounding up in skip/copy code, + // and provides modest protection against decompression bombs. + if (Itm.Size > APT_FILESIZE_LIMIT) + return _error->Error("Tar member too large: %llu > %llu bytes", Itm.Size, APT_FILESIZE_LIMIT); + // Grab the filename and link target: use last long name if one was // set, otherwise use the header value as-is, but remember that it may // fill the entire 100-byte block and needs to be zero-terminated. -- cgit v1.2.3