From bb8d03f29d9738fe9a2c16da30343aae3888a362 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Wed, 13 May 2020 10:51:10 +0200 Subject: Fix location of testdeb in added regression tests --- test/integration/test-github-111-invalid-armember | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/test-github-111-invalid-armember b/test/integration/test-github-111-invalid-armember index ec2163bf6..1e095eef3 100755 --- a/test/integration/test-github-111-invalid-armember +++ b/test/integration/test-github-111-invalid-armember @@ -10,19 +10,19 @@ setupaptarchive # this used to crash, but it should treat it as an invalid member header touch ' ' ar -q test.deb ' ' -testsuccessequal "E: Invalid archive member header" ${BUILDDIRECTORY}/../test/interactive-helper/testdeb test.deb +testsuccessequal "E: Invalid archive member header" ${APTTESTHELPERSBINDIR}/testdeb test.deb rm test.deb touch 'x' ar -q test.deb 'x' -testsuccessequal "E: This is not a valid DEB archive, missing 'debian-binary' member" ${BUILDDIRECTORY}/../test/interactive-helper/testdeb test.deb +testsuccessequal "E: This is not a valid DEB archive, missing 'debian-binary' member" ${APTTESTHELPERSBINDIR}/testdeb test.deb # [ other fields] - name is not nul terminated here, it ends in . msgmsg "Unterminated ar member name" printf '!\0120123456789ABCDE.A123456789A.01234.01234.0123456.012345678.0.' > test.deb -testsuccessequal "E: Invalid archive member header" ${BUILDDIRECTORY}/../test/interactive-helper/testdeb test.deb +testsuccessequal "E: Invalid archive member header" ${APTTESTHELPERSBINDIR}/testdeb test.deb # unused source code for generating $tar below @@ -85,4 +85,4 @@ cp control.tar.gz data.tar.gz touch debian-binary rm test.deb ar -q test.deb debian-binary control.tar.gz data.tar.gz -testsuccessequal "W: Unknown TAR header type 88" ${BUILDDIRECTORY}/../test/interactive-helper/testdeb test.deb +testsuccessequal "W: Unknown TAR header type 88" ${APTTESTHELPERSBINDIR}/testdeb test.deb -- cgit v1.2.3 From 66962a66970a1f816375620c89de7117a470a6af 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-inst/contrib/arfile.cc | 14 +- test/integration/test-cve-2020-27350 | 13 ++ test/interactive-helper/CMakeLists.txt | 2 + .../interactive-helper/createdeb-cve-2020-27350.cc | 235 +++++++++++++++++++++ 4 files changed, 263 insertions(+), 1 deletion(-) create mode 100755 test/integration/test-cve-2020-27350 create mode 100644 test/interactive-helper/createdeb-cve-2020-27350.cc diff --git a/apt-inst/contrib/arfile.cc b/apt-inst/contrib/arfile.cc index 5cb43c690..6d4a1f158 100644 --- a/apt-inst/contrib/arfile.cc +++ b/apt-inst/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; diff --git a/test/integration/test-cve-2020-27350 b/test/integration/test-cve-2020-27350 new file mode 100755 index 000000000..6ee867bb3 --- /dev/null +++ b/test/integration/test-cve-2020-27350 @@ -0,0 +1,13 @@ +#!/bin/sh +set -e + +TESTDIR="$(readlink -f "$(dirname "$0")")" +. "$TESTDIR/framework" +setupenvironment +configarchitecture "amd64" + +${APTTESTHELPERSBINDIR}/createdeb-cve-2020-27350 crash crash.deb +testequal "E: Invalid archive member header" runapt ${APTTESTHELPERSBINDIR}/testdeb ./crash.deb + +${APTTESTHELPERSBINDIR}/createdeb-cve-2020-27350 loop loop.deb +testequal "E: Invalid archive member header" runapt ${APTTESTHELPERSBINDIR}/testdeb ./loop.deb diff --git a/test/interactive-helper/CMakeLists.txt b/test/interactive-helper/CMakeLists.txt index 5a32ca17e..0de7d9c5f 100644 --- a/test/interactive-helper/CMakeLists.txt +++ b/test/interactive-helper/CMakeLists.txt @@ -10,6 +10,8 @@ add_executable(aptdropprivs aptdropprivs.cc) target_link_libraries(aptdropprivs apt-pkg) add_executable(test_fileutl test_fileutl.cc) target_link_libraries(test_fileutl apt-pkg) +add_executable(createdeb-cve-2020-27350 createdeb-cve-2020-27350.cc) + add_library(noprofile SHARED libnoprofile.c) target_link_libraries(noprofile ${CMAKE_DL_LIBS}) diff --git a/test/interactive-helper/createdeb-cve-2020-27350.cc b/test/interactive-helper/createdeb-cve-2020-27350.cc new file mode 100644 index 000000000..9103c2137 --- /dev/null +++ b/test/interactive-helper/createdeb-cve-2020-27350.cc @@ -0,0 +1,235 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct Header +{ + char Name[16]; + char MTime[12]; + char UID[6]; + char GID[6]; + char Mode[8]; + char Size[10]; + char Magic[2]; +}; + +struct TarHeader +{ + char Name[100]; + char Mode[8]; + char UserID[8]; + char GroupID[8]; + char Size[12]; + char MTime[12]; + char Checksum[8]; + char LinkFlag; + char LinkName[100]; + char MagicNumber[8]; + char UserName[32]; + char GroupName[32]; + char Major[8]; + char Minor[8]; +}; + +// Call `write` and check the result. +static void write_chk(int fd, const void *buf, size_t count) +{ + const ssize_t wr = write(fd, buf, count); + if (wr < 0) + { + const int err = errno; + fprintf(stderr, "Write failed: %s\n", strerror(err)); + exit(EXIT_FAILURE); + } + if ((size_t)wr != count) + { + fprintf(stderr, "Incomplete write.\n"); + exit(EXIT_FAILURE); + } +} + +// Triggers a negative integer overflow at https://git.launchpad.net/ubuntu/+source/apt/tree/apt-pkg/contrib/arfile.cc?h=applied/ubuntu/focal-updates&id=4c264e60b524855b211751e1632ba48526f6b44d#n116: +// +// Memb->Size -= Len; +// +// Due to the integer overflow, the value of Memb->Size is 0xFFFFFFFFFFFFFFFF. +// This leads to an out-of-memory error at https://git.launchpad.net/ubuntu/+source/python-apt/tree/python/arfile.cc?h=applied/ubuntu/focal-updates&id=0f7cc93acdb51d943114f1cd79002288c4ca4d24#n602: +// +// char* value = new char[member->Size]; +// +// The out-of-memory error causes aptd to crash. +static void createdeb_crash(const int fd) +{ + // Magic number + static const char *magic = "!\n"; + write_chk(fd, magic, strlen(magic)); + + struct Header h; + memset(&h, 0, sizeof(h)); + + memcpy(h.Name, "control.tar ", sizeof(h.Name)); + write_chk(fd, &h, sizeof(h)); + + memset(&h, 0, sizeof(h)); + memcpy(h.Name, "data.tar ", sizeof(h.Name)); + write_chk(fd, &h, sizeof(h)); + + memset(&h, 0, sizeof(h)); + memcpy(h.Name, "#1/13 ", sizeof(h.Name)); + strcpy(h.Size, "12"); + write_chk(fd, &h, sizeof(h)); + write_chk(fd, "debian-binary", 13); +} + +// Triggers an infinite loop in `ARArchive::LoadHeaders()`. +// The bug is due to the use of `strtoul` at https://git.launchpad.net/ubuntu/+source/apt/tree/apt-pkg/contrib/strutl.cc?h=applied/ubuntu/focal-updates&id=4c264e60b524855b211751e1632ba48526f6b44d#n1169: +// +// Res = strtoul(S,&End,Base); +// +// The problem is that `strtoul` accepts negative numbers. We exploit that here by setting the size string to "-60". +static void createdeb_loop(const int fd) +{ + // Magic number + static const char *magic = "!\n"; + write_chk(fd, magic, strlen(magic)); + + struct Header h; + memset(&h, 0, sizeof(h)); + + memcpy(h.Name, "#1/20 ", sizeof(h.Name)); + strcpy(h.Size, "-60"); + write_chk(fd, &h, sizeof(h)); + + char buf[20]; + memset(buf, 0, sizeof(buf)); + write_chk(fd, buf, sizeof(buf)); +} + +// Leaks a file descriptor in `debfile_new()`: +// +// https://git.launchpad.net/python-apt/tree/python/arfile.cc?h=2.0.0#n588 +// +// If the .deb file is invalid then the function returns without deleting +// `self`, which means that the file descriptor isn't closed. +static void createdeb_leakfd(const int fd) +{ + // Magic number + static const char *magic = "!\n"; + write_chk(fd, magic, strlen(magic)); + + struct Header h; + memset(&h, 0, sizeof(h)); + + memset(&h, 0, sizeof(h)); + memcpy(h.Name, "data.tar ", sizeof(h.Name)); + write_chk(fd, &h, sizeof(h)); +} + +static void set_checksum(unsigned char block[512]) +{ + struct TarHeader *tar = (struct TarHeader *)&block[0]; + memset(tar->Checksum, ' ', sizeof(tar->Checksum)); + uint32_t sum = 0; + for (int i = 0; i < 512; i++) + { + sum += block[i]; + } + snprintf(tar->Checksum, sizeof(tar->Checksum), "%o", sum); +} + +// This does not trigger a vulnerability. It is just a basis for exploration. +static void createdeb_test(const int fd) +{ + // Magic number + static const char *magic = "!\n"; + write_chk(fd, magic, strlen(magic)); + + struct Header h; + + memset(&h, 0, sizeof(h)); + memcpy(h.Name, "data.tar ", sizeof(h.Name)); + write_chk(fd, &h, sizeof(h)); + + memset(&h, 0, sizeof(h)); + memcpy(h.Name, "debian-binary ", sizeof(h.Name)); + strcpy(h.Size, "4"); + write_chk(fd, &h, sizeof(h)); + static const char *debian_binary = "2.0\n"; + write_chk(fd, debian_binary, strlen(debian_binary)); + + static const char *control = + "Architecture: all\n" + "Package: kevsh\n\n"; + memset(&h, 0, sizeof(h)); + memcpy(h.Name, "control.tar ", sizeof(h.Name)); + snprintf(h.Size, sizeof(h.Size), "%ld", (size_t)512 + 512); + write_chk(fd, &h, sizeof(h)); + + unsigned char block[512]; + memset(block, 0, sizeof(block)); + struct TarHeader *tar = (struct TarHeader *)&block[0]; + strcpy(tar->Name, "control"); + strcpy(tar->Mode, "644"); + snprintf(tar->Size, sizeof(tar->Size), "%lo", strlen(control)); + set_checksum(block); + write_chk(fd, block, sizeof(block)); + + memset(block, 0, sizeof(block)); + strcpy((char *)block, control); + write_chk(fd, block, sizeof(block)); +} + +int main(int argc, char *argv[]) +{ + if (argc != 3) + { + const char *progname = argc > 0 ? argv[0] : "a.out"; + fprintf( + stderr, + "usage: %s \n" + "modes: loop, segv, leakfd\n", + progname); + return EXIT_FAILURE; + } + + const char *mode = argv[1]; + const char *filename = argv[2]; + + const int fd = open(filename, O_CREAT | O_WRONLY | O_TRUNC, 00644); + if (fd < 0) + { + const int err = errno; + fprintf(stderr, "Could not open %s: %s\n", filename, strerror(err)); + return EXIT_FAILURE; + } + + if (strcmp(mode, "crash") == 0) + { + createdeb_crash(fd); + } + else if (strcmp(mode, "loop") == 0) + { + createdeb_loop(fd); + } + else if (strcmp(mode, "leakfd") == 0) + { + createdeb_leakfd(fd); + } + else if (strcmp(mode, "test") == 0) + { + createdeb_test(fd); + } + else + { + fprintf(stderr, "Mode not recognized: %s\n", mode); + } + + close(fd); + return EXIT_SUCCESS; +} -- cgit v1.2.3 From 29581d103fc85d988c1f8a9c995ef9a6bb600500 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-inst/contrib/extracttar.cc | 11 ++- test/integration/test-cve-2020-27350 | 6 ++ .../interactive-helper/createdeb-cve-2020-27350.cc | 84 +++++++++++++++++++++- 3 files changed, 99 insertions(+), 2 deletions(-) diff --git a/apt-inst/contrib/extracttar.cc b/apt-inst/contrib/extracttar.cc index 19ac1c8b1..b24b66261 100644 --- a/apt-inst/contrib/extracttar.cc +++ b/apt-inst/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 /*{{{*/ // --------------------------------------------------------------------- /* */ @@ -222,6 +227,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) @@ -241,6 +248,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) diff --git a/test/integration/test-cve-2020-27350 b/test/integration/test-cve-2020-27350 index 6ee867bb3..336dc5b7e 100755 --- a/test/integration/test-cve-2020-27350 +++ b/test/integration/test-cve-2020-27350 @@ -11,3 +11,9 @@ testequal "E: Invalid archive member header" runapt ${APTTESTHELPERSBINDIR}/test ${APTTESTHELPERSBINDIR}/createdeb-cve-2020-27350 loop loop.deb testequal "E: Invalid archive member header" runapt ${APTTESTHELPERSBINDIR}/testdeb ./loop.deb + +${APTTESTHELPERSBINDIR}/createdeb-cve-2020-27350 long-name long-name.deb +testequal "E: Long name to large: 67108865 bytes > 1048576 bytes" runapt ${APTTESTHELPERSBINDIR}/extract-control long-name.deb control + +${APTTESTHELPERSBINDIR}/createdeb-cve-2020-27350 long-link long-link.deb +testequal "E: Long name to large: 67108865 bytes > 1048576 bytes" runapt ${APTTESTHELPERSBINDIR}/extract-control long-link.deb control diff --git a/test/interactive-helper/createdeb-cve-2020-27350.cc b/test/interactive-helper/createdeb-cve-2020-27350.cc index 9103c2137..7c58eb9df 100644 --- a/test/interactive-helper/createdeb-cve-2020-27350.cc +++ b/test/interactive-helper/createdeb-cve-2020-27350.cc @@ -142,8 +142,82 @@ static void set_checksum(unsigned char block[512]) } snprintf(tar->Checksum, sizeof(tar->Checksum), "%o", sum); } +static void base256_encode(char *Str, unsigned long long Num, unsigned int Len) +{ + Str += Len; + while (Len-- > 0) { + *--Str = static_cast(Num & 0xff); + Num >>= 8; + } + + *Str |= 0x80; // mark as base256 +} + +// Create a deb with a control.tar that contains a too large file or link name (GNU extension) +static void createdeb_bigtarfilelength(const int fd, int flag, unsigned long long size = 64llu * 1024 * 1024 + 1) +{ + // Magic number + static const char *magic = "!\n"; + write_chk(fd, magic, strlen(magic)); + + struct Header h; + memset(&h, ' ', sizeof(h)); + memcpy(h.Name, "debian-binary/ ", sizeof(h.Name)); + h.MTime[0] = '0'; + h.UID[0] = '0'; + h.GID[0] = '0'; + memcpy(h.Mode, "644", 3); + h.Size[0] = '0'; + memcpy(h.Magic, "`\n", 2); + + write_chk(fd, &h, sizeof(h)); + + memset(&h, ' ', sizeof(h)); + memcpy(h.Name, "data.tar/ ", sizeof(h.Name)); + h.MTime[0] = '0'; + h.UID[0] = '0'; + h.GID[0] = '0'; + memcpy(h.Mode, "644", 3); + h.Size[0] = '0'; + memcpy(h.Magic, "`\n", 2); + + write_chk(fd, &h, sizeof(h)); + + memset(&h, ' ', sizeof(h)); + memcpy(h.Name, "control.tar/ ", sizeof(h.Name)); + h.MTime[0] = '0'; + h.UID[0] = '0'; + h.GID[0] = '0'; + memcpy(h.Mode, "644", 3); + memcpy(h.Size, "512", 3); + memcpy(h.Magic, "`\n", 2); + + write_chk(fd, &h, sizeof(h)); + union + { + struct TarHeader t; + unsigned char buf[512]; + } t; + for (unsigned int i = 0; i < sizeof(t.buf); i++) + t.buf[i] = '7'; + memcpy(t.t.Name, "control\0 ", 16); + memcpy(t.t.UserName, "userName", 8); + memcpy(t.t.GroupName, "thisIsAGroupNamethisIsAGroupName", 32); + t.t.LinkFlag = flag; + base256_encode(t.t.Size, size, sizeof(t.t.Size)); + memset(t.t.Checksum, ' ', sizeof(t.t.Checksum)); + + unsigned long sum = 0; + for (unsigned int i = 0; i < sizeof(t.buf); i++) + sum += t.buf[i]; + + int written = sprintf(t.t.Checksum, "%lo", sum); + for (unsigned int i = written; i < sizeof(t.t.Checksum); i++) + t.t.Checksum[i] = ' '; + + write_chk(fd, t.buf, sizeof(t.buf)); +} -// This does not trigger a vulnerability. It is just a basis for exploration. static void createdeb_test(const int fd) { // Magic number @@ -221,6 +295,14 @@ int main(int argc, char *argv[]) { createdeb_leakfd(fd); } + else if (strcmp(mode, "long-name") == 0) + { + createdeb_bigtarfilelength(fd, 'L'); + } + else if (strcmp(mode, "long-link") == 0) + { + createdeb_bigtarfilelength(fd, 'K'); + } else if (strcmp(mode, "test") == 0) { createdeb_test(fd); -- cgit v1.2.3 From ed786183bfe9813cf9bc603de0a2beed3e0d0e31 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-inst/deb/debfile.cc | 15 +++++++++++++++ test/integration/test-cve-2020-27350 | 3 +++ test/interactive-helper/createdeb-cve-2020-27350.cc | 4 ++++ 3 files changed, 22 insertions(+) diff --git a/apt-inst/deb/debfile.cc b/apt-inst/deb/debfile.cc index f8d752e7f..bef0cd0d8 100644 --- a/apt-inst/deb/debfile.cc +++ b/apt-inst/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; diff --git a/test/integration/test-cve-2020-27350 b/test/integration/test-cve-2020-27350 index 336dc5b7e..f4bb79bcb 100755 --- a/test/integration/test-cve-2020-27350 +++ b/test/integration/test-cve-2020-27350 @@ -17,3 +17,6 @@ testequal "E: Long name to large: 67108865 bytes > 1048576 bytes" runapt ${APTTE ${APTTESTHELPERSBINDIR}/createdeb-cve-2020-27350 long-link long-link.deb testequal "E: Long name to large: 67108865 bytes > 1048576 bytes" runapt ${APTTESTHELPERSBINDIR}/extract-control long-link.deb control + +${APTTESTHELPERSBINDIR}/createdeb-cve-2020-27350 long-control long-control.deb +testequal "E: Control file too large: 67108865 > 67108864 bytes" runapt ${APTTESTHELPERSBINDIR}/extract-control long-control.deb control diff --git a/test/interactive-helper/createdeb-cve-2020-27350.cc b/test/interactive-helper/createdeb-cve-2020-27350.cc index 7c58eb9df..af049d4e8 100644 --- a/test/interactive-helper/createdeb-cve-2020-27350.cc +++ b/test/interactive-helper/createdeb-cve-2020-27350.cc @@ -303,6 +303,10 @@ int main(int argc, char *argv[]) { createdeb_bigtarfilelength(fd, 'K'); } + else if (strcmp(mode, "long-control") == 0) + { + createdeb_bigtarfilelength(fd, '0'); + } else if (strcmp(mode, "test") == 0) { createdeb_test(fd); -- cgit v1.2.3 From 0e3b54db6d7ec7c7baf151c812b77042927cf44e 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-inst/contrib/extracttar.cc | 10 ++++++++++ test/integration/test-cve-2020-27350 | 3 +++ test/interactive-helper/createdeb-cve-2020-27350.cc | 4 ++++ 3 files changed, 17 insertions(+) diff --git a/apt-inst/contrib/extracttar.cc b/apt-inst/contrib/extracttar.cc index b24b66261..cbee4d12a 100644 --- a/apt-inst/contrib/extracttar.cc +++ b/apt-inst/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 /*{{{*/ // --------------------------------------------------------------------- /* */ @@ -175,6 +180,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. diff --git a/test/integration/test-cve-2020-27350 b/test/integration/test-cve-2020-27350 index f4bb79bcb..a32bf95e5 100755 --- a/test/integration/test-cve-2020-27350 +++ b/test/integration/test-cve-2020-27350 @@ -20,3 +20,6 @@ testequal "E: Long name to large: 67108865 bytes > 1048576 bytes" runapt ${APTTE ${APTTESTHELPERSBINDIR}/createdeb-cve-2020-27350 long-control long-control.deb testequal "E: Control file too large: 67108865 > 67108864 bytes" runapt ${APTTESTHELPERSBINDIR}/extract-control long-control.deb control + +${APTTESTHELPERSBINDIR}/createdeb-cve-2020-27350 too-long-control too-long-control.deb +testequal "E: Tar member too large: $((128 * 1024 * 1024 * 1024 + 1)) > $((128 * 1024 * 1024 * 1024)) bytes" runapt ${APTTESTHELPERSBINDIR}/extract-control too-long-control.deb control diff --git a/test/interactive-helper/createdeb-cve-2020-27350.cc b/test/interactive-helper/createdeb-cve-2020-27350.cc index af049d4e8..8b9619469 100644 --- a/test/interactive-helper/createdeb-cve-2020-27350.cc +++ b/test/interactive-helper/createdeb-cve-2020-27350.cc @@ -307,6 +307,10 @@ int main(int argc, char *argv[]) { createdeb_bigtarfilelength(fd, '0'); } + else if (strcmp(mode, "too-long-control") == 0) + { + createdeb_bigtarfilelength(fd, '0', 128llu * 1024 * 1024 * 1024 + 1); + } else if (strcmp(mode, "test") == 0) { createdeb_test(fd); -- cgit v1.2.3 From 95e417cb069928dfdb5dfacb418f025d71f32c4d Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 7 Dec 2020 12:31:04 +0100 Subject: Release 1.8.2.2 --- CMakeLists.txt | 2 +- debian/changelog | 14 ++++++++++++++ doc/apt-verbatim.ent | 2 +- doc/po/apt-doc.pot | 4 ++-- po/apt-all.pot | 4 ++-- 5 files changed, 20 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 83334baab..4117aebe4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -193,7 +193,7 @@ check_cxx_target(HAVE_FMV_SSE42_AND_CRC32DI "sse4.2" "__builtin_ia32_crc32di(0, # Configure some variables like package, version and architecture. set(PACKAGE ${PROJECT_NAME}) set(PACKAGE_MAIL "APT Development Team ") -set(PACKAGE_VERSION "1.8.2.1") +set(PACKAGE_VERSION "1.8.2.2") if (NOT DEFINED DPKG_DATADIR) execute_process(COMMAND ${PERL_EXECUTABLE} -MDpkg -e "print $Dpkg::DATADIR;" diff --git a/debian/changelog b/debian/changelog index ec4769b9b..44f80d187 100644 --- a/debian/changelog +++ b/debian/changelog @@ -1,3 +1,17 @@ +apt (1.8.2.2) buster-security; urgency=high + + * SECURITY UPDATE: Integer overflow in parsing (LP: #1899193) + - apt-pkg/contrib/arfile.cc: add extra checks. + - apt-pkg/contrib/tarfile.cc: limit tar item sizes to 128 GiB + - apt-pkg/deb/debfile.cc: limit control file sizes to 64 MiB + - test/*: add tests. + - CVE-2020-27350 + * Additional hardening: + - apt-pkg/contrib/tarfile.cc: Limit size of long names and links to 1 MiB + * Fix autopkgtest regression in 1.8.2.1 security update + + -- Julian Andres Klode Mon, 07 Dec 2020 12:31:04 +0100 + apt (1.8.2.1) buster-security; urgency=high * SECURITY UPDATE: Out of bounds read in ar, tar implementations (LP: #1878177) diff --git a/doc/apt-verbatim.ent b/doc/apt-verbatim.ent index 54c81b8f1..b8c33d072 100644 --- a/doc/apt-verbatim.ent +++ b/doc/apt-verbatim.ent @@ -268,7 +268,7 @@ "> - + diff --git a/doc/po/apt-doc.pot b/doc/po/apt-doc.pot index c0ec0859f..d143d0a6c 100644 --- a/doc/po/apt-doc.pot +++ b/doc/po/apt-doc.pot @@ -5,9 +5,9 @@ #, fuzzy msgid "" msgstr "" -"Project-Id-Version: apt-doc 1.8.2.1\n" +"Project-Id-Version: apt-doc 1.8.2.2\n" "Report-Msgid-Bugs-To: APT Development Team \n" -"POT-Creation-Date: 2020-05-12 18:00+0000\n" +"POT-Creation-Date: 2020-12-02 17:16+0000\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "Last-Translator: FULL NAME \n" "Language-Team: LANGUAGE \n" diff --git a/po/apt-all.pot b/po/apt-all.pot index 8d005b831..373c4052c 100644 --- a/po/apt-all.pot +++ b/po/apt-all.pot @@ -5,9 +5,9 @@ #, fuzzy msgid "" msgstr "" -"Project-Id-Version: apt 1.8.2.1\n" +"Project-Id-Version: apt 1.8.2.2\n" "Report-Msgid-Bugs-To: APT Development Team \n" -"POT-Creation-Date: 2020-05-12 18:00+0000\n" +"POT-Creation-Date: 2020-12-02 17:16+0000\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "Last-Translator: FULL NAME \n" "Language-Team: LANGUAGE \n" -- cgit v1.2.3